Skip to content
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

feat(tracing): Add additional Dynamic Sampling Context items to baggage and envelope headers #5292

Merged
merged 5 commits into from Jun 23, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
30 changes: 13 additions & 17 deletions packages/core/src/envelope.ts
@@ -1,5 +1,4 @@
import {
BaggageObj,
DsnComponents,
Event,
EventEnvelope,
Expand All @@ -13,7 +12,7 @@ import {
SessionEnvelope,
SessionItem,
} from '@sentry/types';
import { createEnvelope, dropUndefinedKeys, dsnToString } from '@sentry/utils';
import { createEnvelope, dropUndefinedKeys, dsnToString, getSentryBaggageItems } from '@sentry/utils';

/** Extract sdk info from from the API metadata */
function getSdkMetadataForEnvelopeHeader(metadata?: SdkMetadata): SdkInfo | undefined {
Expand Down Expand Up @@ -77,14 +76,14 @@ export function createEventEnvelope(

enhanceEventWithSdkInfo(event, metadata && metadata.sdk);

const envelopeHeaders = createEventEnvelopeHeaders(event, sdkInfo, tunnel, dsn);

// Prevent this data (which, if it exists, was used in earlier steps in the processing pipeline) from being sent to
// sentry. (Note: Our use of this property comes and goes with whatever we might be debugging, whatever hacks we may
// have temporarily added, etc. Even if we don't happen to be using it at some point in the future, let's not get rid
// of this `delete`, lest we miss putting it back in the next time the property is in use.)
delete event.sdkProcessingMetadata;

const envelopeHeaders = createEventEnvelopeHeaders(event, sdkInfo, tunnel, dsn);

const eventItem: EventItem = [
{
type: eventType,
Expand All @@ -101,27 +100,24 @@ function createEventEnvelopeHeaders(
tunnel: string | undefined,
dsn: DsnComponents,
): EventEnvelopeHeaders {
const baggage = event.contexts && (event.contexts.baggage as BaggageObj);
const { environment, release, transaction, userid, usersegment } = baggage || {};
const baggage = event.sdkProcessingMetadata && event.sdkProcessingMetadata.baggage;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, having this in the sdkProcessingMetadata makes so much more sense.

const { environment, release, transaction, userid, usersegment, samplerate, publickey, traceid } =
(baggage && getSentryBaggageItems(baggage)) || {};

return {
event_id: event.event_id as string,
sent_at: new Date().toISOString(),
...(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 && {
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 as Record<string, unknown>).trace_id as string,
public_key: dsn.publicKey,
environment: environment,
release: release,
transaction: transaction,
trace_id: traceid,
public_key: publickey,
sample_rate: samplerate,
environment,
release,
transaction,
...((userid || usersegment) && {
user: {
id: userid,
Expand Down
67 changes: 23 additions & 44 deletions packages/core/test/lib/envelope.test.ts
Expand Up @@ -14,49 +14,23 @@ describe('createEventEnvelope', () => {
expect(envelopeHeaders.trace).toBeUndefined();
});

it('adds minimal trace data if event is a transaction and no other baggage-related data is available', () => {
const event: Event = {
type: 'transaction',
contexts: {
trace: {
trace_id: '1234',
},
},
};
const envelopeHeaders = createEventEnvelope(event, testDsn)[0];

expect(envelopeHeaders).toBeDefined();
expect(envelopeHeaders.trace).toEqual({ trace_id: '1234', public_key: 'pubKey123' });
});

const testTable: Array<[string, Event, EventTraceContext]> = [
[
'adds one baggage item',
'adds minimal baggage items',
{
type: 'transaction',
contexts: {
trace: {
trace_id: '1234',
},
baggage: {
release: '1.0.0',
},
sdkProcessingMetadata: {
baggage: [{ traceid: '1234', publickey: 'pubKey123' }, '', false],
},
},
{ release: '1.0.0', trace_id: '1234', public_key: 'pubKey123' },
{ trace_id: '1234', public_key: 'pubKey123' },
],
[
'adds two baggage items',
'adds multiple baggage items',
{
type: 'transaction',
contexts: {
trace: {
trace_id: '1234',
},
baggage: {
environment: 'prod',
release: '1.0.0',
},
sdkProcessingMetadata: {
baggage: [{ environment: 'prod', release: '1.0.0', publickey: 'pubKey123', traceid: '1234' }, '', false],
},
},
{ release: '1.0.0', environment: 'prod', trace_id: '1234', public_key: 'pubKey123' },
Expand All @@ -65,17 +39,21 @@ describe('createEventEnvelope', () => {
'adds all baggage items',
{
type: 'transaction',
contexts: {
trace: {
trace_id: '1234',
},
baggage: {
environment: 'prod',
release: '1.0.0',
userid: 'bob',
usersegment: 'segmentA',
transaction: 'TX',
},
sdkProcessingMetadata: {
baggage: [
{
environment: 'prod',
release: '1.0.0',
userid: 'bob',
usersegment: 'segmentA',
transaction: 'TX',
samplerate: '0.95',
publickey: 'pubKey123',
traceid: '1234',
},
'',
false,
],
},
},
{
Expand All @@ -85,6 +63,7 @@ describe('createEventEnvelope', () => {
transaction: 'TX',
trace_id: '1234',
public_key: 'pubKey123',
sample_rate: '0.95',
},
],
];
Expand Down
8 changes: 7 additions & 1 deletion packages/hub/src/scope.ts
Expand Up @@ -454,6 +454,7 @@ export class Scope implements ScopeInterface {
if (this._transactionName) {
event.transaction = this._transactionName;
}

// We want to set the trace context for normal events only if there isn't already
// a trace context on the event. There is a product feature in place where we link
// errors with transaction and it relies on that.
Expand All @@ -470,7 +471,12 @@ export class Scope implements ScopeInterface {
event.breadcrumbs = [...(event.breadcrumbs || []), ...this._breadcrumbs];
event.breadcrumbs = event.breadcrumbs.length > 0 ? event.breadcrumbs : undefined;

event.sdkProcessingMetadata = this._sdkProcessingMetadata;
// Since we're storing dynamic sampling context data in the event.sdkProcessingMetadata
// field We have to re-apply it after we applied the Scope's field.
// (This is because we're storing this data on the span and not on the scope)
const baggage = event.sdkProcessingMetadata && event.sdkProcessingMetadata.baggage;
event.sdkProcessingMetadata = this._sdkProcessingMetadata || {};
event.sdkProcessingMetadata.baggage = baggage;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering. This already seemed kinda sketchy before you changed it. Did this line just overwrite/wipe the entire metadata from before? Would it be an option here to just merge them instead (like we do with the breadcrumbs and basically everything else (extra, tags, user, ...) above)?

Like this for example:

event.sdkProcessingMetadata = { ...event.sdkProcessingMetadata, ...this._sdkProcessingMetadata }; // We probably want this ordering

Wdyt?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did this line just overwrite/wipe the entire metadata from before?

Yes, everything that was on event.sdkProcessingMetadata.

I had the same feeling when writing this hack. Decided to go with it anyway because I thought that there was a reason for why we do this.
However, I just tried your suggestion and our tests still seem to pass, with it in, so I'll take it. Thanks :D


return this._notifyEventProcessors([...getGlobalEventProcessors(), ...this._eventProcessors], event, hint);
}
Expand Down
24 changes: 0 additions & 24 deletions packages/integration-tests/suites/tracing/baggage/test.ts

This file was deleted.

Expand Up @@ -2,6 +2,9 @@
<head>
<meta charset="utf-8" />
<meta name="sentry-trace" content="12312012123120121231201212312012-1121201211212012-1" />
<meta name="baggage" content="sentry-release=2.1.12" />
<meta
name="baggage"
content="sentry-release=2.1.12,sentry-publickey=public,sentry-traceid=123,sentry-samplerate=0.3232"
/>
</head>
</html>
Expand Up @@ -30,9 +30,10 @@ sentryTest(

expect(envHeader.trace).toBeDefined();
expect(envHeader.trace).toEqual({
public_key: 'public',
trace_id: expect.any(String),
release: '2.1.12',
sample_rate: '0.3232',
trace_id: '123',
public_key: 'public',
});
},
);
Expand Down
Expand Up @@ -8,9 +8,10 @@ Sentry.init({
integrations: [new Integrations.BrowserTracing({ tracingOrigins: [/.*/] })],
environment: 'production',
tracesSampleRate: 1,
debug: true,
});

Sentry.configureScope(scope => {
scope.setUser({ id: 'user123', segment: 'segmentB' });
scope.setTransactionName('testTransactionBaggage');
scope.setTransactionName('testTransactionDSC');
});
27 changes: 27 additions & 0 deletions packages/integration-tests/suites/tracing/envelope-header/test.ts
@@ -0,0 +1,27 @@
import { expect } from '@playwright/test';
import { EventEnvelopeHeaders } from '@sentry/types';

import { sentryTest } from '../../../utils/fixtures';
import { envelopeHeaderRequestParser, getFirstSentryEnvelopeRequest } from '../../../utils/helpers';

sentryTest(
'should send dynamic sampling context data in transaction envelope header',
async ({ getLocalTestPath, page }) => {
const url = await getLocalTestPath({ testDir: __dirname });

const envHeader = await getFirstSentryEnvelopeRequest<EventEnvelopeHeaders>(page, url, envelopeHeaderRequestParser);

expect(envHeader.trace).toBeDefined();
expect(envHeader.trace).toEqual({
environment: 'production',
transaction: expect.stringContaining('index.html'),
user: {
id: 'user123',
segment: 'segmentB',
},
sample_rate: '1',
trace_id: expect.any(String),
public_key: 'public',
});
},
);
Expand Up @@ -77,7 +77,10 @@ test('Should populate and propagate sentry baggage if sentry-trace header does n
expect(response).toMatchObject({
test_data: {
host: 'somewhere.not.sentry',
baggage: 'sentry-environment=prod,sentry-release=1.0',
baggage: expect.stringContaining(
'sentry-environment=prod,sentry-release=1.0,sentry-transaction=GET%20%2Ftest%2Fexpress,sentry-samplerate=1,' +
'sentry-publickey=public,sentry-traceid=',
Comment on lines +81 to +82
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't really matter here and it's very subjective but I personally avoid breaking up strings this way, simply because it's less "searchable". No action required though - just wanted to get this thought out ^^

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup but I also don't like super long strings that heavily exceed the characters-per-line limit. Very annoying when working with split editor windows 😅

),
},
});
});
Expand All @@ -93,7 +96,11 @@ test('Should populate Sentry and propagate 3rd party content if sentry-trace hea
expect(response).toMatchObject({
test_data: {
host: 'somewhere.not.sentry',
baggage: 'foo=bar,bar=baz,sentry-environment=prod,sentry-release=1.0',
// TraceId changes, hence we only expect that the string contains the traceid key
baggage: expect.stringContaining(
'foo=bar,bar=baz,sentry-environment=prod,sentry-release=1.0,sentry-transaction=GET%20%2Ftest%2Fexpress,' +
'sentry-samplerate=1,sentry-publickey=public,sentry-traceid=',
),
},
});
});
24 changes: 21 additions & 3 deletions packages/node/test/integrations/http.test.ts
Expand Up @@ -27,12 +27,22 @@ describe('tracing', () => {
const hub = new Hub(new NodeClient(options));
addExtensionMethods();

hub.configureScope(scope =>
scope.setUser({
id: 'uid123',
segment: 'segmentA',
}),
);

// we need to mock both of these because the tracing handler relies on `@sentry/core` while the sampler relies on
// `@sentry/hub`, and mocking breaks the link between the two
jest.spyOn(sentryCore, 'getCurrentHub').mockReturnValue(hub);
jest.spyOn(hubModule, 'getCurrentHub').mockReturnValue(hub);

const transaction = hub.startTransaction({ name: 'dogpark' });
const transaction = hub.startTransaction({
name: 'dogpark',
traceId: '12312012123120121231201212312012',
});
hub.getScope()?.setSpan(transaction);

return transaction;
Expand Down Expand Up @@ -98,7 +108,11 @@ describe('tracing', () => {
const baggageHeader = request.getHeader('baggage') as string;

expect(baggageHeader).toBeDefined();
expect(baggageHeader).toEqual('sentry-environment=production,sentry-release=1.0.0');
expect(baggageHeader).toEqual(
'sentry-environment=production,sentry-release=1.0.0,sentry-transaction=dogpark,sentry-userid=uid123,' +
'sentry-usersegment=segmentA,sentry-samplerate=1,sentry-publickey=dogsarebadatkeepingsecrets,' +
'sentry-traceid=12312012123120121231201212312012',
);
});

it('propagates 3rd party baggage header data to outgoing non-sentry requests', async () => {
Expand All @@ -110,7 +124,11 @@ describe('tracing', () => {
const baggageHeader = request.getHeader('baggage') as string;

expect(baggageHeader).toBeDefined();
expect(baggageHeader).toEqual('dog=great,sentry-environment=production,sentry-release=1.0.0');
expect(baggageHeader).toEqual(
'dog=great,sentry-environment=production,sentry-release=1.0.0,sentry-transaction=dogpark,' +
'sentry-userid=uid123,sentry-usersegment=segmentA,sentry-samplerate=1,' +
'sentry-publickey=dogsarebadatkeepingsecrets,sentry-traceid=12312012123120121231201212312012',
);
});

it("doesn't attach the sentry-trace header to outgoing sentry requests", () => {
Expand Down