From 4faa41900e7086fb97b6b238cbc6bb953c84237f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Zugmeyer?= Date: Thu, 28 Mar 2024 14:48:20 +0100 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20[RUM-2782]=20validate=20resource?= =?UTF-8?q?=20timings=20more=20granularly?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Before this commit, we dropped any `PerformanceResourceEntry` that had inconsistent timings. With this commit, when the experimental feature "tolerant_resource_timings" is enabled, the various timings are validated independently. This should improve the situation on Chrome when a Service Worker is involved. In this case, most of the time, connection timings are unexpectedly set after the `requestStart` timing (see Chromium issue[1]). Before this change, the whole `PerformanceResourceEntry` would be ignored, and no timings would be included. With this change (and the flag enabled), relevant timings will be correctly defined. Related issue: #2566 [1]: https://issues.chromium.org/issues/323703325 --- .../core/src/tools/experimentalFeatures.ts | 1 + .../resource/matchRequestTiming.spec.ts | 20 +++++++++++-- .../src/domain/resource/resourceUtils.spec.ts | 25 +++++++++++++---- .../src/domain/resource/resourceUtils.ts | 28 ++++++++++++------- 4 files changed, 56 insertions(+), 18 deletions(-) diff --git a/packages/core/src/tools/experimentalFeatures.ts b/packages/core/src/tools/experimentalFeatures.ts index e0c2300ab0..36482909f9 100644 --- a/packages/core/src/tools/experimentalFeatures.ts +++ b/packages/core/src/tools/experimentalFeatures.ts @@ -19,6 +19,7 @@ export enum ExperimentalFeature { DISABLE_REPLAY_INLINE_CSS = 'disable_replay_inline_css', WRITABLE_RESOURCE_GRAPHQL = 'writable_resource_graphql', CUSTOM_VITALS = 'custom_vitals', + TOLERANT_RESOURCE_TIMINGS = 'tolerant_resource_timings', } const enabledExperimentalFeatures: Set = new Set() diff --git a/packages/rum-core/src/domain/resource/matchRequestTiming.spec.ts b/packages/rum-core/src/domain/resource/matchRequestTiming.spec.ts index 6e85a2c9cb..bfc6bf9370 100644 --- a/packages/rum-core/src/domain/resource/matchRequestTiming.spec.ts +++ b/packages/rum-core/src/domain/resource/matchRequestTiming.spec.ts @@ -1,5 +1,6 @@ import type { Duration, RelativeTime } from '@datadog/browser-core' -import { isIE, relativeToClocks } from '@datadog/browser-core' +import { ExperimentalFeature, isIE, relativeToClocks } from '@datadog/browser-core' +import { mockExperimentalFeatures } from '@datadog/browser-core/test' import { createPerformanceEntry } from '../../../test' import type { RumPerformanceResourceTiming } from '../../browser/performanceCollection' import { RumPerformanceEntryType } from '../../browser/performanceCollection' @@ -94,7 +95,7 @@ describe('matchRequestTiming', () => { expect(matchingTiming).toEqual(undefined) }) - it('should not match invalid timing nested in the request ', () => { + it('[without tolerant_resource_timings] should not match invalid timing nested in the request ', () => { const entry = createPerformanceEntry(RumPerformanceEntryType.RESOURCE, { // fetchStart < startTime is invalid fetchStart: 0 as RelativeTime, @@ -107,4 +108,19 @@ describe('matchRequestTiming', () => { expect(matchingTiming).toEqual(undefined) }) + + it('[with tolerant_resource_timings] should match invalid timing nested in the request ', () => { + mockExperimentalFeatures([ExperimentalFeature.TOLERANT_RESOURCE_TIMINGS]) + const entry = createPerformanceEntry(RumPerformanceEntryType.RESOURCE, { + // fetchStart < startTime is invalid + fetchStart: 0 as RelativeTime, + startTime: 200 as RelativeTime, + }) + + entries.push(entry) + + const matchingTiming = matchRequestTiming(FAKE_REQUEST as RequestCompleteEvent) + + expect(matchingTiming).toBeDefined() + }) }) diff --git a/packages/rum-core/src/domain/resource/resourceUtils.spec.ts b/packages/rum-core/src/domain/resource/resourceUtils.spec.ts index af31099f28..3fa7c9d20a 100644 --- a/packages/rum-core/src/domain/resource/resourceUtils.spec.ts +++ b/packages/rum-core/src/domain/resource/resourceUtils.spec.ts @@ -1,5 +1,5 @@ -import type { Duration, RelativeTime, ServerDuration } from '@datadog/browser-core' -import { SPEC_ENDPOINTS } from '@datadog/browser-core/test' +import { ExperimentalFeature, type Duration, type RelativeTime, type ServerDuration } from '@datadog/browser-core' +import { SPEC_ENDPOINTS, mockExperimentalFeatures } from '@datadog/browser-core/test' import { RumPerformanceEntryType, type RumPerformanceResourceTiming } from '../../browser/performanceCollection' import type { RumConfiguration } from '../configuration' import { validateAndBuildRumConfiguration } from '../configuration' @@ -196,31 +196,37 @@ describe('computePerformanceResourceDetails', () => { }) ;[ { + timing: 'connect' as const, + reason: 'connectStart > connectEnd', connectEnd: 10 as RelativeTime, connectStart: 20 as RelativeTime, - reason: 'connectStart > connectEnd', }, { + timing: 'dns' as const, + reason: 'domainLookupStart > domainLookupEnd', domainLookupEnd: 10 as RelativeTime, domainLookupStart: 20 as RelativeTime, - reason: 'domainLookupStart > domainLookupEnd', }, { + timing: 'download' as const, reason: 'responseStart > responseEnd', responseEnd: 10 as RelativeTime, responseStart: 20 as RelativeTime, }, { + timing: 'first_byte' as const, reason: 'requestStart > responseStart', requestStart: 20 as RelativeTime, responseStart: 10 as RelativeTime, }, { + timing: 'redirect' as const, reason: 'redirectStart > redirectEnd', redirectEnd: 15 as RelativeTime, redirectStart: 20 as RelativeTime, }, { + timing: 'ssl' as const, connectEnd: 10 as RelativeTime, reason: 'secureConnectionStart > connectEnd', secureConnectionStart: 20 as RelativeTime, @@ -231,10 +237,17 @@ describe('computePerformanceResourceDetails', () => { fetchStart: 10 as RelativeTime, reason: 'negative timing start', }, - ].forEach(({ reason, ...overrides }) => { - it(`should not compute entry when ${reason}`, () => { + ].forEach(({ reason, timing, ...overrides }) => { + it(`[without tolerant-resource-timings] should not compute entry when ${reason}`, () => { expect(computePerformanceResourceDetails(generateResourceWith(overrides))).toBeUndefined() }) + + if (timing) { + it(`[with tolerant-resource-timings] should not include the '${timing}' timing when ${reason}`, () => { + mockExperimentalFeatures([ExperimentalFeature.TOLERANT_RESOURCE_TIMINGS]) + expect(computePerformanceResourceDetails(generateResourceWith(overrides))![timing]).toBeUndefined() + }) + } }) it('should allow really fast document resource', () => { diff --git a/packages/rum-core/src/domain/resource/resourceUtils.ts b/packages/rum-core/src/domain/resource/resourceUtils.ts index 6421aa7e8b..54787eb6ee 100644 --- a/packages/rum-core/src/domain/resource/resourceUtils.ts +++ b/packages/rum-core/src/domain/resource/resourceUtils.ts @@ -2,8 +2,10 @@ import type { RelativeTime, ServerDuration } from '@datadog/browser-core' import { addTelemetryDebug, elapsed, + ExperimentalFeature, getPathName, includes, + isExperimentalFeatureEnabled, isValidUrl, ResourceType, toServerDuration, @@ -19,8 +21,8 @@ export interface PerformanceResourceDetails { dns?: PerformanceResourceDetailsElement connect?: PerformanceResourceDetailsElement ssl?: PerformanceResourceDetailsElement - first_byte: PerformanceResourceDetailsElement - download: PerformanceResourceDetailsElement + first_byte?: PerformanceResourceDetailsElement + download?: PerformanceResourceDetailsElement } export const FAKE_INITIAL_DOCUMENT = 'initial_document' @@ -113,21 +115,22 @@ export function computePerformanceResourceDetails( } // Make sure a connection occurred - if (connectEnd !== fetchStart) { + if (fetchStart < connectEnd) { details.connect = formatTiming(startTime, connectStart, connectEnd) // Make sure a secure connection occurred - if (areInOrder(connectStart, secureConnectionStart, connectEnd)) { + if (connectStart <= secureConnectionStart && secureConnectionStart <= connectEnd) { details.ssl = formatTiming(startTime, secureConnectionStart, connectEnd) } } // Make sure a domain lookup occurred - if (domainLookupEnd !== fetchStart) { + if (fetchStart < domainLookupEnd) { details.dns = formatTiming(startTime, domainLookupStart, domainLookupEnd) } - if (hasRedirection(entry)) { + // Make sure a redirection occurred + if (startTime < redirectEnd) { details.redirect = formatTiming(startTime, redirectStart, redirectEnd) } @@ -135,6 +138,10 @@ export function computePerformanceResourceDetails( } export function toValidEntry(entry: RumPerformanceResourceTiming) { + if (isExperimentalFeatureEnabled(ExperimentalFeature.TOLERANT_RESOURCE_TIMINGS)) { + return entry + } + // Ensure timings are in the right order. On top of filtering out potential invalid // RumPerformanceResourceTiming, it will ignore entries from requests where timings cannot be // collected, for example cross origin requests without a "Timing-Allow-Origin" header allowing @@ -163,11 +170,12 @@ export function toValidEntry(entry: RumPerformanceResourceTiming) { function hasRedirection(entry: RumPerformanceResourceTiming) { return entry.redirectEnd > entry.startTime } - function formatTiming(origin: RelativeTime, start: RelativeTime, end: RelativeTime) { - return { - duration: toServerDuration(elapsed(start, end)), - start: toServerDuration(elapsed(origin, start)), + if (origin <= start && start <= end) { + return { + duration: toServerDuration(elapsed(start, end)), + start: toServerDuration(elapsed(origin, start)), + } } }