chore(otel): make otel api a peer dependency - #6914
Conversation
f79a175 to
2ba638d
Compare
|
Closed #6915. This is the right change |
BridgeAR
left a comment
BridgeAR
left a comment
There was a problem hiding this comment.
This will need the yarn lock file being updated (just running yarn is enough).
BenchmarksBenchmark execution time: 2026-04-16 17:49:36 Comparing candidate commit 0633b0e in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 1379 metrics, 105 unstable metrics. |
Overall package sizeSelf size: 5.5 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | import-in-the-middle | 3.0.1 | 82.56 kB | 817.39 kB | | dc-polyfill | 0.1.10 | 26.73 kB | 26.73 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #6914 +/- ##
==========================================
- Coverage 73.59% 73.59% -0.01%
==========================================
Files 773 774 +1
Lines 36083 36105 +22
==========================================
+ Hits 26557 26573 +16
- Misses 9526 9532 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
✨ Fix all issues with BitsAI or with Cursor
|
57e5122 to
0162595
Compare
mabdinur
changed the title
055922e to
09caa3c
Compare
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
10423e0 to
b63fbec
Compare
BridgeAR
left a comment
BridgeAR
left a comment
There was a problem hiding this comment.
This is looking very good! Just the exc-parser needs to be fixed and the linter for the license. The version should ideally be open ended for testing and if possible, we could find a way to not have the noop provider. Having the noop one will likely otherwise cause problems at some point if the APIs deviate / expectations are not met.
| const noopSpan = { | ||
| spanContext: () => ({}), | ||
| setAttribute: () => {}, | ||
| setAttributes: () => {}, | ||
| addEvent: () => {}, | ||
| updateName: () => {}, | ||
| setStatus: () => {}, | ||
| end: () => {}, | ||
| isRecording: () => false, | ||
| } |
There was a problem hiding this comment.
Would it be possible to prevent creating the noop tracer provider somehow so that this will just never be accessed anywhere in case it is not there?
That would be nicer, since the noop one otherwise needs to stay in sync and that is likely to deviate soon.
There was a problem hiding this comment.
Yup, we can raise an error if TracerProvider is accessed when one isn't registered. A noop TracerProvider can allow tracing to fail silently. It better if we are noisy about it.
Co-authored-by: Ruben Bridgewater <ruben@bridgewater.de>
Co-authored-by: Ruben Bridgewater <ruben@bridgewater.de>
| const NoopProxy = proxyquire.noPreserveCache()('../../src/noop/proxy', { | ||
| '../opentelemetry/check_peer_deps': noopCheckPeerDeps, | ||
| }) | ||
| const getter = Object.getOwnPropertyDescriptor(NoopProxy.prototype, 'TracerProvider').get | ||
| const NoopClass = getter.call({}) | ||
| assert.throws(() => new NoopClass(), { message: expectedMessage }) |
There was a problem hiding this comment.
Could you please change these tests to ones how a user could actually reach this point? These are directly calling in internals in a way that a user would not be. With that, we are uncertain if this is indeed the triggered behavior.
| return class { | ||
| constructor () { | ||
| throw new Error( | ||
| '@opentelemetry/api is required to use TracerProvider. ' + | ||
| 'Install it with: npm install @opentelemetry/api' | ||
| ) | ||
| } |
There was a problem hiding this comment.
I like this, it is a neat way of doing this! I am just unsure if we really want to throw or if logging a warning here would be best instead and to gracefully handle it otherwise. I am unsure, if that is an option. Handling it gracefully would probably require the noop tracer again?
I would have to know how the user could reach this to decide on what to do.
There was a problem hiding this comment.
The main documented usage is
Line 391 in de1327b
const tracer = require('dd-trace').init()
const tracerProvider = new tracer.TracerProvider()
tracerProvider.register()
IMO we should deprecate this. Instead, dd-trace should auto-register its TracerProvider during .init() when @opentelemetry/api is present (similar to how we handle otel logs and metrics). Then users could then just do:
require('dd-trace').init()
// dd-trace auto-registers — use the standard OTel API from here
const { trace } = require("@opentelemetry/api");
const provider = trace.getTracerProvider()
This keeps dd-trace's OTel surface minimal and aligns with how the OTel ecosystem expects providers to be set up.
We can do this in a future PR
There was a problem hiding this comment.
Conceptually, I like that. The question I have is: how can we do that without providing loading overhead for people not using Otel? I guess it could work as a hook: the hook is our activation trigger. @rochdev what do you think?
That aside: what shall we do short term with this? Our tracer should normally handle things gracefully and this would not be the case anymore. I think this is fine as a major version, I am not yet sure about before. @rochdev @bengl opinions?
We would have to document that change though.
|
Closing to reduce noise. Will reopen when this change is in a better place |
3 participants
Resolves: #6882
What does this PR do?
Converts
@opentelemetry/apiand@opentelemetry/api-logsfrom regular dependencies to peer dependencies, and bumps the tested OpenTelemetry API version from v1.8.0 to v1.9.0.Motivation
When users install a different version of
@opentelemetry/apithan what dd-trace bundles, npm can create two separate instances of the module. This breaks the OpenTelemetry singleton pattern - dd-trace sets the tracer/logger provider delegate on one instance while user code imports from the other. The result is custom spans becoming no-ops with all-zero trace IDs and log records not being capturedMaking these peer dependencies ensures only one instance exists in the dependency tree, which is the standard pattern for singleton modules like OpenTelemetry API.
Risk
Converting Opentelemetry API has the potential to break applications that expect the dd-trace-js to bundle opentelemetry interfaces. This could be considered a breaking change.
Additional Notes