fix(core): Don't emit `spanStart` when child span is `SentryNonRecordingSpan` by JPeer264 · Pull Request #21349 · getsentry/sentry-javascript

JPeer264 · 2026-06-05T12:45:47Z

follow up to #21197

Apparently when setting tracesSampleRate: 0 (or any value between 0 and 1), there are SentryNonRecordingSpans. When they are getting generrated spanStart is being emitted and the Cloudflare client was listening to it and added these spans to _pendingSpans. However, spanEnd was never being called and that is why _pendingSpans where never getting less.

With the fix of #21197 the flushLock was 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 spanStart was actually being emitted with SentryNonRecoringSpans, or if there is some edge case I can't think of.

github-actions · 2026-06-05T12:55:20Z

size-limit report 📦

Path	Size	% Change	Change
@sentry/browser	27.21 kB	-	-
@sentry/browser - with treeshaking flags	25.65 kB	-	-
@sentry/browser (incl. Tracing)	45.49 kB	-0.03%	-13 B 🔽
@sentry/browser (incl. Tracing + Span Streaming)	47.72 kB	-0.03%	-13 B 🔽
@sentry/browser (incl. Tracing, Profiling)	50.27 kB	-0.03%	-14 B 🔽
@sentry/browser (incl. Tracing, Replay)	84.7 kB	-0.02%	-15 B 🔽
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags	74.33 kB	-0.02%	-14 B 🔽
@sentry/browser (incl. Tracing, Replay with Canvas)	89.39 kB	-0.02%	-15 B 🔽
@sentry/browser (incl. Tracing, Replay, Feedback)	102.09 kB	-0.02%	-16 B 🔽
@sentry/browser (incl. Feedback)	44.37 kB	-	-
@sentry/browser (incl. sendFeedback)	32.03 kB	-	-
@sentry/browser (incl. FeedbackAsync)	37.12 kB	-	-
@sentry/browser (incl. Metrics)	28.28 kB	-	-
@sentry/browser (incl. Logs)	28.53 kB	-	-
@sentry/browser (incl. Metrics & Logs)	29.22 kB	-	-
@sentry/react	29.02 kB	-	-
@sentry/react (incl. Tracing)	47.76 kB	-0.03%	-12 B 🔽
@sentry/vue	32.21 kB	-0.04%	-11 B 🔽
@sentry/vue (incl. Tracing)	47.39 kB	-0.03%	-11 B 🔽
@sentry/svelte	27.23 kB	-	-
CDN Bundle	29.58 kB	-	-
CDN Bundle (incl. Tracing)	47.94 kB	-0.03%	-12 B 🔽
CDN Bundle (incl. Logs, Metrics)	31.09 kB	-	-
CDN Bundle (incl. Tracing, Logs, Metrics)	49.19 kB	-0.03%	-13 B 🔽
CDN Bundle (incl. Replay, Logs, Metrics)	70.37 kB	-	-
CDN Bundle (incl. Tracing, Replay)	85.32 kB	-0.02%	-15 B 🔽
CDN Bundle (incl. Tracing, Replay, Logs, Metrics)	86.5 kB	-0.02%	-12 B 🔽
CDN Bundle (incl. Tracing, Replay, Feedback)	91.14 kB	-0.02%	-15 B 🔽
CDN Bundle (incl. Tracing, Replay, Feedback, Logs, Metrics)	92.33 kB	-0.02%	-13 B 🔽
CDN Bundle - uncompressed	87.78 kB	-	-
CDN Bundle (incl. Tracing) - uncompressed	144.94 kB	-0.04%	-46 B 🔽
CDN Bundle (incl. Logs, Metrics) - uncompressed	92.27 kB	-	-
CDN Bundle (incl. Tracing, Logs, Metrics) - uncompressed	148.69 kB	-0.04%	-46 B 🔽
CDN Bundle (incl. Replay, Logs, Metrics) - uncompressed	217.08 kB	-	-
CDN Bundle (incl. Tracing, Replay) - uncompressed	263.8 kB	-0.02%	-46 B 🔽
CDN Bundle (incl. Tracing, Replay, Logs, Metrics) - uncompressed	267.55 kB	-0.02%	-46 B 🔽
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed	277.5 kB	-0.02%	-46 B 🔽
CDN Bundle (incl. Tracing, Replay, Feedback, Logs, Metrics) - uncompressed	281.24 kB	-0.02%	-46 B 🔽
@sentry/nextjs (client)	50.25 kB	-0.03%	-11 B 🔽
@sentry/sveltekit (client)	45.91 kB	-0.03%	-11 B 🔽
@sentry/core/server	75.94 kB	-0.02%	-13 B 🔽
@sentry/core/browser	63.08 kB	-0.03%	-13 B 🔽
@sentry/node-core	61.71 kB	-0.02%	-12 B 🔽
@sentry/node	130.41 kB	-0.01%	-11 B 🔽
@sentry/node - without tracing	74.09 kB	-0.02%	-11 B 🔽
@sentry/aws-serverless	86.28 kB	-0.02%	-10 B 🔽
@sentry/cloudflare (withSentry) - minified	173.64 kB	-0.03%	-46 B 🔽
@sentry/cloudflare (withSentry)	433.71 kB	-0.02%	-76 B 🔽

View base workflow run

logaretm

Not sure if we should do this across all core, could we do a narrower fix scoped to the cloudflare's client? Like skip adding to the set if the span is non recording.

WDYT?

