Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: instrumentation description in base class constructor #4716

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

blumamir
Copy link
Member

This PR adds a new type for InstrumentationMetadata which currently holds description.

The description field is currently part of the Instrumentation interface here:

  /**
   * Instrumentation Description - please describe all useful information
   * as Instrumentation might patch different version of different modules,
   * or support different browsers etc.
   */
  instrumentationDescription?: string;

But it was not an argument the the base class constructor, thus not being utilized by any core or contrib instrumentation packages.

This PR replaces the instrumentationDescription with a more general object for metadata which at the moment only contains description but it might hold additional info soon.

It allows existing instrumentations to register the description, making it useable for tolling and distributions to consume.

To make the process automatic, I assume the description in the package.json is the instrumentation description, save it into version.ts with the version-update.js script, and then use it in each instrumentation separately. This gives us cleaner code (no magic long strings), consistency with the text in package.json, and less manual work.

The work on making sure all descriptions are meaningful and consistent is done as #4715 and open-telemetry/opentelemetry-js-contrib#2202. Adding the new argument to the constructor is made after #4695
is merged which also made a fix to the constructor arguments.

The PR is not breaking, besides removing the instrumentationDescription property from Instrumentation interface which was not used anywhere and never referenced in core or contrib, but still part of the public api

@blumamir blumamir requested a review from a team as a code owner May 17, 2024 10:33
@blumamir blumamir changed the title feat: instrumentation description in base class constructor feat: instrumentation metadata in base class constructor May 17, 2024
@blumamir blumamir changed the title feat: instrumentation metadata in base class constructor feat: instrumentation description in base class constructor May 17, 2024
Copy link

codecov bot commented May 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.30%. Comparing base (2610122) to head (a91f419).
Report is 35 commits behind head on main.

Current head a91f419 differs from pull request most recent head 473e032

Please upload reports for the commit 473e032 to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4716      +/-   ##
==========================================
+ Coverage   90.77%   92.30%   +1.53%     
==========================================
  Files          90      178      +88     
  Lines        1930     4563    +2633     
  Branches      417     1012     +595     
==========================================
+ Hits         1752     4212    +2460     
- Misses        178      351     +173     
Files Coverage Δ
...emetry-instrumentation-grpc/src/instrumentation.ts 96.25% <100.00%> (ø)
...ges/opentelemetry-instrumentation-http/src/http.ts 92.94% <100.00%> (ø)

... and 137 files with indirect coverage changes

@blumamir
Copy link
Member Author

@david-luna this PR is a followup to our discussion in #4695

Please feel free to leave any comments or concerns on this proposed change.

@blumamir
Copy link
Member Author

Since this data is stataic per instrumentation, other alternatives to passing this info via the constructor are:

override public function

similar to how InstrumentationModuleDefinition[] are returned from init(), we can add a function like public metadata(): InstrumentationMetadata to returned this info when needed:

  metadata(): InstrumentationMetadata {
    return {
      description: PACKAGE_DESCRIPTION,
    }
  }

set public optional property

we can make this property nullable in Instrumentation interface like so:

export interface Instrumentation {
// ...
  instrumentationMetadata?: InstrumentationMetadata;

and then have each instrumentation optionally defining this property, something like:

export class HttpInstrumentation extends InstrumentationBase<HttpInstrumentationConfig> {

  public InstrumentationMetadata: InstrumentationMetadata = {
    description: PACKAGE_DESCRIPTION,
  }

@@ -27,6 +27,7 @@ All notable changes to experimental packages in this project will be documented
* feat(instrumentation): generic config type in instrumentation base [#4659](https://github.com/open-telemetry/opentelemetry-js/pull/4659) @blumamir
* feat: support node 22 [#4666](https://github.com/open-telemetry/opentelemetry-js/pull/4666) @dyladan
* feat(propagator-aws-xray-lambda): add AWS Xray Lambda propagator [4554](https://github.com/open-telemetry/opentelemetry-js/pull/4554)
* feat: instrumentation description in base class constructor [#4716](https://github.com/open-telemetry/opentelemetry-js/pull/4716) @blumamir
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd still mark this as breaking.
While we don't use this in contrib users may set it in their custom instrumentation packages and that'd be breaking for them.

@@ -80,7 +80,9 @@ export class FetchInstrumentation extends InstrumentationBase<FetchInstrumentati
private _tasksCount = 0;

constructor(config: FetchInstrumentationConfig = {}) {
super('@opentelemetry/instrumentation-fetch', VERSION, config);
super('@opentelemetry/instrumentation-fetch', VERSION, config, {
description: PACKAGE_DESCRIPTION,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this makes sense for instrumentations that target the browser. It adds to the bundle size. We can also not tree-shake it out even though it's never really used.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point. We can choose to omit it for browser instrumentations, or come up with different way to expose this information that is easily integrated into tools and distributions.

I will open a separate issue to discuss my future plans on adding more metadata, and bring it up in the SIG.

Thank you for the feedback and for raising these concerns.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as a reference, collector repo define metadata.yaml files per component which include stuff like this:

type: otlp

status:
  class: exporter
  stability:
    stable: [traces, metrics]
    beta: [logs]
  distributions: [core, contrib]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants