Skip to content

Commit

Permalink
fix(tracing): Fix tracingOrigins not applying (#6079)
Browse files Browse the repository at this point in the history
In the process of working on #5285, we missed the fact that the first two PRs (#6039 and #6041) were interdependent, in that the former accidentally introduced a bug (#6077) which the latter then inadvertently fixed. This would have been fine, except that we published a release after merging the bug-creating PR but before merging the bug-fixing PR. Whoops.

This patch pulls just the bug-fixing part out of the second PR. It also adds tests to cover the buggy cases, using `it.each` to cover all of the different combinations of outcomes for `shouldCreateSpanForRequest` and `shouldAttachHeaders`. Finally, since I was already in the test file, I reorganized it a little:
- `it('does not create span if shouldCreateSpan returns false')` -> absorbed into the `it.each()`
- `it('does not create span if there is no fetch data in handler data')` -> added header check, became `it('adds neither fetch request spans nor fetch request headers if there is no fetch data in handler data')`
- `it('does not add fetch request spans if tracing is disabled')` and `it('does not add fetch request headers if tracing is disabled` -> combined into `it('adds neither fetch request spans nor fetch request headers if tracing is disabled')`
- `it('adds sentry-trace header to fetch requests')` -> absorbed into the `it.each()`
- Similar changes made to XHR tests

Co-authored-by: Tim Fish <tim@timfish.uk>
  • Loading branch information
lobsterkatie and timfish committed Oct 28, 2022
1 parent a800339 commit fecdc3e
Show file tree
Hide file tree
Showing 2 changed files with 161 additions and 104 deletions.
30 changes: 18 additions & 12 deletions packages/tracing/src/browser/request.ts
Expand Up @@ -5,6 +5,7 @@ import {
BAGGAGE_HEADER_NAME,
dynamicSamplingContextToSentryBaggageHeader,
isInstanceOf,
isMatchingPattern,
} from '@sentry/utils';

import { getActiveTransaction, hasTracingEnabled } from '../utils';
Expand Down Expand Up @@ -102,26 +103,27 @@ export const defaultRequestInstrumentationOptions: RequestInstrumentationOptions

/** 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, shouldCreateSpanForRequest } = {
const { traceFetch, traceXHR, tracingOrigins, shouldCreateSpanForRequest } = {
...defaultRequestInstrumentationOptions,
..._options,
};

const shouldCreateSpan =
typeof shouldCreateSpanForRequest === 'function' ? shouldCreateSpanForRequest : (_: string) => true;

const shouldAttachHeaders = (url: string): boolean => tracingOrigins.some(origin => isMatchingPattern(url, origin));

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 @@ -132,6 +134,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 @@ -181,14 +184,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 @@ -262,6 +267,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 @@ -307,7 +313,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

0 comments on commit fecdc3e

Please sign in to comment.