feat(core)!: Make beforeSendSpan compatible with streamed spans by default - #22643
feat(core)!: Make beforeSendSpan compatible with streamed spans by default#22643Lms24 wants to merge 10 commits into
beforeSendSpan compatible with streamed spans by default#22643Conversation
size-limit report 📦
|
a32a10f to
bec33b0
Compare
bec33b0 to
e303625
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
a6f845e to
344055e
Compare
Lms24
changed the title
beforeSendSpan compatible with streamed spans by default
ec83b6f to
9f59f8c
Compare
Make streamed span JSON the default beforeSendSpan contract and provide withStaticSpan for callbacks that still process transaction span JSON. Deprecate withStreamedSpan for removal in version 12. BREAKING CHANGE: beforeSendSpan receives StreamedSpanJSON by default. Fixes #22349 Co-Authored-By: Cursor <cursoragent@cursor.com> Co-authored-by: Cursor <cursoragent@cursor.com>
`withStreamedSpan` no longer marks its callback. Nothing reads `_streamed` since `isStreamedBeforeSendSpanCallback` became "not wrapped with `withStaticSpan`", so the wrapper returns the callback unchanged instead of mutating a function it was handed. Resolve `traceLifecycle` once in the `Client` constructor, where any value other than `'static'` normalizes to the `'stream'` default. Neither span streaming integration writes back to the option anymore, which removes an ordering hazard: integrations that read `hasSpanStreamingEnabled` during their own `setup` could observe the value before or after the mutation depending on integration order. The registration gates now test `!== 'static'` so they agree with the normalized value; previously an unknown value failed the gate but still resolved to `'stream'`, leaving nothing to flush spans. Move the `beforeSendSpan` format check into the constructor as well. A callback is only invoked for the span format matching the trace lifecycle, so a mismatch means it is never called. The client can warn where the integration could not: with `traceLifecycle: 'static'` the streaming integration is never registered, so an unwrapped callback previously went unreported. A `withStaticSpan` callback under `traceLifecycle: 'stream'` no longer downgrades the lifecycle to `'static'`. Opting out of streaming requires setting `traceLifecycle: 'static'` explicitly. Refs #22349 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
9f59f8c to
9176c4b
Compare
isaacs
left a comment
isaacs
left a comment
There was a problem hiding this comment.
I'm not super familiar with this part of the machinery, so leaving it as a comment. If I'm misreading things or someone else disagrees, I'm happy to be overruled :)
Since span streaming is the direction we're moving, and the goal is to make it the default, moving these checks out of the integration and into the client feels like the right call.
| // A `beforeSendSpan` callback is only invoked for the span format matching the trace lifecycle, | ||
| // so a mismatch means it is silently never called. | ||
| if ( | ||
| DEBUG_BUILD && |
There was a problem hiding this comment.
This seems like it might potentially be a bigger deal than we'd want to gate behind a DEBUG_BUILD.
Since the default is switching without any transitional state, it seems very possible that a user who fails to read the migration guide (or just misses it) might not wrap their callback.
Also, because this is in the constructor, it wouldn't be able to do a debug.warn if the user manually constructs a client before enabling logging. If I'm reading that right, then it might be good to not gate this behind DEBUG_BUILD, and move the warning to init() or the first captureSpan() with a guard to only emit the warning once.
There was a problem hiding this comment.
DEBUG_BUILD is on by default (only used for advanced tree shaking to remove the entire log message) but I chatted about the specific warning with @mydea. We'll directly log a console warning without our debug logger, so that users don't have to turn on debug: true to see the warning.
There was a problem hiding this comment.
I'm making the change for this option in this PR. For other options, this is tracked in #22856
| * `profile_id` and `exclusive_time` stay readable through `data`. | ||
| */ | ||
| export function streamedSpanJsonToSpanJson(span: StreamedSpanJSON): SpanJSON { | ||
| const data = { ...span.attributes } as SpanAttributes; |
There was a problem hiding this comment.
Wait, isn't span.attributes a RawAttributes<Record<string, unknown>> here? And it's being cast to SpanAttributes, meaning that if you expect something to be a string in data, it's actually going to be a { value: string }, right? So, down below, we'd be setting op to a non-string, and if it gets stringified, wouldn't it become [object Object]? Or am I misreading how this is used?
There was a problem hiding this comment.
Ok, did some more digging on this. I think it's only an issue if you read the values out during the static beforeSpan callback.
For example, if you have this in your options:
new Client({
// ... other options, dsn, etc
beforeSendSpan: withStaticSpan((span: SpanJSON) => {
// this *should* be fine if it's a SpanJSON, because the value
// is string|number|..., not { value: <whatever> }
console.log(String(span.data['render.duration']));
return span;
}),
})And then something does this:
scope.setAttribute('render.duration', { value: 150, unit: 'millisecond' });
const span = startInactiveSpan({ name: 'my-span', parentSpan: rootSpan });then what gets logged is [object Object], because it's not actually getting a SpanJSON, but a StreamedSpanJSON, so the attributes are a RawAttributes object.
I'm not sure if this is a thing we need to guard against, but it feels like the typecast there is little bit of a landmine.
Assuming you never munge it in the callback, I think it's probably fine though? It just casts as one then back as the other.
There was a problem hiding this comment.
In general, streamedSpanJsonToSpanJson is a bit of a "lossy" conversion. The only reason this exists is because INP spans are now always sent as a v2 span, instead of the intermediate v1 standalone span format we had earlier. So even in static mode, these spans (have been and) will be sent as spans and not transactions. Hence we need to convert them as best as we can to the SpanJSON format so that they still go through users' withStaticSpan beforeSendSpan callback.
Your example is valid though and the absence of measurements isn't ideal either. So I'm currently thinking we should revert #22517 which makes us avoid this conversion logic all together.
There was a problem hiding this comment.
Ok, we'll try to send INP spans without using captureSpan which should remove the need for this entire conversion. Thanks for calling this out!
There was a problem hiding this comment.
#22877 will fix this. I'll adjust this PR and resolve the comment once it lands
| status: !span.status || span.status === 'ok' || span.status === 'cancelled' ? 'ok' : 'error', | ||
| is_segment: false, | ||
| attributes: { ...(span.data as RawAttributes<Record<string, unknown>>) }, | ||
| is_segment: span.is_segment ?? false, |
There was a problem hiding this comment.
low/comment: This function is also called by extractGenAiSpansFromEvent in packages/core/src/tracing/spans/extractGenAiSpans.ts, and the is_segment is changing from a hard-coded false to is_segment ?? false.
Also, the op is copied in the other direction now.
Both changes seem fine, at least currently, but it seems like maybe sharing one converter for both "reconstruct a span the user just edited" and "extract gen-AI spans for the wire" might be weird/surprising if those things need to change differently?
There was a problem hiding this comment.
once #22643 (comment), lands we can most likely revert this, too.
There was a problem hiding this comment.
#22877 will fix this. I'll adjust this PR and resolve the comment once it lands
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 6cd4d44. Configure here.
|
Setting to draft while waiting on #18416 |
…` and log warnings if used with streaming (#22908) This PR: 1. Deprecates `beforeSendTransaction` and `ignoreTransactions` 2. Logs a console warning (intentionally not gated with `debug: true`) to warn users if they use any of the two options with span streaming enabled I intentionally didn't update the migration guide. this is handled in #22643. more details in #22856 closes #22856 closes #20279
2 participants
This PR changes
beforeSendSpanto receiveStreamedSpanJSONby default, matching the new trace lifecycle default.Callbacks that intentionally process legacy transaction span JSON need to add the
withStaticSpanwrapper that marks the callback as "static"-compatible and hands users theSpanJSONtype they used in the callback beforehand.The
withStreamedSpanwrapper remains available as a deprecated compatibility helper and is scheduled for removal in version 12.More changes:
beforeSendSpancallbacks no longer lead to switching thetraceLifecycle. Since it now has a default and users need to actively opt out of span streaming, I think it's fair to treat incompatible callbacks as "invalid" and hence skip over them.beforeSendSpancompatibility checks were moved from the integrations into the core client which ensures that they always run now, even if users selected thestaticlife cycle and hencespanStreamingIntegrationdoesn't get added.SpanJSON) spans in favour of always sending them as v2 spans, we now convert aStreamedSpanJsontoSpanJsonincaptureSpan, hand it to the static callback and then convert it back. Not great but I think we need to let users still scrub INP spans.Closes #22349