logaretm · 2026-06-05T15:34:34Z

+ 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);
+ }
 }


I think spanEnd still fires from the constructor? So it means you get span ends for spans that never triggered a start?

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.

JPeer264 · 2026-06-07T16:53:48Z

I first had it only scoped to Cloudflare, but moved it to core.

It is kinda odd that spanStart is getting emitted, but spanEnd is not. It feels wrong IMO.

Lms24

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.

Lms24 · 2026-06-08T13:21:28Z

Ok, so I ended up making two changes to this PR to fix an issue we overlooked:

we can't reliably use span.isRecording() to guard the call because it also returns false on already ended spans. Which is something we apparently care about but we shouldn't need to (see fix(core): Don't emit spanStart when child span is SentryNonRecordingSpan #21349 (comment)).
I removed the spanEnd emissions from startChildSpan because this indeed handled in the constructor as pointed out by @logaretm. Again, this is rather "undefined behaviour" but for now, we can at least get rid of some bytes.

We currently also live with a bit of an inconsistency:

negatively sampled root spans still emit -Start/-End events because they're always a SentrySpan
OTel-emitted spans also always emit -Start/-End events regardless of sampling state (or if the span is recording/nonRecording)

For the future, I have a couple of improvements in mind:

I don't really like that we emit spanStart and spanEnd events in multiple places. Specifically, I think we should move them all into the SentrySpan class because we kinda have to emit the spanEnd event in there anyway. We likely want to save the client reference while in the constructor and reuse that on end. For OTel, we already use only the SentrySpanProcessor which is fine.
We should remove the endTimestamp parameter from SentrySpanArguments because to me this seems like dead code.
I think with this PR we establish start/end consistency but we likely still need to rethink if we want to emit these hooks at all or not. It's probably easier to always emit these events.

cursor

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

Gonna shamelessly semi self-approve :D

logaretm · 2026-06-08T14:39:18Z

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.

So I suggest we should narrow the fix to cloudflare only for now. Later we can review how the hooks should emit for the different kinds of spans we have and ship that for v11, which will also give us a chance to document, and write tests for these cases.

I don't mind shipping this either, but I feel the changes here are a bit of a scope creep on the intended fix for the issue.

Lms24 · 2026-06-08T14:58:01Z

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)

Lms24 · 2026-06-08T16:42:53Z

Gonna close this in favour of #21367. I'll follow up with a proper fix once I've had more time to think about the various combinations of Sentry(NonRecording)?Span and OTel spans. Tracking in #21368

JPeer264 requested a review from a teamJune 5, 2026 12:45

JPeer264 self-assigned this Jun 5, 2026

JPeer264 requested a review from a team as a code owner June 5, 2026 12:45

JPeer264 requested review from andreiborza, logaretm and mydea and removed request for a teamJune 5, 2026 12:45

JPeer264 requested a review from Lms24 June 5, 2026 13:14

logaretm reviewed Jun 5, 2026
View reviewed changes

Lms24 reviewed Jun 8, 2026
View reviewed changes

Comment threadpackages/core/src/tracing/trace.ts Outdated

Lms24 changed the title ~~fix(core,cloudflare): Do not call spanStart when span is not recording~~fix(core): Do not call spanStart when child span is not recordingJun 8, 2026

JPeer264and others added 2 commits June 8, 2026 15:22

fix(core,cloudflare): Do not call spanStart when span is not recording
ee3bbdc

add test and fix NonRecordingSpan instance check
8c867f6

Lms24 force-pushed the jp/span-start-call branch from 06318ab to 8c867f6Compare June 8, 2026 13:22

Lms24 changed the title ~~fix(core): Do not call spanStart when child span is not recording~~fix(core): Don't emit spanStart when child span is SentryNonRecordingSpanJun 8, 2026

cursorBot reviewed Jun 8, 2026
View reviewed changes

Comment threadpackages/core/src/tracing/trace.ts
Comment threadpackages/core/test/lib/tracing/idleSpan.test.ts Outdated
Comment threadpackages/cloudflare/test/client.test.ts

sentryBot reviewed Jun 8, 2026
View reviewed changes

Comment threadpackages/core/src/tracing/trace.ts

lint
e4e30d6

sentryBot reviewed Jun 8, 2026
View reviewed changes

Comment threadpackages/core/src/tracing/trace.ts

Lms24 approved these changes Jun 8, 2026
View reviewed changes

Lms24 mentioned this pull request Jun 8, 2026
fix(cloudflare): Don't track negatively sampled spans #21367
Merged

Lms24 closed this Jun 8, 2026

Uh oh!

Uh oh!

Conversation

JPeer264 commented Jun 5, 2026

Open Question

Uh oh!

github-actionsBot commented Jun 5, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report 📦

Uh oh!

logaretm left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

logaretmJun 5, 2026

Choose a reason for hiding this comment

Uh oh!

Lms24Jun 8, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JPeer264 commented Jun 7, 2026

Uh oh!

Lms24 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Lms24 commented Jun 8, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cursorBot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Lms24 left a comment

Choose a reason for hiding this comment

Uh oh!

logaretm commented Jun 8, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Lms24 commented Jun 8, 2026

Uh oh!

Lms24 commented Jun 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actionsBot commented Jun 5, 2026•
edited
Loading

logaretm left a comment •
edited
Loading

Lms24Jun 8, 2026•
edited
Loading

Lms24 commented Jun 8, 2026•
edited
Loading

logaretm commented Jun 8, 2026•
edited
Loading