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-1337] Fix incorrect fetch duration #1875

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
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
6 changes: 2 additions & 4 deletions packages/core/src/browser/fetchObservable.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import { instrumentMethod } from '../tools/instrumentMethod'
import { callMonitored, monitor } from '../tools/monitor'
import { Observable } from '../tools/observable'
import type { Duration, ClocksState } from '../tools/timeUtils'
import { elapsed, clocksNow, timeStampNow } from '../tools/timeUtils'
import type { ClocksState } from '../tools/timeUtils'
import { clocksNow } from '../tools/timeUtils'
import { normalizeUrl } from '../tools/urlPolyfill'

interface FetchContextBase {
Expand All @@ -20,7 +20,6 @@ export interface FetchStartContext extends FetchContextBase {
export interface FetchResolveContext extends FetchContextBase {
state: 'resolve'
status: number
resolveDuration?: Duration
response?: Response
responseType?: string
isAborted: boolean
Expand Down Expand Up @@ -96,7 +95,6 @@ function afterSend(
const reportFetch = (response: Response | Error) => {
const context = startContext as unknown as FetchResolveContext
context.state = 'resolve'
context.resolveDuration = elapsed(startContext.startClocks.timeStamp, timeStampNow())
if ('stack' in response || response instanceof Error) {
context.status = 0
context.isAborted = response instanceof DOMException && response.code === DOMException.ABORT_ERR
Expand Down
29 changes: 1 addition & 28 deletions packages/rum-core/src/domain/requestCollection.spec.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { isIE, RequestType, updateExperimentalFeatures, resetExperimentalFeatures } from '@datadog/browser-core'
import { isIE, RequestType } from '@datadog/browser-core'
import type { FetchStub, FetchStubManager } from '../../../core/test/specHelper'
import { SPEC_ENDPOINTS, stubFetch, stubXhr, withXhr } from '../../../core/test/specHelper'
import type { RumConfiguration } from './configuration'
Expand Down Expand Up @@ -100,33 +100,6 @@ describe('collect fetch', () => {
})
})

describe('with feature flag fetch_duration', () => {
beforeEach(() => updateExperimentalFeatures(['fetch_duration']))
afterEach(() => resetExperimentalFeatures())

it('should notify when response is defined', (done) => {
fetchStub(FAKE_URL).resolveWith({ status: 200, responseText: 'ok' })

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

it('should notify when response is undefined', (done) => {
updateExperimentalFeatures(['fetch_duration'])

fetchStub(FAKE_URL).rejectWith(new Error('some fetch error'))

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

describe('tracing', () => {
it('should trace requests by default', (done) => {
fetchStub(FAKE_URL).resolveWith({ status: 200, responseText: 'ok' })
Expand Down
5 changes: 1 addition & 4 deletions packages/rum-core/src/domain/requestCollection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import {
readBytesFromStream,
elapsed,
timeStampNow,
isExperimentalFeatureEnabled,
} from '@datadog/browser-core'
import type { RumSessionManager } from '..'
import type { RumConfiguration } from './configuration'
Expand Down Expand Up @@ -46,7 +45,6 @@ export interface RequestCompleteEvent {
status: number
responseType?: string
startClocks: ClocksState
resolveDuration?: Duration
duration: Duration
spanId?: TraceIdentifier
traceId?: TraceIdentifier
Expand Down Expand Up @@ -130,7 +128,6 @@ export function trackFetch(lifeCycle: LifeCycle, configuration: RumConfiguration
waitForResponseToComplete(context, (duration: Duration) => {
tracer.clearTracingIfNeeded(context)
lifeCycle.notify(LifeCycleEventType.REQUEST_COMPLETED, {
resolveDuration: context.resolveDuration,
duration,
method: context.method,
requestIndex: context.requestIndex,
Expand Down Expand Up @@ -160,7 +157,7 @@ function getNextRequestIndex() {
}

function waitForResponseToComplete(context: RumFetchResolveContext, callback: (duration: Duration) => void) {
if (context.response && isExperimentalFeatureEnabled('fetch_duration')) {
if (context.response) {
const responseClone = context.response.clone()
if (responseClone.body) {
readBytesFromStream(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,9 +96,6 @@ describe('resourceCollection', () => {
type: RumEventType.RESOURCE,
_dd: {
discarded: false,
resolveDuration: undefined,
durationDiff: undefined,
durationPercentageDiff: undefined,
},
})
expect(rawRumEvents[0].domainContext).toEqual({
Expand All @@ -122,7 +119,6 @@ describe('resourceCollection', () => {
LifeCycleEventType.REQUEST_COMPLETED,
createCompletedRequest({
duration: 500 as Duration,
resolveDuration: 50 as Duration,
method: 'GET',
startClocks: relativeToClocks(100 as RelativeTime),
status: 200,
Expand Down Expand Up @@ -150,7 +146,6 @@ describe('resourceCollection', () => {
LifeCycleEventType.REQUEST_COMPLETED,
createCompletedRequest({
duration: 100 as Duration,
resolveDuration: 50 as Duration,
method: 'GET',
startClocks: { relative: 1234 as RelativeTime, timeStamp: 123456789 as TimeStamp },
status: 200,
Expand All @@ -176,9 +171,6 @@ describe('resourceCollection', () => {
type: RumEventType.RESOURCE,
_dd: {
discarded: false,
resolveDuration: (50 * 1e6) as ServerDuration,
durationDiff: (50 * 1e6) as ServerDuration,
durationPercentageDiff: 50,
},
})
expect(rawRumEvents[0].domainContext).toEqual({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {
isNumber,
isExperimentalFeatureEnabled,
} from '@datadog/browser-core'
import type { ClocksState, Duration, ServerDuration } from '@datadog/browser-core'
import type { ClocksState, ServerDuration } from '@datadog/browser-core'
import type { RumConfiguration } from '../../configuration'
import type { RumPerformanceEntry, RumPerformanceResourceTiming } from '../../../browser/performanceCollection'
import type {
Expand Down Expand Up @@ -67,8 +67,6 @@ function processRequest(
const tracingInfo = computeRequestTracingInfo(request, configuration)
const indexingInfo = computeIndexingInfo(sessionManager, startClocks)

const responseDurationInfo = computeResponseDurationInfo(request)

const duration = toServerDuration(request.duration)
const durationOverrideInfo = computeDurationOverrideInfo(duration, correspondingTimingOverrides?.resource.duration)

Expand All @@ -88,7 +86,6 @@ function processRequest(
tracingInfo,
correspondingTimingOverrides,
indexingInfo,
responseDurationInfo,
durationOverrideInfo
)
return {
Expand Down Expand Up @@ -179,22 +176,6 @@ function computeEntryTracingInfo(entry: RumPerformanceResourceTiming, configurat
}
}

function computeResponseDurationInfo(request: RequestCompleteEvent) {
let durationDiff
let durationPercentageDiff
if (request.resolveDuration) {
durationDiff = toServerDuration((request.duration - request.resolveDuration) as Duration)
durationPercentageDiff = Math.round((durationDiff / toServerDuration(request.duration)) * 100)
}
return {
_dd: {
resolveDuration: toServerDuration(request.resolveDuration),
durationDiff,
durationPercentageDiff,
},
}
}

function computeDurationOverrideInfo(
computedDuration: ServerDuration,
performanceEntryDuration: ServerDuration | undefined
Expand Down
3 changes: 0 additions & 3 deletions packages/rum-core/src/rawRumEvent.types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,6 @@ export interface RawRumResourceEvent {
span_id?: string // not available for initial document tracing
rule_psr?: number
discarded: boolean
resolveDuration?: ServerDuration
durationDiff?: ServerDuration
durationPercentageDiff?: number
}
}

Expand Down
2 changes: 1 addition & 1 deletion test/e2e/scenario/rum/resources.scenario.ts
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ describe('rum resources', () => {
})

createTest('track redirect fetch timings')
.withRum({ enableExperimentalFeatures: ['fetch_duration'] })
.withRum()
.run(async ({ serverEvents }) => {
await browserExecuteAsync((done) => {
fetch('/redirect?duration=200').then(
Expand Down