diff --git a/dev-packages/node-integration-tests/suites/tracing/segment-name-update/scenario.ts b/dev-packages/node-integration-tests/suites/tracing/segment-name-update/scenario.ts new file mode 100644 index 000000000000..8fb0ac0784b4 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/segment-name-update/scenario.ts @@ -0,0 +1,16 @@ +import * as Sentry from '@sentry/node'; +import { loggingTransport } from '@sentry-internal/node-integration-tests'; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + tracesSampleRate: 1.0, + transport: loggingTransport, +}); + +Sentry.startSpan({ name: 'GET' }, root => { + Sentry.startSpan({ name: 'early child' }, () => {}); + root.updateName('GET /users/:id'); + Sentry.startSpan({ name: 'late child' }, () => {}); +}); + +void Sentry.flush(); diff --git a/dev-packages/node-integration-tests/suites/tracing/segment-name-update/test.ts b/dev-packages/node-integration-tests/suites/tracing/segment-name-update/test.ts new file mode 100644 index 000000000000..f8d520e6b21e --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/segment-name-update/test.ts @@ -0,0 +1,26 @@ +import { afterAll, expect, test } from 'vitest'; +import { cleanupChildProcesses, createRunner } from '../../../utils/runner'; + +afterAll(() => { + cleanupChildProcesses(); +}); + +test('sends the final segment span name on child spans that ended before the segment span was renamed', async () => { + await createRunner(__dirname, 'scenario.ts') + .expect({ + span: container => { + const segmentNames = Object.fromEntries( + container.items.map(span => [span.name, span.attributes?.['sentry.segment.name']?.value]), + ); + + expect(segmentNames).toEqual({ + 'early child': 'GET /users/:id', + 'late child': 'GET /users/:id', + 'GET /users/:id': 'GET /users/:id', + }); + }, + }) + .unordered() + .start() + .completed(); +}); diff --git a/packages/core/src/tracing/sentrySpan.ts b/packages/core/src/tracing/sentrySpan.ts index 41c3a0257de3..4e209c562723 100644 --- a/packages/core/src/tracing/sentrySpan.ts +++ b/packages/core/src/tracing/sentrySpan.ts @@ -588,7 +588,7 @@ function sendStandaloneSpan(span: SentrySpan, client: Client): void { return; } - const { _segmentSpan, ...serializedSpan } = captureSpan(span, client); + const { _segmentSpan, _segmentNameAtCapture, ...serializedSpan } = captureSpan(span, client); const dsc = getDynamicSamplingContextFromSpan(_segmentSpan); // sendEnvelope should not throw // eslint-disable-next-line @typescript-eslint/no-floating-promises diff --git a/packages/core/src/tracing/spans/captureSpan.ts b/packages/core/src/tracing/spans/captureSpan.ts index 886f0d95cc0e..6a5bcc8b719e 100644 --- a/packages/core/src/tracing/spans/captureSpan.ts +++ b/packages/core/src/tracing/spans/captureSpan.ts @@ -32,6 +32,7 @@ import { export type SerializedStreamedSpanWithSegmentSpan = SerializedStreamedSpan & { _segmentSpan: Span; + _segmentNameAtCapture?: string; }; /** @@ -41,7 +42,7 @@ export type SerializedStreamedSpanWithSegmentSpan = SerializedStreamedSpan & { * to an already-ended OTel span instance (which is blocked by the OTel Span class). * * @returns the final serialized span with a reference to its segment span. This reference - * is needed later on to compute the DSC for the span envelope. + * is needed later on to compute the DSC and the final segment name for the span envelope. */ export function captureSpan(span: Span, client: Client): SerializedStreamedSpanWithSegmentSpan { // Convert to JSON FIRST - we cannot write to an already-ended span @@ -88,6 +89,7 @@ export function captureSpan(span: Span, client: Client): SerializedStreamedSpanW return { ...streamedSpanJsonToSerializedSpan(processedSpan), _segmentSpan: segmentSpan, + _segmentNameAtCapture: serializedSegmentSpan.name, }; } diff --git a/packages/core/src/tracing/spans/spanBuffer.ts b/packages/core/src/tracing/spans/spanBuffer.ts index 3066ae3b2790..fb1385799757 100644 --- a/packages/core/src/tracing/spans/spanBuffer.ts +++ b/packages/core/src/tracing/spans/spanBuffer.ts @@ -1,12 +1,14 @@ import type { Client } from '../../client'; import { DEBUG_BUILD } from '../../debug-build'; -import type { SerializedStreamedSpan } from '../../types/span'; +import type { SerializedStreamedSpan, Span } from '../../types/span'; import { debug } from '../../utils/debug-logger'; +import { spanToJSON } from '../../utils/spanUtils'; import { safeUnref } from '../../utils/timer'; import { getDynamicSamplingContextFromSpan } from '../dynamicSamplingContext'; import type { SerializedStreamedSpanWithSegmentSpan } from './captureSpan'; import { createStreamedSpanEnvelope } from './envelope'; import { estimateSerializedSpanSizeInBytes } from './estimateSize'; +import { SENTRY_SEGMENT_NAME } from '@sentry/conventions/attributes'; /** * We must not send more than 1000 spans in one envelope. @@ -168,9 +170,19 @@ export class SpanBuffer { const dsc = getDynamicSamplingContextFromSpan(segmentSpan); + const currentSegmentNames = new Map(); + const getCurrentSegmentName = (span: Span): string => { + let name = currentSegmentNames.get(span); + if (name === undefined) { + name = spanToJSON(span).name; + currentSegmentNames.set(span, name); + } + return name; + }; + const cleanedSpans: SerializedStreamedSpan[] = spans.map(spanJSON => { - // eslint-disable-next-line @typescript-eslint/no-unused-vars - const { _segmentSpan, ...cleanSpanJSON } = spanJSON; + const { _segmentSpan, _segmentNameAtCapture, ...cleanSpanJSON } = spanJSON; + refreshSegmentName(cleanSpanJSON, _segmentNameAtCapture, () => getCurrentSegmentName(_segmentSpan)); return cleanSpanJSON; }); @@ -193,3 +205,24 @@ export class SpanBuffer { this._traceBuckets.delete(traceId); } } + +/** + * Spans are captured when they end, but the segment span can still be renamed afterwards + * (e.g. once the route is known). Only replace the value we set at capture time, so that + * values set by users or `beforeSendSpan` are kept. + */ +function refreshSegmentName( + spanJSON: SerializedStreamedSpan, + segmentNameAtCapture: string | undefined, + getCurrentSegmentName: () => string, +): void { + const segmentNameAttribute = spanJSON.attributes[SENTRY_SEGMENT_NAME]; + if (segmentNameAtCapture === undefined || segmentNameAttribute?.value !== segmentNameAtCapture) { + return; + } + + const currentSegmentName = getCurrentSegmentName(); + if (currentSegmentName !== segmentNameAtCapture) { + spanJSON.attributes[SENTRY_SEGMENT_NAME] = { ...segmentNameAttribute, value: currentSegmentName }; + } +} diff --git a/packages/core/test/lib/tracing/spans/captureSpan.test.ts b/packages/core/test/lib/tracing/spans/captureSpan.test.ts index 23cdaba20cf6..e7f2bc39a90d 100644 --- a/packages/core/test/lib/tracing/spans/captureSpan.test.ts +++ b/packages/core/test/lib/tracing/spans/captureSpan.test.ts @@ -127,6 +127,7 @@ describe('captureSpan', () => { }, }, _segmentSpan: span, + _segmentNameAtCapture: 'my-span', }); }); @@ -235,6 +236,7 @@ describe('captureSpan', () => { }, }, _segmentSpan: span, + _segmentNameAtCapture: 'my-span', }); }); @@ -307,6 +309,7 @@ describe('captureSpan', () => { }, }, _segmentSpan: span, + _segmentNameAtCapture: 'my-span', }); }); @@ -366,6 +369,7 @@ describe('captureSpan', () => { }, }, _segmentSpan: span, + _segmentNameAtCapture: 'my-span', }); }); diff --git a/packages/core/test/lib/tracing/spans/spanBuffer.test.ts b/packages/core/test/lib/tracing/spans/spanBuffer.test.ts index 224ab4504dd2..281a09485a00 100644 --- a/packages/core/test/lib/tracing/spans/spanBuffer.test.ts +++ b/packages/core/test/lib/tracing/spans/spanBuffer.test.ts @@ -1,9 +1,17 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import type { Client, StreamedSpanEnvelope } from '../../../../src'; -import { SentrySpan, setCurrentClient, SpanBuffer } from '../../../../src'; +import { + SentrySpan, + setCurrentClient, + SpanBuffer, + startInactiveSpan, + startSpan, + withStreamedSpan, +} from '../../../../src'; import type { SerializedStreamedSpanWithSegmentSpan } from '../../../../src/tracing/spans/captureSpan'; +import { captureSpan } from '../../../../src/tracing/spans/captureSpan'; import { getDefaultTestClientOptions, TestClient } from '../../../mocks/client'; -import { HTTP_METHOD, URL_FULL } from '@sentry/conventions/attributes'; +import { HTTP_METHOD, SENTRY_SEGMENT_NAME, URL_FULL } from '@sentry/conventions/attributes'; describe('SpanBuffer', () => { let client: TestClient; @@ -370,4 +378,123 @@ describe('SpanBuffer', () => { expect(sendEnvelopeSpy).toHaveBeenCalledTimes(1); }); }); + + describe('segment name', () => { + function getSentSegmentNames(): Record { + const items = sentEnvelopes[0]?.[1]?.[0]?.[1]?.items ?? []; + return Object.fromEntries(items.map(item => [item.name, item.attributes[SENTRY_SEGMENT_NAME]?.value])); + } + + it('uses the final segment span name for spans that ended before the segment span was renamed', () => { + const buffer = new SpanBuffer(client); + + startSpan({ name: 'GET' }, root => { + startSpan({ name: 'early child' }, span => { + span.end(); + buffer.add(captureSpan(span, client)); + }); + + root.updateName('GET /users/:id'); + + startSpan({ name: 'late child' }, span => { + span.end(); + buffer.add(captureSpan(span, client)); + }); + + root.end(); + buffer.add(captureSpan(root, client)); + }); + + buffer.drain(); + + expect(getSentSegmentNames()).toEqual({ + 'early child': 'GET /users/:id', + 'late child': 'GET /users/:id', + 'GET /users/:id': 'GET /users/:id', + }); + }); + + it('uses the segment span name at flush time if the segment span has not ended yet', () => { + const buffer = new SpanBuffer(client); + + const root = startInactiveSpan({ name: 'GET' }); + const child = startInactiveSpan({ name: 'child', parentSpan: root }); + child.end(); + buffer.add(captureSpan(child, client)); + + root.updateName('GET /users/:id'); + buffer.drain(); + + expect(getSentSegmentNames()).toEqual({ child: 'GET /users/:id' }); + }); + + it('keeps a segment name that differs from the segment span name at capture time', () => { + const buffer = new SpanBuffer(client); + + const root = startInactiveSpan({ name: 'GET' }); + const child = startInactiveSpan({ + name: 'child', + parentSpan: root, + attributes: { [SENTRY_SEGMENT_NAME]: 'custom segment name' }, + }); + child.end(); + buffer.add(captureSpan(child, client)); + + root.updateName('GET /users/:id'); + buffer.drain(); + + expect(getSentSegmentNames()).toEqual({ child: 'custom segment name' }); + }); + + it('keeps a segment name set by beforeSendSpan', () => { + client.getOptions().traceLifecycle = 'stream'; + client.getOptions().beforeSendSpan = withStreamedSpan(span => { + if (span.name === 'child') { + span.attributes = { ...span.attributes, [SENTRY_SEGMENT_NAME]: 'from beforeSendSpan' }; + } + return span; + }); + const buffer = new SpanBuffer(client); + + const root = startInactiveSpan({ name: 'GET' }); + const child = startInactiveSpan({ name: 'child', parentSpan: root }); + child.end(); + buffer.add(captureSpan(child, client)); + + root.updateName('GET /users/:id'); + buffer.drain(); + + expect(getSentSegmentNames()).toEqual({ child: 'from beforeSendSpan' }); + }); + + it("uses the name of each span's own segment span when a trace has several segment spans", () => { + const buffer = new SpanBuffer(client); + + const root1 = startInactiveSpan({ name: 'root 1', forceTransaction: true }); + const root2 = startInactiveSpan({ name: 'root 2', parentSpan: root1, forceTransaction: true }); + const child1 = startInactiveSpan({ name: 'child 1', parentSpan: root1 }); + const child2 = startInactiveSpan({ name: 'child 2', parentSpan: root2 }); + child1.end(); + child2.end(); + buffer.add(captureSpan(child1, client)); + buffer.add(captureSpan(child2, client)); + + root1.updateName('root 1 renamed'); + root2.updateName('root 2 renamed'); + buffer.drain(); + + expect(getSentSegmentNames()).toEqual({ 'child 1': 'root 1 renamed', 'child 2': 'root 2 renamed' }); + }); + + it('does not send the internal segment name field', () => { + const buffer = new SpanBuffer(client); + + const root = startInactiveSpan({ name: 'GET' }); + root.end(); + buffer.add(captureSpan(root, client)); + buffer.drain(); + + expect(sentEnvelopes[0]?.[1]?.[0]?.[1]?.items[0]).not.toHaveProperty('_segmentNameAtCapture'); + }); + }); });