-
-
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
Conversation
size-limit report 📦
|
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
if (isStreamRequested || isStreamingMethod) { | ||
return startSpanManual( | ||
if (isStreamRequested || isStreamingMethod) { | ||
const messageStream = target.apply(context, 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.
m: I think (this is also what cursor is raising I believe) that this should move into the startSpanManual
callback, so this is properly encapsulated by that span.
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.
Ah yea, this should move down. Thanks.
}, | ||
}, | ||
}); | ||
span.end(); |
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.
l/m: We should check if the span is not yet ended here, because it could be ended already from the main code path and throw afterwards in theory, I suppose?
*/ | ||
|
||
function processEvent( | ||
export function processEvent( |
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.
does this need to be exported?
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.
Nope, leftover. Removed.
*/ | ||
|
||
interface StreamingState { | ||
export interface StreamingState { |
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.
does this need to be exported?
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.
Nope, leftover from a previous approach 😅
* each event from the input stream unchanged. | ||
* Finalizes span attributes when stream processing completes | ||
*/ | ||
export function finalizeStreamSpan(state: StreamingState, span: Span, recordOutputs: boolean): void { |
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.
does this need to be exported?
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.
Nope, leftover. Removed.
}); | ||
} | ||
|
||
if (span.isRecording()) { |
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.
l: We could also just check isRecording()
at the top of the function and early return if not, setting attributes is a noop in that case anyhow :)
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.
Updated, thanks!
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.
nice, great catch and improvement here that there are these two paths, and nice tests 🚀
9bf91c6
to
894aa11
Compare
op: 'gen_ai.messages', | ||
origin: 'auto.ai.anthropic', | ||
status: 'ok', | ||
}), |
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 expects gen_ai.request.stream: true
. This attribute is only set when an explicit stream
parameter is passed, which messages.stream
does not use as it's inherently streaming. The instrumentation correctly reflects this by omitting the attribute for messages.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.
Previously, we completely walked over anthropic's SDK and replaced
message.stream
with our own method that returns an async generator. This breaks the SDK asMessageStream
has further user callable api, such as adding event handlers.This fix proxies
message.stream
instead of replacing it with our own method. Instead of returning an async generator, we now hook into various events to do our instrumentation.Streams requested via
stream: true
are expected to return async generators, so the current approach still holds, the only change is that we proxy instead of overwrite.Fixes: #17734