-
Notifications
You must be signed in to change notification settings - Fork 986
feat(instrumentation)!: simplify registerInstrumentations() API
#4675
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)!: simplify registerInstrumentations() API
#4675
Conversation
|
These aren't arguments against. I'm just pointing them out for discussion. One convenience of having allowed nested arrays was something like: const sdk = new NodeSDK({
instrumentations: [
getNodeInstrumentations(/* ... */), // this ends up being a nested array
someThirdPartyInstrumentation,
someOtherThirdPartyInstrumentation,
]
});The fairly simple workaround is to call So I suppose that is fine.
To be honest, I was surprised that passing in just the class was supported at all when I first noticed it a while back. |
|
@trentm I see, I did not consider that. Actually keeping support for this is pretty straightforward so I'd keep it around, but only drop the Since opening the PR I've also seen that we recommend this way in our docs so it's probably better to keep it around.
Yes I'm also surprised that this works. I updated the changelog to reflect that this is not an option anymore. 970e57b |
…en-telemetry#4675) * feat(instrumentation)!: reduce complexity * fixup! feat(instrumentation)!: reduce complexity * fixup! fixup! feat(instrumentation)!: reduce complexity * fixup! fixup! feat(instrumentation)!: reduce complexity * fix: re-add accidenally dropped markdownlint-disable
The
@opentelemetry/instrumentationpackage exports aInstrumentationOptiontype:However,
InstrumentationBaseimplementsInstrumentationalready. Further, this requires extra code to make whats passed toinstrumentationsjust slightly more convenient, for instance it supports:registerInstrumentations({instrumentations: fooInstrumentation})registerInstrumentations({instrumentations: [fooInstrumentation, [barInstrumentation]]})This PR proposes removing this
InstrumentationOptiontype, and replacing all usages ofInstrumentationOptionandInstrumentationOption[]with(Instrumentation | Insturmentation[])[]. This drops support for the first case listed above in favor of always having to provide a list of instrumentations. This simplifies the code in bothsdk-nodeandinstrumentation.