fix(node): Preserve CallbackManager handlers in LangChain instrumentation#20849
fix(node): Preserve CallbackManager handlers in LangChain instrumentation#20849mdnanocom wants to merge 8 commits into
Conversation
…tion
`augmentCallbackHandlers` previously wrapped a `CallbackManager` instance
into `[manager, sentryHandler]` whenever `options.callbacks` was a single
object. LangChain downstream then treats the whole manager as one opaque
handler — its inheritable children (notably LangGraph's
`StreamMessagesHandler` installed by `streamMode: ['messages']`, plus the
LangSmith tracer) are never unpacked, so per-token streaming events and
nested tracing silently disappear.
Detect `CallbackManager` via duck-typing (avoids coupling to a specific
`@langchain/core` resolution) and register Sentry's handler as an
inheritable child on a copy, so the manager's existing handlers continue
to receive `handleLLMNewToken` and friends.
Repro: LangGraph compiled graph + `ChatOpenAI` (or any provider with
`bindTools(...)`), `graph.stream(..., { streamMode: ['values','messages'] })`
piped through `@ai-sdk/langchain`'s `toUIMessageStream`. Without the fix,
the SSE output collapses to a single aggregated `text-delta`. With the
fix, every token is delivered as the model produces it.
92409bd to
4c74bfa
Compare
In practice CallbackManager keeps `inheritableHandlers ⊆ handlers` (both `addHandler` and `setHandlers` maintain the invariant), so checking `handlers` alone suffices. But an externally-constructed manager subclass could in theory leak the Sentry handler onto `inheritableHandlers` without mirroring it. Checking both lists costs nothing and pre-empts the duplicate-span class of bug Sentry's own reviewer flagged.
|
Thanks for the review — wanted to share what I found before pushing the fix. Tracing through
That means in normal operation That said, the suggested fix costs nothing and pre-empts the failure mode for any externally-constructed subclass that bypasses |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b33352b. Configure here.
|
Done in 7d61cf5b (pushed). Lifted the fixed implementation to Net: 5 files, +163 / -215. Coverage is now in a single |
b2b6b6c to
0c2c666
Compare
…ryCallback
The langchain instrumentation's `augmentCallbackHandlers` and the
langgraph instrumentation's `mergeSentryCallback` solved the same
problem two different ways. The langgraph helper had three latent
bugs that this PR's fix already covered for langchain:
- mutated the caller's CallbackManager (handlers accumulate across runs)
- called `addHandler(handler)` without `inherit=true`, so the handler
never propagated to child managers created by `getChild`
- deduped only against `manager.handlers`, not `inheritableHandlers`
Lift the fixed implementation up to `@sentry/core` so both
instrumentations share it. The langgraph integration silently picks
up the streaming fix as a bonus. Drops the duplicate `augmentCallback`
helper and its test; behavior is covered by the expanded
`mergeSentryCallback` suite (14 cases).
0c2c666 to
9e2939d
Compare
| return false; | ||
| } | ||
| const candidate = value as { addHandler?: unknown; copy?: unknown }; | ||
| return typeof candidate.addHandler === 'function' && typeof candidate.copy === 'function'; |
There was a problem hiding this comment.
m: Is it 100% guaranteed that this works? addHandler and copy seem to be a little vague to check against these. Since CallbackManager is a class, maybe there is the possibility to check against its name?
We do something similar in Cloudflare:
sentry-javascript/packages/cloudflare/src/utils/isCloudflareClass.ts
Lines 38 to 61 in 62096ee
There was a problem hiding this comment.
Done, adopted the Cloudflare pattern: walk the prototype chain looking for constructor.name === 'CallbackManager', then confirm the shape.
| /** | ||
| * Augments a callback handler list with Sentry's handler if not already present | ||
| */ | ||
| function augmentCallbackHandlers(handlers: unknown, sentryHandler: unknown): unknown { |
There was a problem hiding this comment.
l: I wonder if this was missed to be removed when mergeSentryCallback got added. @andreiborza any insights on this?
There was a problem hiding this comment.
Not dead — they cover different entry points:
wrapRunnableMethod patches chat-model prototypes (ChatOpenAI, ChatAnthropic, …) so model.invoke/stream/batch calls merge the Sentry handler. This is the bare-LangChain path (no graph involved).
The mergeSentryCallback call site in tracing/langgraph/index.ts patches StateGraph.compile() and merges at graph-entry — also tagging metadata with sentry_langgraph.
Inside a graph the two overlap (chat-model invokes also flow through wrapRunnableMethod), but for users on LangChain-only there's no graph patch to fall back on. Both call sites now share the same helper, which is what this PR consolidates.
Address review feedback on PR getsentry#20849: - `isCallbackManager`: in addition to duck-typing `addHandler`/`copy`, walk the prototype chain and require `constructor.name === 'CallbackManager'` (mirroring `packages/cloudflare/src/utils/isCloudflareClass.ts`). Filters out unrelated objects that coincidentally expose the same shape; the prototype walk keeps subclasses working. - Drop the changelog entry — release process generates the changelog manually. Two new test cases lock the behavior in: rejects lookalike duck-typed objects, and recognizes subclasses via the prototype walk (16/16).
Address review feedback on PR getsentry#20849: - `isCallbackManager`: in addition to duck-typing `addHandler`/`copy`, walk the prototype chain and require `constructor.name === 'CallbackManager'` (mirroring `packages/cloudflare/src/utils/isCloudflareClass.ts`). Filters out unrelated objects that coincidentally expose the same shape; the prototype walk keeps subclasses working. - Drop the changelog entry — release process generates the changelog manually. Two new test cases lock the behavior in: rejects lookalike duck-typed objects, and recognizes subclasses via the prototype walk (16/16).
9f0dbc5 to
7dcdd93
Compare

Summary
augmentCallbackHandlersinpackages/node/src/integrations/tracing/langchain/instrumentation.tspreviously fell into itstypeof handlers === 'object'branch wheneveroptions.callbackswas a single object and wrapped it into[handlers, sentryHandler]. When that object was a LangChainCallbackManager, downstream code treats the whole manager as one opaque handler — its inheritable children (notably LangGraph'sStreamMessagesHandlerinstalled bystreamMode: ['messages'], and the LangSmith tracer) are never unpacked, so per-token streaming events and nested tracing silently disappear.This PR detects
CallbackManagervia duck-typing (avoids coupling to a specific@langchain/coreresolution) and registers Sentry's handler as an inheritable child on a copy via.addHandler(handler, true). The manager's existing children continue to receivehandleLLMNewTokenand friends.Repro
A LangGraph compiled graph +
ChatOpenAI(or any LangChain provider withbindTools(...)), driven through@ai-sdk/langchain'stoUIMessageStream:text-deltaat end of run. Probing inside_streamResponseChunksshows the per-token chunks firerunManager.handleLLMNewTokencorrectly, but the LLM-level run-manager's handlers list contains only[CallbackManager, SentryCallbackHandler, langchain_tracer]—StreamMessagesHandleris missing.gpt-5.5+useResponsesApi: true+bindToolsand againstgpt-4o-miniwith no tools).Tests
Added
packages/node/test/integrations/tracing/langchain.test.tswith 7 unit tests covering:undefinedcallbacks → single-handler arrayCallbackManager→ preserves children, copies rather than mutates, deduplicatesVerify
yarn buildsucceedsyarn test:unitpasses (381/381 inpackages/node)