-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(core): Prevent instrumentAnthropicAiClient
breaking MessageStream api
#17754
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
Changes from all commits
b33939e
bf90b41
23dae8b
8267cfb
35a1a02
2c68a7c
894aa11
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 |
---|---|---|
|
@@ -25,7 +25,7 @@ import { | |
} from '../ai/gen-ai-attributes'; | ||
import { buildMethodPath, getFinalOperationName, getSpanOperation, setTokenUsageAttributes } from '../ai/utils'; | ||
import { handleCallbackErrors } from '../handleCallbackErrors'; | ||
import { instrumentStream } from './streaming'; | ||
import { instrumentAsyncIterableStream, instrumentMessageStream } from './streaming'; | ||
import type { | ||
AnthropicAiInstrumentedMethod, | ||
AnthropicAiOptions, | ||
|
@@ -194,6 +194,74 @@ function addResponseAttributes(span: Span, response: AnthropicAiResponse, record | |
addMetadataAttributes(span, response); | ||
} | ||
|
||
/** | ||
* Handle common error catching and reporting for streaming requests | ||
*/ | ||
function handleStreamingError(error: unknown, span: Span, methodPath: string): never { | ||
captureException(error, { | ||
mechanism: { handled: false, type: 'auto.ai.anthropic', data: { function: methodPath } }, | ||
}); | ||
|
||
if (span.isRecording()) { | ||
span.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' }); | ||
span.end(); | ||
} | ||
throw error; | ||
} | ||
|
||
/** | ||
* Handle streaming cases with common logic | ||
*/ | ||
function handleStreamingRequest<T extends unknown[], R>( | ||
originalMethod: (...args: T) => Promise<R>, | ||
target: (...args: T) => Promise<R>, | ||
context: unknown, | ||
args: T, | ||
requestAttributes: Record<string, unknown>, | ||
operationName: string, | ||
methodPath: string, | ||
params: Record<string, unknown> | undefined, | ||
options: AnthropicAiOptions, | ||
isStreamRequested: boolean, | ||
): Promise<R> { | ||
const model = requestAttributes[GEN_AI_REQUEST_MODEL_ATTRIBUTE] ?? 'unknown'; | ||
const spanConfig = { | ||
name: `${operationName} ${model} stream-response`, | ||
op: getSpanOperation(methodPath), | ||
attributes: requestAttributes as Record<string, SpanAttributeValue>, | ||
}; | ||
|
||
if (isStreamRequested) { | ||
return startSpanManual(spanConfig, async span => { | ||
try { | ||
if (options.recordInputs && params) { | ||
addPrivateRequestAttributes(span, params); | ||
} | ||
const result = await originalMethod.apply(context, args); | ||
return instrumentAsyncIterableStream( | ||
result as AsyncIterable<AnthropicAiStreamingEvent>, | ||
span, | ||
options.recordOutputs ?? false, | ||
) as unknown as R; | ||
} catch (error) { | ||
return handleStreamingError(error, span, methodPath); | ||
} | ||
}); | ||
} else { | ||
return startSpanManual(spanConfig, span => { | ||
try { | ||
if (options.recordInputs && params) { | ||
addPrivateRequestAttributes(span, params); | ||
} | ||
const messageStream = target.apply(context, args); | ||
return instrumentMessageStream(messageStream, span, options.recordOutputs ?? false); | ||
} catch (error) { | ||
return handleStreamingError(error, span, methodPath); | ||
} | ||
}); | ||
} | ||
This comment was marked as outdated.
Sorry, something went wrong. 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. Bug: Promise Handling and Context Propagation IssuesFor |
||
} | ||
|
||
/** | ||
* Instrument a method with Sentry spans | ||
* Following Sentry AI Agents Manual Instrumentation conventions | ||
|
@@ -205,82 +273,62 @@ function instrumentMethod<T extends unknown[], R>( | |
context: unknown, | ||
options: AnthropicAiOptions, | ||
): (...args: T) => Promise<R> { | ||
return async function instrumentedMethod(...args: T): Promise<R> { | ||
const requestAttributes = extractRequestAttributes(args, methodPath); | ||
const model = requestAttributes[GEN_AI_REQUEST_MODEL_ATTRIBUTE] ?? 'unknown'; | ||
const operationName = getFinalOperationName(methodPath); | ||
return new Proxy(originalMethod, { | ||
apply(target, thisArg, args: T): Promise<R> { | ||
const requestAttributes = extractRequestAttributes(args, methodPath); | ||
const model = requestAttributes[GEN_AI_REQUEST_MODEL_ATTRIBUTE] ?? 'unknown'; | ||
const operationName = getFinalOperationName(methodPath); | ||
|
||
const params = typeof args[0] === 'object' ? (args[0] as Record<string, unknown>) : undefined; | ||
const isStreamRequested = Boolean(params?.stream); | ||
const isStreamingMethod = methodPath === 'messages.stream'; | ||
const params = typeof args[0] === 'object' ? (args[0] as Record<string, unknown>) : undefined; | ||
const isStreamRequested = Boolean(params?.stream); | ||
const isStreamingMethod = methodPath === 'messages.stream'; | ||
|
||
if (isStreamRequested || isStreamingMethod) { | ||
return startSpanManual( | ||
if (isStreamRequested || isStreamingMethod) { | ||
return handleStreamingRequest( | ||
originalMethod, | ||
target, | ||
context, | ||
args, | ||
requestAttributes, | ||
operationName, | ||
methodPath, | ||
params, | ||
options, | ||
isStreamRequested, | ||
); | ||
} | ||
|
||
return startSpan( | ||
{ | ||
name: `${operationName} ${model} stream-response`, | ||
name: `${operationName} ${model}`, | ||
op: getSpanOperation(methodPath), | ||
attributes: requestAttributes as Record<string, SpanAttributeValue>, | ||
}, | ||
async span => { | ||
try { | ||
if (options.recordInputs && params) { | ||
addPrivateRequestAttributes(span, params); | ||
} | ||
|
||
const result = await originalMethod.apply(context, args); | ||
return instrumentStream( | ||
result as AsyncIterable<AnthropicAiStreamingEvent>, | ||
span, | ||
options.recordOutputs ?? false, | ||
) as unknown as R; | ||
} catch (error) { | ||
span.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' }); | ||
captureException(error, { | ||
mechanism: { | ||
handled: false, | ||
type: 'auto.ai.anthropic', | ||
data: { | ||
function: methodPath, | ||
}, | ||
}, | ||
}); | ||
span.end(); | ||
throw error; | ||
span => { | ||
if (options.recordInputs && params) { | ||
addPrivateRequestAttributes(span, params); | ||
} | ||
}, | ||
); | ||
} | ||
|
||
return startSpan( | ||
{ | ||
name: `${operationName} ${model}`, | ||
op: getSpanOperation(methodPath), | ||
attributes: requestAttributes as Record<string, SpanAttributeValue>, | ||
}, | ||
span => { | ||
if (options.recordInputs && params) { | ||
addPrivateRequestAttributes(span, params); | ||
} | ||
|
||
return handleCallbackErrors( | ||
() => originalMethod.apply(context, args), | ||
error => { | ||
captureException(error, { | ||
mechanism: { | ||
handled: false, | ||
type: 'auto.ai.anthropic', | ||
data: { | ||
function: methodPath, | ||
return handleCallbackErrors( | ||
() => target.apply(context, args), | ||
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. 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. sounds reasonable I guess, or do we bind this to context on purpose here? 🤔 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. I'm not entirely sure tbh, Rola did this initially and tests seem to pass. I can try changing it but didn't want to introduce another potential problem in this PR. |
||
error => { | ||
captureException(error, { | ||
mechanism: { | ||
handled: false, | ||
type: 'auto.ai.anthropic', | ||
data: { | ||
function: methodPath, | ||
}, | ||
}, | ||
}, | ||
}); | ||
}, | ||
() => {}, | ||
result => addResponseAttributes(span, result as AnthropicAiResponse, options.recordOutputs), | ||
); | ||
}, | ||
); | ||
}; | ||
}); | ||
}, | ||
() => {}, | ||
result => addResponseAttributes(span, result as AnthropicAiResponse, options.recordOutputs), | ||
); | ||
}, | ||
); | ||
}, | ||
}) as (...args: T) => Promise<R>; | ||
} | ||
|
||
/** | ||
|
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: Stream Attribute Mismatch in Test
The test for the
messages.stream
span incorrectly expectsgen_ai.request.stream: true
. This attribute is only set when an explicitstream
parameter is passed, whichmessages.stream
does not use as it's inherently streaming. The instrumentation correctly reflects this by omitting the attribute formessages.stream
calls.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.
Not true, we add this in both cases.