From 63ea300395f3fb9926998c4c513d2cd04d25fbbd Mon Sep 17 00:00:00 2001 From: Sigrid Huemer <32902192+s1gr1d@users.noreply.github.com> Date: Mon, 24 Feb 2025 14:26:27 +0100 Subject: [PATCH] feat(core): Add `addLink(s)` to Sentry span (#15452) part of https://github.com/getsentry/sentry-javascript/issues/14991 --- .../suites/tracing/linking-addLink/init.js | 9 ++ .../suites/tracing/linking-addLink/subject.js | 28 ++++++ .../suites/tracing/linking-addLink/test.ts | 76 +++++++++++++++ .../suites/tracing/linking-addLinks/init.js | 9 ++ .../tracing/linking-addLinks/subject.js | 35 +++++++ .../suites/tracing/linking-addLinks/test.ts | 93 +++++++++++++++++++ packages/core/src/tracing/sentrySpan.ts | 18 +++- packages/core/src/utils/spanUtils.ts | 3 +- packages/core/test/lib/tracing/trace.test.ts | 68 ++++++++++++++ packages/opentelemetry/test/trace.test.ts | 4 +- 10 files changed, 338 insertions(+), 5 deletions(-) create mode 100644 dev-packages/browser-integration-tests/suites/tracing/linking-addLink/init.js create mode 100644 dev-packages/browser-integration-tests/suites/tracing/linking-addLink/subject.js create mode 100644 dev-packages/browser-integration-tests/suites/tracing/linking-addLink/test.ts create mode 100644 dev-packages/browser-integration-tests/suites/tracing/linking-addLinks/init.js create mode 100644 dev-packages/browser-integration-tests/suites/tracing/linking-addLinks/subject.js create mode 100644 dev-packages/browser-integration-tests/suites/tracing/linking-addLinks/test.ts diff --git a/dev-packages/browser-integration-tests/suites/tracing/linking-addLink/init.js b/dev-packages/browser-integration-tests/suites/tracing/linking-addLink/init.js new file mode 100644 index 000000000000..3ec6adbbdb75 --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/tracing/linking-addLink/init.js @@ -0,0 +1,9 @@ +import * as Sentry from '@sentry/browser'; + +window.Sentry = Sentry; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + integrations: [], + tracesSampleRate: 1, +}); diff --git a/dev-packages/browser-integration-tests/suites/tracing/linking-addLink/subject.js b/dev-packages/browser-integration-tests/suites/tracing/linking-addLink/subject.js new file mode 100644 index 000000000000..510fb07540ad --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/tracing/linking-addLink/subject.js @@ -0,0 +1,28 @@ +// REGULAR --- +const rootSpan1 = Sentry.startInactiveSpan({ name: 'rootSpan1' }); +rootSpan1.end(); + +Sentry.startSpan({ name: 'rootSpan2' }, rootSpan2 => { + rootSpan2.addLink({ + context: rootSpan1.spanContext(), + attributes: { 'sentry.link.type': 'previous_trace' }, + }); +}); + +// NESTED --- +Sentry.startSpan({ name: 'rootSpan3' }, async rootSpan3 => { + Sentry.startSpan({ name: 'childSpan3.1' }, async childSpan1 => { + childSpan1.addLink({ + context: rootSpan1.spanContext(), + attributes: { 'sentry.link.type': 'previous_trace' }, + }); + + childSpan1.end(); + }); + + Sentry.startSpan({ name: 'childSpan3.2' }, async childSpan2 => { + childSpan2.addLink({ context: rootSpan3.spanContext() }); + + childSpan2.end(); + }); +}); diff --git a/dev-packages/browser-integration-tests/suites/tracing/linking-addLink/test.ts b/dev-packages/browser-integration-tests/suites/tracing/linking-addLink/test.ts new file mode 100644 index 000000000000..9c556335651e --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/tracing/linking-addLink/test.ts @@ -0,0 +1,76 @@ +import { expect } from '@playwright/test'; +import type { SpanJSON, TransactionEvent } from '@sentry/core'; +import { sentryTest } from '../../../utils/fixtures'; +import { envelopeRequestParser, shouldSkipTracingTest, waitForTransactionRequest } from '../../../utils/helpers'; + +sentryTest('should link spans with addLink() in trace context', async ({ getLocalTestUrl, page }) => { + if (shouldSkipTracingTest()) { + sentryTest.skip(); + } + + const rootSpan1Promise = waitForTransactionRequest(page, event => event.transaction === 'rootSpan1'); + const rootSpan2Promise = waitForTransactionRequest(page, event => event.transaction === 'rootSpan2'); + + const url = await getLocalTestUrl({ testDir: __dirname }); + await page.goto(url); + + const rootSpan1 = envelopeRequestParser(await rootSpan1Promise); + const rootSpan2 = envelopeRequestParser(await rootSpan2Promise); + + const rootSpan1_traceId = rootSpan1.contexts?.trace?.trace_id as string; + const rootSpan1_spanId = rootSpan1.contexts?.trace?.span_id as string; + + expect(rootSpan1.transaction).toBe('rootSpan1'); + expect(rootSpan1.spans).toEqual([]); + + expect(rootSpan2.transaction).toBe('rootSpan2'); + expect(rootSpan2.spans).toEqual([]); + + expect(rootSpan2.contexts?.trace?.links?.length).toBe(1); + expect(rootSpan2.contexts?.trace?.links?.[0]).toMatchObject({ + attributes: { 'sentry.link.type': 'previous_trace' }, + sampled: true, + span_id: rootSpan1_spanId, + trace_id: rootSpan1_traceId, + }); +}); + +sentryTest('should link spans with addLink() in nested startSpan() calls', async ({ getLocalTestUrl, page }) => { + if (shouldSkipTracingTest()) { + sentryTest.skip(); + } + + const rootSpan1Promise = waitForTransactionRequest(page, event => event.transaction === 'rootSpan1'); + const rootSpan3Promise = waitForTransactionRequest(page, event => event.transaction === 'rootSpan3'); + + const url = await getLocalTestUrl({ testDir: __dirname }); + await page.goto(url); + + const rootSpan1 = envelopeRequestParser(await rootSpan1Promise); + const rootSpan3 = envelopeRequestParser(await rootSpan3Promise); + + const rootSpan1_traceId = rootSpan1.contexts?.trace?.trace_id as string; + const rootSpan1_spanId = rootSpan1.contexts?.trace?.span_id as string; + + const [childSpan_3_1, childSpan_3_2] = rootSpan3.spans as [SpanJSON, SpanJSON]; + const rootSpan3_traceId = rootSpan3.contexts?.trace?.trace_id as string; + const rootSpan3_spanId = rootSpan3.contexts?.trace?.span_id as string; + + expect(rootSpan3.transaction).toBe('rootSpan3'); + + expect(childSpan_3_1.description).toBe('childSpan3.1'); + expect(childSpan_3_1.links?.length).toBe(1); + expect(childSpan_3_1.links?.[0]).toMatchObject({ + attributes: { 'sentry.link.type': 'previous_trace' }, + sampled: true, + span_id: rootSpan1_spanId, + trace_id: rootSpan1_traceId, + }); + + expect(childSpan_3_2.description).toBe('childSpan3.2'); + expect(childSpan_3_2.links?.[0]).toMatchObject({ + sampled: true, + span_id: rootSpan3_spanId, + trace_id: rootSpan3_traceId, + }); +}); diff --git a/dev-packages/browser-integration-tests/suites/tracing/linking-addLinks/init.js b/dev-packages/browser-integration-tests/suites/tracing/linking-addLinks/init.js new file mode 100644 index 000000000000..3ec6adbbdb75 --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/tracing/linking-addLinks/init.js @@ -0,0 +1,9 @@ +import * as Sentry from '@sentry/browser'; + +window.Sentry = Sentry; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + integrations: [], + tracesSampleRate: 1, +}); diff --git a/dev-packages/browser-integration-tests/suites/tracing/linking-addLinks/subject.js b/dev-packages/browser-integration-tests/suites/tracing/linking-addLinks/subject.js new file mode 100644 index 000000000000..af6c89848fd3 --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/tracing/linking-addLinks/subject.js @@ -0,0 +1,35 @@ +// REGULAR --- +const rootSpan1 = Sentry.startInactiveSpan({ name: 'rootSpan1' }); +rootSpan1.end(); + +const rootSpan2 = Sentry.startInactiveSpan({ name: 'rootSpan2' }); +rootSpan2.end(); + +Sentry.startSpan({ name: 'rootSpan3' }, rootSpan3 => { + rootSpan3.addLinks([ + { context: rootSpan1.spanContext() }, + { + context: rootSpan2.spanContext(), + attributes: { 'sentry.link.type': 'previous_trace' }, + }, + ]); +}); + +// NESTED --- +Sentry.startSpan({ name: 'rootSpan4' }, async rootSpan4 => { + Sentry.startSpan({ name: 'childSpan4.1' }, async childSpan1 => { + Sentry.startSpan({ name: 'childSpan4.2' }, async childSpan2 => { + childSpan2.addLinks([ + { context: rootSpan4.spanContext() }, + { + context: rootSpan2.spanContext(), + attributes: { 'sentry.link.type': 'previous_trace' }, + }, + ]); + + childSpan2.end(); + }); + + childSpan1.end(); + }); +}); diff --git a/dev-packages/browser-integration-tests/suites/tracing/linking-addLinks/test.ts b/dev-packages/browser-integration-tests/suites/tracing/linking-addLinks/test.ts new file mode 100644 index 000000000000..529eae04ae03 --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/tracing/linking-addLinks/test.ts @@ -0,0 +1,93 @@ +import { expect } from '@playwright/test'; +import type { SpanJSON, TransactionEvent } from '@sentry/core'; +import { sentryTest } from '../../../utils/fixtures'; +import { envelopeRequestParser, shouldSkipTracingTest, waitForTransactionRequest } from '../../../utils/helpers'; + +sentryTest('should link spans with addLinks() in trace context', async ({ getLocalTestUrl, page }) => { + if (shouldSkipTracingTest()) { + sentryTest.skip(); + } + + const rootSpan1Promise = waitForTransactionRequest(page, event => event.transaction === 'rootSpan1'); + const rootSpan2Promise = waitForTransactionRequest(page, event => event.transaction === 'rootSpan2'); + const rootSpan3Promise = waitForTransactionRequest(page, event => event.transaction === 'rootSpan3'); + + const url = await getLocalTestUrl({ testDir: __dirname }); + await page.goto(url); + + const rootSpan1 = envelopeRequestParser(await rootSpan1Promise); + const rootSpan2 = envelopeRequestParser(await rootSpan2Promise); + const rootSpan3 = envelopeRequestParser(await rootSpan3Promise); + + const rootSpan1_traceId = rootSpan1.contexts?.trace?.trace_id as string; + const rootSpan1_spanId = rootSpan1.contexts?.trace?.span_id as string; + + expect(rootSpan1.transaction).toBe('rootSpan1'); + expect(rootSpan1.spans).toEqual([]); + + const rootSpan2_traceId = rootSpan2.contexts?.trace?.trace_id as string; + const rootSpan2_spanId = rootSpan2.contexts?.trace?.span_id as string; + + expect(rootSpan2.transaction).toBe('rootSpan2'); + expect(rootSpan2.spans).toEqual([]); + + expect(rootSpan3.transaction).toBe('rootSpan3'); + expect(rootSpan3.spans).toEqual([]); + expect(rootSpan3.contexts?.trace?.links?.length).toBe(2); + expect(rootSpan3.contexts?.trace?.links).toEqual([ + { + sampled: true, + span_id: rootSpan1_spanId, + trace_id: rootSpan1_traceId, + }, + { + attributes: { 'sentry.link.type': 'previous_trace' }, + sampled: true, + span_id: rootSpan2_spanId, + trace_id: rootSpan2_traceId, + }, + ]); +}); + +sentryTest('should link spans with addLinks() in nested startSpan() calls', async ({ getLocalTestUrl, page }) => { + if (shouldSkipTracingTest()) { + sentryTest.skip(); + } + + const rootSpan2Promise = waitForTransactionRequest(page, event => event.transaction === 'rootSpan2'); + const rootSpan4Promise = waitForTransactionRequest(page, event => event.transaction === 'rootSpan4'); + + const url = await getLocalTestUrl({ testDir: __dirname }); + await page.goto(url); + + const rootSpan2 = envelopeRequestParser(await rootSpan2Promise); + const rootSpan4 = envelopeRequestParser(await rootSpan4Promise); + + const rootSpan2_traceId = rootSpan2.contexts?.trace?.trace_id as string; + const rootSpan2_spanId = rootSpan2.contexts?.trace?.span_id as string; + + const [childSpan_4_1, childSpan_4_2] = rootSpan4.spans as [SpanJSON, SpanJSON]; + const rootSpan4_traceId = rootSpan4.contexts?.trace?.trace_id as string; + const rootSpan4_spanId = rootSpan4.contexts?.trace?.span_id as string; + + expect(rootSpan4.transaction).toBe('rootSpan4'); + + expect(childSpan_4_1.description).toBe('childSpan4.1'); + expect(childSpan_4_1.links).toBe(undefined); + + expect(childSpan_4_2.description).toBe('childSpan4.2'); + expect(childSpan_4_2.links?.length).toBe(2); + expect(childSpan_4_2.links).toEqual([ + { + sampled: true, + span_id: rootSpan4_spanId, + trace_id: rootSpan4_traceId, + }, + { + attributes: { 'sentry.link.type': 'previous_trace' }, + sampled: true, + span_id: rootSpan2_spanId, + trace_id: rootSpan2_traceId, + }, + ]); +}); diff --git a/packages/core/src/tracing/sentrySpan.ts b/packages/core/src/tracing/sentrySpan.ts index ddf036f88cdb..3194a45f707f 100644 --- a/packages/core/src/tracing/sentrySpan.ts +++ b/packages/core/src/tracing/sentrySpan.ts @@ -24,6 +24,7 @@ import type { TransactionEvent, TransactionSource, } from '../types-hoist'; +import type { SpanLink } from '../types-hoist/link'; import { logger } from '../utils-hoist/logger'; import { dropUndefinedKeys } from '../utils-hoist/object'; import { generateSpanId, generateTraceId } from '../utils-hoist/propagationContext'; @@ -31,6 +32,7 @@ import { timestampInSeconds } from '../utils-hoist/time'; import { TRACE_FLAG_NONE, TRACE_FLAG_SAMPLED, + convertSpanLinksForEnvelope, getRootSpan, getSpanDescendants, getStatusMessage, @@ -55,6 +57,7 @@ export class SentrySpan implements Span { protected _sampled: boolean | undefined; protected _name?: string | undefined; protected _attributes: SpanAttributes; + protected _links?: SpanLink[]; /** Epoch timestamp in seconds when the span started. */ protected _startTime: number; /** Epoch timestamp in seconds when the span ended. */ @@ -110,12 +113,22 @@ export class SentrySpan implements Span { } /** @inheritDoc */ - public addLink(_link: unknown): this { + public addLink(link: SpanLink): this { + if (this._links) { + this._links.push(link); + } else { + this._links = [link]; + } return this; } /** @inheritDoc */ - public addLinks(_links: unknown[]): this { + public addLinks(links: SpanLink[]): this { + if (this._links) { + this._links.push(...links); + } else { + this._links = links; + } return this; } @@ -225,6 +238,7 @@ export class SentrySpan implements Span { measurements: timedEventsToMeasurements(this._events), is_segment: (this._isStandaloneSpan && getRootSpan(this) === this) || undefined, segment_id: this._isStandaloneSpan ? getRootSpan(this).spanContext().spanId : undefined, + links: convertSpanLinksForEnvelope(this._links), }); } diff --git a/packages/core/src/utils/spanUtils.ts b/packages/core/src/utils/spanUtils.ts index d23a08a96808..7cb19fbacf3c 100644 --- a/packages/core/src/utils/spanUtils.ts +++ b/packages/core/src/utils/spanUtils.ts @@ -40,7 +40,7 @@ let hasShownSpanDropWarning = false; */ export function spanToTransactionTraceContext(span: Span): TraceContext { const { spanId: span_id, traceId: trace_id } = span.spanContext(); - const { data, op, parent_span_id, status, origin } = spanToJSON(span); + const { data, op, parent_span_id, status, origin, links } = spanToJSON(span); return dropUndefinedKeys({ parent_span_id, @@ -50,6 +50,7 @@ export function spanToTransactionTraceContext(span: Span): TraceContext { op, status, origin, + links, }); } diff --git a/packages/core/test/lib/tracing/trace.test.ts b/packages/core/test/lib/tracing/trace.test.ts index c33b50c01a85..00ee444d6f69 100644 --- a/packages/core/test/lib/tracing/trace.test.ts +++ b/packages/core/test/lib/tracing/trace.test.ts @@ -399,6 +399,40 @@ describe('startSpan', () => { }); }); + it('allows to add span links', () => { + const rawSpan1 = startInactiveSpan({ name: 'pageload_span' }); + + // @ts-expect-error _links exists on span + expect(rawSpan1?._links).toEqual(undefined); + + const span1JSON = spanToJSON(rawSpan1); + + startSpan({ name: '/users/:id' }, rawSpan2 => { + rawSpan2.addLink({ + context: rawSpan1.spanContext(), + attributes: { + 'sentry.link.type': 'previous_trace', + }, + }); + + const span2LinkJSON = spanToJSON(rawSpan2).links?.[0]; + + expect(span2LinkJSON?.attributes?.['sentry.link.type']).toBe('previous_trace'); + + // @ts-expect-error _links and _traceId exist on SentrySpan + expect(rawSpan2._links?.[0].context.traceId).toEqual(rawSpan1._traceId); + // @ts-expect-error _links and _traceId exist on SentrySpan + expect(rawSpan2?._links?.[0].context.traceId).toEqual(span1JSON.trace_id); + expect(span2LinkJSON?.trace_id).toBe(span1JSON.trace_id); + + // @ts-expect-error _links and _traceId exist on SentrySpan + expect(rawSpan2?._links?.[0].context.spanId).toEqual(rawSpan1?._spanId); + // @ts-expect-error _links and _traceId exist on SentrySpan + expect(rawSpan2?._links?.[0].context.spanId).toEqual(span1JSON.span_id); + expect(span2LinkJSON?.span_id).toBe(span1JSON.span_id); + }); + }); + it('allows to force a transaction with forceTransaction=true', async () => { const options = getDefaultTestClientOptions({ tracesSampleRate: 1.0 }); client = new TestClient(options); @@ -900,6 +934,40 @@ describe('startSpanManual', () => { }); }); + it('allows to add span links', () => { + const rawSpan1 = startInactiveSpan({ name: 'pageload_span' }); + + // @ts-expect-error _links exists on span + expect(rawSpan1?._links).toEqual(undefined); + + const span1JSON = spanToJSON(rawSpan1); + + startSpanManual({ name: '/users/:id' }, rawSpan2 => { + rawSpan2.addLink({ + context: rawSpan1.spanContext(), + attributes: { + 'sentry.link.type': 'previous_trace', + }, + }); + + const span2LinkJSON = spanToJSON(rawSpan2).links?.[0]; + + expect(span2LinkJSON?.attributes?.['sentry.link.type']).toBe('previous_trace'); + + // @ts-expect-error _links and _traceId exist on SentrySpan + expect(rawSpan2?._links?.[0].context.traceId).toEqual(rawSpan1._traceId); + // @ts-expect-error _links and _traceId exist on SentrySpan + expect(rawSpan2?._links?.[0].context.traceId).toEqual(span1JSON.trace_id); + expect(span2LinkJSON?.trace_id).toBe(span1JSON.trace_id); + + // @ts-expect-error _links and _traceId exist on SentrySpan + expect(rawSpan2?._links?.[0].context.spanId).toEqual(rawSpan1?._spanId); + // @ts-expect-error _links and _traceId exist on SentrySpan + expect(rawSpan2?._links?.[0].context.spanId).toEqual(span1JSON.span_id); + expect(span2LinkJSON?.span_id).toBe(span1JSON.span_id); + }); + }); + it('allows to force a transaction with forceTransaction=true', async () => { const options = getDefaultTestClientOptions({ tracesSampleRate: 1.0 }); client = new TestClient(options); diff --git a/packages/opentelemetry/test/trace.test.ts b/packages/opentelemetry/test/trace.test.ts index 0222264ad6de..93d5fa448dda 100644 --- a/packages/opentelemetry/test/trace.test.ts +++ b/packages/opentelemetry/test/trace.test.ts @@ -356,7 +356,7 @@ describe('trace', () => { }); }); - it('allows to pass span links', () => { + it('allows to add span links', () => { const rawSpan1 = startInactiveSpan({ name: 'pageload_span' }); // @ts-expect-error links exists on span @@ -1016,7 +1016,7 @@ describe('trace', () => { }); }); - it('allows to pass span links', () => { + it('allows to add span links', () => { const rawSpan1 = startInactiveSpan({ name: 'pageload_span' }); // @ts-expect-error links exists on span