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.

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

🐛[RUMF-650] exclude intake request from performance/request collection #528

Merged
merged 3 commits into from
Sep 11, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
31 changes: 19 additions & 12 deletions packages/rum/src/performanceCollection.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import { DOM_EVENT, getRelativeTime, isNumber, monitor } from '@datadog/browser-core'
import { Configuration, DOM_EVENT, getRelativeTime, isNumber, monitor } from '@datadog/browser-core'

import { getDocumentTraceId } from './getDocumentTraceId'
import { LifeCycle, LifeCycleEventType } from './lifeCycle'
import { FAKE_INITIAL_DOCUMENT } from './resourceUtils'
import { FAKE_INITIAL_DOCUMENT, isAllowedRequestUrl } from './resourceUtils'

interface BrowserWindow extends Window {
PerformanceObserver?: PerformanceObserver
Expand Down Expand Up @@ -67,17 +67,17 @@ function supportPerformanceNavigationTimingEvent() {
)
}

export function startPerformanceCollection(lifeCycle: LifeCycle) {
export function startPerformanceCollection(lifeCycle: LifeCycle, configuration: Configuration) {
retrieveInitialDocumentResourceTiming((timing) => {
handleRumPerformanceEntry(lifeCycle, timing)
handleRumPerformanceEntry(lifeCycle, configuration, timing)
})

if (supportPerformanceObject()) {
handlePerformanceEntries(lifeCycle, performance.getEntries())
handlePerformanceEntries(lifeCycle, configuration, performance.getEntries())
}
if ((window as BrowserWindow).PerformanceObserver) {
const observer = new PerformanceObserver(
monitor((entries) => handlePerformanceEntries(lifeCycle, entries.getEntries()))
monitor((entries) => handlePerformanceEntries(lifeCycle, configuration, entries.getEntries()))
)
const entryTypes = ['resource', 'navigation', 'longtask']

Expand All @@ -96,7 +96,7 @@ export function startPerformanceCollection(lifeCycle: LifeCycle) {
}
if (!supportPerformanceNavigationTimingEvent()) {
retrieveNavigationTiming((timing) => {
handleRumPerformanceEntry(lifeCycle, timing)
handleRumPerformanceEntry(lifeCycle, configuration, timing)
})
}
}
Expand Down Expand Up @@ -171,24 +171,31 @@ function computeRelativePerformanceTiming() {
return result as PerformanceTiming
}

function handlePerformanceEntries(lifeCycle: LifeCycle, entries: PerformanceEntry[]) {
function handlePerformanceEntries(lifeCycle: LifeCycle, configuration: Configuration, entries: PerformanceEntry[]) {
entries.forEach((entry) => {
if (
entry.entryType === 'resource' ||
entry.entryType === 'navigation' ||
entry.entryType === 'paint' ||
entry.entryType === 'longtask'
) {
handleRumPerformanceEntry(lifeCycle, entry as RumPerformanceEntry)
handleRumPerformanceEntry(lifeCycle, configuration, entry as RumPerformanceEntry)
}
})
}

function handleRumPerformanceEntry(lifeCycle: LifeCycle, entry: RumPerformanceEntry) {
// Exclude incomplete navigation entries by filtering out those who have a loadEventEnd at 0
if (entry.entryType === 'navigation' && entry.loadEventEnd <= 0) {
function handleRumPerformanceEntry(lifeCycle: LifeCycle, configuration: Configuration, entry: RumPerformanceEntry) {
if (isIncompleteNavigation(entry) || isForbiddenResource(configuration, entry)) {
return
}

lifeCycle.notify(LifeCycleEventType.PERFORMANCE_ENTRY_COLLECTED, entry)
}

function isIncompleteNavigation(entry: RumPerformanceEntry) {
return entry.entryType === 'navigation' && entry.loadEventEnd <= 0
}

function isForbiddenResource(configuration: Configuration, entry: RumPerformanceEntry) {
return entry.entryType === 'resource' && !isAllowedRequestUrl(configuration, entry.name)
}
111 changes: 64 additions & 47 deletions packages/rum/src/requestCollection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
startXhrProxy,
XhrContext,
} from '@datadog/browser-core'
import { isAllowedRequestUrl } from './resourceUtils'
import { startTracer, TraceIdentifier, Tracer } from './tracer'

export interface RequestStartEvent {
Expand Down Expand Up @@ -34,8 +35,8 @@ let nextRequestIndex = 1
export function startRequestCollection(configuration: Configuration) {
const requestObservables: RequestObservables = [new Observable(), new Observable()]
const tracer = startTracer(configuration)
trackXhr(requestObservables, tracer)
trackFetch(requestObservables, tracer)
trackXhr(configuration, requestObservables, tracer)
trackFetch(configuration, requestObservables, tracer)
return requestObservables
}

Expand All @@ -45,33 +46,41 @@ interface CustomXhrContext extends XhrContext {
requestIndex: number
}

export function trackXhr([requestStartObservable, requestCompleteObservable]: RequestObservables, tracer: Tracer) {
export function trackXhr(
configuration: Configuration,
[requestStartObservable, requestCompleteObservable]: RequestObservables,
tracer: Tracer
) {
const xhrProxy = startXhrProxy<CustomXhrContext>()
xhrProxy.beforeSend((context, xhr) => {
const tracingResult = tracer.traceXhr(context, xhr)
if (tracingResult) {
context.traceId = tracingResult.traceId
context.spanId = tracingResult.spanId
}
context.requestIndex = getNextRequestIndex()
if (isAllowedRequestUrl(configuration, context.url!)) {
bcaudan marked this conversation as resolved.
Show resolved Hide resolved
const tracingResult = tracer.traceXhr(context, xhr)
if (tracingResult) {
context.traceId = tracingResult.traceId
context.spanId = tracingResult.spanId
}
context.requestIndex = getNextRequestIndex()

requestStartObservable.notify({
requestIndex: context.requestIndex,
})
requestStartObservable.notify({
requestIndex: context.requestIndex,
})
}
})
xhrProxy.onRequestComplete((context) => {
requestCompleteObservable.notify({
duration: context.duration,
method: context.method,
requestIndex: context.requestIndex,
response: context.response,
spanId: context.spanId,
startTime: context.startTime,
status: context.status,
traceId: context.traceId,
type: RequestType.XHR,
url: context.url,
})
if (isAllowedRequestUrl(configuration, context.url!)) {
bcaudan marked this conversation as resolved.
Show resolved Hide resolved
requestCompleteObservable.notify({
duration: context.duration,
method: context.method,
requestIndex: context.requestIndex,
response: context.response,
spanId: context.spanId,
startTime: context.startTime,
status: context.status,
traceId: context.traceId,
type: RequestType.XHR,
url: context.url,
})
}
})
return xhrProxy
}
Expand All @@ -82,34 +91,42 @@ interface CustomFetchContext extends FetchContext {
requestIndex: number
}

export function trackFetch([requestStartObservable, requestCompleteObservable]: RequestObservables, tracer: Tracer) {
export function trackFetch(
configuration: Configuration,
[requestStartObservable, requestCompleteObservable]: RequestObservables,
tracer: Tracer
) {
const fetchProxy = startFetchProxy<CustomFetchContext>()
fetchProxy.beforeSend((context) => {
const tracingResult = tracer.traceFetch(context)
if (tracingResult) {
context.traceId = tracingResult.traceId
context.spanId = tracingResult.spanId
}
context.requestIndex = getNextRequestIndex()
if (isAllowedRequestUrl(configuration, context.url!)) {
bcaudan marked this conversation as resolved.
Show resolved Hide resolved
const tracingResult = tracer.traceFetch(context)
if (tracingResult) {
context.traceId = tracingResult.traceId
context.spanId = tracingResult.spanId
}
context.requestIndex = getNextRequestIndex()

requestStartObservable.notify({
requestIndex: context.requestIndex,
})
requestStartObservable.notify({
requestIndex: context.requestIndex,
})
}
})
fetchProxy.onRequestComplete((context) => {
requestCompleteObservable.notify({
duration: context.duration,
method: context.method,
requestIndex: context.requestIndex,
response: context.response,
responseType: context.responseType,
spanId: context.spanId,
startTime: context.startTime,
status: context.status,
traceId: context.traceId,
type: RequestType.FETCH,
url: context.url,
})
if (isAllowedRequestUrl(configuration, context.url!)) {
bcaudan marked this conversation as resolved.
Show resolved Hide resolved
requestCompleteObservable.notify({
duration: context.duration,
method: context.method,
requestIndex: context.requestIndex,
response: context.response,
responseType: context.responseType,
spanId: context.spanId,
startTime: context.startTime,
status: context.status,
traceId: context.traceId,
type: RequestType.FETCH,
url: context.url,
})
}
})
return fetchProxy
}
Expand Down
4 changes: 2 additions & 2 deletions packages/rum/src/resourceUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,6 @@ export function computeSize(entry: RumPerformanceResourceTiming) {
return undefined
}

export function shouldTrackResource(url: string, configuration: Configuration, session: RumSession) {
return session.isTrackedWithResource() && url && !isIntakeRequest(url, configuration)
export function isAllowedRequestUrl(configuration: Configuration, url: string) {
return url && !isIntakeRequest(url, configuration)
}
2 changes: 1 addition & 1 deletion packages/rum/src/rum.entry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ export function makeRumGlobal(stub: RumGlobal) {
)

const [requestStartObservable, requestCompleteObservable] = startRequestCollection(configuration)
startPerformanceCollection(lifeCycle)
startPerformanceCollection(lifeCycle, configuration)
startDOMMutationCollection(lifeCycle)
if (configuration.trackInteractions) {
startUserActionCollection(lifeCycle)
Expand Down
5 changes: 2 additions & 3 deletions packages/rum/src/rum.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ import {
computePerformanceResourceDuration,
computeResourceKind,
computeSize,
shouldTrackResource,
} from './resourceUtils'
import { InternalContext, RumGlobal } from './rum.entry'
import { RumSession } from './rumSession'
Expand Down Expand Up @@ -440,7 +439,7 @@ function trackRequests(
handler: (startTime: number, event: RumResourceEvent) => void
) {
lifeCycle.subscribe(LifeCycleEventType.REQUEST_COMPLETED, (request: RequestCompleteEvent) => {
if (!shouldTrackResource(request.url, configuration, session)) {
if (!session.isTrackedWithResource()) {
return
}
const timing = matchRequestTiming(request)
Expand Down Expand Up @@ -504,7 +503,7 @@ export function handleResourceEntry(
handler: (startTime: number, event: RumResourceEvent) => void,
entry: RumPerformanceResourceTiming
) {
if (!shouldTrackResource(entry.name, configuration, session)) {
if (!session.isTrackedWithResource()) {
return
}
const resourceKind = computeResourceKind(entry)
Expand Down
37 changes: 35 additions & 2 deletions packages/rum/test/requestCollection.spec.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
import {
Configuration,
DEFAULT_CONFIGURATION,
FetchProxy,
FetchStub,
FetchStubManager,
isIE,
Observable,
RequestType,
SPEC_ENDPOINTS,
stubFetch,
withXhr,
} from '@datadog/browser-core'
Expand All @@ -19,6 +22,12 @@ import {
} from '../src/requestCollection'
import { Tracer } from '../src/tracer'

const configuration = {
...DEFAULT_CONFIGURATION,
...SPEC_ENDPOINTS,
maxBatchSize: 1,
}

describe('collect fetch', () => {
const FAKE_URL = 'http://fake-url/'
let fetchStub: FetchStub
Expand All @@ -41,7 +50,7 @@ describe('collect fetch', () => {
const tracerStub: Partial<Tracer> = {
traceFetch: () => undefined,
}
fetchProxy = trackFetch(requestObservables, tracerStub as Tracer)
fetchProxy = trackFetch(configuration as Configuration, requestObservables, tracerStub as Tracer)

fetchStub = window.fetch as FetchStub
window.onunhandledrejection = (ev: PromiseRejectionEvent) => {
Expand Down Expand Up @@ -91,6 +100,16 @@ describe('collect fetch', () => {
done()
})
})

it('should ignore intake requests', (done) => {
fetchStub(SPEC_ENDPOINTS.rumEndpoint!).resolveWith({ status: 200, responseText: 'foo' })

fetchStubManager.whenAllComplete(() => {
expect(startSpy).not.toHaveBeenCalled()
expect(completeSpy).not.toHaveBeenCalled()
done()
})
})
})

describe('collect xhr', () => {
Expand All @@ -110,7 +129,7 @@ describe('collect xhr', () => {
const tracerStub: Partial<Tracer> = {
traceXhr: () => undefined,
}
trackXhr(requestObservables, tracerStub as Tracer)
trackXhr(configuration as Configuration, requestObservables, tracerStub as Tracer)
})

afterEach(() => {
Expand Down Expand Up @@ -164,4 +183,18 @@ describe('collect xhr', () => {
},
})
})

it('should ignore intake requests', (done) => {
withXhr({
setup(xhr) {
xhr.open('GET', SPEC_ENDPOINTS.rumEndpoint!)
xhr.send()
},
onComplete() {
expect(startSpy).not.toHaveBeenCalled()
expect(completeSpy).not.toHaveBeenCalled()
done()
},
})
})
})
Loading