Skip to content

Commit 77dd704

Browse files
Lms24Lms24
authored
fix(tracing): Pass tracePropagationTargets to instrumentOutgoingRequests (#6259)
Fix a bug in our `BrowserTracing` integration which caused the new `tracePropagationTargets` option not to be passed to `instrumentOutgoingRequests` where it was needed to decide if our tracing headers should be attached to outgoing requests or not. Because we never passed this value to the instrumentation function, custom-defined `tracePropagationTargets` values were not respected by the SDK and headers were attached to requests whose URLs matched the default targets or custom specified `tracingOrigins`. With this fix, we also make a change how we internally handle the co-existance between the deprecated `tracingOrigins` and `tracePropagationTargets` options. We now simply overwrite the default `tracePropagationTargets` values with custom `tracingOrigins` (if available and no custom `tracePropagationTargets` were set). This enables us to internally only rely on `tracePropagationTargets`. Note that we still have to keep setting `tracingOrigins` to `browserTracing.options`, as removing this field or changing the type would break users. This field however is not used internally anymore. This patch also adds a bunch of unit and integration tests to make sure, `tracePropagationTargets` works as expected this time. Also, the tests check that `tracingOrigins` is still respected properly.
1 parent 74810e0 commit 77dd704

File tree

19 files changed

+270
-41
lines changed

19 files changed

+270
-41
lines changed

‎packages/integration-tests/suites/tracing/browsertracing/tracePropagationTargets/customTargets/init.js

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
import * as Sentry from '@sentry/browser';
2+
import { Integrations } from '@sentry/tracing';
3+
4+
window.Sentry = Sentry;
5+
6+
Sentry.init({
7+
dsn: 'https://public@dsn.ingest.sentry.io/1337',
8+
integrations: [new Integrations.BrowserTracing({ tracePropagationTargets: ['http://example.com'] })],
9+
tracesSampleRate: 1,
10+
});

‎packages/integration-tests/suites/tracing/browsertracing/tracePropagationTargets/customTargets/subject.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
fetch('http://example.com/0').then(fetch('http://example.com/1').then(fetch('http://example.com/2')));

‎packages/integration-tests/suites/tracing/browsertracing/tracePropagationTargets/customTargets/test.ts

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
import { expect, Request } from '@playwright/test';
2+
3+
import { sentryTest } from '../../../../../utils/fixtures';
4+
5+
sentryTest(
6+
'should attach `sentry-trace` and `baggage` header to request matching tracePropagationTargets',
7+
async ({ getLocalTestPath, page }) => {
8+
const url = await getLocalTestPath({ testDir: __dirname });
9+
10+
const requests = (
11+
await Promise.all([
12+
page.goto(url),
13+
Promise.all([0, 1, 2].map(idx => page.waitForRequest(`http://example.com/${idx}`))),
14+
])
15+
)[1];
16+
17+
expect(requests).toHaveLength(3);
18+
19+
requests?.forEach(async (request: Request) => {
20+
const requestHeaders = await request.allHeaders();
21+
expect(requestHeaders).toMatchObject({
22+
'sentry-trace': expect.any(String),
23+
baggage: expect.any(String),
24+
});
25+
});
26+
},
27+
);

‎packages/integration-tests/suites/tracing/browsertracing/tracePropagationTargets/customTargetsAndOrigins/init.js

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
import * as Sentry from '@sentry/browser';
2+
import { Integrations } from '@sentry/tracing';
3+
4+
window.Sentry = Sentry;
5+
6+
Sentry.init({
7+
dsn: 'https://public@dsn.ingest.sentry.io/1337',
8+
integrations: [
9+
new Integrations.BrowserTracing({ tracePropagationTargets: [], tracingOrigins: ['http://example.com'] }),
10+
],
11+
tracesSampleRate: 1,
12+
});

‎packages/integration-tests/suites/tracing/browsertracing/tracePropagationTargets/customTargetsAndOrigins/subject.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
fetch('http://example.com/0').then(fetch('http://example.com/1').then(fetch('http://example.com/2')));

‎packages/integration-tests/suites/tracing/browsertracing/tracePropagationTargets/customTargetsAndOrigins/test.ts

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
import { expect, Request } from '@playwright/test';
2+
3+
import { sentryTest } from '../../../../../utils/fixtures';
4+
5+
sentryTest(
6+
'[pre-v8] should prefer custom tracePropagationTargets over tracingOrigins',
7+
async ({ getLocalTestPath, page }) => {
8+
const url = await getLocalTestPath({ testDir: __dirname });
9+
10+
const requests = (
11+
await Promise.all([
12+
page.goto(url),
13+
Promise.all([0, 1, 2].map(idx => page.waitForRequest(`http://example.com/${idx}`))),
14+
])
15+
)[1];
16+
17+
expect(requests).toHaveLength(3);
18+
19+
requests?.forEach(async (request: Request) => {
20+
const requestHeaders = await request.allHeaders();
21+
expect(requestHeaders).not.toMatchObject({
22+
'sentry-trace': expect.any(String),
23+
baggage: expect.any(String),
24+
});
25+
});
26+
},
27+
);

‎packages/integration-tests/suites/tracing/browsertracing/tracePropagationTargets/customTracingOrigins/init.js

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
import * as Sentry from '@sentry/browser';
2+
import { Integrations } from '@sentry/tracing';
3+
4+
window.Sentry = Sentry;
5+
6+
Sentry.init({
7+
dsn: 'https://public@dsn.ingest.sentry.io/1337',
8+
integrations: [new Integrations.BrowserTracing({ tracingOrigins: ['http://example.com'] })],
9+
tracesSampleRate: 1,
10+
});

‎packages/integration-tests/suites/tracing/browsertracing/tracePropagationTargets/customTracingOrigins/subject.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
fetch('http://example.com/0').then(fetch('http://example.com/1').then(fetch('http://example.com/2')));

‎packages/integration-tests/suites/tracing/browsertracing/tracePropagationTargets/customTracingOrigins/test.ts

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
import { expect, Request } from '@playwright/test';
2+
3+
import { sentryTest } from '../../../../../utils/fixtures';
4+
5+
sentryTest(
6+
'[pre-v8] should attach `sentry-trace` and `baggage` header to request matching tracingOrigins',
7+
async ({ getLocalTestPath, page }) => {
8+
const url = await getLocalTestPath({ testDir: __dirname });
9+
10+
const requests = (
11+
await Promise.all([
12+
page.goto(url),
13+
Promise.all([0, 1, 2].map(idx => page.waitForRequest(`http://example.com/${idx}`))),
14+
])
15+
)[1];
16+
17+
expect(requests).toHaveLength(3);
18+
19+
requests?.forEach(async (request: Request) => {
20+
const requestHeaders = await request.allHeaders();
21+
expect(requestHeaders).toMatchObject({
22+
'sentry-trace': expect.any(String),
23+
baggage: expect.any(String),
24+
});
25+
});
26+
},
27+
);

‎packages/integration-tests/suites/tracing/browsertracing/tracePropagationTargets/defaultTargetsMatch/init.js

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
import * as Sentry from '@sentry/browser';
2+
import { Integrations } from '@sentry/tracing';
3+
4+
window.Sentry = Sentry;
5+
6+
Sentry.init({
7+
dsn: 'https://public@dsn.ingest.sentry.io/1337',
8+
integrations: [new Integrations.BrowserTracing()],
9+
tracesSampleRate: 1,
10+
});

‎packages/integration-tests/suites/tracing/browsertracing/tracePropagationTargets/defaultTargetsMatch/subject.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
fetch('http://localhost:4200/0').then(fetch('http://localhost:4200/1').then(fetch('http://localhost:4200/2')));

‎packages/integration-tests/suites/tracing/browsertracing/tracePropagationTargets/defaultTargetsMatch/test.ts

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
import { expect, Request } from '@playwright/test';
2+
3+
import { sentryTest } from '../../../../../utils/fixtures';
4+
5+
sentryTest(
6+
'should attach `sentry-trace` and `baggage` header to request matching default tracePropagationTargets',
7+
async ({ getLocalTestPath, page }) => {
8+
const url = await getLocalTestPath({ testDir: __dirname });
9+
10+
const requests = (
11+
await Promise.all([
12+
page.goto(url),
13+
Promise.all([0, 1, 2].map(idx => page.waitForRequest(`http://localhost:4200/${idx}`))),
14+
])
15+
)[1];
16+
17+
expect(requests).toHaveLength(3);
18+
19+
requests?.forEach(async (request: Request) => {
20+
const requestHeaders = await request.allHeaders();
21+
expect(requestHeaders).toMatchObject({
22+
'sentry-trace': expect.any(String),
23+
baggage: expect.any(String),
24+
});
25+
});
26+
},
27+
);

‎packages/integration-tests/suites/tracing/browsertracing/tracePropagationTargets/defaultTargetsNoMatch/init.js

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
import * as Sentry from '@sentry/browser';
2+
import { Integrations } from '@sentry/tracing';
3+
4+
window.Sentry = Sentry;
5+
6+
Sentry.init({
7+
dsn: 'https://public@dsn.ingest.sentry.io/1337',
8+
integrations: [new Integrations.BrowserTracing()],
9+
tracesSampleRate: 1,
10+
});

‎packages/integration-tests/suites/tracing/browsertracing/tracePropagationTargets/defaultTargetsNoMatch/subject.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
fetch('http://example.com/0').then(fetch('http://example.com/1').then(fetch('http://example.com/2')));

‎packages/integration-tests/suites/tracing/browsertracing/tracePropagationTargets/defaultTargetsNoMatch/test.ts

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
import { expect, Request } from '@playwright/test';
2+
3+
import { sentryTest } from '../../../../../utils/fixtures';
4+
5+
sentryTest(
6+
'should not attach `sentry-trace` and `baggage` header to request not matching default tracePropagationTargets',
7+
async ({ getLocalTestPath, page }) => {
8+
const url = await getLocalTestPath({ testDir: __dirname });
9+
10+
const requests = (
11+
await Promise.all([
12+
page.goto(url),
13+
Promise.all([0, 1, 2].map(idx => page.waitForRequest(`http://example.com/${idx}`))),
14+
])
15+
)[1];
16+
17+
expect(requests).toHaveLength(3);
18+
19+
requests?.forEach(async (request: Request) => {
20+
const requestHeaders = await request.allHeaders();
21+
expect(requestHeaders).not.toMatchObject({
22+
'sentry-trace': expect.any(String),
23+
baggage: expect.any(String),
24+
});
25+
});
26+
},
27+
);

‎packages/tracing/src/browser/browsertracing.ts

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,7 @@ export interface BrowserTracingOptions extends RequestInstrumentationOptions {
112112
): void;
113113
}
114114

