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.

Sign up for GitHub

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

Jump to bottom

ref(tracing): Deprecate tracingOrigins #6176

Merged
merged 2 commits into from
Nov 10, 2022
Merged

ref(tracing): Deprecate tracingOrigins #6176

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4ee4d18
Deprecate tracingOrigins
timfish Nov 9, 2022
4c07f8b
Fix test lint issues
timfish Nov 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/nextjs/src/index.client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ export function init(options: NextjsOptions): void {

function createClientIntegrations(userIntegrations: UserIntegrations = []): UserIntegrations {
const defaultBrowserTracingIntegration = new BrowserTracing({
// eslint-disable-next-line deprecation/deprecation
tracingOrigins: [...defaultRequestInstrumentationOptions.tracingOrigins, /^(api\/)/],
routingInstrumentation: nextRouterInstrumentation,
});
Expand Down
25 changes: 1 addition & 24 deletions packages/tracing/src/browser/browsertracing.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,23 +147,10 @@ export class BrowserTracing implements Integration {

private _getCurrentHub?: () => Hub;

private readonly _emitOptionsWarning?: boolean;

public constructor(_options?: Partial<BrowserTracingOptions>) {
let tracingOrigins = defaultRequestInstrumentationOptions.tracingOrigins;
// NOTE: Logger doesn't work in constructors, as it's initialized after integrations instances
if (_options) {
if (_options.tracingOrigins && Array.isArray(_options.tracingOrigins)) {
tracingOrigins = _options.tracingOrigins;
} else {
__DEBUG_BUILD__ && (this._emitOptionsWarning = true);
}
}

this.options = {
...DEFAULT_BROWSER_TRACING_OPTIONS,
..._options,
tracingOrigins,
};

const { _metricOptions } = this.options;
Expand All @@ -179,17 +166,6 @@ export class BrowserTracing implements Integration {
public setupOnce(_: (callback: EventProcessor) => void, getCurrentHub: () => Hub): void {
this._getCurrentHub = getCurrentHub;

if (this._emitOptionsWarning) {
__DEBUG_BUILD__ &&
logger.warn(
'[Tracing] You need to define `tracingOrigins` in the options. Set an array of urls or patterns to trace.',
);
__DEBUG_BUILD__ &&
logger.warn(
`[Tracing] We added a reasonable default for you: ${defaultRequestInstrumentationOptions.tracingOrigins}`,
);
}

// eslint-disable-next-line @typescript-eslint/unbound-method
const {
routingInstrumentation: instrumentRouting,
Expand All @@ -198,6 +174,7 @@ export class BrowserTracing implements Integration {
markBackgroundTransactions,
traceFetch,
traceXHR,
// eslint-disable-next-line deprecation/deprecation
tracingOrigins,
shouldCreateSpanForRequest,
} = this.options;
Expand Down
10 changes: 5 additions & 5 deletions packages/tracing/src/browser/request.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,9 @@ export const DEFAULT_TRACE_PROPAGATION_TARGETS = ['localhost', /^\//];
/** Options for Request Instrumentation */
export interface RequestInstrumentationOptions {
/**
* List of strings / regex where the integration should create Spans out of. Additionally this will be used
* to define which outgoing requests the `sentry-trace` header will be attached to.
*
* Default: ['localhost', /^\//] {@see DEFAULT_TRACING_ORIGINS}
* @deprecated Will be removed in v8.
* Use `shouldCreateSpanForRequest` to control span creation and `tracePropagationTargets` to control
* trace header attachment.
*/
tracingOrigins: Array<string | RegExp>;

Expand Down Expand Up @@ -50,7 +49,7 @@ export interface RequestInstrumentationOptions {
* This function will be called before creating a span for a request with the given url.
* Return false if you don't want a span for the given url.
*
* By default it uses the `tracingOrigins` options as a url match.
* Default: (url: string) => true
*/
shouldCreateSpanForRequest?(url: string): boolean;
}
Expand Down Expand Up @@ -114,6 +113,7 @@ export const defaultRequestInstrumentationOptions: RequestInstrumentationOptions

/** Registers span creators for xhr and fetch requests */
export function instrumentOutgoingRequests(_options?: Partial<RequestInstrumentationOptions>): void {
// eslint-disable-next-line deprecation/deprecation
const { traceFetch, traceXHR, tracingOrigins, tracePropagationTargets, shouldCreateSpanForRequest } = {
...defaultRequestInstrumentationOptions,
..._options,
Expand Down
28 changes: 2 additions & 26 deletions packages/tracing/test/browser/browsertracing.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,6 @@ jest.mock('@sentry/utils', () => {

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

const { logger } = jest.requireActual('@sentry/utils');
const warnSpy = jest.spyOn(logger, 'warn');

beforeAll(() => {
const dom = new JSDOM();
// @ts-ignore need to override global document
Expand All @@ -55,8 +52,6 @@ describe('BrowserTracing', () => {
hub = new Hub(new BrowserClient(options));
makeMain(hub);
document.head.innerHTML = '';

warnSpy.mockClear();
});

afterEach(() => {
Expand Down Expand Up @@ -134,33 +129,14 @@ describe('BrowserTracing', () => {
});

describe('tracingOrigins', () => {
it('warns and uses default tracing origins if none are provided', () => {
const inst = createBrowserTracing(true, {
routingInstrumentation: customInstrumentRouting,
});

expect(warnSpy).toHaveBeenCalledTimes(2);
expect(inst.options.tracingOrigins).toEqual(defaultRequestInstrumentationOptions.tracingOrigins);
});

it('warns and uses default tracing origins if tracing origins are not defined', () => {
const inst = createBrowserTracing(true, {
routingInstrumentation: customInstrumentRouting,
tracingOrigins: undefined,
});

expect(warnSpy).toHaveBeenCalledTimes(2);
expect(inst.options.tracingOrigins).toEqual(defaultRequestInstrumentationOptions.tracingOrigins);
});

it('sets tracing origins if provided and does not warn', () => {
const sampleTracingOrigins = ['something'];
const inst = createBrowserTracing(true, {
routingInstrumentation: customInstrumentRouting,
tracingOrigins: sampleTracingOrigins,
});

expect(warnSpy).toHaveBeenCalledTimes(0);
// eslint-disable-next-line deprecation/deprecation
expect(inst.options.tracingOrigins).toEqual(sampleTracingOrigins);
});

Expand All @@ -171,7 +147,7 @@ describe('BrowserTracing', () => {
tracingOrigins: sampleTracingOrigins,
});

expect(warnSpy).toHaveBeenCalledTimes(0);
// eslint-disable-next-line deprecation/deprecation
expect(inst.options.tracingOrigins).toEqual(sampleTracingOrigins);
});
});
Expand Down