fix(ai): replace Object.create(span) with delegating wrapper to preserve OTel span private-field access#9152
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d190a9a806
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| shimmer.wrap(freshSpan, 'recordException', function (recordException) { | ||
| return function (exception) { | ||
| return span.setAttributes(attributes) |
There was a problem hiding this comment.
Preserve chainable span methods on wrapper
When a wrapped tracer callback uses the OTel chainable API, e.g. span.setAttributes(attrs).end(), this returns the underlying span instead of freshSpan, so the subsequent end() runs on the unwrapped span and skips the dd-trace:vercel-ai asyncEnd publication. The old prototype clone kept chains on the wrapper for span implementations that return this; the delegating wrapper should return freshSpan after forwarding chainable calls such as setAttribute, setAttributes, addEvent, addLink(s), setStatus, and updateName.
Useful? React with 👍 / 👎.
sabrenner
left a comment
There was a problem hiding this comment.
hi @aperezfals, thanks for the contribution & resolution to the issue you identified! i left one comment related to testing this.
but additionally, another thought - i think you should not need to rely on both our integration and the underlying OTel interface/integration when using the Datadog OTel trace provider, but rather one or the other. so, from your issue, using either
import tracer from 'dd-trace'
tracer.init(...)
import { generateText } from 'ai'
await generateText({ ... }) // leave out experimental telemetry and tracer as the DD instrumentation will handle it properlyor
export DD_TRACE_DISABLED_INSTRUMENTATIONS=aiimport tracer from 'dd-trace'
tracer.init(...)
new tracer.TracerProvider().register()
import { generateText } from 'ai'
await generateText({ ..., experimental_telemetry: { isEnabled: true, tracer: trace.getTracer('ai') } }) // leave out experimental telemetry and tracer as the DD instrumentation will handle it properlybut not both. let me know if migrating to one of these scenarios (either relying on the DD instrumentation but not OTel trace provider, or not relying on the DD instrumentation but on the OTel trace provider) helps in your case without this fix.
| } | ||
|
|
||
| module.exports = { wrapModelWithLifecycle } | ||
| module.exports = { wrapModelWithLifecycle, wrapTracer } |
There was a problem hiding this comment.
i prefer we do not export the wrapper function here (it's a practice in this repo we generally try not to follow, wrapModelWithLifecycle was an exception). as an alternative, to test it, let's instead add a test in this test file that properly reproduces the issue as you were experiencing it, for a more true end-to-end regression test, is possible.
Hi @sabrenner, thanks for looking at this and for the thoughtful suggestion! I want to clarify why I think the fix is still needed even setting aside the "dual path" scenario. The bug actually reproduces with a single path through dd-trace — no explicit const tracer = require('dd-trace').init({ ... })
tracer.use('ai', { enabled: true }) // DD ai instrumentation active
new tracer.TracerProvider().register() // DD registered as global OTel providerawait generateText({ model, messages }) // no experimental_telemetry at allHere's the sequence that leads to the crash:
So the issue is not about using two overlapping export paths — it's about two dd-trace subsystems (the ai instrumentation and the OTel bridge) being incompatible with each other when used together. The ai instrumentation forces OTel telemetry on even when the user doesn't configure it explicitly, and if DD is the global OTel provider, that's enough to hit the crash. Your Option A avoids the crash because it skips registering the OTel provider (so The |
f7b1af9 to
99b5672
Compare
sabrenner
left a comment
There was a problem hiding this comment.
it's about two dd-trace subsystems (the ai instrumentation and the OTel bridge) being incompatible with each other when used together
i think this is a valid concern. i would say it might make sense to still disable the dd-trace AI instrumentation in this case and use the OTel instrumentation from the vercel AI sdk, but for the cases where you explicitly want to use the DD instrumentation while still registering it as the global OTel provider, this change makes sense.
i'll get CI run on this PR, and if it all looks good, we'll get this landed!
4779546 to
407f413
Compare
|
🔄 Datadog auto-retried 1 job - 1 passed on retry 🔗 Commit SHA: fcb83e9 | Docs | Datadog PR Page | Give us feedback! |
| // "Cannot read private member from an object whose class did not declare it". | ||
| // The delegating wrapper must call through to the real span instance to avoid this. | ||
| class PrivateFieldSpan { | ||
| #statusCode = 0 |
There was a problem hiding this comment.
feel free to add an eslint ignore to this line, the test here is that it's able to be accessed to set, which is done below, it's just never actually used, which is fine for the point of this test
|
@sabrenner can you run CI again? |
…rve OTel span private-field access
…rve OTel span private-field access and chaining
…rve OTel span private-field access
…rve OTel span private-field access and chaining
6f5d0b2 to
fcb83e9
Compare
|
Hey @aperezfals, I've gone ahead and re-run the CI + merged since everything looks good :) Thanks again for the contribution! |
…rve OTel span private-field access (#9152) * fix(ai): replace Object.create(span) with delegating wrapper to preserve OTel span private-field access * fix(ai): replace Object.create(span) with delegating wrapper to preserve OTel span private-field access and chaining * test(ai): replace exported wrapTracer unit tests with E2E regression in index.spec.js * fix(ai): replace Object.create(span) with delegating wrapper to preserve OTel span private-field access * fix(ai): replace Object.create(span) with delegating wrapper to preserve OTel span private-field access and chaining * test(ai): replace exported wrapTracer unit tests with E2E regression in index.spec.js * test(ai): eslint ignore non used statusCode var
…rve OTel span private-field access (#9152) * fix(ai): replace Object.create(span) with delegating wrapper to preserve OTel span private-field access * fix(ai): replace Object.create(span) with delegating wrapper to preserve OTel span private-field access and chaining * test(ai): replace exported wrapTracer unit tests with E2E regression in index.spec.js * fix(ai): replace Object.create(span) with delegating wrapper to preserve OTel span private-field access * fix(ai): replace Object.create(span) with delegating wrapper to preserve OTel span private-field access and chaining * test(ai): replace exported wrapTracer unit tests with E2E regression in index.spec.js * test(ai): eslint ignore non used statusCode var
…rve OTel span private-field access (#9152) * fix(ai): replace Object.create(span) with delegating wrapper to preserve OTel span private-field access * fix(ai): replace Object.create(span) with delegating wrapper to preserve OTel span private-field access and chaining * test(ai): replace exported wrapTracer unit tests with E2E regression in index.spec.js * fix(ai): replace Object.create(span) with delegating wrapper to preserve OTel span private-field access * fix(ai): replace Object.create(span) with delegating wrapper to preserve OTel span private-field access and chaining * test(ai): replace exported wrapTracer unit tests with E2E regression in index.spec.js * test(ai): eslint ignore non used statusCode var
…rve OTel span private-field access (#9152) * fix(ai): replace Object.create(span) with delegating wrapper to preserve OTel span private-field access * fix(ai): replace Object.create(span) with delegating wrapper to preserve OTel span private-field access and chaining * test(ai): replace exported wrapTracer unit tests with E2E regression in index.spec.js * fix(ai): replace Object.create(span) with delegating wrapper to preserve OTel span private-field access * fix(ai): replace Object.create(span) with delegating wrapper to preserve OTel span private-field access and chaining * test(ai): replace exported wrapTracer unit tests with E2E regression in index.spec.js * test(ai): eslint ignore non used statusCode var
What does this PR do?
Replaces the Object.create(span) prototype-clone pattern in wrapTracer (in packages/datadog-instrumentations/src/ai.js) with an explicit delegating wrapper object. The wrapper implements the full OTel Span interface and forwards every method call directly to the real span instance, overriding only end, setAttributes, and recordException to publish to diagnostic channels before delegating.
Motivation
When dd-trace is registered as the global OTel TracerProvider, the spans passed to startActiveSpan callbacks are BridgeSpanBase instances, which store span status in the private class field #statusCode. Object.create(span) produces a prototype-chain clone that fails JavaScript's private-field brand check — so any call to setStatus() on the clone throws:
TypeError: Cannot read private member #statusCode from an object whose class did not declare it
The AI SDK calls setStatus({ code: ERROR }) only on its error path, so this TypeError is thrown while handling another error, masking the original failure entirely. All the user sees is the cryptic #statusCode message.
new Proxy(span) has the same limitation — there is no prototype-chain-based approach that can cross a private-field brand check. The fix delegates every method call to span directly, so this === span inside every implementation and private-field access always resolves on the real instance. This works for any span implementation regardless of what private fields it uses now or in the future.
Side benefit: resolves the existing // TODO: does this cause memory leaks? comment — the new wrapper is a plain object with no extra references and is eligible for GC as soon as the callback returns.
Fixes #9151.
Additional Notes
wrapTracer is now exported alongside wrapModelWithLifecycle so the regression test can call it directly without going through the full orchestrion channel setup.
The regression test (describe('wrapTracer') in packages/datadog-instrumentations/test/ai.spec.js) creates a PrivateFieldSpan class with a private #statusCode field — the same pattern as BridgeSpanBase — and asserts that setStatus() on the wrapped span does not throw.
new Proxy(span) was considered and ruled out: it has the same brand-check limitation as Object.create.