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

fix(instrumentation)!: remove unused supportedVersions from Instrumentation interface #4694

Merged
merged 5 commits into from
May 13, 2024

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 as a code owner 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

@@ -102,8 +94,6 @@ export interface InstrumentationModuleFile {
/** 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
18 checks passed
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

4 participants