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): Propagate environment and release values in baggage Http headers #5193

Merged
merged 5 commits into from Jun 7, 2022
Merged
Show file tree
Hide file tree
Changes from 4 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
Expand Up @@ -3,7 +3,7 @@ import * as path from 'path';
import { getAPIResponse, runServer } from '../../../../utils/index';
import { TestAPIResponse } from '../server';

test('Should assign `baggage` header which contains 3rd party trace baggage data of an outgoing request.', async () => {
test('Should assign `baggage` header which contains 3rd party trace baggage data to an outgoing request.', async () => {
const url = await runServer(__dirname, `${path.resolve(__dirname, '..')}/server.ts`);

const response = (await getAPIResponse(new URL(`${url}/express`), {
Expand All @@ -14,39 +14,39 @@ test('Should assign `baggage` header which contains 3rd party trace baggage data
expect(response).toMatchObject({
test_data: {
host: 'somewhere.not.sentry',
baggage: expect.stringContaining('foo=bar,bar=baz'),
baggage: 'foo=bar,bar=baz,sentry-environment=prod,sentry-release=1.0',
},
});
});

test('Should assign `baggage` header which contains sentry trace baggage data of an outgoing request.', async () => {
test('Should assign `baggage` header which contains sentry trace baggage data to an outgoing request.', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

Can we say in this test title explaining, that we don't overwrite baggage that's already on the outgoing request?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, thanks for bringing this up. Changed this title and the one below it
(this makes me think that I should add more tests covering different mutability cases here in #5205)

Copy link
Member Author

Choose a reason for hiding this comment

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

const url = await runServer(__dirname, `${path.resolve(__dirname, '..')}/server.ts`);

const response = (await getAPIResponse(new URL(`${url}/express`), {
baggage: 'sentry-version=1.0.0,sentry-environment=production',
baggage: 'sentry-version=2.0.0,sentry-environment=myEnv',
})) as TestAPIResponse;

expect(response).toBeDefined();
expect(response).toMatchObject({
test_data: {
host: 'somewhere.not.sentry',
baggage: expect.stringContaining('sentry-version=1.0.0,sentry-environment=production'),
baggage: 'sentry-version=2.0.0,sentry-environment=myEnv',
},
});
});

test('Should assign `baggage` header which contains sentry and 3rd party trace baggage data of an outgoing request.', async () => {
test('Should assign `baggage` header which contains sentry and 3rd party trace baggage data to an outgoing request.', async () => {
const url = await runServer(__dirname, `${path.resolve(__dirname, '..')}/server.ts`);

const response = (await getAPIResponse(new URL(`${url}/express`), {
baggage: 'sentry-version=1.0.0,sentry-environment=production,dogs=great',
baggage: 'sentry-version=2.0.0,sentry-environment=myEnv,dogs=great',
})) as TestAPIResponse;

expect(response).toBeDefined();
expect(response).toMatchObject({
test_data: {
host: 'somewhere.not.sentry',
baggage: expect.stringContaining('dogs=great,sentry-version=1.0.0,sentry-environment=production'),
baggage: expect.stringContaining('dogs=great,sentry-version=2.0.0,sentry-environment=myEnv'),
},
});
});
Expand Up @@ -12,8 +12,7 @@ test('should attach a `baggage` header to an outgoing request.', async () => {
expect(response).toMatchObject({
test_data: {
host: 'somewhere.not.sentry',
// TODO this is currently still empty but eventually it should contain sentry data
baggage: expect.stringMatching(''),
baggage: expect.stringMatching('sentry-environment=prod,sentry-release=1.0'),
},
});
});
Expand Up @@ -11,6 +11,7 @@ export type TestAPIResponse = { test_data: { host: string; 'sentry-trace': strin
Sentry.init({
dsn: 'https://public@dsn.ingest.sentry.io/1337',
release: '1.0',
environment: 'prod',
integrations: [new Sentry.Integrations.Http({ tracing: true }), new Tracing.Integrations.Express({ app })],
tracesSampleRate: 1.0,
});
Expand Down
12 changes: 6 additions & 6 deletions packages/node/test/integrations/http.test.ts
Expand Up @@ -21,6 +21,8 @@ describe('tracing', () => {
dsn: 'https://dogsarebadatkeepingsecrets@squirrelchasers.ingest.sentry.io/12312012',
tracesSampleRate: 1.0,
integrations: [new HttpIntegration({ tracing: true })],
release: '1.0.0',
environment: 'production',
});
const hub = new Hub(new NodeClient(options));
addExtensionMethods();
Expand All @@ -40,7 +42,7 @@ describe('tracing', () => {
nock('http://dogs.are.great').get('/').reply(200);

const transaction = createTransactionOnScope();
const spans = (transaction as Span).spanRecorder?.spans as Span[];
const spans = (transaction as unknown as Span).spanRecorder?.spans as Span[];

http.get('http://dogs.are.great/');

Expand All @@ -55,7 +57,7 @@ describe('tracing', () => {
nock('http://squirrelchasers.ingest.sentry.io').get('/api/12312012/store/').reply(200);

const transaction = createTransactionOnScope();
const spans = (transaction as Span).spanRecorder?.spans as Span[];
const spans = (transaction as unknown as Span).spanRecorder?.spans as Span[];

http.get('http://squirrelchasers.ingest.sentry.io/api/12312012/store/');

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

expect(baggageHeader).toBeDefined();
// this might change once we actually add our baggage data to the header
expect(baggageHeader).toEqual('');
expect(baggageHeader).toEqual('sentry-environment=production,sentry-release=1.0.0');
});

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

expect(baggageHeader).toBeDefined();
// this might change once we actually add our baggage data to the header
expect(baggageHeader).toEqual('dog=great');
expect(baggageHeader).toEqual('dog=great,sentry-environment=production,sentry-release=1.0.0');
});

it("doesn't attach the sentry-trace header to outgoing sentry requests", () => {
Expand Down
2 changes: 1 addition & 1 deletion packages/tracing/src/browser/request.ts
@@ -1,4 +1,5 @@
/* eslint-disable max-lines */
import type { Span } from '@sentry/types';
import {
addInstrumentationHandler,
BAGGAGE_HEADER_NAME,
Expand All @@ -7,7 +8,6 @@ import {
mergeAndSerializeBaggage,
} from '@sentry/utils';

import { Span } from '../span';
import { getActiveTransaction, hasTracingEnabled } from '../utils';

export const DEFAULT_TRACING_ORIGINS = ['localhost', /^\//];
Expand Down
40 changes: 37 additions & 3 deletions packages/tracing/src/span.ts
@@ -1,6 +1,15 @@
/* eslint-disable max-lines */
import { Baggage, Primitive, Span as SpanInterface, SpanContext, Transaction } from '@sentry/types';
import { dropUndefinedKeys, timestampWithMs, uuid4 } from '@sentry/utils';
import { getCurrentHub } from '@sentry/hub';
import { Baggage, Hub, Primitive, Span as SpanInterface, SpanContext, Transaction } from '@sentry/types';
import {
createBaggage,
dropUndefinedKeys,
isBaggageEmpty,
isSentryBaggageEmpty,
setBaggageValue,
timestampWithMs,
uuid4,
} from '@sentry/utils';

/**
* Keeps track of finished spans for a given transaction
Expand Down Expand Up @@ -302,7 +311,14 @@ export class Span implements SpanInterface {
* @inheritdoc
*/
public getBaggage(): Baggage | undefined {
return this.transaction && this.transaction.metadata.baggage;
const existingBaggage = this.transaction && this.transaction.metadata.baggage;

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Add sentry baggage data to baggage, only when baggage is either empty or does not exist yet

Took me a while to get what's happening here. Wdyt about adding a comment like this to speed things up for future readers?

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, a note would be good. I slightly rewrote this part (and the condition on when to modify baggage) in the follow-up PR (#5205). Added a note there (50e0d19) instead because adding it here would require it to be rewritten.

const finalBaggage =
!existingBaggage || isSentryBaggageEmpty(existingBaggage)
? this._getBaggageWithSentryValues(existingBaggage)
: existingBaggage;

return isBaggageEmpty(finalBaggage) ? undefined : finalBaggage;
}

/**
Expand Down Expand Up @@ -334,6 +350,24 @@ export class Span implements SpanInterface {
trace_id: this.traceId,
});
}

/**
*
Copy link
Member

Choose a reason for hiding this comment

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

I think we forgot to add a comment here ^^

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, good catch, thanks! Same thing though as above, I made modifications in #5205 (2a95a39) and added the JSdoc there.

* @param baggage
* @returns
*/
private _getBaggageWithSentryValues(baggage: Baggage = createBaggage({})): Baggage {
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-explicit-any
const hub: Hub = ((this.transaction as any) && (this.transaction as any)._hub) || getCurrentHub();
Copy link
Member

Choose a reason for hiding this comment

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

To me this seems a bit dangerous. I was wondering if we can get away with only having getCurrentHub() here, because in a node context, we'll get the right hub anyways because of domains, and in a browser context we only have one proper hub anyways (?).

I'll let you make the final call on this though. It's probably a bit dangerous but should also be fine.

Copy link
Member Author

@Lms24 Lms24 Jun 3, 2022

Choose a reason for hiding this comment

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

I think it should be fine if we take the Hub from the transaction. When creating a Transaction, users can pass in a hub as an option in the TransactionContext. Also for example when calling hub.startTransaction. People who use a multi-hub/client setup (which comes with its own set of issues) do this sometimes.

Do you think it's dangerous because there might be a "wrong" hub in the transaction or because of the casts?

Copy link
Member

Choose a reason for hiding this comment

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

Because of the casts.

const client = hub.getClient();

const { environment, release } = (client && client.getOptions()) || {};

environment && setBaggageValue(baggage, 'environment', environment);
release && setBaggageValue(baggage, 'release', release);

return baggage;
}
}

export type SpanStatusType =
Expand Down
6 changes: 3 additions & 3 deletions packages/tracing/src/transaction.ts
Expand Up @@ -17,12 +17,12 @@ export class Transaction extends SpanClass implements TransactionInterface {

public metadata: TransactionMetadata;

private _measurements: Measurements = {};

/**
* The reference to the current hub.
*/
private readonly _hub: Hub;
protected readonly _hub: Hub;

private _measurements: Measurements = {};

private _trimEnd?: boolean;

Expand Down
44 changes: 40 additions & 4 deletions packages/tracing/test/browser/browsertracing.test.ts
@@ -1,6 +1,6 @@
import { BrowserClient } from '@sentry/browser';
import { Hub, makeMain } from '@sentry/hub';
import { BaggageObj } from '@sentry/types';
import type { BaggageObj, BaseTransportOptions, ClientOptions } from '@sentry/types';
import { getGlobalObject, InstrumentHandlerCallback, InstrumentHandlerType } from '@sentry/utils';
import { JSDOM } from 'jsdom';

Expand Down Expand Up @@ -415,7 +415,16 @@ describe('BrowserTracing', () => {
});
});

describe('using the data', () => {
describe('using the <meta> tag data', () => {
beforeEach(() => {
hub.getClient()!.getOptions = () => {
return {
release: '1.0.0',
environment: 'production',
} as ClientOptions<BaseTransportOptions>;
};
});

it('uses the tracing data for pageload transactions', () => {
// make sampled false here, so we can see that it's being used rather than the tracesSampleRate-dictated one
document.head.innerHTML =
Expand All @@ -439,11 +448,34 @@ describe('BrowserTracing', () => {
expect(baggage[1]).toEqual('foo=bar');
});

it('adds Sentry baggage data to pageload transactions if not present in meta tags', () => {
// make sampled false here, so we can see that it's being used rather than the tracesSampleRate-dictated one
document.head.innerHTML =
'<meta name="sentry-trace" content="12312012123120121231201212312012-1121201211212012-0">' +
'<meta name="baggage" content="foo=bar">';

// pageload transactions are created as part of the BrowserTracing integration's initialization
createBrowserTracing(true);
const transaction = getActiveTransaction(hub) as IdleTransaction;
const baggage = transaction.getBaggage()!;

expect(transaction).toBeDefined();
expect(transaction.op).toBe('pageload');
expect(transaction.traceId).toEqual('12312012123120121231201212312012');
expect(transaction.parentSpanId).toEqual('1121201211212012');
expect(transaction.sampled).toBe(false);
expect(baggage).toBeDefined();
expect(baggage[0]).toBeDefined();
expect(baggage[0]).toEqual({ environment: 'production', release: '1.0.0' });
expect(baggage[1]).toBeDefined();
expect(baggage[1]).toEqual('foo=bar');
});

it('ignores the data for navigation transactions', () => {
mockChangeHistory = () => undefined;
document.head.innerHTML =
'<meta name="sentry-trace" content="12312012123120121231201212312012-1121201211212012-0">' +
'<meta name="baggage" content="sentry-release=2.1.14,foo=bar">';
'<meta name="baggage" content="sentry-release=2.1.14">';

createBrowserTracing(true);

Expand All @@ -455,7 +487,11 @@ describe('BrowserTracing', () => {
expect(transaction.op).toBe('navigation');
expect(transaction.traceId).not.toEqual('12312012123120121231201212312012');
expect(transaction.parentSpanId).toBeUndefined();
expect(baggage).toBeUndefined();
expect(baggage).toBeDefined();
expect(baggage[0]).toBeDefined();
expect(baggage[0]).toEqual({ release: '1.0.0', environment: 'production' });
expect(baggage[1]).toBeDefined();
expect(baggage[1]).toEqual('');
});
});
});
Expand Down
55 changes: 55 additions & 0 deletions packages/tracing/test/span.test.ts
@@ -1,5 +1,7 @@
import { BrowserClient } from '@sentry/browser';
import { Hub, makeMain, Scope } from '@sentry/hub';
import { BaseTransportOptions, ClientOptions } from '@sentry/types';
import { createBaggage, getSentryBaggageItems, getThirdPartyBaggage, isSentryBaggageEmpty } from '@sentry/utils';

import { Span, Transaction } from '../src';
import { TRACEPARENT_REGEXP } from '../src/utils';
Expand Down Expand Up @@ -390,4 +392,57 @@ describe('Span', () => {
expect(span.data).toStrictEqual({ data0: 'foo', data1: 'bar' });
});
});

describe('getBaggage and _getBaggageWithSentryValues', () => {
beforeEach(() => {
hub.getClient()!.getOptions = () => {
Copy link
Member

Choose a reason for hiding this comment

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

Do you think it's necessary to add a cleanup for this? Wanna avoid flakeyness in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking the same thing when I wrote the tests here but then I saw that at the top of this file there already is a beforeEach function. This means that the modifications on the hub from my beforeSend, are reverted as soon as we leave the closure and move to other tests.

Copy link
Member

Choose a reason for hiding this comment

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

Got it. Seems good.

return {
release: '1.0.1',
environment: 'production',
} as ClientOptions<BaseTransportOptions>;
};
});

test('leave baggage content untouched and just return baggage if there already is Sentry content in it', () => {
const transaction = new Transaction(
{
name: 'tx',
metadata: { baggage: createBaggage({ environment: 'myEnv' }, '') },
},
hub,
);

const hubSpy = jest.spyOn(hub.getClient()!, 'getOptions');

const span = transaction.startChild();

const baggage = span.getBaggage();

expect(hubSpy).toHaveBeenCalledTimes(0);
expect(baggage && isSentryBaggageEmpty(baggage)).toBeFalsy();
expect(baggage && getSentryBaggageItems(baggage)).toStrictEqual({ environment: 'myEnv' });
expect(baggage && getThirdPartyBaggage(baggage)).toStrictEqual('');
});

test('add Sentry baggage data to baggage if Sentry content is empty', () => {
const transaction = new Transaction(
{
name: 'tx',
metadata: { baggage: createBaggage({}, '') },
},
hub,
);

const hubSpy = jest.spyOn(hub.getClient()!, 'getOptions');

const span = transaction.startChild();

const baggage = span.getBaggage();

expect(hubSpy).toHaveBeenCalledTimes(1);
expect(baggage && isSentryBaggageEmpty(baggage)).toBeFalsy();
expect(baggage && getSentryBaggageItems(baggage)).toStrictEqual({ release: '1.0.1', environment: 'production' });
expect(baggage && getThirdPartyBaggage(baggage)).toStrictEqual('');
});
});
});
10 changes: 8 additions & 2 deletions packages/utils/src/baggage.ts
Expand Up @@ -31,11 +31,17 @@ export function setBaggageValue(baggage: Baggage, key: keyof BaggageObj, value:
baggage[0][key] = value;
}

/** Check if the baggage object (i.e. the first element in the tuple) is empty */
export function isBaggageEmpty(baggage: Baggage): boolean {
/** Check if the Sentry part of the passed baggage (i.e. the first element in the tuple) is empty */
export function isSentryBaggageEmpty(baggage: Baggage): boolean {
return Object.keys(baggage[0]).length === 0;
}

/** Check if the Sentry part of the passed baggage (i.e. the first element in the tuple) is empty */
export function isBaggageEmpty(baggage: Baggage): boolean {
const thirdPartyBaggage = getThirdPartyBaggage(baggage);
return isSentryBaggageEmpty(baggage) && (thirdPartyBaggage == undefined || thirdPartyBaggage.length === 0);
}

/** Returns Sentry specific baggage values */
export function getSentryBaggageItems(baggage: Baggage): BaggageObj {
return baggage[0];
Expand Down
6 changes: 3 additions & 3 deletions packages/utils/test/baggage.test.ts
@@ -1,7 +1,7 @@
import {
createBaggage,
getBaggageValue,
isBaggageEmpty,
isSentryBaggageEmpty,
mergeAndSerializeBaggage,
parseBaggageString,
serializeBaggage,
Expand Down Expand Up @@ -98,12 +98,12 @@ describe('Baggage', () => {
});
});

describe('isBaggageEmpty', () => {
describe('isSentryBaggageEmpty', () => {
it.each([
['returns true if the modifyable part of baggage is empty', createBaggage({}), true],
['returns false if the modifyable part of baggage is not empty', createBaggage({ release: '10.0.2' }), false],
])('%s', (_: string, baggage, outcome) => {
expect(isBaggageEmpty(baggage)).toEqual(outcome);
expect(isSentryBaggageEmpty(baggage)).toEqual(outcome);
});
});

Expand Down