-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat(core): Seal SentryTracerProvider spans against mutation after they end #21842
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -50,7 +50,12 @@ import { getDynamicSamplingContextFromSpan } from './dynamicSamplingContext'; | |
| import { logSpanEnd } from './logSpans'; | ||
| import { timedEventsToMeasurements } from './measurement'; | ||
| import { hasSpanStreamingEnabled } from './spans/hasSpanStreamingEnabled'; | ||
| import { getCapturedScopesOnSpan, markSpanSourceAsExplicit, spanShouldInferOtelSource } from './utils'; | ||
| import { | ||
| getCapturedScopesOnSpan, | ||
| markSpanSourceAsExplicit, | ||
| spanIsTracerProviderSpan, | ||
| spanShouldInferOtelSource, | ||
| } from './utils'; | ||
|
|
||
| const MAX_SPAN_COUNT = 1000; | ||
|
|
||
|
|
@@ -130,6 +135,9 @@ export class SentrySpan implements Span { | |
| /** if true, treat span as a standalone span (not part of a transaction) */ | ||
| private _isStandaloneSpan?: boolean; | ||
|
|
||
| /** if true, the span is sealed and ignores further mutations (set after end for tracer-provider spans) */ | ||
| private _frozen?: boolean; | ||
|
|
||
| /** | ||
| * You should never call the constructor manually, always use `Sentry.startSpan()` | ||
| * or other span methods. | ||
|
|
@@ -175,6 +183,9 @@ export class SentrySpan implements Span { | |
|
|
||
| /** @inheritDoc */ | ||
| public addLink(link: SpanLink): this { | ||
| if (this._frozen) { | ||
| return this; | ||
| } | ||
| if (this._links) { | ||
| this._links.push(link); | ||
| } else { | ||
|
|
@@ -185,6 +196,9 @@ export class SentrySpan implements Span { | |
|
|
||
| /** @inheritDoc */ | ||
| public addLinks(links: SpanLink[]): this { | ||
| if (this._frozen) { | ||
| return this; | ||
| } | ||
| if (this._links) { | ||
| this._links.push(...links); | ||
| } else { | ||
|
|
@@ -216,6 +230,10 @@ export class SentrySpan implements Span { | |
|
|
||
| /** @inheritdoc */ | ||
| public setAttribute(key: string, value: SpanAttributeValue | undefined): this { | ||
| if (this._frozen) { | ||
| return this; | ||
| } | ||
|
|
||
| if (value === undefined) { | ||
| // eslint-disable-next-line @typescript-eslint/no-dynamic-delete | ||
| delete this._attributes[key]; | ||
|
|
@@ -247,13 +265,19 @@ export class SentrySpan implements Span { | |
| * @internal | ||
| */ | ||
| public updateStartTime(timeInput: SpanTimeInput): void { | ||
| if (this._frozen) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (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 E.g. here we would update the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| return; | ||
| } | ||
| this._startTime = spanTimeInputToSeconds(timeInput); | ||
| } | ||
|
|
||
| /** | ||
| * @inheritDoc | ||
| */ | ||
| public setStatus(value: SpanStatus): this { | ||
| if (this._frozen) { | ||
| return this; | ||
| } | ||
| this._status = value; | ||
| return this; | ||
| } | ||
|
|
@@ -262,6 +286,9 @@ export class SentrySpan implements Span { | |
| * @inheritDoc | ||
| */ | ||
| public updateName(name: string): this { | ||
| if (this._frozen) { | ||
| return this; | ||
| } | ||
| this._name = name; | ||
| // Renaming a span marks its name as explicitly chosen, so we stamp `custom`. | ||
| // The exception is spans created by SentryTraceProvider: those are branded for | ||
|
|
@@ -276,15 +303,29 @@ export class SentrySpan implements Span { | |
|
|
||
| /** @inheritdoc */ | ||
| public end(endTimestamp?: SpanTimeInput): void { | ||
| // If already ended, skip | ||
| // If already ended, skip the end-of-span processing, but still seal a tracer-provider span. The | ||
| // seal at the bottom of this method is skipped on this early return, and `_endTime` may have been | ||
| // set before this first `end()` call (e.g. via the constructor's `endTimestamp`), which would | ||
| // otherwise leave the span mutable after `end()`. End-of-span processing already ran in that case. | ||
| if (this._endTime) { | ||
| this._frozen = spanIsTracerProviderSpan(this); | ||
| return; | ||
| } | ||
|
|
||
| this._endTime = spanTimeInputToSeconds(endTimestamp); | ||
| logSpanEnd(this); | ||
|
|
||
| this._onSpanEnded(); | ||
|
|
||
|
cursor[bot] marked this conversation as resolved.
|
||
| // A span created by the SentryTracerProvider is handed to OTel instrumentations as an OTel span, | ||
| // so once end-of-span processing is done (including the `spanEnd` hook where `applyOtelSpanData` | ||
| // finalizes status/source) it is sealed against further writes — mirroring the OpenTelemetry SDK, | ||
| // where setters no-op after a span has ended. Without this, an instrumentation that sets | ||
| // status/attributes after `end()` (e.g. Next.js on a render error) would overwrite the finalized | ||
| // values, and the deferred capture would then serialize those late writes. Spans created directly | ||
| // through the core API (e.g. the browser SDK, which backfills resource-timing attributes after a | ||
| // span ends) are not tracer-provider spans and stay mutable. | ||
| this._frozen = spanIsTracerProviderSpan(this); | ||
|
cursor[bot] marked this conversation as resolved.
andreiborza marked this conversation as resolved.
|
||
| } | ||
|
|
||
| /** | ||
|
|
@@ -353,6 +394,9 @@ export class SentrySpan implements Span { | |
| attributesOrStartTime?: SpanAttributes | SpanTimeInput, | ||
| startTime?: SpanTimeInput, | ||
| ): this { | ||
| if (this._frozen) { | ||
| return this; | ||
| } | ||
| DEBUG_BUILD && debug.log('[Tracing] Adding an event to span:', name); | ||
|
|
||
| const time = isSpanTimeInput(attributesOrStartTime) ? attributesOrStartTime : startTime || timestampInSeconds(); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.