Skip to content

Conversation

@blumamir
Copy link
Member

The Instrumentation interface in the @opentelemetry/instrumentation class, contains a property supportedVersions?: string[] which is not used anywhere, not in the package itself or by any contrib instrumentation:

  /**
   * Contains all supported versions.
   * All versions must be compatible with [semver](https://semver.org/spec/v2.0.0.html) format.
   * If the version is not supported, we won't apply instrumentation patch (see `enable` method).
   * If omitted, all versions of the module will be patched.
   */
  supportedVersions?: string[];
}

It has no effect on anything really, just a property which downstream interface consumers can reference, and it is not set anywhere.

There exists 2 other configurations for supportedVersions here and here which are being used.

My investigation leads me to #1540 which added it to the interface but never used it. It uses the supportedVersions on each InstrumentationModuleDefinition, but the one in the Instrumentation interface was never utilized.

The comment on this property is also misleading and references (see enable method), but this function uses the other 2 supporetedVersions definitions on InstrumentationModuleFile and InstrumentationModuleDefinition but not the one which is commented.

Therefore, I think it is safe to remove it as a cleanup

@blumamir blumamir requested a review from a team May 10, 2024 08:02
@blumamir
Copy link
Member Author

I wonder if this is considered breaking change, as the property was never actually used, but it is part of a public interface therefore someone might have used it for any reason. very unlikely but possible.

@blumamir blumamir changed the title fix(instrumentation): remove unused supportedVersions from Instrumentation interface fix(instrumentation)!: remove unused supportedVersions from Instrumentation interface May 10, 2024
@blumamir
Copy link
Member Author

related #4693

/** Method to patch the instrumentation */
patch(moduleExports: unknown, moduleVersion?: string): unknown;

/** Method to patch the instrumentation */
Copy link
Member Author

Choose a reason for hiding this comment

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

this is just a cleanup i spotted along the way

@dyladan
Copy link
Member

dyladan commented May 10, 2024

I wonder if this is considered breaking change, as the property was never actually used, but it is part of a public interface therefore someone might have used it for any reason. very unlikely but possible.

I'd consider it breaking but I think it's a pretty acceptable change

@pichlermarc pichlermarc merged commit 1c6e8b2 into open-telemetry:main May 13, 2024
Zirak pushed a commit to Zirak/opentelemetry-js that referenced this pull request Sep 14, 2024
…tation interface (open-telemetry#4694)

* chore: CHANGLOG

* fix(instrumentation): remove unused property from instrumentations

* chore: CHANGELOG

---------

Co-authored-by: Marc Pichler <[email protected]>
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.

4 participants