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 7 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}
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 e303625. Configure here.
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.
| this._options = { | ||
| attachStacktrace: true, | ||
| ...options, | ||
| traceLifecycle: options.traceLifecycle === 'static' ? 'static' : 'stream', |
There was a problem hiding this comment.
l: It's good to coerce this to a known value, but feels somewhat unsafe to coerce a typo without any real indicator. I guess most of our users are using typescript, and the type does defend against that? I'd just worry about something like { traceLifecycle: 'stattic' } quietly doing the opposite of what the user intends.
EDIT: actually, when we read it from the env, we have essentially the same logic, in the end. The values static and stream are passed through as-is, and anything else is undefined, which ends up being stream anyway. So, nevermind, this is fine. Actually, it's really good, because before this exact same logic was spread all over the place.
| * `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.
| 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?
| * @returns The modified span payload that will be sent. | ||
| */ | ||
| beforeSendSpan?: ((span: SpanJSON) => SpanJSON) & { _streamed?: true }; | ||
| beforeSendSpan?: BeforeSendStreamedSpanCallback | BeforeSendStaticSpanCallbackMarker; |
There was a problem hiding this comment.
Wouldn't this mean that beforeSendSpan: { _streamed: true } would be a type-allowed option? 🤔
| expect(serialized.attributes['sentry.op']).toEqual({ type: 'string', value: 'http.client' }); | ||
| }); | ||
|
|
||
| it("doesn't a default beforeSendSpan callback without the withStaticSpan wrapper", () => { |
There was a problem hiding this comment.
| it("doesn't a default beforeSendSpan callback without the withStaticSpan wrapper", () => { | |
| it("doesn't apply a default beforeSendSpan callback without the withStaticSpan wrapper", () => { |
| endTimestamp: 2, | ||
| sampled: true, | ||
| expect(mockSend).toHaveBeenCalled(); | ||
| const sentSpan = mockSend.mock.calls[0]![0]![1][0][1].items[0]; |
There was a problem hiding this comment.
Yikes. Is it possible to have a helper or something to flesh this out more readably? Or at least a comment explaining the structure here? I trust that it's the right test, but it's getting a bit line-noise.
| links: [{ trace_id: 'aabb', span_id: 'ccdd', sampled: true, attributes: { foo: 'bar' } }], | ||
| }); | ||
|
|
||
| expect(spanJsonToStreamedSpanJson(streamedSpanJsonToSpanJson(streamedSpan))).toEqual(streamedSpan); |
There was a problem hiding this comment.
This test looks like it's asserting an identity (yToX(xToY(x)) === x) but in this case, there are actually some things that will change. Eg, status is forced to either error or ok, the attributes['sentry.op'] is potentially updated, etc. It'd be good to have a test here that shows those intentional changes, as well, so the intention is locked.
| | `data` | `attributes` | | ||
| | `op` | `attributes['sentry.op']` | | ||
| | `timestamp` | `end_timestamp` | | ||
| | `status` (`string`) | `status` (`'ok'` or `'error'`) | |
There was a problem hiding this comment.
It'd be good to have a note here that any other status values like cancelled or unauthenticated are now coerced to ok, and that the added information now resides in the sentry.status.message attribute. Anyone who was previously doing something with that field's data needs to look elsewhere for it.
| startTimestamp: 1, | ||
| endTimestamp: 2, | ||
| sampled: false, | ||
| describe('standalone spans', () => { |
There was a problem hiding this comment.
This is a nice test reorganization, explains what's actually going on more clearly. 👍
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