fix(core): Don't emit spanStart when child span is SentryNonRecordingSpan#21349
fix(core): Don't emit spanStart when child span is SentryNonRecordingSpan#21349JPeer264 wants to merge 3 commits into
spanStart when child span is SentryNonRecordingSpan#21349Conversation
size-limit report 📦
|
| if (childSpan.isRecording()) { | ||
| client.emit('spanStart', childSpan); | ||
| // If it has an endTimestamp, it's already ended | ||
| if (spanArguments.endTimestamp) { | ||
| client.emit('spanEnd', childSpan); | ||
| client.emit('afterSpanEnd', childSpan); | ||
| } | ||
| } |
There was a problem hiding this comment.
I think spanEnd still fires from the constructor? So it means you get span ends for spans that never triggered a start?
There was a problem hiding this comment.
I checked this. The funny thing is, the constructor endTimestamp check for SentrySpan is basically dead code (I think). The user-facing start*Span* APIs don't allow you to pass an endTimestamp. We then convert the user-facing StartSpanOptions into SpanArguments in parseSentrySpanArguments. But in there, we don't set an endTimestamp either. So there's no typed user-facing way how a started span can be ended immediately.
There is one exception though: users still pass in endTimeStamp despite this not being typed out. It still works because we spread the options into the SpanArguments. I'd argue this is undefined behaviour and the public contract is that users always have to explicitly call .end().
I think we should clean this up in the long-term but it's fine for this PR.
|
I first had it only scoped to Cloudflare, but moved it to core. It is kinda odd that |
Lms24
left a comment
There was a problem hiding this comment.
It is kinda odd that spanStart is getting emitted, but spanEnd is not. It feels wrong IMO.
Agreed, this sounds like a bug we should fix. Quickly talked about this offline with Sigrid and Charly and we'd slightly tend towards not emitting hooks for non-recording spans. We just need to check that this doesn't break any "side effect" behaviour our callbacks depend on.
|
Ok, so I ended up making two changes to this PR to fix an issue we overlooked:
We currently also live with a bit of an inconsistency:
For the future, I have a couple of improvements in mind:
|
spanStart when child span is SentryNonRecordingSpan
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 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 8c867f6. Configure here.
Lms24
left a comment
There was a problem hiding this comment.
Gonna shamelessly semi self-approve :D
|
Those changes make sense but I think changing lifecycle hooks behavior, particularly in this case (skipping them entirely, but also "sometimes") could be a breaking behavior. |
|
Yeah, it's a good point. Especially given the inconsistencies left after this PR, it probably makes sense to just not ship this and fix it for cloudflare. Then we follow up with a proper fix. Good call! Fwiw, we shouldn't consider client hooks (behaviour) public API. We don't document them for users. That being said, we have hybrid SDKs (react native) that might subscribe, so we shouldn't break them xD. |
supersedes #21349 for the time being. This does not fix the case where a `spanStart` event is emitted not a `spanEnd` event isn't. It only locally fixes an occurance of the symptom. We're gonna merge this first to resolve the cloudflare SDK bug but we'll need a more reliable fix than the one I hacked together in #21349. See #21349 (comment)
follow up to #21197
Apparently when setting
tracesSampleRate: 0(or any value between 0 and 1), there areSentryNonRecordingSpans. When they are getting generratedspanStartis being emitted and the Cloudflare client was listening to it and added these spans to_pendingSpans. However,spanEndwas never being called and that is why_pendingSpanswhere never getting less.With the fix of #21197 the
flushLockwas now finally waiting for these spans to be sent, but they were never being sent and would cause another leak (only under very high load).Open Question
I wonder if it was on purpose that
spanStartwas actually being emitted withSentryNonRecoringSpans, or if there is some edge case I can't think of.