Repository navigation
feat(tracing): Add Sentry.startNewTrace - #4036
gbolahan507 wants to merge 8 commits into
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
buenaflor
left a comment
buenaflor
left a comment
There was a problem hiding this comment.
Thanks for the contribution. Please have a look at the AI flagged reviews first.
|
Thanks for the pointer @buenaflor — the bots were spot on, they were all really pointing at the same problem. I was swapping in a fresh propagation context for the new trace but leaving three references to the outer span in place, so anything that reached for those (nested spans, HTTP headers, error events) ended up back on the old trace. Pushed a fix in 146da91. Short version: The zone-forked scope was inheriting I added a small zone sentinel that lets Let me know if you'd like me to split anything out or reshape the API. |
|
Good catch — fixed in 255946c. |
|
Good catch — fixed in 64c1582. Logger and metrics setup integrations now wire |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4036 +/- ##
==========================================
- Coverage 88.17% 88.15% -0.02%
==========================================
Files 347 347
Lines 12955 12996 +41
==========================================
+ Hits 11423 11457 +34
- Misses 1532 1539 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
|
Friendly ping @buenaflor : all AI-flagged reviews are addressed, and CI is green (the two failures are the known changelog-preview / android-beta infra flakes). Happy to rebase on the main whenever you have a moment to take another look. |
|
@gbolahan507 thanks, we will be releasing a new major next week so let's wait for that to get out before continuing with this |
|
Pls rebase to main |
Public API for manually starting a new trace, scoped to a callback. Forks the current scope with a fresh PropagationContext and runs the callback in a Zone using the same _scopeKey mechanism startSpan uses, so transactions, events, and outgoing trace headers inside the callback pick up the new trace while the global scope is untouched. Async callbacks are covered — the zone survives awaits. Closes getsentry#3930
Cursor Bugbot and Seer flagged that startNewTrace let the outer trace leak into the callback through three span references on the hub scope: - Scope.clone copied _activeSpan onto the forked scope, so getActiveSpan still returned an outer span. - getActiveSpan fell back to _currentIdleSpan unconditionally, so nested startSpan calls parented onto the outer idle span and inherited its trace id (Flutter idle-span case). - getSpan and Scope.applyToEvent read the hub scope's bound transaction, so getSpan(), HTTP header propagation and captured events inside the callback stayed on the outer trace (Flutter navigation-transaction case). Add a Zone sentinel _newTraceKey set alongside _scopeKey in startNewTrace. Inside that zone: - Also clearActiveSpan() on the forked scope. - getActiveSpan skips the _currentIdleSpan fallback. - getSpan reads from the zone-forked scope, not the hub scope. - _cloneAndRunWithScope clears the incoming scope's span and active span, so applyToEvent derives trace context from the fresh PropagationContext instead of an outer span. Three new tests cover: idle span active on hub, hub-bound transaction with an event captured inside the callback, and getSpan() inside the callback. Refs getsentry#3930
Bugbot flagged that startTransaction(bindToScope: true) inside a startNewTrace callback wrote the tracer to the hub's scope stack, so the previous fixup — which taught getSpan and applyToEvent to read from the zone-forked scope — no longer saw it. Errors and outgoing HTTP requests inside the callback would not attach to that transaction, and the write leaked into the hub scope for code running outside. Route the bindToScope write to the zone-forked scope when the current zone was established by startNewTrace, so the bound transaction is visible for the duration of the callback and the hub scope stays untouched. Refs getsentry#3930
Bugbot flagged that structured logs and metrics still stamped the outer trace id when emitted inside a startNewTrace callback: DefaultSentryLogger and DefaultSentryMetrics build the log/metric synchronously (before hub.captureLog/captureMetric runs) and read propagationContext from the scope provider — which was wired to hub.scope, not the zone-forked scope. Wire the scope provider to hub.traceScope in both setup integrations, so the logger and metrics implementations pick up the callback-scoped propagation context inside startNewTrace and fall back to the hub scope everywhere else. Refs getsentry#3930
64c1582 to
2c94a73
Compare
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.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 2c94a73. Configure here.
Bugbot flagged that in static mode, markActiveSpanAsErrored used configureScope which only reaches the hub scope. A transaction bound inside a startNewTrace callback lives on the zone-forked scope, so an unhandled error silently failed to mark it as errored while the outer hub span stayed clean. Read the span from traceScope directly, same pattern the rest of startNewTrace already uses for getSpan and event capture. Refs getsentry#3930
|
Good catch — fixed in 58b7cb8. |
buenaflor
left a comment
buenaflor
left a comment
There was a problem hiding this comment.
Nice, two things to fix:
- A transaction bound inside
startNewTracestays bound afterfinish(): it binds to the zone scope, but cleanup still clears the hub scope.getSpan()keeps returning the finished transaction. - Idle spans still bind globally. One started inside
startNewTraceis invisible inside the callback but becomes active outside it.
Please add regression tests for both.
Two leaks flagged by @buenaflor: 1. A transaction bound inside startNewTrace via bindToScope was written to the zone-forked scope, but SentryTracer.finish went through configureScope which only reaches the hub scope. The finished tx stayed on the zone scope and getSpan kept returning it. Add Hub.unbindSpanFromScope that clears from either scope and call it from finish instead of configureScope. 2. startIdleSpan always wrote to the hub-level _idleSpan, so an idle span started inside the callback (which was invisible inside, since getActiveSpan skips the idle-span fallback in a new-trace zone) became active for all code outside once the callback exited. Store the idle span in a zone-local holder when inside startNewTrace, and have _currentIdleSpan read from the holder in that zone. Regression tests for both. Refs getsentry#3930
|
Both fixed in 5cc7559. |
…into ahabeeblahi/feat/start-new-trace
Closes #3930
Description
Adds
Sentry.startNewTrace<T>(T Function() callback), a public API formanually starting a new trace scoped to a callback — parity with the JS
SDK's
Sentry.startNewTrace().Forks the current scope with a fresh
PropagationContextand runs thecallback in a
Zone, reusing the samerunZoned+_scopeKeymechanism
Hub.startSpanalready uses. Transactions, events, andoutgoing trace headers created inside the callback pick up the new
trace; the global scope is untouched, so concurrent work outside the
callback keeps its trace. The zone survives
awaits, so asynccallbacks and
Timer.periodicpolls (the reporter's use case) work.Motivation
Users need to segment background work — polling loops, per-message
handlers, per-job units — into distinct traces without clobbering the
global trace state. The existing internal
Hub.generateNewTrace()mutates the global scope, which isn't callback-scoped and breaks
concurrent work.
Design follows @buenaflor's proposal in #3930.
How tested
left untouched, transactions inside the callback share the new
trace id, awaits survive, nested calls, streaming spans, captured
events pick up the new trace, `rethrow` still ends the span
Deliberate choice
This does not dispatch `OnGenerateNewTrace` to native SDKs. Doing
so would push the callback-scoped trace id into the native layer
globally and break the "doesn't override the global scope" contract
this API is built around. If native sync is desired for this API,
happy to do it as a follow-up with a separate scoping story.
Changelog Entry
Features
trace (#3930)