feat(core): Seal SentryTracerProvider spans against mutation after they end#21842
feat(core): Seal SentryTracerProvider spans against mutation after they end#21842andreiborza wants to merge 2 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}
size-limit report 📦
|
andreiborza
force-pushed
the
ab/sentry-trace-provider-seal
branch
from
920659d to
8d1b3be
Compare
There was a problem hiding this comment.
dumb q but is there anything we could do to avoid having this logic hard-baked into SentrySpan? No super strong opinion, but maybe medium-strong: Ideally we can keep our SentrySpan as size-efficient as possible and rather special-case provider-emitted spans. wdyt? I might be missing something that makes this more complicated/infeasible, so feel free to disregard
|
@Lms24 as discussed, this adds a lot of extra code. The SentryTracerProvider doesn't instantiate spans itself, it calls core startSpan apis so there's not an easy way to instantiate spans that have this logic in them just for the tracer provider path. This change is about 60 bytes gzipped, so I think the impact shouldn't be too bad. Also, spans should probably be finalized for core and browser as well, but that's something we can take a look at a later point. |
andreiborza
force-pushed
the
ab/sentry-trace-provider-seal
branch
from
8d1b3be to
bb677d7
Compare
| * @internal | ||
| */ | ||
| public updateStartTime(timeInput: SpanTimeInput): void { | ||
| if (this._frozen) { |
There was a problem hiding this comment.
(After looking at other functions the following is a bad idea, but I'll keep it here for food for thoughts)
q: This might be an issue for a future method when we forget to add this._frozen. Would it make sense to add an this._update() function where we update certain fields and protect that?
E.g. here we would update the startTime with this._update({ startTime: '' }), while the this._update is guarded with the this._frozen check.
There was a problem hiding this comment.
I'd rather not add more code to the SentrySpan, see Lukas' original objection. I think this kind of issue of forgetting to add is easily caught by review bots nowadays.
Also, realistically, I don't think we'll expose much more api on this class.
andreiborza
force-pushed
the
ab/sentry-trace-provider-core-capture
branch
from
29ce501 to
c741940
Compare
andreiborza
force-pushed
the
ab/sentry-trace-provider-seal
branch
from
bb677d7 to
c1392df
Compare
andreiborza
force-pushed
the
ab/sentry-trace-provider-core-capture
branch
from
c741940 to
4b3fc03
Compare
andreiborza
force-pushed
the
ab/sentry-trace-provider-seal
branch
from
c1392df to
8fdd6d8
Compare
andreiborza
force-pushed
the
ab/sentry-trace-provider-core-capture
branch
from
4b3fc03 to
14cb421
Compare
andreiborza
force-pushed
the
ab/sentry-trace-provider-seal
branch
from
8fdd6d8 to
3ce0cf9
Compare
andreiborza
force-pushed
the
ab/sentry-trace-provider-core-capture
branch
from
14cb421 to
6b8db6e
Compare
andreiborza
force-pushed
the
ab/sentry-trace-provider-seal
branch
from
3ce0cf9 to
a416506
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.
Reviewed by Cursor Bugbot for commit a416506. Configure here.
andreiborza
force-pushed
the
ab/sentry-trace-provider-seal
branch
from
9a3a9c9 to
9e658ff
Compare
4 participants
What
Seal spans created by the
SentryTracerProvideronce they end. AfterSentrySpan.end()finishes its end-of-span processing, every mutator no-ops, gated on thespanIsTracerProviderSpanbrand:setAttribute/setAttributessetStatusupdateNameupdateStartTimeaddLink/addLinksaddEventSpans created directly through core (e.g. the browser SDK) are never branded, so they stay mutable.
Why
OpenTelemetry SDK spans are immutable after
end()(setters no-op). TheSentryTracerProviderhands nativeSentrySpans to OTel instrumentations as OTel spans, so they must honor that contract. Some instrumentations write to a span afterend()(e.g. Next.js sets a status on a render error); without sealing, those late writes overwrite the finalized values.This matters most once segment-span capture is deferred (#21839): the transaction snapshot is then taken on a later tick, so any late post-
end()write (status, attribute, name, start time, link, or event) would be serialized into it. Sealing the span onend()keeps the finalized values intact.Notes
SentryTracer, never by the browser SDK, so browser spans (including web-vitals start-time adjustments viaupdateStartTime) are unaffected.sentrySpan.test.tscover both directions: a branded span where all mutators no-op afterend(), and a non-branded span that stays mutable.Stacked on #21839 (deferred capture / orphan emission) and #21666 (which provides the
spanIsTracerProviderSpanbrand). Dormant until #21680 wires the provider; no provider-branded spans exist before then.