diff --git a/packages/rum/src/domain/requestCollection.spec.ts b/packages/rum/src/domain/requestCollection.spec.ts index 949369ce6a..1fcddfbfc8 100644 --- a/packages/rum/src/domain/requestCollection.spec.ts +++ b/packages/rum/src/domain/requestCollection.spec.ts @@ -15,7 +15,7 @@ import { } from '@datadog/browser-core' import { LifeCycle, LifeCycleEventType } from './lifeCycle' import { RequestCompleteEvent, RequestStartEvent, trackFetch, trackXhr } from './requestCollection' -import { Tracer } from './tracing/tracer' +import { clearTracingIfCancelled, Tracer } from './tracing/tracer' const configuration = { ...DEFAULT_CONFIGURATION, @@ -43,6 +43,7 @@ describe('collect fetch', () => { lifeCycle.subscribe(LifeCycleEventType.REQUEST_STARTED, startSpy) lifeCycle.subscribe(LifeCycleEventType.REQUEST_COMPLETED, completeSpy) const tracerStub: Partial = { + clearTracingIfCancelled, traceFetch: () => undefined, } fetchProxy = trackFetch(lifeCycle, configuration as Configuration, tracerStub as Tracer) @@ -84,6 +85,18 @@ describe('collect fetch', () => { }) }) + it('should not trace cancelled requests', (done) => { + fetchStub(FAKE_URL).resolveWith({ status: 0, responseText: 'fetch cancelled' }) + + fetchStubManager.whenAllComplete(() => { + const request = completeSpy.calls.argsFor(0)[0] + + expect(request.status).toEqual(0) + expect(request.traceId).toEqual(undefined) + done() + }) + }) + it('should assign a request id', (done) => { fetchStub(FAKE_URL).resolveWith({ status: 500, responseText: 'fetch error' }) @@ -123,6 +136,7 @@ describe('collect xhr', () => { lifeCycle.subscribe(LifeCycleEventType.REQUEST_STARTED, startSpy) lifeCycle.subscribe(LifeCycleEventType.REQUEST_COMPLETED, completeSpy) const tracerStub: Partial = { + clearTracingIfCancelled, traceXhr: () => undefined, } trackXhr(lifeCycle, configuration as Configuration, tracerStub as Tracer) @@ -198,4 +212,20 @@ describe('collect xhr', () => { }, }) }) + + it('should not trace cancelled requests', (done) => { + withXhr({ + setup(xhr) { + xhr.open('GET', '/ok') + xhr.send() + xhr.complete(0) + }, + onComplete() { + const request = completeSpy.calls.argsFor(0)[0] + expect(request.status).toEqual(0) + expect(request.traceId).toEqual(undefined) + done() + }, + }) + }) }) diff --git a/packages/rum/src/domain/requestCollection.ts b/packages/rum/src/domain/requestCollection.ts index 3058342940..fe2b6baf76 100644 --- a/packages/rum/src/domain/requestCollection.ts +++ b/packages/rum/src/domain/requestCollection.ts @@ -13,6 +13,16 @@ import { LifeCycle, LifeCycleEventType } from './lifeCycle' import { isAllowedRequestUrl } from './rumEventsCollection/resource/resourceUtils' import { startTracer, TraceIdentifier, Tracer } from './tracing/tracer' +export interface CustomContext { + requestIndex: number + spanId?: TraceIdentifier + traceId?: TraceIdentifier +} +export interface RumFetchStartContext extends FetchStartContext, CustomContext {} +export interface RumFetchCompleteContext extends FetchCompleteContext, CustomContext {} +export interface RumXhrStartContext extends XhrStartContext, CustomContext {} +export interface RumXhrCompleteContext extends XhrCompleteContext, CustomContext {} + export interface RequestStartEvent { requestIndex: number } @@ -27,14 +37,8 @@ export interface RequestCompleteEvent { responseType?: string startTime: number duration: number - traceId?: TraceIdentifier spanId?: TraceIdentifier -} - -interface CustomContext { - traceId: TraceIdentifier | undefined - spanId: TraceIdentifier | undefined - requestIndex: number + traceId?: TraceIdentifier } export type RequestObservables = [Observable, Observable] @@ -48,14 +52,10 @@ export function startRequestCollection(lifeCycle: LifeCycle, configuration: Conf } export function trackXhr(lifeCycle: LifeCycle, configuration: Configuration, tracer: Tracer) { - const xhrProxy = startXhrProxy() + const xhrProxy = startXhrProxy() xhrProxy.beforeSend((context, xhr) => { if (isAllowedRequestUrl(configuration, context.url)) { - const tracingResult = tracer.traceXhr(context, xhr) - if (tracingResult) { - context.traceId = tracingResult.traceId - context.spanId = tracingResult.spanId - } + tracer.traceXhr(context, xhr) context.requestIndex = getNextRequestIndex() lifeCycle.notify(LifeCycleEventType.REQUEST_STARTED, { @@ -65,6 +65,7 @@ export function trackXhr(lifeCycle: LifeCycle, configuration: Configuration, tra }) xhrProxy.onRequestComplete((context) => { if (isAllowedRequestUrl(configuration, context.url)) { + tracer.clearTracingIfCancelled(context) lifeCycle.notify(LifeCycleEventType.REQUEST_COMPLETED, { duration: context.duration, method: context.method, @@ -83,14 +84,10 @@ export function trackXhr(lifeCycle: LifeCycle, configuration: Configuration, tra } export function trackFetch(lifeCycle: LifeCycle, configuration: Configuration, tracer: Tracer) { - const fetchProxy = startFetchProxy() + const fetchProxy = startFetchProxy() fetchProxy.beforeSend((context) => { if (isAllowedRequestUrl(configuration, context.url)) { - const tracingResult = tracer.traceFetch(context) - if (tracingResult) { - context.traceId = tracingResult.traceId - context.spanId = tracingResult.spanId - } + tracer.traceFetch(context) context.requestIndex = getNextRequestIndex() lifeCycle.notify(LifeCycleEventType.REQUEST_STARTED, { @@ -100,6 +97,7 @@ export function trackFetch(lifeCycle: LifeCycle, configuration: Configuration, t }) fetchProxy.onRequestComplete((context) => { if (isAllowedRequestUrl(configuration, context.url)) { + tracer.clearTracingIfCancelled(context) lifeCycle.notify(LifeCycleEventType.REQUEST_COMPLETED, { duration: context.duration, method: context.method, diff --git a/packages/rum/src/domain/tracing/tracer.spec.ts b/packages/rum/src/domain/tracing/tracer.spec.ts index 517de7aa78..7dd7987470 100644 --- a/packages/rum/src/domain/tracing/tracer.spec.ts +++ b/packages/rum/src/domain/tracing/tracer.spec.ts @@ -1,12 +1,11 @@ -import { - Configuration, - DEFAULT_CONFIGURATION, - FetchCompleteContext, - isIE, - objectEntries, - XhrCompleteContext, -} from '@datadog/browser-core' +import { Configuration, DEFAULT_CONFIGURATION, isIE, objectEntries } from '@datadog/browser-core' import { setup, TestSetupBuilder } from '../../../test/specHelper' +import { + RumFetchCompleteContext, + RumFetchStartContext, + RumXhrCompleteContext, + RumXhrStartContext, +} from '../requestCollection' import { startTracer, TraceIdentifier } from './tracer' describe('tracer', () => { @@ -14,8 +13,12 @@ describe('tracer', () => { ...DEFAULT_CONFIGURATION, allowedTracingOrigins: [window.location.origin], } - const ALLOWED_DOMAIN_CONTEXT: Partial = { url: window.location.origin } - const DISALLOWED_DOMAIN_CONTEXT: Partial = { url: 'http://foo.com' } + const ALLOWED_DOMAIN_CONTEXT: Partial = { + url: window.location.origin, + } + const DISALLOWED_DOMAIN_CONTEXT: Partial = { + url: 'http://foo.com', + } let setupBuilder: TestSetupBuilder beforeEach(() => { @@ -43,19 +46,23 @@ describe('tracer', () => { } }) - it('should return traceId and add tracing headers', () => { + it('should add traceId and spanId to context and add tracing headers', () => { const tracer = startTracer(configuration as Configuration) - const tracingResult = tracer.traceXhr(ALLOWED_DOMAIN_CONTEXT, (xhrStub as unknown) as XMLHttpRequest)! + const context = { ...ALLOWED_DOMAIN_CONTEXT } + tracer.traceXhr(context, (xhrStub as unknown) as XMLHttpRequest) - expect(tracingResult).toBeDefined() - expect(xhrStub.headers).toEqual(tracingHeadersFor(tracingResult.traceId, tracingResult.spanId)) + expect(context.traceId).toBeDefined() + expect(context.spanId).toBeDefined() + expect(xhrStub.headers).toEqual(tracingHeadersFor(context.traceId!, context.spanId!)) }) it('should not trace request on disallowed domain', () => { const tracer = startTracer(configuration as Configuration) - const tracingResult = tracer.traceXhr(DISALLOWED_DOMAIN_CONTEXT, (xhrStub as unknown) as XMLHttpRequest) + const context = { ...DISALLOWED_DOMAIN_CONTEXT } + tracer.traceXhr(context, (xhrStub as unknown) as XMLHttpRequest) - expect(tracingResult).toBeUndefined() + expect(context.traceId).toBeUndefined() + expect(context.spanId).toBeUndefined() expect(xhrStub.headers).toEqual({}) }) @@ -68,8 +75,14 @@ describe('tracer', () => { const tracer = startTracer(configurationWithTracingUrls as Configuration) - expect(tracer.traceXhr({ url: 'http://qux.com' }, stub)).toBeDefined() - expect(tracer.traceXhr({ url: 'http://bar.com' }, stub)).toBeDefined() + let context: Partial = { url: 'http://qux.com' } + tracer.traceXhr(context, stub) + expect(context.traceId).toBeDefined() + expect(context.spanId).toBeDefined() + context = { url: 'http://bar.com' } + tracer.traceXhr(context, stub) + expect(context.traceId).toBeDefined() + expect(context.spanId).toBeDefined() }) }) @@ -80,47 +93,47 @@ describe('tracer', () => { } }) - it('should return traceId and add tracing headers', () => { - const context: Partial = { ...ALLOWED_DOMAIN_CONTEXT } - + it('should add traceId and spanId to context, and add tracing headers', () => { + const context: Partial = { ...ALLOWED_DOMAIN_CONTEXT } const tracer = startTracer(configuration as Configuration) - const tracingResult = tracer.traceFetch(context)! + tracer.traceFetch(context) - expect(tracingResult).toBeDefined() - expect(context.init!.headers).toEqual(tracingHeadersAsArrayFor(tracingResult.traceId, tracingResult.spanId)) + expect(context.traceId).toBeDefined() + expect(context.spanId).toBeDefined() + expect(context.init!.headers).toEqual(tracingHeadersAsArrayFor(context.traceId!, context.spanId!)) }) it('should preserve original request init', () => { const init = { method: 'POST' } - const context: Partial = { + const context: Partial = { ...ALLOWED_DOMAIN_CONTEXT, init, } const tracer = startTracer(configuration as Configuration) - const tracingResult = tracer.traceFetch(context)! + tracer.traceFetch(context) expect(context.init).not.toBe(init) expect(context.init!.method).toBe('POST') - expect(context.init!.headers).toEqual(tracingHeadersAsArrayFor(tracingResult.traceId, tracingResult.spanId)) + expect(context.init!.headers).toEqual(tracingHeadersAsArrayFor(context.traceId!, context.spanId!)) }) it('should preserve original headers object', () => { const headers = new Headers() headers.set('foo', 'bar') - const context: Partial = { + const context: Partial = { ...ALLOWED_DOMAIN_CONTEXT, init: { headers, method: 'POST' }, } const tracer = startTracer(configuration as Configuration) - const tracingResult = tracer.traceFetch(context)! + tracer.traceFetch(context) expect(context.init!.headers).not.toBe(headers) expect(context.init!.headers).toEqual([ ['foo', 'bar'], - ...tracingHeadersAsArrayFor(tracingResult.traceId, tracingResult.spanId), + ...tracingHeadersAsArrayFor(context.traceId!, context.spanId!), ]) expect(toPlainObject(headers)).toEqual({ foo: 'bar', @@ -130,18 +143,18 @@ describe('tracer', () => { it('should preserve original headers plain object', () => { const headers = { foo: 'bar' } - const context: Partial = { + const context: Partial = { ...ALLOWED_DOMAIN_CONTEXT, init: { headers, method: 'POST' }, } const tracer = startTracer(configuration as Configuration) - const tracingResult = tracer.traceFetch(context)! + tracer.traceFetch(context) expect(context.init!.headers).not.toBe(headers) expect(context.init!.headers).toEqual([ ['foo', 'bar'], - ...tracingHeadersAsArrayFor(tracingResult.traceId, tracingResult.spanId), + ...tracingHeadersAsArrayFor(context.traceId!, context.spanId!), ]) expect(headers).toEqual({ @@ -152,31 +165,32 @@ describe('tracer', () => { it('should preserve original headers array', () => { const headers = [['foo', 'bar'], ['foo', 'baz']] - const context: Partial = { + const context: Partial = { ...ALLOWED_DOMAIN_CONTEXT, init: { headers, method: 'POST' }, } const tracer = startTracer(configuration as Configuration) - const tracingResult = tracer.traceFetch(context)! + tracer.traceFetch(context) expect(context.init!.headers).not.toBe(headers) expect(context.init!.headers).toEqual([ ['foo', 'bar'], ['foo', 'baz'], - ...tracingHeadersAsArrayFor(tracingResult.traceId, tracingResult.spanId), + ...tracingHeadersAsArrayFor(context.traceId!, context.spanId!), ]) expect(headers).toEqual([['foo', 'bar'], ['foo', 'baz']]) }) it('should not trace request on disallowed domain', () => { - const context: Partial = { ...DISALLOWED_DOMAIN_CONTEXT } + const context: Partial = { ...DISALLOWED_DOMAIN_CONTEXT } const tracer = startTracer(configuration as Configuration) - const tracingResult = tracer.traceFetch(context) + tracer.traceFetch(context) - expect(tracingResult).toBeUndefined() + expect(context.traceId).toBeUndefined() + expect(context.spanId).toBeUndefined() expect(context.init).toBeUndefined() }) @@ -185,13 +199,47 @@ describe('tracer', () => { ...configuration, allowedTracingOrigins: [/^https?:\/\/qux\.com.*/, 'http://bar.com'], } - const quxDomainContext: Partial = { url: 'http://qux.com' } - const barDomainContext: Partial = { url: 'http://bar.com' } + const quxDomainContext: Partial = { url: 'http://qux.com' } + const barDomainContext: Partial = { url: 'http://bar.com' } const tracer = startTracer(configurationWithTracingUrls as Configuration) - expect(tracer.traceFetch(quxDomainContext)).toBeDefined() - expect(tracer.traceFetch(barDomainContext)).toBeDefined() + tracer.traceFetch(quxDomainContext) + tracer.traceFetch(barDomainContext) + expect(quxDomainContext.traceId).toBeDefined() + expect(quxDomainContext.spanId).toBeDefined() + expect(barDomainContext.traceId).toBeDefined() + expect(barDomainContext.spanId).toBeDefined() + }) + }) + + describe('clearTracingIfCancelled', () => { + it('should clear tracing if status is 0', () => { + const tracer = startTracer(configuration as Configuration) + const context: RumFetchCompleteContext = { + status: 0, + + spanId: new TraceIdentifier(), + traceId: new TraceIdentifier(), + } as any + tracer.clearTracingIfCancelled(context) + + expect(context.traceId).toBeUndefined() + expect(context.spanId).toBeUndefined() + }) + + it('should not clear tracing if status is not 0', () => { + const tracer = startTracer(configuration as Configuration) + const context: RumFetchCompleteContext = { + status: 200, + + spanId: new TraceIdentifier(), + traceId: new TraceIdentifier(), + } as any + tracer.clearTracingIfCancelled(context) + + expect(context.traceId).toBeDefined() + expect(context.spanId).toBeDefined() }) }) }) diff --git a/packages/rum/src/domain/tracing/tracer.ts b/packages/rum/src/domain/tracing/tracer.ts index d185e901f4..60e4f0a6b3 100644 --- a/packages/rum/src/domain/tracing/tracer.ts +++ b/packages/rum/src/domain/tracing/tracer.ts @@ -1,29 +1,33 @@ +import { Configuration, getOrigin, objectEntries } from '@datadog/browser-core' import { - Configuration, - FetchCompleteContext, - getOrigin, - objectEntries, - XhrCompleteContext, -} from '@datadog/browser-core' - -export interface TracingResult { - spanId: TraceIdentifier - traceId: TraceIdentifier -} + RumFetchCompleteContext, + RumFetchStartContext, + RumXhrCompleteContext, + RumXhrStartContext, +} from '../requestCollection' export interface Tracer { - traceFetch: (context: Partial) => TracingResult | undefined - traceXhr: (context: Partial, xhr: XMLHttpRequest) => TracingResult | undefined + traceFetch: (context: Partial) => void + traceXhr: (context: Partial, xhr: XMLHttpRequest) => void + clearTracingIfCancelled: (context: RumFetchCompleteContext | RumXhrCompleteContext) => void } interface TracingHeaders { [key: string]: string } +export function clearTracingIfCancelled(context: RumFetchCompleteContext | RumXhrCompleteContext) { + if (context.status === 0) { + context.traceId = undefined + context.spanId = undefined + } +} + export function startTracer(configuration: Configuration): Tracer { return { + clearTracingIfCancelled, traceFetch: (context) => - injectHeadersIfTracingAllowed(configuration, context.url!, (tracingHeaders: TracingHeaders) => { + injectHeadersIfTracingAllowed(configuration, context, (tracingHeaders: TracingHeaders) => { context.init = { ...context.init } const headers: string[][] = [] if (context.init.headers instanceof Headers) { @@ -42,7 +46,7 @@ export function startTracer(configuration: Configuration): Tracer { context.init.headers = headers.concat(objectEntries(tracingHeaders) as string[][]) }), traceXhr: (context, xhr) => - injectHeadersIfTracingAllowed(configuration, context.url!, (tracingHeaders: TracingHeaders) => { + injectHeadersIfTracingAllowed(configuration, context, (tracingHeaders: TracingHeaders) => { Object.keys(tracingHeaders).forEach((name) => { xhr.setRequestHeader(name, tracingHeaders[name]) }) @@ -52,17 +56,16 @@ export function startTracer(configuration: Configuration): Tracer { function injectHeadersIfTracingAllowed( configuration: Configuration, - url: string, + context: Partial, inject: (tracingHeaders: TracingHeaders) => void -): TracingResult | undefined { - if (!isTracingSupported() || !isAllowedUrl(configuration, url)) { - return undefined +) { + if (!isTracingSupported() || !isAllowedUrl(configuration, context.url!)) { + return } - const traceId = new TraceIdentifier() - const spanId = new TraceIdentifier() - inject(makeTracingHeaders(traceId, spanId)) - return { traceId, spanId } + context.traceId = new TraceIdentifier() + context.spanId = new TraceIdentifier() + inject(makeTracingHeaders(context.traceId, context.spanId)) } function isAllowedUrl(configuration: Configuration, requestUrl: string) {