feat(core): Instrument workers-ai-provider#22119
Conversation
size-limit report 📦
|
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a22515d. Configure here.
0aa52a5 to
8011b0b
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
isaacs
left a comment
isaacs
left a comment
There was a problem hiding this comment.
A few minor comments/questions. Overall a worthwhile and well-factored addition that follows the patterns established already. 👍
| }, | ||
| error => { | ||
| span.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' }); | ||
| captureException(error, { |
There was a problem hiding this comment.
low/question: I thought the prevailing wisdom was that we don't call captureException, except in cases where we know that the user has not caught the error, and prefer instead to only do that at the top level. Is this different in cf workers? It seems like someone could try { await ai.run(...) } catch { /* this is fine */ }, and end up with exceptions in their dashboard, no?
There was a problem hiding this comment.
Very good question indeed. I actually tried to replicate the original integrations as much as possible.
There was a problem hiding this comment.
We should only do this if there is no way to capture these exceptions otherwise. E.g. in vercel ai v4, tool errors bubble up so we do not capture them here. In v5+ we capture them in the integration because they otherwise do not bubble up, as an example.
There was a problem hiding this comment.
I actually tried to replicate the original integrations as much as possible.
Makes sense. There's definitely a good argument leave to as-is and addressed in a follow-up (or we can decide not to).
In v5+ we capture them in the integration because they otherwise do not bubble up
@mydea What happens to these errors in v5?
If they don't bubble up, are they really "exceptions"? That is, isn't the purpose of captureException to be sort of "this is why your program crashed"?
nicohrubec
left a comment
nicohrubec
left a comment
There was a problem hiding this comment.
I have two questions:
- could you maybe add a screenshot showing a trace in sentry with this instrumentation?
- the main reason our AI instrumentations still live in core is that we currently still export these to the browser (which will change in v11, we'll then move all AI integrations to server-utils). given that this is specific to cloudflare, we could think about moving this to the cloudflare package as you mentioned or if not that maybe move it to server utils?
| return result; | ||
| }, | ||
| error => { | ||
| captureException(error, { |
There was a problem hiding this comment.
m: do we want to immediately capture the exception here? I think we only want to do that if we are fairly certain that the user basically can't handle this exception manually not sure if that is the case here. I know this is what we do in other instrumentations currently, but we might have to revisit this there too eventually
There was a problem hiding this comment.
Not sure if it is good if we start to be inconsistent here. I'd vote for removing the captureException here, but I'm not sure if it is great if one out of 7 is different to the others
There was a problem hiding this comment.
We can also follow up on this, since we have the same pattern in all other integrations probably something to look into holistically. Could you please create a follow-up ticket for this so we don't forget?
#bakingassistant https://sentry-sdks.sentry.io/explore/traces/trace/7b5d5e7a7c3542edba07821a7025c804/ 
I like the idea of having it in Cloudflare only, as there I could also use the types directly. I don't think these bindings would ever be available outside of Cloudflare, and if it would be - then we can still move it to |
| if (Array.isArray(response.tool_calls) && response.tool_calls.length > 0) { | ||
| span.setAttribute(GEN_AI_OUTPUT_MESSAGES, JSON.stringify(response.tool_calls)); | ||
| } |
There was a problem hiding this comment.
Bug: The addResponseAttributes function overwrites the response text attribute with tool call data when both are present in an AI response, causing data loss in traces.
Severity: MEDIUM
Suggested Fix
Instead of calling span.setAttribute twice, combine the response text and tool_calls into a single array of parts, similar to how the Vercel AI integration handles it. Then, set the GEN_AI_OUTPUT_MESSAGES attribute just once with the combined, stringified data.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/core/src/tracing/workers-ai/utils.ts#L162-L164
Potential issue: In the `addResponseAttributes` function, when a Workers AI response
contains both a text `response` and a `tool_calls` array, the span attribute
`GEN_AI_OUTPUT_MESSAGES` is set twice. The first call sets the text response, but the
second call immediately overwrites it with the `tool_calls` data. This results in the
silent loss of the text response from the span attributes, leading to incomplete tracing
data for tool-use scenarios that also include explanatory text.
b7852b2 to
90177eb
Compare
1b95295 to
16771a7
Compare
| } | ||
|
|
||
| return startSpan(spanConfig, (span: Span) => { | ||
| const originalResult = originalRun.apply(context, args) as Promise<unknown>; |
There was a problem hiding this comment.
Bug: Synchronous errors in the non-streaming run method are not reported to Sentry as exceptions, only as a span status.
Severity: LOW
Suggested Fix
Wrap the originalRun.apply(context, args) call in a try...catch block, similar to the streaming path. In the catch block, call captureException to report the error to Sentry before re-throwing it.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/core/src/tracing/workers-ai/index.ts#L90
Potential issue: In the non-streaming path for Workers AI tracing, if the original `run`
method throws a synchronous error, the error is not reported to Sentry via
`captureException`. The error is caught by `startSpan`'s internal `handleCallbackErrors`
function, which sets the span status to error but does not create a Sentry exception
event. This is inconsistent with the streaming path, which explicitly catches
synchronous errors and reports them. While synchronous throws from async methods are
unlikely, this represents a gap in error monitoring.
| } | ||
|
|
||
| if (Array.isArray(response.tool_calls) && response.tool_calls.length > 0) { | ||
| span.setAttribute(GEN_AI_OUTPUT_MESSAGES, JSON.stringify(response.tool_calls)); |
There was a problem hiding this comment.
Bug: JSON.stringify in addResponseAttributes can throw an unhandled exception on non-serializable data, which is not reported to Sentry.
Severity: MEDIUM
Suggested Fix
Wrap the JSON.stringify calls within addResponseAttributes in a try...catch block, similar to the getTruncatedJsonString utility used in other integrations. On error, a placeholder like '[unserializable]' should be used for the attribute value. Alternatively, wrap the call to addResponseAttributes in the non-streaming path with error handling that calls captureException.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/core/src/tracing/workers-ai/utils.ts#L164
Potential issue: In the non-streaming path for Workers AI tracing,
`addResponseAttributes` is called on the successful promise resolution. This function
calls `JSON.stringify` on the AI model's response without a `try...catch` block. If the
response contains non-serializable data (e.g., circular references), `JSON.stringify`
will throw an exception. This exception is not caught and reported to Sentry via
`captureException`, leading to a silent monitoring failure where the error goes
undetected. The streaming path correctly handles this case.
5 participants
closes #20839
closes #20839
This is instrumenting
workers-ai, but it needs to be wrapped, as in Cloudflare there is no monkey patching for bindings. In a follow-up PR this will be included in theinstrumentEnvwithin Cloudflare.Tbh I would have added this into
packages/cloudflare, but all AI integrations do live in core, and there are also some utils which are only there and would have needed to export, such asendStreamSpanand all the gen-ai-attributes. The downside of this is that the workers-ai types needed to be vendored in (in thepackages/cloudflarewe could have taken them from the@cloudflare/workers-typespackage.