Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 46 additions & 2 deletions packages/core/src/tracing/sentrySpan.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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.
Comment thread
sentry[bot] marked this conversation as resolved.
Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand Down Expand Up @@ -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];
Expand Down Expand Up @@ -247,13 +265,19 @@ export class SentrySpan implements Span {
* @internal
*/
public updateStartTime(timeInput: SpanTimeInput): void {
if (this._frozen) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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 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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The 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;
}
Expand All @@ -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
Expand All @@ -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();

Comment thread
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);
Comment thread
cursor[bot] marked this conversation as resolved.
Comment thread
andreiborza marked this conversation as resolved.
}

/**
Expand Down Expand Up @@ -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();
Expand Down
84 changes: 82 additions & 2 deletions packages/core/test/lib/tracing/sentrySpan.test.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,18 @@
import { describe, expect, it, test, vi } from 'vitest';
import { getCurrentScope } from '../../../src/currentScopes';
import { setCurrentClient } from '../../../src/sdk';
import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE } from '../../../src/semanticAttributes';
import {
SEMANTIC_ATTRIBUTE_SENTRY_MEASUREMENT_UNIT,
SEMANTIC_ATTRIBUTE_SENTRY_MEASUREMENT_VALUE,
SEMANTIC_ATTRIBUTE_SENTRY_SOURCE,
} from '../../../src/semanticAttributes';
import { _INTERNAL_setDeferSegmentSpanCapture, SentrySpan } from '../../../src/tracing/sentrySpan';
import { SPAN_STATUS_ERROR } from '../../../src/tracing/spanstatus';
import { markSpanForOtelSourceInference, spanSourceWasExplicitlySet } from '../../../src/tracing/utils';
import {
markSpanAsTracerProviderSpan,
markSpanForOtelSourceInference,
spanSourceWasExplicitlySet,
} from '../../../src/tracing/utils';
import type { SpanJSON } from '../../../src/types/span';
import { spanToJSON, TRACE_FLAG_NONE, TRACE_FLAG_SAMPLED } from '../../../src/utils/spanUtils';
import { timestampInSeconds } from '../../../src/utils/time';
Expand Down Expand Up @@ -144,6 +152,78 @@ describe('SentrySpan', () => {
});
});

describe('tracer-provider span sealing', () => {
it('seals a tracer-provider span against all mutation after it ends', () => {
const span = new SentrySpan({ name: 'original', startTimestamp: 1, attributes: { key: 'before' } });
span.setStatus({ code: SPAN_STATUS_ERROR, message: 'before' });
span.addEvent('measurement', {
[SEMANTIC_ATTRIBUTE_SENTRY_MEASUREMENT_VALUE]: 1,
[SEMANTIC_ATTRIBUTE_SENTRY_MEASUREMENT_UNIT]: 'millisecond',
});
const linked = new SentrySpan({ name: 'linked' });

markSpanAsTracerProviderSpan(span);
span.end();

// Every mutator must no-op on a tracer-provider span once it has ended, mirroring OTel SDK spans.
span.setAttribute('key', 'after');
span.setAttributes({ key2: 'after' });
span.setStatus({ code: SPAN_STATUS_ERROR, message: 'after' });
span.updateName('after');
span.updateStartTime(999);
span.addLink({ context: linked.spanContext() });
span.addLinks([{ context: linked.spanContext() }]);
span.addEvent('measurement', {
[SEMANTIC_ATTRIBUTE_SENTRY_MEASUREMENT_VALUE]: 2,
[SEMANTIC_ATTRIBUTE_SENTRY_MEASUREMENT_UNIT]: 'millisecond',
});

const json = spanToJSON(span);
expect(json.data?.['key']).toBe('before');
expect(json.data?.['key2']).toBeUndefined();
expect(json.status).toBe('before');
expect(json.description).toBe('original');
expect(json.start_timestamp).toBe(1);
expect(json.links).toBeUndefined();
expect(json.measurements).toEqual({ measurement: { value: 1, unit: 'millisecond' } });
});

it('keeps a span that is not a tracer-provider span mutable after it ends', () => {
const span = new SentrySpan({ name: 'original', startTimestamp: 1, attributes: { key: 'before' } });
const linked = new SentrySpan({ name: 'linked' });

span.end();

span.setAttribute('key', 'after');
span.updateName('after');
span.updateStartTime(999);
span.addLink({ context: linked.spanContext() });

const json = spanToJSON(span);
expect(json.data?.['key']).toBe('after');
expect(json.description).toBe('after');
expect(json.start_timestamp).toBe(999);
expect(json.links).toHaveLength(1);
});

it('seals a tracer-provider span that ended via the constructor endTimestamp', () => {
// `_endTime` is set in the constructor, so `end()` early-returns before reaching the seal at the
// bottom of its body. The span must still be sealed once `end()` is invoked.
const span = new SentrySpan({
name: 'original',
startTimestamp: 1,
endTimestamp: 2,
attributes: { key: 'before' },
});
markSpanAsTracerProviderSpan(span);

span.end();

span.setAttribute('key', 'after');
expect(spanToJSON(span).data?.['key']).toBe('before');
});
});

describe('end', () => {
test('simple', () => {
const span = new SentrySpan({});
Expand Down
Loading