From 16877b9da42c9da6dec850e7753705945c17fd83 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 7 Jun 2022 15:09:32 +0200 Subject: [PATCH 1/9] ref(tracing): Sync baggage data in Http and envelope headers --- packages/core/src/envelope.ts | 11 ++--------- packages/tracing/src/span.ts | 7 +++---- packages/tracing/src/transaction.ts | 3 ++- 3 files changed, 7 insertions(+), 14 deletions(-) diff --git a/packages/core/src/envelope.ts b/packages/core/src/envelope.ts index 0fb974d66bc7..338e966b88ec 100644 --- a/packages/core/src/envelope.ts +++ b/packages/core/src/envelope.ts @@ -128,19 +128,12 @@ function createEventEnvelopeHeaders( ...(event.type === 'transaction' && event.contexts && event.contexts.trace && { - // TODO: Grab this from baggage trace: dropUndefinedKeys({ // Trace context must be defined for transactions // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - trace_id: event.contexts!.trace.trace_id as string, - environment: event.environment, - release: event.release, - transaction: event.transaction, - user: event.user && { - id: event.user.id, - segment: event.user.segment, - }, + trace_id: (event.contexts!.trace as Record).trace_id as string, public_key: dsn.publicKey, + ...event.contexts.baggage, }), }), }; diff --git a/packages/tracing/src/span.ts b/packages/tracing/src/span.ts index c10299996e0f..800608c62488 100644 --- a/packages/tracing/src/span.ts +++ b/packages/tracing/src/span.ts @@ -4,7 +4,6 @@ import { Baggage, Hub, Primitive, Span as SpanInterface, SpanContext, Transactio import { createBaggage, dropUndefinedKeys, - isBaggageEmpty, isBaggageMutable, isSentryBaggageEmpty, setBaggageImmutable, @@ -312,7 +311,7 @@ export class Span implements SpanInterface { /** * @inheritdoc */ - public getBaggage(): Baggage | undefined { + public getBaggage(): Baggage { const existingBaggage = this.transaction && this.transaction.metadata.baggage; // Only add Sentry baggage items to baggage, if baggage does not exist yet or it is still @@ -322,7 +321,7 @@ export class Span implements SpanInterface { ? this._getBaggageWithSentryValues(existingBaggage) : existingBaggage; - return isBaggageEmpty(finalBaggage) ? undefined : finalBaggage; + return finalBaggage; } /** @@ -366,7 +365,7 @@ export class Span implements SpanInterface { * * @param baggage * - * @returns modified and immutable maggage object + * @returns modified and immutable baggage object */ private _getBaggageWithSentryValues(baggage: Baggage = createBaggage({})): Baggage { // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-explicit-any diff --git a/packages/tracing/src/transaction.ts b/packages/tracing/src/transaction.ts index d51f4578717a..0e31ae583a41 100644 --- a/packages/tracing/src/transaction.ts +++ b/packages/tracing/src/transaction.ts @@ -6,7 +6,7 @@ import { TransactionContext, TransactionMetadata, } from '@sentry/types'; -import { dropUndefinedKeys, logger } from '@sentry/utils'; +import { dropUndefinedKeys, getSentryBaggageItems, logger } from '@sentry/utils'; import { Span as SpanClass, SpanRecorder } from './span'; @@ -122,6 +122,7 @@ export class Transaction extends SpanClass implements TransactionInterface { const transaction: Event = { contexts: { trace: this.getTraceContext(), + baggage: getSentryBaggageItems(this.getBaggage()), }, spans: finishedSpans, start_timestamp: this.startTimestamp, From 3b6577616d637f75561574b8083c2f210eaf5664 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 14 Jun 2022 13:28:24 +0200 Subject: [PATCH 2/9] fix tests and normalization bug --- packages/core/src/baseclient.ts | 1 + packages/core/src/envelope.ts | 17 ++++++++++++-- packages/core/test/lib/envelope.test.ts | 23 ++++++++++++------- .../suites/tracing/baggage/test.ts | 11 +++++---- .../tracing/browsertracing/meta/test.ts | 2 +- packages/utils/src/baggage.ts | 6 ++--- 6 files changed, 41 insertions(+), 19 deletions(-) diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index ef5635f98541..fd09ce82d75f 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -507,6 +507,7 @@ export abstract class BaseClient implements Client { if (event.contexts && event.contexts.trace) { normalized.contexts = {}; normalized.contexts.trace = event.contexts.trace; + normalized.contexts.baggage = event.contexts.baggage; // event.contexts.trace.data may contain circular/dangerous data so we need to normalize it if (event.contexts.trace.data) { diff --git a/packages/core/src/envelope.ts b/packages/core/src/envelope.ts index 338e966b88ec..f8b807d7487e 100644 --- a/packages/core/src/envelope.ts +++ b/packages/core/src/envelope.ts @@ -1,9 +1,11 @@ import { + BaggageObj, DsnComponents, Event, EventEnvelope, EventEnvelopeHeaders, EventItem, + EventTraceContext, SdkInfo, SdkMetadata, Session, @@ -120,6 +122,8 @@ function createEventEnvelopeHeaders( tunnel: string | undefined, dsn: DsnComponents, ): EventEnvelopeHeaders { + const baggage = event.contexts && (event.contexts.baggage as BaggageObj); + return { event_id: event.event_id as string, sent_at: new Date().toISOString(), @@ -133,8 +137,17 @@ function createEventEnvelopeHeaders( // eslint-disable-next-line @typescript-eslint/no-non-null-assertion trace_id: (event.contexts!.trace as Record).trace_id as string, public_key: dsn.publicKey, - ...event.contexts.baggage, - }), + environment: baggage && baggage.environment, + release: baggage && baggage.release, + transaction: baggage && baggage.transaction, + ...(baggage && + (baggage.userid || baggage.usersegment) && { + user: { + id: baggage.userid, + segment: baggage.usersegment, + }, + }), + } as EventTraceContext), }), }; } diff --git a/packages/core/test/lib/envelope.test.ts b/packages/core/test/lib/envelope.test.ts index 13dda0e276d4..9b9d2700b358 100644 --- a/packages/core/test/lib/envelope.test.ts +++ b/packages/core/test/lib/envelope.test.ts @@ -31,14 +31,16 @@ describe('createEventEnvelope', () => { const testTable: Array<[string, Event, EventTraceContext]> = [ [ - 'adds only baggage item', + 'adds only one baggage item', { type: 'transaction', - release: '1.0.0', contexts: { trace: { trace_id: '1234', }, + baggage: { + release: '1.0.0', + }, }, }, { release: '1.0.0', trace_id: '1234', public_key: 'pubKey123' }, @@ -47,12 +49,14 @@ describe('createEventEnvelope', () => { 'adds two baggage items', { type: 'transaction', - release: '1.0.0', - environment: 'prod', contexts: { trace: { trace_id: '1234', }, + baggage: { + environment: 'prod', + release: '1.0.0', + }, }, }, { release: '1.0.0', environment: 'prod', trace_id: '1234', public_key: 'pubKey123' }, @@ -61,14 +65,17 @@ describe('createEventEnvelope', () => { 'adds all baggageitems', { type: 'transaction', - release: '1.0.0', - environment: 'prod', - user: { id: 'bob', segment: 'segmentA' }, - transaction: 'TX', contexts: { trace: { trace_id: '1234', }, + baggage: { + environment: 'prod', + release: '1.0.0', + userid: 'bob', + usersegment: 'segmentA', + transaction: 'TX', + }, }, }, { diff --git a/packages/integration-tests/suites/tracing/baggage/test.ts b/packages/integration-tests/suites/tracing/baggage/test.ts index 5dcb8e82bcf1..4a8bbf497806 100644 --- a/packages/integration-tests/suites/tracing/baggage/test.ts +++ b/packages/integration-tests/suites/tracing/baggage/test.ts @@ -12,11 +12,12 @@ sentryTest('should send trace context data in transaction envelope header', asyn expect(envHeader.trace).toBeDefined(); expect(envHeader.trace).toMatchObject({ environment: 'production', - transaction: 'testTransactionBaggage', - user: { - id: 'user123', - segment: 'segmentB', - }, + // TODO comment back in once we properly support transaction and user data in Baggage + // transaction: 'testTransactionBaggage', + // user: { + // id: 'user123', + // segment: 'segmentB', + // }, public_key: 'public', trace_id: expect.any(String), }); diff --git a/packages/integration-tests/suites/tracing/browsertracing/meta/test.ts b/packages/integration-tests/suites/tracing/browsertracing/meta/test.ts index d2d44ae74050..11801c7f3b08 100644 --- a/packages/integration-tests/suites/tracing/browsertracing/meta/test.ts +++ b/packages/integration-tests/suites/tracing/browsertracing/meta/test.ts @@ -23,7 +23,7 @@ sentryTest( // TODO this we can't really test until we actually propagate sentry- entries in baggage // skipping for now but this must be adjusted later on -sentryTest.skip( +sentryTest( 'should pick up `baggage` tag and propagate the content in transaction', async ({ getLocalTestPath, page }) => { const url = await getLocalTestPath({ testDir: __dirname }); diff --git a/packages/utils/src/baggage.ts b/packages/utils/src/baggage.ts index f6739ff9f736..a3ad316a8026 100644 --- a/packages/utils/src/baggage.ts +++ b/packages/utils/src/baggage.ts @@ -151,14 +151,14 @@ export function mergeAndSerializeBaggage(incomingBaggage?: Baggage, headerBaggag * Extracted this logic to a function because it's duplicated in a lot of places. * * @param rawBaggageString - * @param sentryTraceData + * @param sentryTraceHeader */ export function parseBaggageSetMutability( rawBaggageString: string | false | undefined | null, - sentryTraceData: TraceparentData | string | false | undefined | null, + sentryTraceHeader: TraceparentData | string | false | undefined | null, ): Baggage { const baggage = parseBaggageString(rawBaggageString || ''); - if (!isSentryBaggageEmpty(baggage) || (sentryTraceData && isSentryBaggageEmpty(baggage))) { + if (!isSentryBaggageEmpty(baggage) || (sentryTraceHeader && isSentryBaggageEmpty(baggage))) { setBaggageImmutable(baggage); } return baggage; From b5be6ad65c96c2f1e31280e33bd9d7862a6a77aa Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 14 Jun 2022 13:45:29 +0200 Subject: [PATCH 3/9] activate and fix meta tag integration test --- .../suites/tracing/browsertracing/meta/template.html | 2 +- .../suites/tracing/browsertracing/meta/test.ts | 8 +++++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/packages/integration-tests/suites/tracing/browsertracing/meta/template.html b/packages/integration-tests/suites/tracing/browsertracing/meta/template.html index 60c6c062c7f3..bff098c0f4f2 100644 --- a/packages/integration-tests/suites/tracing/browsertracing/meta/template.html +++ b/packages/integration-tests/suites/tracing/browsertracing/meta/template.html @@ -2,6 +2,6 @@ - + diff --git a/packages/integration-tests/suites/tracing/browsertracing/meta/test.ts b/packages/integration-tests/suites/tracing/browsertracing/meta/test.ts index 11801c7f3b08..2f42a3d7ce2a 100644 --- a/packages/integration-tests/suites/tracing/browsertracing/meta/test.ts +++ b/packages/integration-tests/suites/tracing/browsertracing/meta/test.ts @@ -21,8 +21,6 @@ sentryTest( }, ); -// TODO this we can't really test until we actually propagate sentry- entries in baggage -// skipping for now but this must be adjusted later on sentryTest( 'should pick up `baggage` tag and propagate the content in transaction', async ({ getLocalTestPath, page }) => { @@ -31,7 +29,11 @@ sentryTest( const envHeader = await getFirstSentryEnvelopeRequest(page, url, envelopeHeaderRequestParser); expect(envHeader.trace).toBeDefined(); - expect(envHeader.trace).toEqual('{version:2.1.12}'); + expect(envHeader.trace).toMatchObject({ + public_key: 'public', + trace_id: expect.any(String), + release: '2.1.12', + }); }, ); From 4a8de2811aa19edc20fd48f0909f507bb800d2c0 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 14 Jun 2022 15:25:26 +0200 Subject: [PATCH 4/9] improve meta tag integration test --- .../suites/tracing/browsertracing/meta/init.js | 11 +++++++++++ .../suites/tracing/browsertracing/meta/test.ts | 2 +- 2 files changed, 12 insertions(+), 1 deletion(-) create mode 100644 packages/integration-tests/suites/tracing/browsertracing/meta/init.js diff --git a/packages/integration-tests/suites/tracing/browsertracing/meta/init.js b/packages/integration-tests/suites/tracing/browsertracing/meta/init.js new file mode 100644 index 000000000000..50f4a898e251 --- /dev/null +++ b/packages/integration-tests/suites/tracing/browsertracing/meta/init.js @@ -0,0 +1,11 @@ +import * as Sentry from '@sentry/browser'; +import { Integrations } from '@sentry/tracing'; + +window.Sentry = Sentry; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + integrations: [new Integrations.BrowserTracing()], + tracesSampleRate: 1, + environment: 'staging', +}); diff --git a/packages/integration-tests/suites/tracing/browsertracing/meta/test.ts b/packages/integration-tests/suites/tracing/browsertracing/meta/test.ts index 2f42a3d7ce2a..8db4f05305b3 100644 --- a/packages/integration-tests/suites/tracing/browsertracing/meta/test.ts +++ b/packages/integration-tests/suites/tracing/browsertracing/meta/test.ts @@ -22,7 +22,7 @@ sentryTest( ); sentryTest( - 'should pick up `baggage` tag and propagate the content in transaction', + 'should pick up `baggage` tag, propagate the content in transaction and not add own data', async ({ getLocalTestPath, page }) => { const url = await getLocalTestPath({ testDir: __dirname }); From eebeb6b53f0b1daafbd26c160bf4414d56f54da9 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 15 Jun 2022 12:01:30 +0200 Subject: [PATCH 5/9] remove line that in _normalizeEvent that cleared contexts --- packages/core/src/baseclient.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index fd09ce82d75f..e3222e2bc4ac 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -504,10 +504,8 @@ export abstract class BaseClient implements Client { // For now the decision is to skip normalization of Transactions and Spans, // so this block overwrites the normalized event to add back the original // Transaction information prior to normalization. - if (event.contexts && event.contexts.trace) { - normalized.contexts = {}; + if (event.contexts && event.contexts.trace && normalized.contexts) { normalized.contexts.trace = event.contexts.trace; - normalized.contexts.baggage = event.contexts.baggage; // event.contexts.trace.data may contain circular/dangerous data so we need to normalize it if (event.contexts.trace.data) { From dbd9b7f99486fc65c1df486021aa8277bf7dd934 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 15 Jun 2022 12:12:44 +0200 Subject: [PATCH 6/9] Apply suggestions from code review Co-authored-by: Katie Byers --- packages/core/test/lib/envelope.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/core/test/lib/envelope.test.ts b/packages/core/test/lib/envelope.test.ts index 9b9d2700b358..8577805330b7 100644 --- a/packages/core/test/lib/envelope.test.ts +++ b/packages/core/test/lib/envelope.test.ts @@ -31,7 +31,7 @@ describe('createEventEnvelope', () => { const testTable: Array<[string, Event, EventTraceContext]> = [ [ - 'adds only one baggage item', + 'adds one baggage item', { type: 'transaction', contexts: { @@ -62,7 +62,7 @@ describe('createEventEnvelope', () => { { release: '1.0.0', environment: 'prod', trace_id: '1234', public_key: 'pubKey123' }, ], [ - 'adds all baggageitems', + 'adds all baggage items', { type: 'transaction', contexts: { From 561d49201239cccff67f6dac4a753ecf517b7eac Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 15 Jun 2022 12:16:48 +0200 Subject: [PATCH 7/9] use object destructuring --- packages/core/src/envelope.ts | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/packages/core/src/envelope.ts b/packages/core/src/envelope.ts index f8b807d7487e..7fdcbbc69298 100644 --- a/packages/core/src/envelope.ts +++ b/packages/core/src/envelope.ts @@ -123,6 +123,7 @@ function createEventEnvelopeHeaders( dsn: DsnComponents, ): EventEnvelopeHeaders { const baggage = event.contexts && (event.contexts.baggage as BaggageObj); + const { environment, release, transaction, userid, usersegment } = baggage || {}; return { event_id: event.event_id as string, @@ -137,17 +138,16 @@ function createEventEnvelopeHeaders( // eslint-disable-next-line @typescript-eslint/no-non-null-assertion trace_id: (event.contexts!.trace as Record).trace_id as string, public_key: dsn.publicKey, - environment: baggage && baggage.environment, - release: baggage && baggage.release, - transaction: baggage && baggage.transaction, - ...(baggage && - (baggage.userid || baggage.usersegment) && { - user: { - id: baggage.userid, - segment: baggage.usersegment, - }, - }), - } as EventTraceContext), + environment: environment, + release: release, + transaction: transaction, + ...((userid || usersegment) && { + user: { + id: userid, + segment: usersegment, + }, + }), + }) as EventTraceContext, }), }; } From 212605faaf1dee63eb80590fda4e6a32139e0392 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 15 Jun 2022 12:19:55 +0200 Subject: [PATCH 8/9] make meta integration test more strict --- .../suites/tracing/browsertracing/meta/test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/integration-tests/suites/tracing/browsertracing/meta/test.ts b/packages/integration-tests/suites/tracing/browsertracing/meta/test.ts index 8db4f05305b3..a1f18646a9af 100644 --- a/packages/integration-tests/suites/tracing/browsertracing/meta/test.ts +++ b/packages/integration-tests/suites/tracing/browsertracing/meta/test.ts @@ -29,7 +29,7 @@ sentryTest( const envHeader = await getFirstSentryEnvelopeRequest(page, url, envelopeHeaderRequestParser); expect(envHeader.trace).toBeDefined(); - expect(envHeader.trace).toMatchObject({ + expect(envHeader.trace).toEqual({ public_key: 'public', trace_id: expect.any(String), release: '2.1.12', From 3063b23235f5b8b218d6ce8de287c027d35033fe Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Fri, 17 Jun 2022 09:27:08 +0200 Subject: [PATCH 9/9] Update packages/core/src/envelope.ts Co-authored-by: Katie Byers --- packages/core/src/envelope.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/core/src/envelope.ts b/packages/core/src/envelope.ts index 7fdcbbc69298..82df1837db22 100644 --- a/packages/core/src/envelope.ts +++ b/packages/core/src/envelope.ts @@ -131,6 +131,8 @@ function createEventEnvelopeHeaders( ...(sdkInfo && { sdk: sdkInfo }), ...(!!tunnel && { dsn: dsnToString(dsn) }), ...(event.type === 'transaction' && + // If we don't already have a trace context in the event, we can't get the trace id, which makes adding any other + // trace data pointless event.contexts && event.contexts.trace && { trace: dropUndefinedKeys({