Skip to content

Commit

Permalink
feat(tracing): Favour client options tracePropagationTargets (#8399)
Browse files Browse the repository at this point in the history
ref #8352

As we work toward adding tracing without performance support, this PR
updates the `BrowserTracing` integration to use and favour the top level
`tracePropagationTargets` option if it exists.

This option was made top level in
#8395

`tracePropagationTargets` is now part of the unified API for distributed
tracing. It's also expected that electron/react native will behave the
same way as well. This also leaves us the flexibility to extract tracing
out of BrowserTracing, or create a new integration that just does
tracing but no performance monitoring.

We can make sure this migration is smooth and easy to understand with a
good set of docs, which is what I will be working on next. In these docs
changes, we'll be updating the automatic instrumentation sections, and
formally documented `tracePropagationTargets` as a high level option.
  • Loading branch information
AbhiPrasad committed Jun 27, 2023
1 parent 0c19446 commit ac435dd
Show file tree
Hide file tree
Showing 2 changed files with 90 additions and 1 deletion.
32 changes: 31 additions & 1 deletion packages/tracing-internal/src/browser/browsertracing.ts
Original file line number Diff line number Diff line change
Expand Up @@ -177,9 +177,19 @@ export class BrowserTracing implements Integration {

private _collectWebVitals: () => void;

private _hasSetTracePropagationTargets: boolean = false;

public constructor(_options?: Partial<BrowserTracingOptions>) {
addTracingExtensions();

if (__DEBUG_BUILD__) {
this._hasSetTracePropagationTargets = !!(
_options &&
// eslint-disable-next-line deprecation/deprecation
(_options.tracePropagationTargets || _options.tracingOrigins)
);
}

this.options = {
...DEFAULT_BROWSER_TRACING_OPTIONS,
..._options,
Expand Down Expand Up @@ -214,6 +224,9 @@ export class BrowserTracing implements Integration {
*/
public setupOnce(_: (callback: EventProcessor) => void, getCurrentHub: () => Hub): void {
this._getCurrentHub = getCurrentHub;
const hub = getCurrentHub();
const client = hub.getClient();
const clientOptions = client && client.getOptions();

const {
routingInstrumentation: instrumentRouting,
Expand All @@ -222,11 +235,28 @@ export class BrowserTracing implements Integration {
markBackgroundTransactions,
traceFetch,
traceXHR,
tracePropagationTargets,
shouldCreateSpanForRequest,
_experiments,
} = this.options;

const clientOptionsTracePropagationTargets = clientOptions && clientOptions.tracePropagationTargets;
// There are three ways to configure tracePropagationTargets:
// 1. via top level client option `tracePropagationTargets`
// 2. via BrowserTracing option `tracePropagationTargets`
// 3. via BrowserTracing option `tracingOrigins` (deprecated)
//
// To avoid confusion, favour top level client option `tracePropagationTargets`, and fallback to
// BrowserTracing option `tracePropagationTargets` and then `tracingOrigins` (deprecated).
// This is done as it minimizes bundle size (we don't have to have undefined checks).
//
// If both 1 and either one of 2 or 3 are set (from above), we log out a warning.
const tracePropagationTargets = clientOptionsTracePropagationTargets || this.options.tracePropagationTargets;
if (__DEBUG_BUILD__ && this._hasSetTracePropagationTargets && clientOptionsTracePropagationTargets) {
logger.warn(
'[Tracing] The `tracePropagationTargets` option was set in the BrowserTracing integration and top level `Sentry.init`. The top level `Sentry.init` value is being used.',
);
}

instrumentRouting(
(context: TransactionContext) => {
const transaction = this._createRouteTransaction(context);
Expand Down
59 changes: 59 additions & 0 deletions packages/tracing-internal/test/browser/browsertracing.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,65 @@ describe('BrowserTracing', () => {
tracePropagationTargets: ['something'],
});
});

it('uses `tracePropagationTargets` set by client over integration set targets', () => {
jest.clearAllMocks();
hub.getClient()!.getOptions().tracePropagationTargets = ['something-else'];
const sampleTracePropagationTargets = ['something'];
createBrowserTracing(true, {
routingInstrumentation: customInstrumentRouting,
tracePropagationTargets: sampleTracePropagationTargets,
});

expect(instrumentOutgoingRequestsMock).toHaveBeenCalledWith({
traceFetch: true,
traceXHR: true,
tracePropagationTargets: ['something-else'],
});
});

it.each([
[true, 'tracePropagationTargets', 'defined', { tracePropagationTargets: ['something'] }],
[false, 'tracePropagationTargets', 'undefined', { tracePropagationTargets: undefined }],
[true, 'tracingOrigins', 'defined', { tracingOrigins: ['something'] }],
[false, 'tracingOrigins', 'undefined', { tracingOrigins: undefined }],
[
true,
'tracePropagationTargets and tracingOrigins',
'defined',
{ tracePropagationTargets: ['something'], tracingOrigins: ['something-else'] },
],
[
false,
'tracePropagationTargets and tracingOrigins',
'undefined',
{ tracePropagationTargets: undefined, tracingOrigins: undefined },
],
[
true,
'tracePropagationTargets and tracingOrigins',
'defined and undefined',
{ tracePropagationTargets: ['something'], tracingOrigins: undefined },
],
[
true,
'tracePropagationTargets and tracingOrigins',
'undefined and defined',
{ tracePropagationTargets: undefined, tracingOrigins: ['something'] },
],
])(
'sets `_hasSetTracePropagationTargets` to %s if %s is %s',
(hasSet: boolean, _: string, __: string, options: Partial<BrowserTracingOptions>) => {
jest.clearAllMocks();
const inst = createBrowserTracing(true, {
routingInstrumentation: customInstrumentRouting,
...options,
});

// @ts-ignore accessing private property
expect(inst._hasSetTracePropagationTargets).toBe(hasSet);
},
);
});

describe('beforeNavigate', () => {
Expand Down

0 comments on commit ac435dd

Please sign in to comment.