115-
const DEFAULT_BROWSER_TRACING_OPTIONS = {
115+
const DEFAULT_BROWSER_TRACING_OPTIONS: BrowserTracingOptions = {
116116
idleTimeout: DEFAULT_IDLE_TIMEOUT,
117117
finalTimeout: DEFAULT_FINAL_TIMEOUT,
118118
heartbeatInterval: DEFAULT_HEARTBEAT_INTERVAL,
@@ -153,6 +153,15 @@ export class BrowserTracing implements Integration {
153153
..._options,
154154
};
155155

156+
// TODO (v8): remove this block after tracingOrigins is removed
157+
// Set tracePropagationTargets to tracingOrigins if specified by the user
158+
// In case both are specified, tracePropagationTargets takes precedence
159+
// eslint-disable-next-line deprecation/deprecation
160+
if (_options && !_options.tracePropagationTargets && _options.tracingOrigins) {
161+
// eslint-disable-next-line deprecation/deprecation
162+
this.options.tracePropagationTargets = _options.tracingOrigins;
163+
}
164+
156165
const { _metricOptions } = this.options;
157166
startTrackingWebVitals(_metricOptions && _metricOptions._reportAllChanges);
158167
if (this.options._experiments?.enableLongTask) {
@@ -174,8 +183,7 @@ export class BrowserTracing implements Integration {
174183
markBackgroundTransactions,
175184
traceFetch,
176185
traceXHR,
177-
// eslint-disable-next-line deprecation/deprecation
178-
tracingOrigins,
186+
tracePropagationTargets,
179187
shouldCreateSpanForRequest,
180188
} = this.options;
181189

@@ -189,7 +197,12 @@ export class BrowserTracing implements Integration {
189197
registerBackgroundTabDetection();
190198
}
191199

192-
instrumentOutgoingRequests({ traceFetch, traceXHR, tracingOrigins, shouldCreateSpanForRequest });
200+
instrumentOutgoingRequests({
201+
traceFetch,
202+
traceXHR,
203+
tracePropagationTargets,
204+
shouldCreateSpanForRequest,
205+
});
193206
}
194207

195208
/** Create routing idle transaction. */

‎packages/tracing/src/browser/request.ts

Lines changed: 7 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,7 @@ export const defaultRequestInstrumentationOptions: RequestInstrumentationOptions
113113
/** Registers span creators for xhr and fetch requests */
114114
export function instrumentOutgoingRequests(_options?: Partial<RequestInstrumentationOptions>): void {
115115
// eslint-disable-next-line deprecation/deprecation
116-
const { traceFetch, traceXHR, tracingOrigins, tracePropagationTargets, shouldCreateSpanForRequest } = {
116+
const { traceFetch, traceXHR, tracePropagationTargets, tracingOrigins, shouldCreateSpanForRequest } = {
117117
traceFetch: defaultRequestInstrumentationOptions.traceFetch,
118118
traceXHR: defaultRequestInstrumentationOptions.traceXHR,
119119
..._options,
@@ -122,8 +122,11 @@ export function instrumentOutgoingRequests(_options?: Partial<RequestInstrumenta
122122
const shouldCreateSpan =
123123
typeof shouldCreateSpanForRequest === 'function' ? shouldCreateSpanForRequest : (_: string) => true;
124124

125+
// TODO(v8) Remove tracingOrigins here
126+
// The only reason we're passing it in here is because this instrumentOutgoingRequests function is publicly exported
127+
// and we don't want to break the API. We can remove it in v8.
125128
const shouldAttachHeadersWithTargets = (url: string): boolean =>
126-
shouldAttachHeaders(url, tracingOrigins, tracePropagationTargets);
129+
shouldAttachHeaders(url, tracePropagationTargets || tracingOrigins);
127130

128131
const spans: Record<string, Span> = {};
129132

@@ -144,20 +147,9 @@ export function instrumentOutgoingRequests(_options?: Partial<RequestInstrumenta
144147
* A function that determines whether to attach tracing headers to a request.
145148
* This was extracted from `instrumentOutgoingRequests` to make it easier to test shouldAttachHeaders.
146149
* We only export this fuction for testing purposes.
147-
*
148-
* TODO (v8): Remove `tracingOrigins` which should drastically simplify this function.
149150
*/
150-
export function shouldAttachHeaders(
151-
url: string,
152-
tracePropagationTargets: (string | RegExp)[] | undefined,
153-
tracingOrigins: (string | RegExp)[] | undefined,
154-
): boolean {
155-
// TODO (v8): Replace the entire code below with this one-liner:
156-
// return stringMatchesSomePattern(url, tracePropagationTargets || DEFAULT_TRACE_PROPAGATION_TARGETS);
157-
if (tracePropagationTargets || tracingOrigins) {
158-
return stringMatchesSomePattern(url, tracePropagationTargets || tracingOrigins);
159-
}
160-
return stringMatchesSomePattern(url, DEFAULT_TRACE_PROPAGATION_TARGETS);
151+
export function shouldAttachHeaders(url: string, tracePropagationTargets: (string | RegExp)[] | undefined): boolean {
152+
return stringMatchesSomePattern(url, tracePropagationTargets || DEFAULT_TRACE_PROPAGATION_TARGETS);
161153
}
162154

163155
/**

‎packages/tracing/test/browser/browsertracing.test.ts

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,15 @@ jest.mock('@sentry/utils', () => {
3434

3535
jest.mock('../../src/browser/metrics');
3636

37+
const instrumentOutgoingRequestsMock = jest.fn();
38+
jest.mock('./../../src/browser/request', () => {
39+
const actual = jest.requireActual('./../../src/browser/request');
40+
return {
41+
...actual,
42+
instrumentOutgoingRequests: (options: Partial<BrowserTracingOptions>) => instrumentOutgoingRequestsMock(options),
43+
};
44+
});
45+
3746
beforeAll(() => {
3847
const dom = new JSDOM();
3948
// @ts-ignore need to override global document
@@ -128,6 +137,7 @@ describe('BrowserTracing', () => {
128137
expect(transaction.endTimestamp).toBe(span.endTimestamp);
129138
});
130139

140+
// TODO (v8): remove these tests
131141
describe('tracingOrigins', () => {
132142
it('sets tracing origins if provided and does not warn', () => {
133143
const sampleTracingOrigins = ['something'];
@@ -152,6 +162,43 @@ describe('BrowserTracing', () => {
152162
});
153163
});
154164

165+
describe('tracePropagationTargets', () => {
166+
it('sets tracePropagationTargets if provided', () => {
167+
const sampleTracePropagationTargets = ['something'];
168+
const inst = createBrowserTracing(true, {
169+
routingInstrumentation: customInstrumentRouting,
170+
tracePropagationTargets: sampleTracePropagationTargets,
171+
});
172+
173+
expect(inst.options.tracePropagationTargets).toEqual(sampleTracePropagationTargets);
174+
});
175+
176+
it('sets tracePropagationTargets to an empty array and does not warn', () => {
177+
const sampleTracePropagationTargets: string[] = [];
178+
const inst = createBrowserTracing(true, {
179+
routingInstrumentation: customInstrumentRouting,
180+
tracePropagationTargets: sampleTracePropagationTargets,
181+
});
182+
183+
expect(inst.options.tracePropagationTargets).toEqual(sampleTracePropagationTargets);
184+
});
185+
186+
it('correctly passes tracePropagationTargets to `instrumentOutgoingRequests` in `setupOnce`', () => {
187+
jest.clearAllMocks();
188+
const sampleTracePropagationTargets = ['something'];
189+
createBrowserTracing(true, {
190+
routingInstrumentation: customInstrumentRouting,
191+
tracePropagationTargets: sampleTracePropagationTargets,
192+
});
193+
194+
expect(instrumentOutgoingRequestsMock).toHaveBeenCalledWith({
195+
traceFetch: true,
196+
traceXHR: true,
197+
tracePropagationTargets: ['something'],
198+
});
199+
});
200+
});
201+
155202
describe('beforeNavigate', () => {
156203
it('is called on transaction creation', () => {
157204
const mockBeforeNavigation = jest.fn().mockReturnValue({ name: 'here/is/my/path' });

0 commit comments

Comments
 (0)