From c7f0b42cadd03d2029def96df15cd08b8b8cc356 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 29 Sep 2020 09:12:17 -0700 Subject: [PATCH 01/15] move tracesSampleRate vs tracesSampler tests --- packages/tracing/test/hub.test.ts | 54 +++++++++++++++---------------- 1 file changed, 27 insertions(+), 27 deletions(-) diff --git a/packages/tracing/test/hub.test.ts b/packages/tracing/test/hub.test.ts index 8f57070827b7..3848430172ca 100644 --- a/packages/tracing/test/hub.test.ts +++ b/packages/tracing/test/hub.test.ts @@ -41,33 +41,6 @@ describe('Hub', () => { }); describe('transaction sampling', () => { - describe('tracesSampleRate and tracesSampler options', () => { - it("should call tracesSampler if it's defined", () => { - const tracesSampler = jest.fn(); - const hub = new Hub(new BrowserClient({ tracesSampler })); - hub.startTransaction({ name: 'dogpark' }); - - expect(tracesSampler).toHaveBeenCalled(); - }); - - it('should prefer tracesSampler to tracesSampleRate', () => { - const tracesSampler = jest.fn(); - const hub = new Hub(new BrowserClient({ tracesSampleRate: 1, tracesSampler })); - hub.startTransaction({ name: 'dogpark' }); - - expect(tracesSampler).toHaveBeenCalled(); - }); - - it('tolerates tracesSampler returning a boolean', () => { - const tracesSampler = jest.fn().mockReturnValue(true); - const hub = new Hub(new BrowserClient({ tracesSampler })); - const transaction = hub.startTransaction({ name: 'dogpark' }); - - expect(tracesSampler).toHaveBeenCalled(); - expect(transaction.sampled).toBe(true); - }); - }); - describe('default sample context', () => { it('should extract request data for default sampling context when in node', () => { // make sure we look like we're in node @@ -196,6 +169,33 @@ describe('Hub', () => { expect(transaction.sampled).toBe(true); }); + + it("should call tracesSampler if it's defined", () => { + const tracesSampler = jest.fn(); + const hub = new Hub(new BrowserClient({ tracesSampler })); + hub.startTransaction({ name: 'dogpark' }); + + expect(tracesSampler).toHaveBeenCalled(); + }); + + it('should prefer tracesSampler to tracesSampleRate', () => { + // make the two options do opposite things to prove precedence + const tracesSampler = jest.fn().mockReturnValue(true); + const hub = new Hub(new BrowserClient({ tracesSampleRate: 0, tracesSampler })); + const transaction = hub.startTransaction({ name: 'dogpark' }); + + expect(tracesSampler).toHaveBeenCalled(); + expect(transaction.sampled).toBe(true); + }); + + it('should tolerate tracesSampler returning a boolean', () => { + const tracesSampler = jest.fn().mockReturnValue(true); + const hub = new Hub(new BrowserClient({ tracesSampler })); + const transaction = hub.startTransaction({ name: 'dogpark' }); + + expect(tracesSampler).toHaveBeenCalled(); + expect(transaction.sampled).toBe(true); + }); }); describe('isValidSampleRate()', () => { From 5855f2694a8fdf28e7c29b5cecdda7f6811bcb76 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 29 Sep 2020 09:19:39 -0700 Subject: [PATCH 02/15] rename fetch callback --- packages/tracing/src/browser/request.ts | 4 ++-- packages/tracing/test/browser/request.test.ts | 14 +++++++------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/packages/tracing/src/browser/request.ts b/packages/tracing/src/browser/request.ts index c2727c524584..b1d03fb252f5 100644 --- a/packages/tracing/src/browser/request.ts +++ b/packages/tracing/src/browser/request.ts @@ -114,7 +114,7 @@ export function registerRequestInstrumentation(_options?: Partial { - _fetchCallback(handlerData, shouldCreateSpan, spans); + fetchCallback(handlerData, shouldCreateSpan, spans); }, type: 'fetch', }); @@ -133,7 +133,7 @@ export function registerRequestInstrumentation(_options?: Partial boolean, spans: Record, diff --git a/packages/tracing/test/browser/request.test.ts b/packages/tracing/test/browser/request.test.ts index 994b1756e928..2ddc970a8b35 100644 --- a/packages/tracing/test/browser/request.test.ts +++ b/packages/tracing/test/browser/request.test.ts @@ -2,7 +2,7 @@ import { BrowserClient } from '@sentry/browser'; import { Hub, makeMain } from '@sentry/hub'; import { Span, SpanStatus, Transaction } from '../../src'; -import { _fetchCallback, FetchData, registerRequestInstrumentation } from '../../src/browser/request'; +import { fetchCallback, FetchData, registerRequestInstrumentation } from '../../src/browser/request'; import { addExtensionMethods } from '../../src/hubextensions'; declare global { @@ -96,7 +96,7 @@ describe('_fetchCallback()', () => { }; const spans = {}; - _fetchCallback(data, shouldCreateSpan, spans); + fetchCallback(data, shouldCreateSpan, spans); expect(spans).toEqual({}); }); @@ -108,7 +108,7 @@ describe('_fetchCallback()', () => { }; const spans = {}; - _fetchCallback(data, shouldCreateSpan, spans); + fetchCallback(data, shouldCreateSpan, spans); expect(spans).toEqual({}); }); @@ -125,7 +125,7 @@ describe('_fetchCallback()', () => { const spans: Record = {}; // Start fetch request - _fetchCallback(data, shouldCreateSpan, spans); + fetchCallback(data, shouldCreateSpan, spans); const spanKey = Object.keys(spans)[0]; const fetchSpan = spans[spanKey]; @@ -149,7 +149,7 @@ describe('_fetchCallback()', () => { }; // End fetch request - _fetchCallback(newData, shouldCreateSpan, spans); + fetchCallback(newData, shouldCreateSpan, spans); expect(spans).toEqual({}); if (transaction.spanRecorder) { expect(transaction.spanRecorder.spans[1].endTimestamp).toBeDefined(); @@ -171,7 +171,7 @@ describe('_fetchCallback()', () => { const spans: Record = {}; // Start fetch request - _fetchCallback(data, shouldCreateSpan, spans); + fetchCallback(data, shouldCreateSpan, spans); const newData = { ...data, @@ -179,7 +179,7 @@ describe('_fetchCallback()', () => { response: { status: 404 } as Response, }; // End fetch request - _fetchCallback(newData, shouldCreateSpan, spans); + fetchCallback(newData, shouldCreateSpan, spans); if (transaction.spanRecorder) { expect(transaction.spanRecorder.spans[1].status).toBe(SpanStatus.fromHttpCode(404)); } else { From b18cd3794cb55d2b23b4a03887d1a779dde56423 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 29 Sep 2020 09:36:58 -0700 Subject: [PATCH 03/15] remove default beforeNavigate --- packages/tracing/src/browser/browsertracing.ts | 16 ++++++++-------- packages/tracing/src/browser/router.ts | 5 ----- .../tracing/test/browser/browsertracing.test.ts | 1 - packages/tracing/test/browser/router.test.ts | 9 +-------- 4 files changed, 9 insertions(+), 22 deletions(-) diff --git a/packages/tracing/src/browser/browsertracing.ts b/packages/tracing/src/browser/browsertracing.ts index dc3a604eea26..c6f99ab7bc83 100644 --- a/packages/tracing/src/browser/browsertracing.ts +++ b/packages/tracing/src/browser/browsertracing.ts @@ -13,7 +13,7 @@ import { registerRequestInstrumentation, RequestInstrumentationOptions, } from './request'; -import { defaultBeforeNavigate, defaultRoutingInstrumentation } from './router'; +import { defaultRoutingInstrumentation } from './router'; export const DEFAULT_MAX_TRANSACTION_DURATION_SECONDS = 600; @@ -66,7 +66,7 @@ export interface BrowserTracingOptions extends RequestInstrumentationOptions { * * If undefined is returned, a pageload/navigation transaction will not be created. */ - beforeNavigate(context: TransactionContext): TransactionContext | undefined; + beforeNavigate?(context: TransactionContext): TransactionContext | undefined; /** * Instrumentation that creates routing change transactions. By default creates @@ -80,7 +80,6 @@ export interface BrowserTracingOptions extends RequestInstrumentationOptions { } const DEFAULT_BROWSER_TRACING_OPTIONS = { - beforeNavigate: defaultBeforeNavigate, idleTimeout: DEFAULT_IDLE_TIMEOUT, markBackgroundTransactions: true, maxTransactionDuration: DEFAULT_MAX_TRANSACTION_DURATION_SECONDS, @@ -189,20 +188,21 @@ export class BrowserTracing implements Integration { const { beforeNavigate, idleTimeout, maxTransactionDuration } = this.options; // if beforeNavigate returns undefined, we should not start a transaction. - const ctx = beforeNavigate({ + const expandedContext = { ...context, ...getHeaderContext(), trimEnd: true, - }); + }; + const modifiedContext = typeof beforeNavigate === 'function' ? beforeNavigate(expandedContext) : expandedContext; - if (ctx === undefined) { + if (modifiedContext === undefined) { logger.log(`[Tracing] Did not create ${context.op} idleTransaction due to beforeNavigate`); return undefined; } const hub = this._getCurrentHub(); - logger.log(`[Tracing] starting ${ctx.op} idleTransaction on scope`); - const idleTransaction = startIdleTransaction(hub, ctx, idleTimeout, true); + logger.log(`[Tracing] starting ${modifiedContext.op} idleTransaction on scope`); + const idleTransaction = startIdleTransaction(hub, modifiedContext, idleTimeout, true); idleTransaction.registerBeforeFinishCallback((transaction, endTimestamp) => { this._metrics.addPerformanceEntries(transaction); adjustTransactionDuration(secToMs(maxTransactionDuration), transaction, endTimestamp); diff --git a/packages/tracing/src/browser/router.ts b/packages/tracing/src/browser/router.ts index 8c42494a7746..2e7e3398bc75 100644 --- a/packages/tracing/src/browser/router.ts +++ b/packages/tracing/src/browser/router.ts @@ -54,8 +54,3 @@ export function defaultRoutingInstrumentation( }); } } - -/** default implementation of Browser Tracing before navigate */ -export function defaultBeforeNavigate(context: TransactionContext): TransactionContext | undefined { - return context; -} diff --git a/packages/tracing/test/browser/browsertracing.test.ts b/packages/tracing/test/browser/browsertracing.test.ts index 7c1664c973af..312b5eba6c14 100644 --- a/packages/tracing/test/browser/browsertracing.test.ts +++ b/packages/tracing/test/browser/browsertracing.test.ts @@ -76,7 +76,6 @@ describe('BrowserTracing', () => { const browserTracing = createBrowserTracing(); expect(browserTracing.options).toEqual({ - beforeNavigate: expect.any(Function), idleTimeout: DEFAULT_IDLE_TIMEOUT, markBackgroundTransactions: true, maxTransactionDuration: DEFAULT_MAX_TRANSACTION_DURATION_SECONDS, diff --git a/packages/tracing/test/browser/router.test.ts b/packages/tracing/test/browser/router.test.ts index 5dfaf222399c..bbaf5e31863f 100644 --- a/packages/tracing/test/browser/router.test.ts +++ b/packages/tracing/test/browser/router.test.ts @@ -1,6 +1,6 @@ import { JSDOM } from 'jsdom'; -import { defaultBeforeNavigate, defaultRoutingInstrumentation } from '../../src/browser/router'; +import { defaultRoutingInstrumentation } from '../../src/browser/router'; let mockChangeHistory: ({ to, from }: { to: string; from?: string }) => void = () => undefined; let addInstrumentationHandlerType: string = ''; @@ -15,13 +15,6 @@ jest.mock('@sentry/utils', () => { }; }); -describe('defaultBeforeNavigate()', () => { - it('returns a context', () => { - const ctx = { name: 'testing', status: 'ok' }; - expect(defaultBeforeNavigate(ctx)).toBe(ctx); - }); -}); - describe('defaultRoutingInstrumentation', () => { const mockFinish = jest.fn(); const startTransaction = jest.fn().mockReturnValue({ finish: mockFinish }); From 5d842280e596a40e54a5afc93445a97e22695b30 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 29 Sep 2020 09:49:23 -0700 Subject: [PATCH 04/15] random language cleanup --- packages/tracing/src/browser/browsertracing.ts | 2 +- packages/tracing/src/browser/request.ts | 4 +++- packages/tracing/src/hubextensions.ts | 5 +++-- packages/tracing/test/hub.test.ts | 4 ++-- packages/utils/src/instrument.ts | 12 ++++++------ 5 files changed, 15 insertions(+), 12 deletions(-) diff --git a/packages/tracing/src/browser/browsertracing.ts b/packages/tracing/src/browser/browsertracing.ts index c6f99ab7bc83..371d86ee5de2 100644 --- a/packages/tracing/src/browser/browsertracing.ts +++ b/packages/tracing/src/browser/browsertracing.ts @@ -201,8 +201,8 @@ export class BrowserTracing implements Integration { } const hub = this._getCurrentHub(); - logger.log(`[Tracing] starting ${modifiedContext.op} idleTransaction on scope`); const idleTransaction = startIdleTransaction(hub, modifiedContext, idleTimeout, true); + logger.log(`[Tracing] Starting ${modifiedContext.op} transaction on scope`); idleTransaction.registerBeforeFinishCallback((transaction, endTimestamp) => { this._metrics.addPerformanceEntries(transaction); adjustTransactionDuration(secToMs(maxTransactionDuration), transaction, endTimestamp); diff --git a/packages/tracing/src/browser/request.ts b/packages/tracing/src/browser/request.ts index b1d03fb252f5..99f86233c326 100644 --- a/packages/tracing/src/browser/request.ts +++ b/packages/tracing/src/browser/request.ts @@ -41,7 +41,7 @@ export interface RequestInstrumentationOptions { /** Data returned from fetch callback */ export interface FetchData { // eslint-disable-next-line @typescript-eslint/no-explicit-any - args: any[]; + args: any[]; // the arguments passed to the fetch call itself fetchData?: { method: string; url: string; @@ -217,6 +217,7 @@ function xhrCallback( return; } + // check first if the request has finished and is tracked by an existing span which should now end if (handlerData.endTimestamp && handlerData.xhr.__sentry_xhr_span_id__) { const span = spans[handlerData.xhr.__sentry_xhr_span_id__]; if (span) { @@ -231,6 +232,7 @@ function xhrCallback( return; } + // if not, create a new span to track it const activeTransaction = getActiveTransaction(); if (activeTransaction) { const span = activeTransaction.startChild({ diff --git a/packages/tracing/src/hubextensions.ts b/packages/tracing/src/hubextensions.ts index 7b1bf36f0066..3edad9d6f0d5 100644 --- a/packages/tracing/src/hubextensions.ts +++ b/packages/tracing/src/hubextensions.ts @@ -184,8 +184,9 @@ function isValidSampleRate(rate: unknown): boolean { /** * Creates a new transaction and adds a sampling decision if it doesn't yet have one. * - * The Hub.startTransaction method delegates to this method to do its work, passing the Hub instance in as `this`. - * Exists as a separate function so that it can be injected into the class as an "extension method." + * The Hub.startTransaction method delegates to this method to do its work, passing the Hub instance in as `this`, as if + * it had been called on the hub directly. Exists as a separate function so that it can be injected into the class as an + * "extension method." * * @param this: The Hub starting the transaction * @param transactionContext: Data used to configure the transaction diff --git a/packages/tracing/test/hub.test.ts b/packages/tracing/test/hub.test.ts index 3848430172ca..bff54c656ee7 100644 --- a/packages/tracing/test/hub.test.ts +++ b/packages/tracing/test/hub.test.ts @@ -156,14 +156,14 @@ describe('Hub', () => { expect(transaction.sampled).toBe(false); }); - it('should not sample transactions when tracesSampleRate is 0', () => { + it('should set sampled = false if tracesSampleRate is 0', () => { const hub = new Hub(new BrowserClient({ tracesSampleRate: 0 })); const transaction = hub.startTransaction({ name: 'dogpark' }); expect(transaction.sampled).toBe(false); }); - it('should sample transactions when tracesSampleRate is 1', () => { + it('should set sampled = true if tracesSampleRate is 1', () => { const hub = new Hub(new BrowserClient({ tracesSampleRate: 1 })); const transaction = hub.startTransaction({ name: 'dogpark' }); diff --git a/packages/utils/src/instrument.ts b/packages/utils/src/instrument.ts index a3967e3de502..2c5e4e419fa6 100644 --- a/packages/utils/src/instrument.ts +++ b/packages/utils/src/instrument.ts @@ -141,7 +141,7 @@ function instrumentFetch(): void { fill(global, 'fetch', function(originalFetch: () => void): () => void { return function(...args: any[]): void { - const commonHandlerData = { + const handlerData = { args, fetchData: { method: getFetchMethod(args), @@ -151,14 +151,14 @@ function instrumentFetch(): void { }; triggerHandlers('fetch', { - ...commonHandlerData, + ...handlerData, }); // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access return originalFetch.apply(global, args).then( (response: Response) => { triggerHandlers('fetch', { - ...commonHandlerData, + ...handlerData, endTimestamp: Date.now(), response, }); @@ -166,7 +166,7 @@ function instrumentFetch(): void { }, (error: Error) => { triggerHandlers('fetch', { - ...commonHandlerData, + ...handlerData, endTimestamp: Date.now(), error, }); @@ -223,7 +223,7 @@ function instrumentXHR(): void { return; } - // Poor man implementation of ES6 `Map` by tracking and keeping in sync key and value separately. + // Poor man's implementation of ES6 `Map`, tracking and keeping in sync key and value separately. const requestKeys: XMLHttpRequest[] = []; const requestValues: Array[] = []; const xhrproto = XMLHttpRequest.prototype; @@ -260,7 +260,7 @@ function instrumentXHR(): void { try { const requestPos = requestKeys.indexOf(xhr); if (requestPos !== -1) { - // Make sure to pop both, key and value to keep it in sync. + // Make sure to pop both key and value to keep it in sync. requestKeys.splice(requestPos); const args = requestValues.splice(requestPos)[0]; if (xhr.__sentry_xhr__ && args[0] !== undefined) { From 8f1a21ab91a45b967c2c5702b434f609703435ed Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 29 Sep 2020 09:56:53 -0700 Subject: [PATCH 05/15] export things for future test use --- packages/tracing/src/browser/browsertracing.ts | 2 +- packages/tracing/src/browser/request.ts | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/tracing/src/browser/browsertracing.ts b/packages/tracing/src/browser/browsertracing.ts index 371d86ee5de2..0a76c5e5ac50 100644 --- a/packages/tracing/src/browser/browsertracing.ts +++ b/packages/tracing/src/browser/browsertracing.ts @@ -217,7 +217,7 @@ export class BrowserTracing implements Integration { * * @returns Transaction context data from the header or undefined if there's no header or the header is malformed */ -function getHeaderContext(): Partial | undefined { +export function getHeaderContext(): Partial | undefined { const header = getMetaContent('sentry-trace'); if (header) { return extractTraceparentData(header); diff --git a/packages/tracing/src/browser/request.ts b/packages/tracing/src/browser/request.ts index 99f86233c326..45b81486e49f 100644 --- a/packages/tracing/src/browser/request.ts +++ b/packages/tracing/src/browser/request.ts @@ -54,7 +54,7 @@ export interface FetchData { } /** Data returned from XHR request */ -interface XHRData { +export interface XHRData { xhr?: { __sentry_xhr__?: { method: string; @@ -198,7 +198,7 @@ export function fetchCallback( /** * Create and track xhr request spans */ -function xhrCallback( +export function xhrCallback( handlerData: XHRData, shouldCreateSpan: (url: string) => boolean, spans: Record, From e2a16ed829f082e5a695bff0d090b9a74e346437 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 29 Sep 2020 10:19:07 -0700 Subject: [PATCH 06/15] clean up existing request tests --- packages/tracing/test/browser/request.test.ts | 240 +++++++----------- 1 file changed, 98 insertions(+), 142 deletions(-) diff --git a/packages/tracing/test/browser/request.test.ts b/packages/tracing/test/browser/request.test.ts index 2ddc970a8b35..dc77f923f264 100644 --- a/packages/tracing/test/browser/request.test.ts +++ b/packages/tracing/test/browser/request.test.ts @@ -1,79 +1,63 @@ import { BrowserClient } from '@sentry/browser'; import { Hub, makeMain } from '@sentry/hub'; +import * as utils from '@sentry/utils'; import { Span, SpanStatus, Transaction } from '../../src'; import { fetchCallback, FetchData, registerRequestInstrumentation } from '../../src/browser/request'; import { addExtensionMethods } from '../../src/hubextensions'; -declare global { - // eslint-disable-next-line @typescript-eslint/no-namespace - namespace NodeJS { - interface Global { - // Have to mock out Request because it is not defined in jest environment - Request: Request; - } - } -} - beforeAll(() => { addExtensionMethods(); - // @ts-ignore need to override global Request + // @ts-ignore need to override global Request because it's not in the jest environment (even with an + // `@jest-environment jsdom` directive, for some reason) global.Request = {}; }); -const mockAddInstrumentationHandler = jest.fn(); -let mockFetchCallback = jest.fn(); -let mockXHRCallback = jest.fn(); -jest.mock('@sentry/utils', () => { - const actual = jest.requireActual('@sentry/utils'); - return { - ...actual, - addInstrumentationHandler: ({ callback, type }: any) => { - if (type === 'fetch') { - mockFetchCallback = jest.fn(callback); - } - if (type === 'xhr') { - mockXHRCallback = jest.fn(callback); - } - if (typeof mockAddInstrumentationHandler === 'function') { - return mockAddInstrumentationHandler({ callback, type }); - } - }, - }; -}); +const addInstrumentationHandler = jest.spyOn(utils, 'addInstrumentationHandler'); describe('registerRequestInstrumentation', () => { beforeEach(() => { - mockFetchCallback.mockReset(); - mockXHRCallback.mockReset(); - mockAddInstrumentationHandler.mockReset(); + jest.clearAllMocks(); }); - it('tracks fetch and xhr requests', () => { + it('instruments fetch and xhr requests', () => { registerRequestInstrumentation(); - expect(mockAddInstrumentationHandler).toHaveBeenCalledTimes(2); - // fetch - expect(mockAddInstrumentationHandler).toHaveBeenNthCalledWith(1, { callback: expect.any(Function), type: 'fetch' }); - // xhr - expect(mockAddInstrumentationHandler).toHaveBeenNthCalledWith(2, { callback: expect.any(Function), type: 'xhr' }); + + expect(addInstrumentationHandler).toHaveBeenCalledWith({ + callback: expect.any(Function), + type: 'fetch', + }); + expect(addInstrumentationHandler).toHaveBeenCalledWith({ + callback: expect.any(Function), + type: 'xhr', + }); }); - it('does not add fetch requests spans if traceFetch is false', () => { + it('does not instrument fetch requests if traceFetch is false', () => { registerRequestInstrumentation({ traceFetch: false }); - expect(mockAddInstrumentationHandler).toHaveBeenCalledTimes(1); - expect(mockFetchCallback()).toBe(undefined); + + expect(addInstrumentationHandler).not.toHaveBeenCalledWith({ callback: expect.any(Function), type: 'fetch' }); }); - it('does not add xhr requests spans if traceXHR is false', () => { + it('does not instrument xhr requests if traceXHR is false', () => { registerRequestInstrumentation({ traceXHR: false }); - expect(mockAddInstrumentationHandler).toHaveBeenCalledTimes(1); - expect(mockXHRCallback()).toBe(undefined); + + expect(addInstrumentationHandler).not.toHaveBeenCalledWith({ callback: expect.any(Function), type: 'xhr' }); }); }); -describe('_fetchCallback()', () => { +describe('callbacks', () => { let hub: Hub; let transaction: Transaction; + const alwaysCreateSpan = jest.fn().mockReturnValue(true); + const neverCreateSpan = jest.fn().mockReturnValue(false); + const fetchHandlerData: FetchData = { + args: ['http://dogs.are.great/', {}], + fetchData: { url: 'http://dogs.are.great/', method: 'GET' }, + startTimestamp: 1356996072000, + }; + const endTimestamp = 1356996072000; + beforeAll(() => { hub = new Hub(new BrowserClient({ tracesSampleRate: 1 })); makeMain(hub); @@ -84,106 +68,78 @@ describe('_fetchCallback()', () => { hub.configureScope(scope => scope.setSpan(transaction)); }); - it('does not create span if it should not be created', () => { - const shouldCreateSpan = (url: string): boolean => url === '/organizations'; - const data: FetchData = { - args: ['/users'], - fetchData: { - method: 'GET', - url: '/users', - }, - startTimestamp: 1595509730275, - }; - const spans = {}; - - fetchCallback(data, shouldCreateSpan, spans); - expect(spans).toEqual({}); - }); + describe('fetchCallback()', () => { + it('does not create span if shouldCreateSpan returns false', () => { + const spans = {}; - it('does not create span if there is no fetch data', () => { - const shouldCreateSpan = (_: string): boolean => true; - const data: FetchData = { - args: [], - startTimestamp: 1595509730275, - }; - const spans = {}; + fetchCallback(fetchHandlerData, neverCreateSpan, spans); - fetchCallback(data, shouldCreateSpan, spans); - expect(spans).toEqual({}); - }); + expect(spans).toEqual({}); + }); - it('creates and finishes fetch span on active transaction', () => { - const shouldCreateSpan = (_: string): boolean => true; - const data: FetchData = { - args: ['/users'], - fetchData: { - method: 'GET', - url: '/users', - }, - startTimestamp: 1595509730275, - }; - const spans: Record = {}; - - // Start fetch request - fetchCallback(data, shouldCreateSpan, spans); - const spanKey = Object.keys(spans)[0]; - - const fetchSpan = spans[spanKey]; - expect(fetchSpan).toBeInstanceOf(Span); - expect(fetchSpan.data).toEqual({ - method: 'GET', - type: 'fetch', - url: '/users', + it('does not create span if there is no fetch data in handler data', () => { + const noFetchData = { args: fetchHandlerData.args, startTimestamp: fetchHandlerData.startTimestamp }; + const spans = {}; + + fetchCallback(noFetchData, alwaysCreateSpan, spans); + expect(spans).toEqual({}); }); - expect(fetchSpan.description).toBe('GET /users'); - expect(fetchSpan.op).toBe('http'); - if (data.fetchData) { - expect(data.fetchData.__span).toBeDefined(); - } else { - fail('Fetch data does not exist'); - } - - const newData = { - ...data, - endTimestamp: data.startTimestamp + 12343234, - }; - - // End fetch request - fetchCallback(newData, shouldCreateSpan, spans); - expect(spans).toEqual({}); - if (transaction.spanRecorder) { - expect(transaction.spanRecorder.spans[1].endTimestamp).toBeDefined(); - } else { - fail('Transaction does not have span recorder'); - } - }); - it('sets response status on finish', () => { - const shouldCreateSpan = (_: string): boolean => true; - const data: FetchData = { - args: ['/users'], - fetchData: { + + it('creates and finishes fetch span on active transaction', () => { + const spans = {}; + + // triggered by request being sent + fetchCallback(fetchHandlerData, alwaysCreateSpan, spans); + + const newSpan = transaction.spanRecorder?.spans[1]; + + expect(newSpan).toBeDefined(); + expect(newSpan).toBeInstanceOf(Span); + expect(newSpan!.data).toEqual({ method: 'GET', - url: '/users', - }, - startTimestamp: 1595509730275, - }; - const spans: Record = {}; - - // Start fetch request - fetchCallback(data, shouldCreateSpan, spans); - - const newData = { - ...data, - endTimestamp: data.startTimestamp + 12343234, - response: { status: 404 } as Response, - }; - // End fetch request - fetchCallback(newData, shouldCreateSpan, spans); - if (transaction.spanRecorder) { - expect(transaction.spanRecorder.spans[1].status).toBe(SpanStatus.fromHttpCode(404)); - } else { - fail('Transaction does not have span recorder'); - } + type: 'fetch', + url: 'http://dogs.are.great/', + }); + expect(newSpan!.description).toBe('GET http://dogs.are.great/'); + expect(newSpan!.op).toBe('http'); + expect(fetchHandlerData.fetchData?.__span).toBeDefined(); + + const postRequestFetchHandlerData = { + ...fetchHandlerData, + endTimestamp, + }; + + // triggered by response coming back + fetchCallback(postRequestFetchHandlerData, alwaysCreateSpan, spans); + + expect(newSpan!.endTimestamp).toBeDefined(); + }); + + it('sets response status on finish', () => { + const spans: Record = {}; + + // triggered by request being sent + fetchCallback(fetchHandlerData, alwaysCreateSpan, spans); + + const newSpan = transaction.spanRecorder?.spans[1]; + + expect(newSpan).toBeDefined(); + + const postRequestFetchHandlerData = { + ...fetchHandlerData, + endTimestamp, + response: { status: 404 } as Response, + }; + + // triggered by response coming back + fetchCallback(postRequestFetchHandlerData, alwaysCreateSpan, spans); + + expect(newSpan!.status).toBe(SpanStatus.fromHttpCode(404)); + }); + + it('adds sentry-trace header to fetch requests', () => { + // TODO + }); }); }); From b1e1c7809c03e7e39e382a58c16b0a594169d3f5 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 29 Sep 2020 10:23:37 -0700 Subject: [PATCH 07/15] add method and url data at span start rather than end --- packages/tracing/src/browser/request.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/tracing/src/browser/request.ts b/packages/tracing/src/browser/request.ts index 45b81486e49f..99876e88ef4a 100644 --- a/packages/tracing/src/browser/request.ts +++ b/packages/tracing/src/browser/request.ts @@ -221,8 +221,6 @@ export function xhrCallback( if (handlerData.endTimestamp && handlerData.xhr.__sentry_xhr_span_id__) { const span = spans[handlerData.xhr.__sentry_xhr_span_id__]; if (span) { - span.setData('url', xhr.url); - span.setData('method', xhr.method); span.setHttpStatus(xhr.status_code); span.finish(); @@ -239,6 +237,8 @@ export function xhrCallback( data: { ...xhr.data, type: 'xhr', + method: xhr.method, + url: xhr.url, }, description: `${xhr.method} ${xhr.url}`, op: 'http', From d7b1e879144db943fbf2b22e430db2e4ed853eb0 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 29 Sep 2020 10:22:13 -0700 Subject: [PATCH 08/15] add request tests for xhrs --- packages/tracing/src/browser/request.ts | 2 +- packages/tracing/test/browser/request.test.ts | 97 ++++++++++++++++++- 2 files changed, 97 insertions(+), 2 deletions(-) diff --git a/packages/tracing/src/browser/request.ts b/packages/tracing/src/browser/request.ts index 99876e88ef4a..51f0a02bc162 100644 --- a/packages/tracing/src/browser/request.ts +++ b/packages/tracing/src/browser/request.ts @@ -64,8 +64,8 @@ export interface XHRData { data: Record; }; __sentry_xhr_span_id__?: string; - __sentry_own_request__: boolean; setRequestHeader?: (key: string, val: string) => void; + __sentry_own_request__?: boolean; }; startTimestamp: number; endTimestamp?: number; diff --git a/packages/tracing/test/browser/request.test.ts b/packages/tracing/test/browser/request.test.ts index dc77f923f264..727c11c28791 100644 --- a/packages/tracing/test/browser/request.test.ts +++ b/packages/tracing/test/browser/request.test.ts @@ -3,8 +3,15 @@ import { Hub, makeMain } from '@sentry/hub'; import * as utils from '@sentry/utils'; import { Span, SpanStatus, Transaction } from '../../src'; -import { fetchCallback, FetchData, registerRequestInstrumentation } from '../../src/browser/request'; +import { + fetchCallback, + FetchData, + registerRequestInstrumentation, + xhrCallback, + XHRData, +} from '../../src/browser/request'; import { addExtensionMethods } from '../../src/hubextensions'; +import * as tracingUtils from '../../src/utils'; beforeAll(() => { addExtensionMethods(); @@ -14,6 +21,7 @@ beforeAll(() => { }); const addInstrumentationHandler = jest.spyOn(utils, 'addInstrumentationHandler'); +const setRequestHeader = jest.fn(); describe('registerRequestInstrumentation', () => { beforeEach(() => { @@ -56,6 +64,21 @@ describe('callbacks', () => { fetchData: { url: 'http://dogs.are.great/', method: 'GET' }, startTimestamp: 1356996072000, }; + const xhrHandlerData: XHRData = { + xhr: { + __sentry_xhr__: { + method: 'GET', + url: 'http://dogs.are.great/', + status_code: 200, + data: {}, + }, + __sentry_xhr_span_id__: '1231201211212012', + // eslint-disable-next-line @typescript-eslint/unbound-method + // setRequestHeader: XMLHttpRequest.prototype.setRequestHeader, + setRequestHeader, + }, + startTimestamp: 1353501072000, + }; const endTimestamp = 1356996072000; beforeAll(() => { @@ -142,4 +165,76 @@ describe('callbacks', () => { // TODO }); }); + + describe('xhrCallback()', () => { + it('does not create span if shouldCreateSpan returns false', () => { + const spans = {}; + + xhrCallback(xhrHandlerData, neverCreateSpan, spans); + + expect(spans).toEqual({}); + }); + + it('adds sentry-trace header to XHR requests', () => { + xhrCallback(xhrHandlerData, alwaysCreateSpan, {}); + + expect(setRequestHeader).toHaveBeenCalledWith( + 'sentry-trace', + expect.stringMatching(tracingUtils.TRACEPARENT_REGEXP), + ); + }); + + it('creates and finishes XHR span on active transaction', () => { + const spans = {}; + + // triggered by request being sent + xhrCallback(xhrHandlerData, alwaysCreateSpan, spans); + + const newSpan = transaction.spanRecorder?.spans[1]; + + expect(newSpan).toBeDefined(); + expect(newSpan).toBeInstanceOf(Span); + expect(newSpan!.data).toEqual({ + method: 'GET', + type: 'xhr', + url: 'http://dogs.are.great/', + }); + expect(newSpan!.description).toBe('GET http://dogs.are.great/'); + expect(newSpan!.op).toBe('http'); + expect(xhrHandlerData.xhr!.__sentry_xhr_span_id__).toBeDefined(); + expect(xhrHandlerData.xhr!.__sentry_xhr_span_id__).toEqual(newSpan?.spanId); + + const postRequestXHRHandlerData = { + ...xhrHandlerData, + endTimestamp, + }; + + // triggered by response coming back + xhrCallback(postRequestXHRHandlerData, alwaysCreateSpan, spans); + + expect(newSpan!.endTimestamp).toBeDefined(); + }); + + it('sets response status on finish', () => { + const spans = {}; + + // triggered by request being sent + xhrCallback(xhrHandlerData, alwaysCreateSpan, spans); + + const newSpan = transaction.spanRecorder?.spans[1]; + + expect(newSpan).toBeDefined(); + + const postRequestXHRHandlerData = { + ...xhrHandlerData, + endTimestamp, + }; + postRequestXHRHandlerData.xhr!.__sentry_xhr__!.status_code = 404; + + // triggered by response coming back + xhrCallback(postRequestXHRHandlerData, alwaysCreateSpan, spans); + + expect(newSpan!.status).toBe(SpanStatus.fromHttpCode(404)); + }); + }); }); From 46119aa224890f4ff84d01c6aef7c5a3cf8f867f Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 29 Sep 2020 10:31:39 -0700 Subject: [PATCH 09/15] clean up existing browser tracing tests --- .../test/browser/browsertracing.test.ts | 51 +++++++++++++------ 1 file changed, 35 insertions(+), 16 deletions(-) diff --git a/packages/tracing/test/browser/browsertracing.test.ts b/packages/tracing/test/browser/browsertracing.test.ts index 312b5eba6c14..cd0d1906704a 100644 --- a/packages/tracing/test/browser/browsertracing.test.ts +++ b/packages/tracing/test/browser/browsertracing.test.ts @@ -11,6 +11,7 @@ import { } from '../../src/browser/browsertracing'; import { defaultRequestInstrumentionOptions } from '../../src/browser/request'; import { defaultRoutingInstrumentation } from '../../src/browser/router'; +import * as hubExtensions from '../../src/hubextensions'; import { DEFAULT_IDLE_TIMEOUT, IdleTransaction } from '../../src/idletransaction'; import { getActiveTransaction, secToMs } from '../../src/utils'; @@ -209,12 +210,26 @@ describe('BrowserTracing', () => { const name = 'sentry-trace'; const content = '126de09502ae4e0fb26c6967190756a4-b6e54397b12a2a0f-1'; document.head.innerHTML = ``; + const startIdleTransaction = jest.spyOn(hubExtensions, 'startIdleTransaction'); + createBrowserTracing(true, { routingInstrumentation: customRoutingInstrumentation }); - const transaction = getActiveTransaction(hub) as IdleTransaction; - expect(transaction.traceId).toBe('126de09502ae4e0fb26c6967190756a4'); - expect(transaction.parentSpanId).toBe('b6e54397b12a2a0f'); - expect(transaction.sampled).toBe(true); + // we match on the calls themselves (rather than calling .toHaveBeenCalledWith()) because that method requires all + // of the arguments be supplied, when we really only care about the transaction context which gets passed + expect(startIdleTransaction.mock.calls).toEqual( + // all calls + expect.arrayContaining([ + // all arguments + expect.arrayContaining([ + // one of the arguments + expect.objectContaining({ + traceId: '126de09502ae4e0fb26c6967190756a4', + parentSpanId: 'b6e54397b12a2a0f', + parentSampled: true, + }), + ]), + ]), + ); }); describe('idleTimeout', () => { @@ -342,20 +357,24 @@ describe('BrowserTracing', () => { }); }); -describe('getMeta', () => { - it('returns a found meta tag contents', () => { - const name = 'sentry-trace'; - const content = '126de09502ae4e0fb26c6967190756a4-b6e54397b12a2a0f-1'; - document.head.innerHTML = ``; + describe('sentry-trace element', () => { + describe('getMetaContent', () => { + it('finds the specified tag and extracts the value', () => { + const name = 'sentry-trace'; + const content = '126de09502ae4e0fb26c6967190756a4-b6e54397b12a2a0f-1'; + document.head.innerHTML = ``; - const meta = getMetaContent(name); - expect(meta).toBe(content); - }); + const metaTagValue = getMetaContent(name); + expect(metaTagValue).toBe(content); + }); - it('only returns meta tags queried for', () => { - document.head.innerHTML = ``; + it("doesn't return meta tags other than the one specified", () => { + document.head.innerHTML = ``; - const meta = getMetaContent('test'); - expect(meta).toBe(null); + const metaTagValue = getMetaContent('dogpark'); + expect(metaTagValue).toBe(null); + }); + + }); }); }); From 75ad86dba015693084aa81b35905ad1b90f4c06b Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 29 Sep 2020 10:34:37 -0700 Subject: [PATCH 10/15] add new meta-header-reading test --- .../test/browser/browsertracing.test.ts | 39 ++++++++++++------- 1 file changed, 24 insertions(+), 15 deletions(-) diff --git a/packages/tracing/test/browser/browsertracing.test.ts b/packages/tracing/test/browser/browsertracing.test.ts index cd0d1906704a..a753986ac051 100644 --- a/packages/tracing/test/browser/browsertracing.test.ts +++ b/packages/tracing/test/browser/browsertracing.test.ts @@ -357,24 +357,33 @@ describe('BrowserTracing', () => { }); }); - describe('sentry-trace element', () => { - describe('getMetaContent', () => { - it('finds the specified tag and extracts the value', () => { - const name = 'sentry-trace'; - const content = '126de09502ae4e0fb26c6967190756a4-b6e54397b12a2a0f-1'; - document.head.innerHTML = ``; - - const metaTagValue = getMetaContent(name); - expect(metaTagValue).toBe(content); - }); +describe('sentry-trace element', () => { + describe('getMetaContent', () => { + it('finds the specified tag and extracts the value', () => { + const name = 'sentry-trace'; + const content = '126de09502ae4e0fb26c6967190756a4-b6e54397b12a2a0f-1'; + document.head.innerHTML = ``; - it("doesn't return meta tags other than the one specified", () => { - document.head.innerHTML = ``; + const metaTagValue = getMetaContent(name); + expect(metaTagValue).toBe(content); + }); - const metaTagValue = getMetaContent('dogpark'); - expect(metaTagValue).toBe(null); - }); + it("doesn't return meta tags other than the one specified", () => { + document.head.innerHTML = ``; + + const metaTagValue = getMetaContent('dogpark'); + expect(metaTagValue).toBe(null); + }); + + it('can pick the correct tag out of multiple options', () => { + const name = 'sentry-trace'; + const content = '126de09502ae4e0fb26c6967190756a4-b6e54397b12a2a0f-1'; + const sentryTraceMeta = ``; + const otherMeta = ``; + document.head.innerHTML = `${sentryTraceMeta} ${otherMeta}`; + const metaTagValue = getMetaContent(name); + expect(metaTagValue).toBe(content); }); }); }); From d53dea3a169be46dbc09b5fb9ddbdbfd35fd341a Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 29 Sep 2020 10:44:32 -0700 Subject: [PATCH 11/15] stop using browser Response type --- packages/tracing/src/browser/request.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/packages/tracing/src/browser/request.ts b/packages/tracing/src/browser/request.ts index 51f0a02bc162..8d4b05a3e0ad 100644 --- a/packages/tracing/src/browser/request.ts +++ b/packages/tracing/src/browser/request.ts @@ -48,7 +48,11 @@ export interface FetchData { // span_id __span?: string; }; - response?: Response; + + // TODO (kmclb) Once type fix goes through, turn this back into a Response + // eslint-disable-next-line @typescript-eslint/no-explicit-any + response?: any; + startTimestamp: number; endTimestamp?: number; } @@ -147,6 +151,8 @@ export function fetchCallback( if (span) { const response = handlerData.response; if (response) { + // TODO (kmclb) remove this once types PR goes through + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access span.setHttpStatus(response.status); } span.finish(); From cbe2971da2e8a611dccdcd9ca676cb683fb23901 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 29 Sep 2020 10:52:17 -0700 Subject: [PATCH 12/15] add suggested tests --- packages/tracing/test/hub.test.ts | 57 +++++++++++++++++++++++++++++-- 1 file changed, 55 insertions(+), 2 deletions(-) diff --git a/packages/tracing/test/hub.test.ts b/packages/tracing/test/hub.test.ts index bff54c656ee7..4963b8c87c61 100644 --- a/packages/tracing/test/hub.test.ts +++ b/packages/tracing/test/hub.test.ts @@ -9,6 +9,7 @@ import { addExtensionMethods } from '../src/hubextensions'; addExtensionMethods(); +const mathRandom = jest.spyOn(Math, 'random'); describe('Hub', () => { beforeEach(() => { jest.spyOn(logger, 'warn'); @@ -178,6 +179,24 @@ describe('Hub', () => { expect(tracesSampler).toHaveBeenCalled(); }); + it('should set sampled = false if tracesSampler returns 0', () => { + const tracesSampler = jest.fn().mockReturnValue(0); + const hub = new Hub(new BrowserClient({ tracesSampler })); + const transaction = hub.startTransaction({ name: 'dogpark' }); + + expect(tracesSampler).toHaveBeenCalled(); + expect(transaction.sampled).toBe(false); + }); + + it('should set sampled = true if tracesSampler returns 1', () => { + const tracesSampler = jest.fn().mockReturnValue(1); + const hub = new Hub(new BrowserClient({ tracesSampler })); + const transaction = hub.startTransaction({ name: 'dogpark' }); + + expect(tracesSampler).toHaveBeenCalled(); + expect(transaction.sampled).toBe(true); + }); + it('should prefer tracesSampler to tracesSampleRate', () => { // make the two options do opposite things to prove precedence const tracesSampler = jest.fn().mockReturnValue(true); @@ -296,7 +315,24 @@ describe('Hub', () => { // TODO fix this and write the test }); - it("should inherit parent's sampling decision when creating a new transaction if tracesSampler is undefined", () => { + it("should inherit parent's positive sampling decision if tracesSampler is undefined", () => { + // we know that without inheritance we'll get sampled = false (since our "random" number won't be below the + // sample rate), so make parent's decision the opposite to prove that inheritance takes precedence over + // tracesSampleRate + mathRandom.mockReturnValueOnce(1); + const hub = new Hub(new BrowserClient({ tracesSampleRate: 0.5 })); + const parentSamplingDecsion = true; + + const transaction = hub.startTransaction({ + name: 'dogpark', + parentSpanId: '12312012', + parentSampled: parentSamplingDecsion, + }); + + expect(transaction.sampled).toBe(parentSamplingDecsion); + }); + + it("should inherit parent's negative sampling decision if tracesSampler is undefined", () => { // tracesSampleRate = 1 means every transaction should end up with sampled = true, so make parent's decision the // opposite to prove that inheritance takes precedence over tracesSampleRate const hub = new Hub(new BrowserClient({ tracesSampleRate: 1 })); @@ -311,7 +347,7 @@ describe('Hub', () => { expect(transaction.sampled).toBe(parentSamplingDecsion); }); - it("should ignore parent's sampling decision when tracesSampler is defined", () => { + it("should ignore parent's positive sampling decision when tracesSampler is defined", () => { // this tracesSampler causes every transaction to end up with sampled = true, so make parent's decision the // opposite to prove that tracesSampler takes precedence over inheritance const tracesSampler = () => true; @@ -327,6 +363,23 @@ describe('Hub', () => { expect(transaction.sampled).not.toBe(parentSamplingDecsion); }); + + it("should ignore parent's negative sampling decision when tracesSampler is defined", () => { + // this tracesSampler causes every transaction to end up with sampled = false, so make parent's decision the + // opposite to prove that tracesSampler takes precedence over inheritance + const tracesSampler = () => false; + const parentSamplingDecsion = true; + + const hub = new Hub(new BrowserClient({ tracesSampler })); + + const transaction = hub.startTransaction({ + name: 'dogpark', + parentSpanId: '12312012', + parentSampled: parentSamplingDecsion, + }); + + expect(transaction.sampled).not.toBe(parentSamplingDecsion); + }); }); }); }); From c5276404cd8e9e51fa4f479ee4602874ef7c228f Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 29 Sep 2020 14:44:55 -0700 Subject: [PATCH 13/15] move meta tests inside main describe --- .../test/browser/browsertracing.test.ts | 47 ++++++++++--------- 1 file changed, 24 insertions(+), 23 deletions(-) diff --git a/packages/tracing/test/browser/browsertracing.test.ts b/packages/tracing/test/browser/browsertracing.test.ts index a753986ac051..e722e9ec6bcf 100644 --- a/packages/tracing/test/browser/browsertracing.test.ts +++ b/packages/tracing/test/browser/browsertracing.test.ts @@ -355,35 +355,36 @@ describe('BrowserTracing', () => { }); }); }); -}); -describe('sentry-trace element', () => { - describe('getMetaContent', () => { - it('finds the specified tag and extracts the value', () => { - const name = 'sentry-trace'; - const content = '126de09502ae4e0fb26c6967190756a4-b6e54397b12a2a0f-1'; - document.head.innerHTML = ``; + describe('sentry-trace element', () => { + describe('getMetaContent', () => { + it('finds the specified tag and extracts the value', () => { + const name = 'sentry-trace'; + const content = '126de09502ae4e0fb26c6967190756a4-b6e54397b12a2a0f-1'; + document.head.innerHTML = ``; - const metaTagValue = getMetaContent(name); - expect(metaTagValue).toBe(content); - }); + const metaTagValue = getMetaContent(name); + expect(metaTagValue).toBe(content); + }); - it("doesn't return meta tags other than the one specified", () => { - document.head.innerHTML = ``; + it("doesn't return meta tags other than the one specified", () => { + document.head.innerHTML = ``; - const metaTagValue = getMetaContent('dogpark'); - expect(metaTagValue).toBe(null); - }); + const metaTagValue = getMetaContent('dogpark'); + expect(metaTagValue).toBe(null); + }); - it('can pick the correct tag out of multiple options', () => { - const name = 'sentry-trace'; - const content = '126de09502ae4e0fb26c6967190756a4-b6e54397b12a2a0f-1'; - const sentryTraceMeta = ``; - const otherMeta = ``; - document.head.innerHTML = `${sentryTraceMeta} ${otherMeta}`; + it('can pick the correct tag out of multiple options', () => { + const name = 'sentry-trace'; + const content = '126de09502ae4e0fb26c6967190756a4-b6e54397b12a2a0f-1'; + const sentryTraceMeta = ``; + const otherMeta = ``; + document.head.innerHTML = `${sentryTraceMeta} ${otherMeta}`; - const metaTagValue = getMetaContent(name); - expect(metaTagValue).toBe(content); + const metaTagValue = getMetaContent(name); + expect(metaTagValue).toBe(content); + }); }); + }); }); From 58f7f3fa17c4ed15ae91104516c4e6984b177afd Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 29 Sep 2020 17:15:10 -0700 Subject: [PATCH 14/15] fix linting issues --- packages/tracing/test/browser/browsertracing.test.ts | 1 - packages/tracing/test/browser/request.test.ts | 1 - 2 files changed, 2 deletions(-) diff --git a/packages/tracing/test/browser/browsertracing.test.ts b/packages/tracing/test/browser/browsertracing.test.ts index e722e9ec6bcf..05eeb1dc3151 100644 --- a/packages/tracing/test/browser/browsertracing.test.ts +++ b/packages/tracing/test/browser/browsertracing.test.ts @@ -385,6 +385,5 @@ describe('BrowserTracing', () => { expect(metaTagValue).toBe(content); }); }); - }); }); diff --git a/packages/tracing/test/browser/request.test.ts b/packages/tracing/test/browser/request.test.ts index 727c11c28791..5e975ea12aca 100644 --- a/packages/tracing/test/browser/request.test.ts +++ b/packages/tracing/test/browser/request.test.ts @@ -108,7 +108,6 @@ describe('callbacks', () => { expect(spans).toEqual({}); }); - it('creates and finishes fetch span on active transaction', () => { const spans = {}; From 29299259bf7e40d27ce31abc39079079938a0982 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Wed, 30 Sep 2020 08:39:52 -0700 Subject: [PATCH 15/15] address PR feedback --- packages/tracing/src/browser/request.ts | 2 +- .../test/browser/browsertracing.test.ts | 24 +++++++------------ packages/tracing/test/browser/request.test.ts | 5 ++-- 3 files changed, 12 insertions(+), 19 deletions(-) diff --git a/packages/tracing/src/browser/request.ts b/packages/tracing/src/browser/request.ts index 8d4b05a3e0ad..c65af5d8e674 100644 --- a/packages/tracing/src/browser/request.ts +++ b/packages/tracing/src/browser/request.ts @@ -49,7 +49,7 @@ export interface FetchData { __span?: string; }; - // TODO (kmclb) Once type fix goes through, turn this back into a Response + // TODO Should this be unknown instead? If we vendor types, make it a Response // eslint-disable-next-line @typescript-eslint/no-explicit-any response?: any; diff --git a/packages/tracing/test/browser/browsertracing.test.ts b/packages/tracing/test/browser/browsertracing.test.ts index 05eeb1dc3151..90a3b8659bb3 100644 --- a/packages/tracing/test/browser/browsertracing.test.ts +++ b/packages/tracing/test/browser/browsertracing.test.ts @@ -214,21 +214,15 @@ describe('BrowserTracing', () => { createBrowserTracing(true, { routingInstrumentation: customRoutingInstrumentation }); - // we match on the calls themselves (rather than calling .toHaveBeenCalledWith()) because that method requires all - // of the arguments be supplied, when we really only care about the transaction context which gets passed - expect(startIdleTransaction.mock.calls).toEqual( - // all calls - expect.arrayContaining([ - // all arguments - expect.arrayContaining([ - // one of the arguments - expect.objectContaining({ - traceId: '126de09502ae4e0fb26c6967190756a4', - parentSpanId: 'b6e54397b12a2a0f', - parentSampled: true, - }), - ]), - ]), + expect(startIdleTransaction).toHaveBeenCalledWith( + expect.any(Object), + expect.objectContaining({ + traceId: '126de09502ae4e0fb26c6967190756a4', + parentSpanId: 'b6e54397b12a2a0f', + parentSampled: true, + }), + expect.any(Number), + expect.any(Boolean), ); }); diff --git a/packages/tracing/test/browser/request.test.ts b/packages/tracing/test/browser/request.test.ts index 5e975ea12aca..186589a3a552 100644 --- a/packages/tracing/test/browser/request.test.ts +++ b/packages/tracing/test/browser/request.test.ts @@ -57,8 +57,8 @@ describe('registerRequestInstrumentation', () => { describe('callbacks', () => { let hub: Hub; let transaction: Transaction; - const alwaysCreateSpan = jest.fn().mockReturnValue(true); - const neverCreateSpan = jest.fn().mockReturnValue(false); + const alwaysCreateSpan = () => true; + const neverCreateSpan = () => false; const fetchHandlerData: FetchData = { args: ['http://dogs.are.great/', {}], fetchData: { url: 'http://dogs.are.great/', method: 'GET' }, @@ -191,7 +191,6 @@ describe('callbacks', () => { const newSpan = transaction.spanRecorder?.spans[1]; - expect(newSpan).toBeDefined(); expect(newSpan).toBeInstanceOf(Span); expect(newSpan!.data).toEqual({ method: 'GET',