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

feat(tracing): Add tracePropagationTargets option for adding sentry-trace header #6041

Closed
wants to merge 2 commits into from
Closed

feat(tracing): Add tracePropagationTargets option for adding sentry-trace header #6041

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
f8a01c9
Add `tracePropagationTargets` and combine with `tracingOrigins` for a…
timfish Oct 25, 2022
fbdb1a4
Fix tests
timfish Oct 25, 2022
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
39 changes: 27 additions & 12 deletions packages/tracing/src/browser/request.ts
View file
Open in desktop
Expand Up @@ -22,6 +22,13 @@ export interface RequestInstrumentationOptions {
*/
tracingOrigins: Array<string | RegExp>;

/**
* List of strings / regex used to define which outgoing requests the `sentry-trace` header will be attached to.
*
* Default: ['localhost', /^\//] {@see DEFAULT_TRACING_ORIGINS}
*/
tracePropagationTargets: Array<string | RegExp>;

/**
* Flag to disable patching all together for fetch requests.
*
Expand Down Expand Up @@ -99,12 +106,12 @@ export const defaultRequestInstrumentationOptions: RequestInstrumentationOptions
traceFetch: true,
traceXHR: true,
tracingOrigins: DEFAULT_TRACING_ORIGINS,
tracePropagationTargets: DEFAULT_TRACING_ORIGINS,
Copy link
Member

Choose a reason for hiding this comment

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

For simplicity when it comes time to delete tracingOrigins, can we give this its own DEFAULT_TRACE_PROPAGATION_TARGETS variable? That way, we won't have to change this line when we do that.

};

/** Registers span creators for xhr and fetch requests */
export function instrumentOutgoingRequests(_options?: Partial<RequestInstrumentationOptions>): void {
// eslint-disable-next-line @typescript-eslint/unbound-method
const { traceFetch, traceXHR, tracingOrigins, shouldCreateSpanForRequest } = {
const { traceFetch, traceXHR, tracingOrigins, tracePropagationTargets, shouldCreateSpanForRequest } = {
...defaultRequestInstrumentationOptions,
..._options,
};
Expand Down Expand Up @@ -133,17 +140,21 @@ export function instrumentOutgoingRequests(_options?: Partial<RequestInstrumenta
};
}

const shouldAttachHeaders = (url: string): boolean =>
tracingOrigins.some(origin => isMatchingPattern(url, origin)) ||
tracePropagationTargets.some(origin => isMatchingPattern(url, origin));
Comment on lines +143 to +145
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 torn between loving the simplicity of this implementation and thinking we should include the kind of result-caching which is being pulled out of shouldCreateSpan in #6039.

Realistically, in a modern browser on modern hardware, maybe it doesn't make enough of a difference to be worth the extra bytes. Opinions, @getsentry/team-web-sdk-frontend?

Copy link
Member

Choose a reason for hiding this comment

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

as the person who put the cache setup there, it was a bit of a premature optimization.

I think it shouldn’t be that hard to add it back, so let’s just proceed with this.


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

if (traceFetch) {
addInstrumentationHandler('fetch', (handlerData: FetchData) => {
fetchCallback(handlerData, shouldCreateSpan, spans);
fetchCallback(handlerData, shouldCreateSpan, shouldAttachHeaders, spans);
});
}

if (traceXHR) {
addInstrumentationHandler('xhr', (handlerData: XHRData) => {
xhrCallback(handlerData, shouldCreateSpan, spans);
xhrCallback(handlerData, shouldCreateSpan, shouldAttachHeaders, spans);
});
}
}
Expand All @@ -154,6 +165,7 @@ export function instrumentOutgoingRequests(_options?: Partial<RequestInstrumenta
export function fetchCallback(
handlerData: FetchData,
shouldCreateSpan: (url: string) => boolean,
shouldAttachHeaders: (url: string) => boolean,
spans: Record<string, Span>,
): void {
if (!hasTracingEnabled() || !(handlerData.fetchData && shouldCreateSpan(handlerData.fetchData.url))) {
Expand Down Expand Up @@ -203,14 +215,16 @@ export function fetchCallback(
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const options: { [key: string]: any } = handlerData.args[1];

options.headers = addTracingHeadersToFetchRequest(
request,
activeTransaction.getDynamicSamplingContext(),
span,
options,
);
if (shouldAttachHeaders(handlerData.fetchData.url)) {
options.headers = addTracingHeadersToFetchRequest(
request,
activeTransaction.getDynamicSamplingContext(),
span,
options,
);

activeTransaction.metadata.propagations += 1;
activeTransaction.metadata.propagations += 1;
}
}
}

Expand Down Expand Up @@ -284,6 +298,7 @@ function addTracingHeadersToFetchRequest(
export function xhrCallback(
handlerData: XHRData,
shouldCreateSpan: (url: string) => boolean,
shouldAttachHeaders: (url: string) => boolean,
spans: Record<string, Span>,
): void {
if (
Expand Down Expand Up @@ -329,7 +344,7 @@ export function xhrCallback(
handlerData.xhr.__sentry_xhr_span_id__ = span.spanId;
spans[handlerData.xhr.__sentry_xhr_span_id__] = span;

if (handlerData.xhr.setRequestHeader) {
if (handlerData.xhr.setRequestHeader && shouldAttachHeaders(handlerData.xhr.__sentry_xhr__.url)) {
try {
handlerData.xhr.setRequestHeader('sentry-trace', span.toTraceparent());

Expand Down
52 changes: 27 additions & 25 deletions packages/tracing/test/browser/request.test.ts
View file
Open in desktop
Expand Up @@ -47,8 +47,8 @@ describe('instrumentOutgoingRequests', () => {
describe('callbacks', () => {
let hub: Hub;
let transaction: Transaction;
const alwaysCreateSpan = () => true;
const neverCreateSpan = () => false;
const always = () => true;
const never = () => false;
Comment on lines +50 to +51
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
const always = () => true;
const never = () => false;
const alwaysCreateSpan = () => true;
const neverCreateSpan = () => false;
const alwaysAttachHeaders = () => true;
const neverAttachHeaders = () => false;

I get that to a certain extent this is redundant, but then it allows your tests below to look like

fetchCallback(fetchHandlerData, alwaysCreateSpan, neverAttachHeaders, spans)

rather than

fetchCallback(fetchHandlerData, always, never, spans)

which is significantly more self-documenting.

const startTimestamp = 1356996072000;
const endTimestamp = 1356996072000;
const fetchHandlerData: FetchData = {
Expand Down Expand Up @@ -87,7 +87,7 @@ describe('callbacks', () => {
it('does not create span if shouldCreateSpan returns false', () => {
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 combine this and the 'adds sentry-trace header to fetch requests' test below into a group of four tests testing the four combinations of always and never for both shouldCreateSpan and shouldAttachHeaders, each testing both span creation and header addition?

const spans = {};

fetchCallback(fetchHandlerData, neverCreateSpan, spans);
fetchCallback(fetchHandlerData, never, never, spans);

expect(spans).toEqual({});
});
Expand All @@ -96,15 +96,15 @@ describe('callbacks', () => {
const noFetchData = { args: fetchHandlerData.args, startTimestamp: fetchHandlerData.startTimestamp };
Copy link
Member

Choose a reason for hiding this comment

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

It won't let me put this on the line above with the actual test name for some reason, but can this become a 'neither creates span nor adds headers if there is no fetch data in handler data' test, using always, always rather than always, never?

const spans = {};

fetchCallback(noFetchData, alwaysCreateSpan, spans);
fetchCallback(noFetchData, always, never, spans);
expect(spans).toEqual({});
});

it('does not add fetch request spans if tracing is disabled', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Similarly, we could combine this test and the 'does not add fetch request headers if tracing is disabled' test below into a 'neither creates span nor adds headers if tracing is disabled' test. And again similarly, I think it should be always, always rather than always, never, to show that tracing being disabled takes precedence over both of the other options.

hasTracingEnabled.mockReturnValueOnce(false);
const spans = {};

fetchCallback(fetchHandlerData, alwaysCreateSpan, spans);
fetchCallback(fetchHandlerData, always, never, spans);
expect(spans).toEqual({});
});

Expand All @@ -118,7 +118,7 @@ describe('callbacks', () => {
startTimestamp: 1353501072000,
};

fetchCallback(handlerData, alwaysCreateSpan, {});
fetchCallback(handlerData, always, never, {});

const headers = (handlerData.args[1].headers as Record<string, string>) || {};
expect(headers['sentry-trace']).not.toBeDefined();
Expand All @@ -128,7 +128,7 @@ describe('callbacks', () => {
const spans = {};

// triggered by request being sent
fetchCallback(fetchHandlerData, alwaysCreateSpan, spans);
fetchCallback(fetchHandlerData, always, never, spans);

const newSpan = transaction.spanRecorder?.spans[1] as Span;

Expand All @@ -149,7 +149,7 @@ describe('callbacks', () => {
};

// triggered by response coming back
fetchCallback(postRequestFetchHandlerData, alwaysCreateSpan, spans);
fetchCallback(postRequestFetchHandlerData, always, never, spans);

expect(newSpan.endTimestamp).toBeDefined();
});
Expand All @@ -158,7 +158,7 @@ describe('callbacks', () => {
const spans: Record<string, Span> = {};

// triggered by request being sent
fetchCallback(fetchHandlerData, alwaysCreateSpan, spans);
fetchCallback(fetchHandlerData, always, never, spans);

const newSpan = transaction.spanRecorder?.spans[1] as Span;

Expand All @@ -171,7 +171,7 @@ describe('callbacks', () => {
};

// triggered by response coming back
fetchCallback(postRequestFetchHandlerData, alwaysCreateSpan, spans);
fetchCallback(postRequestFetchHandlerData, always, never, spans);

expect(newSpan.status).toBe(spanStatusfromHttpCode(404));
});
Expand All @@ -186,7 +186,7 @@ describe('callbacks', () => {
};

// in that case, the response coming back will be ignored
fetchCallback(postRequestFetchHandlerData, alwaysCreateSpan, {});
fetchCallback(postRequestFetchHandlerData, always, never, {});

const newSpan = transaction.spanRecorder?.spans[1];

Expand All @@ -199,23 +199,25 @@ describe('callbacks', () => {

expect(transaction.metadata.propagations).toBe(0);

fetchCallback(firstReqData, alwaysCreateSpan, {});
fetchCallback(firstReqData, always, always, {});
expect(transaction.metadata.propagations).toBe(1);

fetchCallback(secondReqData, alwaysCreateSpan, {});
fetchCallback(secondReqData, always, always, {});
expect(transaction.metadata.propagations).toBe(2);
});

it('adds sentry-trace header to fetch requests', () => {
// TODO
fetchCallback(fetchHandlerData, always, always, {});

expect(fetchHandlerData.args[1].headers['sentry-trace']).toBeDefined();
});
});

describe('xhrCallback()', () => {
it('does not create span if shouldCreateSpan returns false', () => {
const spans = {};

xhrCallback(xhrHandlerData, neverCreateSpan, spans);
xhrCallback(xhrHandlerData, never, never, spans);

expect(spans).toEqual({});
});
Expand All @@ -224,20 +226,20 @@ describe('callbacks', () => {
hasTracingEnabled.mockReturnValueOnce(false);
const spans = {};

xhrCallback(xhrHandlerData, alwaysCreateSpan, spans);
xhrCallback(xhrHandlerData, always, never, spans);
expect(spans).toEqual({});
});

it('does not add xhr request headers if tracing is disabled', () => {
hasTracingEnabled.mockReturnValueOnce(false);

xhrCallback(xhrHandlerData, alwaysCreateSpan, {});
xhrCallback(xhrHandlerData, always, always, {});

expect(setRequestHeader).not.toHaveBeenCalled();
});

it('adds sentry-trace header to XHR requests', () => {
xhrCallback(xhrHandlerData, alwaysCreateSpan, {});
xhrCallback(xhrHandlerData, always, always, {});

expect(setRequestHeader).toHaveBeenCalledWith(
'sentry-trace',
Expand All @@ -249,7 +251,7 @@ describe('callbacks', () => {
const spans = {};

// triggered by request being sent
xhrCallback(xhrHandlerData, alwaysCreateSpan, spans);
xhrCallback(xhrHandlerData, always, never, spans);

const newSpan = transaction.spanRecorder?.spans[1] as Span;

Expand All @@ -270,7 +272,7 @@ describe('callbacks', () => {
};

// triggered by response coming back
xhrCallback(postRequestXHRHandlerData, alwaysCreateSpan, spans);
xhrCallback(postRequestXHRHandlerData, always, never, spans);

expect(newSpan.endTimestamp).toBeDefined();
});
Expand All @@ -279,7 +281,7 @@ describe('callbacks', () => {
const spans = {};

// triggered by request being sent
xhrCallback(xhrHandlerData, alwaysCreateSpan, spans);
xhrCallback(xhrHandlerData, always, never, spans);

const newSpan = transaction.spanRecorder?.spans[1] as Span;

Expand All @@ -292,7 +294,7 @@ describe('callbacks', () => {
postRequestXHRHandlerData.xhr.__sentry_xhr__.status_code = 404;

// triggered by response coming back
xhrCallback(postRequestXHRHandlerData, alwaysCreateSpan, spans);
xhrCallback(postRequestXHRHandlerData, always, never, spans);

expect(newSpan.status).toBe(spanStatusfromHttpCode(404));
});
Expand All @@ -311,7 +313,7 @@ describe('callbacks', () => {
};

// in that case, the response coming back will be ignored
xhrCallback(postRequestXHRHandlerData, alwaysCreateSpan, {});
xhrCallback(postRequestXHRHandlerData, always, never, {});

const newSpan = transaction.spanRecorder?.spans[1];

Expand All @@ -324,10 +326,10 @@ describe('callbacks', () => {

expect(transaction.metadata.propagations).toBe(0);

xhrCallback(firstReqData, alwaysCreateSpan, {});
xhrCallback(firstReqData, always, always, {});
expect(transaction.metadata.propagations).toBe(1);

xhrCallback(secondReqData, alwaysCreateSpan, {});
xhrCallback(secondReqData, always, always, {});
expect(transaction.metadata.propagations).toBe(2);
});
});
Expand Down