-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
fix(node): Preserve CallbackManager handlers in LangChain instrumentation #20849
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
base: develop
Are you sure you want to change the base?
Changes from all commits
e2b872a
4c74bfa
b33352b
9e2939d
7dcdd93
fb95ad5
5657854
7333d66
d88c503
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -335,7 +335,41 @@ export function setResponseAttributes(span: Span, inputMessages: LangChainMessag | |||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| /** Merge `sentryHandler` into a langchain `callbacks` value (`BaseCallbackHandler[]` or `BaseCallbackManager`). */ | ||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||
| * Detects a LangChain `CallbackManager` (or subclass) without depending on `instanceof`. | ||||||||||||||||||||||||||
| * `@langchain/core` is frequently bundled or deduped, so the imported constructor doesn't | ||||||||||||||||||||||||||
| * necessarily match the one at the user's call site. We walk the prototype chain looking | ||||||||||||||||||||||||||
| * for the class name, then confirm the shape — the constructor-name check rules out | ||||||||||||||||||||||||||
| * unrelated objects that happen to expose `addHandler`/`copy`. | ||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||
| function isCallbackManager(value: unknown): value is { | ||||||||||||||||||||||||||
| addHandler: (handler: unknown, inherit?: boolean) => void; | ||||||||||||||||||||||||||
| copy: () => unknown; | ||||||||||||||||||||||||||
| handlers?: unknown[]; | ||||||||||||||||||||||||||
| } { | ||||||||||||||||||||||||||
| if (!value || typeof value !== 'object') { | ||||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| let proto: object | null = Object.getPrototypeOf(value); | ||||||||||||||||||||||||||
| while (proto) { | ||||||||||||||||||||||||||
| if ((proto as { constructor?: { name?: string } }).constructor?.name === 'CallbackManager') { | ||||||||||||||||||||||||||
| const candidate = value as { addHandler?: unknown; copy?: unknown }; | ||||||||||||||||||||||||||
| return typeof candidate.addHandler === 'function' && typeof candidate.copy === 'function'; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| proto = Object.getPrototypeOf(proto); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||
| * Merge `sentryHandler` into a langchain `callbacks` value (undefined, `BaseCallbackHandler[]`, or `BaseCallbackManager`). | ||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||
| * Wrapping a `CallbackManager` into `[manager, sentryHandler]` would make LangChain treat the whole manager | ||||||||||||||||||||||||||
| * as one opaque handler and drop its inheritable children — notably LangGraph's `StreamMessagesHandler`, | ||||||||||||||||||||||||||
| * which silently breaks per-token streaming. We register on a `.copy()` (so caller state stays clean across | ||||||||||||||||||||||||||
| * runs) and add ourselves as inheritable so `getChild()` propagates us into nested calls. | ||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||
|
Comment on lines
+365
to
+372
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. h: Let's clean this up a bit and mark the function internal.
Suggested change
|
||||||||||||||||||||||||||
| export function mergeSentryCallback(existing: unknown, sentryHandler: unknown): unknown { | ||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. h: Couple of things here:
|
||||||||||||||||||||||||||
| if (!existing) { | ||||||||||||||||||||||||||
| return [sentryHandler]; | ||||||||||||||||||||||||||
|
|
@@ -348,12 +382,15 @@ export function mergeSentryCallback(existing: unknown, sentryHandler: unknown): | |||||||||||||||||||||||||
| return [...existing, sentryHandler]; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| const manager = existing as { addHandler?: (h: unknown) => void; handlers?: unknown[] }; | ||||||||||||||||||||||||||
| if (typeof manager.addHandler === 'function') { | ||||||||||||||||||||||||||
| const alreadyAdded = Array.isArray(manager.handlers) && manager.handlers.includes(sentryHandler); | ||||||||||||||||||||||||||
| if (!alreadyAdded) { | ||||||||||||||||||||||||||
| manager.addHandler(sentryHandler); | ||||||||||||||||||||||||||
| if (isCallbackManager(existing)) { | ||||||||||||||||||||||||||
| const copied = existing.copy() as { | ||||||||||||||||||||||||||
| addHandler: (handler: unknown, inherit?: boolean) => void; | ||||||||||||||||||||||||||
| handlers?: unknown[]; | ||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||
| if (!copied.handlers?.includes(sentryHandler)) { | ||||||||||||||||||||||||||
| copied.addHandler(sentryHandler, true); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| return copied; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| return existing; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -48,6 +48,34 @@ describe('extractAgentNameFromParams', () => { | |||||||||||
| describe('mergeSentryCallback', () => { | ||||||||||||
| const sentryHandler = { _sentry: true }; | ||||||||||||
|
|
||||||||||||
| /** | ||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. m: Let's move all the additions you made to this test file to https://github.com/getsentry/sentry-javascript/blob/develop/packages/core/test/lib/tracing/langchain-utils.test.ts instead. |
||||||||||||
| * Minimal `CallbackManager` stand-in. Mirrors `@langchain/core`'s real | ||||||||||||
| * semantics: `addHandler(_, inherit)` pushes to both `handlers` and | ||||||||||||
| * `inheritableHandlers` when `inherit !== false`, and `copy()` returns | ||||||||||||
| * a fresh manager carrying the same handlers — so we don't accidentally | ||||||||||||
| * test against a degenerate shape that bypasses `addHandler`. | ||||||||||||
| */ | ||||||||||||
| function makeFakeCallbackManager(existingHandlers: unknown[] = [], existingInheritableHandlers?: unknown[]) { | ||||||||||||
| // Use a class so `Object.getPrototypeOf(instance).constructor.name === 'CallbackManager'`, | ||||||||||||
| // which is how the production detector identifies a real LangChain CallbackManager. | ||||||||||||
|
Comment on lines
+59
to
+60
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. m: Not needed once we relax the duck typing again to not look at constructor name.
Suggested change
|
||||||||||||
| class CallbackManager { | ||||||||||||
| public handlers: unknown[]; | ||||||||||||
| public inheritableHandlers: unknown[]; | ||||||||||||
| public addHandler = vi.fn((handler: unknown, inherit?: boolean) => { | ||||||||||||
| this.handlers.push(handler); | ||||||||||||
| if (inherit !== false) { | ||||||||||||
| this.inheritableHandlers.push(handler); | ||||||||||||
| } | ||||||||||||
| }); | ||||||||||||
| public copy = vi.fn(() => makeFakeCallbackManager(this.handlers, this.inheritableHandlers)); | ||||||||||||
| constructor(initialHandlers: unknown[], initialInheritableHandlers: unknown[]) { | ||||||||||||
| this.handlers = [...initialHandlers]; | ||||||||||||
| this.inheritableHandlers = [...initialInheritableHandlers]; | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
| return new CallbackManager(existingHandlers, existingInheritableHandlers ?? existingHandlers); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| it('returns a fresh array when no existing callbacks are present', () => { | ||||||||||||
| expect(mergeSentryCallback(undefined, sentryHandler)).toStrictEqual([sentryHandler]); | ||||||||||||
| expect(mergeSentryCallback(null, sentryHandler)).toStrictEqual([sentryHandler]); | ||||||||||||
|
|
@@ -65,19 +93,81 @@ describe('mergeSentryCallback', () => { | |||||||||||
| expect(mergeSentryCallback(existing, sentryHandler)).toBe(existing); | ||||||||||||
| }); | ||||||||||||
|
|
||||||||||||
| it('calls addHandler on a CallbackManager-like object', () => { | ||||||||||||
| const addHandler = vi.fn(); | ||||||||||||
| const manager = { addHandler, handlers: [] as unknown[] }; | ||||||||||||
| const result = mergeSentryCallback(manager, sentryHandler); | ||||||||||||
| expect(result).toBe(manager); | ||||||||||||
| expect(addHandler).toHaveBeenCalledWith(sentryHandler); | ||||||||||||
| expect(addHandler).toHaveBeenCalledTimes(1); | ||||||||||||
| it('preserves inheritable handlers when callbacks is a CallbackManager', () => { | ||||||||||||
| // Reproduces the LangGraph `streamMode: ['messages']` setup: a | ||||||||||||
| // CallbackManager carrying a StreamMessagesHandler is passed via | ||||||||||||
| // options.callbacks. Wrapping it as `[manager, sentryHandler]` would | ||||||||||||
| // drop the manager's inheritable children — instead we register | ||||||||||||
| // Sentry on a copy and keep the existing handler chain intact. | ||||||||||||
|
Comment on lines
+97
to
+101
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. m: Not needed, the test name describes it sufficiently.
Suggested change
|
||||||||||||
| const streamMessagesHandler = { | ||||||||||||
| name: 'StreamMessagesHandler', | ||||||||||||
| lc_prefer_streaming: true, | ||||||||||||
| }; | ||||||||||||
| const manager = makeFakeCallbackManager([streamMessagesHandler]); | ||||||||||||
| const result = mergeSentryCallback(manager, sentryHandler) as { | ||||||||||||
| handlers: unknown[]; | ||||||||||||
| }; | ||||||||||||
| expect(Array.isArray(result)).toBe(false); | ||||||||||||
| expect(result.handlers).toEqual([streamMessagesHandler, sentryHandler]); | ||||||||||||
| }); | ||||||||||||
|
|
||||||||||||
| it('copies the manager and registers Sentry as an inheritable handler', () => { | ||||||||||||
| // Two adjacent contracts: we operate on a copy (so repeat invocations | ||||||||||||
| // don't accumulate handlers on the caller), and we pass `inherit=true` | ||||||||||||
| // so LangChain's `getChild()` propagates Sentry into nested calls. | ||||||||||||
| const manager = makeFakeCallbackManager([]); | ||||||||||||
| const result = mergeSentryCallback(manager, sentryHandler) as { | ||||||||||||
| addHandler: ReturnType<typeof vi.fn>; | ||||||||||||
| inheritableHandlers: unknown[]; | ||||||||||||
| }; | ||||||||||||
| expect(manager.copy).toHaveBeenCalledTimes(1); | ||||||||||||
| expect(manager.handlers).toEqual([]); | ||||||||||||
| expect(result.addHandler).toHaveBeenCalledWith(sentryHandler, true); | ||||||||||||
| expect(result.inheritableHandlers).toEqual([sentryHandler]); | ||||||||||||
| }); | ||||||||||||
|
|
||||||||||||
| it('does not double-register when the copied manager already contains the handler', () => { | ||||||||||||
| const manager = makeFakeCallbackManager([sentryHandler]); | ||||||||||||
| const result = mergeSentryCallback(manager, sentryHandler) as { | ||||||||||||
| handlers: unknown[]; | ||||||||||||
| addHandler: ReturnType<typeof vi.fn>; | ||||||||||||
| }; | ||||||||||||
| expect(result.handlers).toEqual([sentryHandler]); | ||||||||||||
| expect(result.addHandler).not.toHaveBeenCalled(); | ||||||||||||
| }); | ||||||||||||
|
|
||||||||||||
| it('returns the value unchanged when it is neither an array nor a CallbackManager', () => { | ||||||||||||
| const opaque = { name: 'NotAManager' }; | ||||||||||||
| expect(mergeSentryCallback(opaque, sentryHandler)).toBe(opaque); | ||||||||||||
| }); | ||||||||||||
|
|
||||||||||||
| it('does not treat a coincidentally duck-typed object as a CallbackManager', () => { | ||||||||||||
| // A plain object that happens to expose `addHandler`/`copy` shouldn't be | ||||||||||||
| // mistaken for a real LangChain CallbackManager — the constructor-name | ||||||||||||
| // check guards against false positives. | ||||||||||||
| const lookalike = { addHandler: vi.fn(), copy: vi.fn(), handlers: [] }; | ||||||||||||
| expect(mergeSentryCallback(lookalike, sentryHandler)).toBe(lookalike); | ||||||||||||
| expect(lookalike.addHandler).not.toHaveBeenCalled(); | ||||||||||||
| expect(lookalike.copy).not.toHaveBeenCalled(); | ||||||||||||
| }); | ||||||||||||
|
|
||||||||||||
| it('does not re-add when the manager already has the sentry handler', () => { | ||||||||||||
| const addHandler = vi.fn(); | ||||||||||||
| const manager = { addHandler, handlers: [sentryHandler] }; | ||||||||||||
| mergeSentryCallback(manager, sentryHandler); | ||||||||||||
| expect(addHandler).not.toHaveBeenCalled(); | ||||||||||||
| it('recognizes subclasses of CallbackManager via the prototype walk', () => { | ||||||||||||
| class CallbackManager { | ||||||||||||
| public handlers: unknown[] = []; | ||||||||||||
| public inheritableHandlers: unknown[] = []; | ||||||||||||
| public addHandler = vi.fn((handler: unknown, inherit?: boolean) => { | ||||||||||||
| this.handlers.push(handler); | ||||||||||||
| if (inherit !== false) { | ||||||||||||
| this.inheritableHandlers.push(handler); | ||||||||||||
| } | ||||||||||||
| }); | ||||||||||||
| public copy = vi.fn(() => new CallbackManager()); | ||||||||||||
| } | ||||||||||||
| class CustomCallbackManager extends CallbackManager {} | ||||||||||||
| const subclass = new CustomCallbackManager(); | ||||||||||||
| const result = mergeSentryCallback(subclass, sentryHandler) as { | ||||||||||||
| addHandler: ReturnType<typeof vi.fn>; | ||||||||||||
| }; | ||||||||||||
| expect(result.addHandler).toHaveBeenCalledWith(sentryHandler, true); | ||||||||||||
| }); | ||||||||||||
| }); | ||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,7 @@ import { | |
| createLangChainCallbackHandler, | ||
| GOOGLE_GENAI_INTEGRATION_NAME, | ||
| instrumentLangChainEmbeddings, | ||
| mergeSentryCallback, | ||
| OPENAI_INTEGRATION_NAME, | ||
| SDK_VERSION, | ||
| } from '@sentry/core'; | ||
|
|
@@ -27,34 +28,6 @@ interface PatchedLangChainExports { | |
| [key: string]: unknown; | ||
| } | ||
|
|
||
| /** | ||
| * Augments a callback handler list with Sentry's handler if not already present | ||
| */ | ||
| function augmentCallbackHandlers(handlers: unknown, sentryHandler: unknown): unknown { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. l: I wonder if this was missed to be removed when
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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).
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @JPeer264 nope, |
||
| // Handle null/undefined - return array with just our handler | ||
| if (!handlers) { | ||
| return [sentryHandler]; | ||
| } | ||
|
|
||
| // If handlers is already an array | ||
| if (Array.isArray(handlers)) { | ||
| // Check if our handler is already in the list | ||
| if (handlers.includes(sentryHandler)) { | ||
| return handlers; | ||
| } | ||
| // Add our handler to the list | ||
| return [...handlers, sentryHandler]; | ||
| } | ||
|
|
||
| // If it's a single handler object, convert to array | ||
| if (typeof handlers === 'object') { | ||
| return [handlers, sentryHandler]; | ||
| } | ||
|
|
||
| // Unknown type - return original | ||
| return handlers; | ||
| } | ||
|
|
||
| /** | ||
| * Wraps Runnable methods (invoke, stream, batch) to inject Sentry callbacks at request time | ||
| * Uses a Proxy to intercept method calls and augment the options.callbacks | ||
|
|
@@ -82,9 +55,7 @@ function wrapRunnableMethod( | |
| } | ||
|
|
||
| // Inject our callback handler into options.callbacks (request time callbacks) | ||
| const existingCallbacks = options.callbacks; | ||
| const augmentedCallbacks = augmentCallbackHandlers(existingCallbacks, sentryHandler); | ||
| options.callbacks = augmentedCallbacks; | ||
| options.callbacks = mergeSentryCallback(options.callbacks, sentryHandler); | ||
|
|
||
| // Call original method with augmented options | ||
| return Reflect.apply(target, thisArg, args); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: The
isCallbackManagercheck usesconstructor.name, which fails when minified, causing Sentry tracing handlers to be silently dropped in bundled environments.Severity: HIGH
Suggested Fix
Replace the string comparison on
constructor.namewith a more robust type-checking mechanism that is resilient to minification. For example, add a unique, non-enumerable property to theCallbackManagerclass prototype (e.g.,_isCallbackManager = true) and check for its presence instead. This provides a reliableinstanceof-like check that will not break during bundling.Prompt for AI Agent