Skip to content

Commit

Permalink
🔥 Cleanup experimental features (#2768)
Browse files Browse the repository at this point in the history
  • Loading branch information
amortemousque authored May 22, 2024
1 parent 5cf9e60 commit 05769ac
Show file tree
Hide file tree
Showing 16 changed files with 22 additions and 273 deletions.
16 changes: 1 addition & 15 deletions packages/core/src/browser/pageExitObservable.spec.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import type { Configuration } from '../domain/configuration'
import { createNewEvent, restorePageVisibility, setPageVisibility, registerCleanupTask } from '../../test'
import { resetExperimentalFeatures, addExperimentalFeatures, ExperimentalFeature } from '../tools/experimentalFeatures'
import type { PageExitEvent } from './pageExitObservable'
import { PageExitReason, createPageExitObservable } from './pageExitObservable'

Expand All @@ -16,22 +15,9 @@ describe('createPageExitObservable', () => {

afterEach(() => {
restorePageVisibility()
resetExperimentalFeatures()
})

it('notifies when the page fires pagehide if ff pagehide is enabled', () => {
addExperimentalFeatures([ExperimentalFeature.PAGEHIDE])
onExitSpy = jasmine.createSpy()
registerCleanupTask(createPageExitObservable(configuration).subscribe(onExitSpy).unsubscribe)

window.dispatchEvent(createNewEvent('pagehide'))
window.dispatchEvent(createNewEvent('beforeunload'))

expect(onExitSpy).toHaveBeenCalledOnceWith({ reason: PageExitReason.PAGEHIDE })
})

it('notifies when the page fires beforeunload if ff pagehide is disabled', () => {
window.dispatchEvent(createNewEvent('pagehide'))
it('notifies when the page fires beforeunload', () => {
window.dispatchEvent(createNewEvent('beforeunload'))

expect(onExitSpy).toHaveBeenCalledOnceWith({ reason: PageExitReason.UNLOADING })
Expand Down
21 changes: 5 additions & 16 deletions packages/core/src/browser/pageExitObservable.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
import { isExperimentalFeatureEnabled, ExperimentalFeature } from '../tools/experimentalFeatures'
import { Observable } from '../tools/observable'
import { objectValues, includes } from '../tools/utils/polyfills'
import { noop } from '../tools/utils/functionUtils'
import type { Configuration } from '../domain/configuration'
import { addEventListeners, addEventListener, DOM_EVENT } from './addEventListener'

Expand All @@ -20,18 +18,12 @@ export interface PageExitEvent {

export function createPageExitObservable(configuration: Configuration): Observable<PageExitEvent> {
return new Observable<PageExitEvent>((observable) => {
const pagehideEnabled = isExperimentalFeatureEnabled(ExperimentalFeature.PAGEHIDE)
const { stop: stopListeners } = addEventListeners(
configuration,
window,
[DOM_EVENT.VISIBILITY_CHANGE, DOM_EVENT.FREEZE, DOM_EVENT.PAGE_HIDE],
[DOM_EVENT.VISIBILITY_CHANGE, DOM_EVENT.FREEZE],
(event) => {
if (event.type === DOM_EVENT.PAGE_HIDE && pagehideEnabled) {
/**
* Only event that detect page unload events while being compatible with the back/forward cache (bfcache)
*/
observable.notify({ reason: PageExitReason.PAGEHIDE })
} else if (event.type === DOM_EVENT.VISIBILITY_CHANGE && document.visibilityState === 'hidden') {
if (event.type === DOM_EVENT.VISIBILITY_CHANGE && document.visibilityState === 'hidden') {
/**
* Only event that guarantee to fire on mobile devices when the page transitions to background state
* (e.g. when user switches to a different application, goes to homescreen, etc), or is being unloaded.
Expand All @@ -48,12 +40,9 @@ export function createPageExitObservable(configuration: Configuration): Observab
{ capture: true }
)

let stopBeforeUnloadListener = noop
if (!pagehideEnabled) {
stopBeforeUnloadListener = addEventListener(configuration, window, DOM_EVENT.BEFORE_UNLOAD, () => {
observable.notify({ reason: PageExitReason.UNLOADING })
}).stop
}
const stopBeforeUnloadListener = addEventListener(configuration, window, DOM_EVENT.BEFORE_UNLOAD, () => {
observable.notify({ reason: PageExitReason.UNLOADING })
}).stop

return () => {
stopListeners()
Expand Down
26 changes: 0 additions & 26 deletions packages/core/src/domain/configuration/endpointBuilder.spec.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,4 @@
import type { BuildEnvWindow } from '../../../test'
import {
ExperimentalFeature,
resetExperimentalFeatures,
addExperimentalFeatures,
} from '../../tools/experimentalFeatures'
import { startsWith } from '../../tools/utils/polyfills'
import type { Payload } from '../../transport'
import type { InitConfiguration } from './configuration'
Expand All @@ -18,7 +13,6 @@ describe('endpointBuilder', () => {
beforeEach(() => {
initConfiguration = { clientToken }
;(window as unknown as BuildEnvWindow).__BUILD_ENV__SDK_VERSION__ = 'some_version'
resetExperimentalFeatures()
})

describe('query parameters', () => {
Expand Down Expand Up @@ -125,31 +119,11 @@ describe('endpointBuilder', () => {
})
).toContain('retry_count%3A5%2Cretry_after%3A408')
})

it('should contain flush reason when ff collect_flush_reason is enabled', () => {
addExperimentalFeatures([ExperimentalFeature.COLLECT_FLUSH_REASON])
expect(
createEndpointBuilder(initConfiguration, 'rum', []).build('xhr', {
...DEFAULT_PAYLOAD,
flushReason: 'bytes_limit',
})
).toContain('flush_reason%3Abytes_limit')
})

it('should not contain flush reason when ff collect_flush_reason is disabled', () => {
expect(
createEndpointBuilder(initConfiguration, 'rum', []).build('xhr', {
...DEFAULT_PAYLOAD,
flushReason: 'bytes_limit',
})
).not.toContain('flush_reason')
})
})

describe('PCI compliance intake with option', () => {
beforeEach(() => {
;(window as unknown as BuildEnvWindow).__BUILD_ENV__SDK_VERSION__ = 'some_version'
resetExperimentalFeatures()
})

it('should return PCI compliance intake endpoint if site is us1', () => {
Expand Down
6 changes: 1 addition & 5 deletions packages/core/src/domain/configuration/endpointBuilder.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import type { Payload } from '../../transport'
import { timeStampNow } from '../../tools/utils/timeUtils'
import { normalizeUrl } from '../../tools/utils/urlPolyfill'
import { ExperimentalFeature, isExperimentalFeatureEnabled } from '../../tools/experimentalFeatures'
import { generateUUID } from '../../tools/utils/stringUtils'
import type { InitConfiguration } from './configuration'
import { INTAKE_SITE_US1, INTAKE_SITE_FED_STAGING, PCI_INTAKE_HOST_US1 } from './intakeSites'
Expand Down Expand Up @@ -88,12 +87,9 @@ function buildEndpointParameters(
trackType: TrackType,
configurationTags: string[],
api: ApiType,
{ retry, flushReason, encoding }: Payload
{ retry, encoding }: Payload
) {
const tags = [`sdk_version:${__BUILD_ENV__SDK_VERSION__}`, `api:${api}`].concat(configurationTags)
if (flushReason && isExperimentalFeatureEnabled(ExperimentalFeature.COLLECT_FLUSH_REASON)) {
tags.push(`flush_reason:${flushReason}`)
}
if (retry) {
tags.push(`retry_count:${retry.count}`, `retry_after:${retry.lastFailureStatus}`)
}
Expand Down
5 changes: 0 additions & 5 deletions packages/core/src/tools/experimentalFeatures.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,6 @@
// string is an expected feature flag
// eslint-disable-next-line no-restricted-syntax
export enum ExperimentalFeature {
PAGEHIDE = 'pagehide',
RESOURCE_PAGE_STATES = 'resource_page_states',
COLLECT_FLUSH_REASON = 'collect_flush_reason',
ZERO_LCP_TELEMETRY = 'zero_lcp_telemetry',
DISABLE_REPLAY_INLINE_CSS = 'disable_replay_inline_css',
WRITABLE_RESOURCE_GRAPHQL = 'writable_resource_graphql',
CUSTOM_VITALS = 'custom_vitals',
TOLERANT_RESOURCE_TIMINGS = 'tolerant_resource_timings',
Expand Down
7 changes: 0 additions & 7 deletions packages/core/src/transport/batch.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import { display } from '../tools/display'
import type { Encoder } from '../tools/encoder'
import { createIdentityEncoder } from '../tools/encoder'
import { Batch } from './batch'
import type { FlushReason } from './flushController'
import type { HttpRequest } from './httpRequest'

describe('batch', () => {
Expand All @@ -21,7 +20,6 @@ describe('batch', () => {
sendOnExit: jasmine.Spy<HttpRequest['sendOnExit']>
}

const flushReason: FlushReason = 'bytes_limit'
let flushController: MockFlushController
let encoder: Encoder<string>

Expand All @@ -43,7 +41,6 @@ describe('batch', () => {
expect(transport.send.calls.mostRecent().args[0]).toEqual({
data: '{"message":"hello"}',
bytesCount: SMALL_MESSAGE_BYTES_COUNT,
flushReason,
encoding: undefined,
})
})
Expand Down Expand Up @@ -114,7 +111,6 @@ describe('batch', () => {
expect(transport.send.calls.mostRecent().args[0]).toEqual({
data: '{"message":"2"}\n{"message":"3"}\n{"message":"4"}',
bytesCount: jasmine.any(Number),
flushReason,
encoding: undefined,
})

Expand All @@ -126,7 +122,6 @@ describe('batch', () => {
expect(transport.send.calls.mostRecent().args[0]).toEqual({
data: '{"message":"5"}\n{"message":"6"}\n{"message":"7"}',
bytesCount: jasmine.any(Number),
flushReason,
encoding: undefined,
})

Expand All @@ -139,7 +134,6 @@ describe('batch', () => {
expect(transport.send.calls.mostRecent().args[0]).toEqual({
data: '{"message":"10"}\n{"message":"11"}',
bytesCount: jasmine.any(Number),
flushReason,
encoding: undefined,
})
})
Expand All @@ -154,7 +148,6 @@ describe('batch', () => {
expect(transport.send.calls.mostRecent().args[0]).toEqual({
data: '{"message":"1"}\n{"message":"2"}',
bytesCount: jasmine.any(Number),
flushReason,
encoding: undefined,
})
})
Expand Down
8 changes: 3 additions & 5 deletions packages/core/src/transport/batch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ export class Batch {

// Send encoded messages
if (encoderResult.outputBytesCount) {
send(formatPayloadFromEncoder(encoderResult, event))
send(formatPayloadFromEncoder(encoderResult))
}

// Send messages that are not yet encoded at this point
Expand All @@ -62,15 +62,14 @@ export class Batch {
send({
data: pendingMessages,
bytesCount: computeBytesCount(pendingMessages),
flushReason: event.reason,
})
}
} else {
if (upsertMessages) {
this.encoder.write(this.encoder.isEmpty ? upsertMessages : `\n${upsertMessages}`)
}
this.encoder.finish((encoderResult) => {
send(formatPayloadFromEncoder(encoderResult, event))
send(formatPayloadFromEncoder(encoderResult))
})
}
}
Expand Down Expand Up @@ -122,7 +121,7 @@ export class Batch {
}
}

function formatPayloadFromEncoder(encoderResult: EncoderResult, flushEvent: FlushEvent): Payload {
function formatPayloadFromEncoder(encoderResult: EncoderResult): Payload {
let data: string | Blob
if (typeof encoderResult.output === 'string') {
data = encoderResult.output
Expand All @@ -142,6 +141,5 @@ function formatPayloadFromEncoder(encoderResult: EncoderResult, flushEvent: Flus
data,
bytesCount: encoderResult.outputBytesCount,
encoding: encoderResult.encoding,
flushReason: flushEvent.reason,
}
}
2 changes: 0 additions & 2 deletions packages/core/src/transport/httpRequest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import { monitor } from '../tools/monitor'
import { addEventListener } from '../browser/addEventListener'
import type { RawError } from '../domain/error/error.types'
import { newRetryState, sendWithRetryStrategy } from './sendWithRetryStrategy'
import type { FlushReason } from './flushController'

/**
* Use POST request without content type to:
Expand All @@ -27,7 +26,6 @@ export interface Payload {
data: string | FormData | Blob
bytesCount: number
retry?: RetryInfo
flushReason?: FlushReason
encoding?: 'deflate'
}

Expand Down
55 changes: 1 addition & 54 deletions packages/rum-core/src/domain/resource/resourceCollection.spec.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,5 @@
import type { Duration, RelativeTime, ServerDuration, TimeStamp } from '@datadog/browser-core'
import {
resetExperimentalFeatures,
addExperimentalFeatures,
isIE,
RequestType,
ResourceType,
ExperimentalFeature,
} from '@datadog/browser-core'
import { isIE, RequestType, ResourceType } from '@datadog/browser-core'
import type { RumFetchResourceEventDomainContext } from '../../domainContext.types'
import { setup, createRumSessionManagerMock, createPerformanceEntry } from '../../../test'
import type { TestSetupBuilder } from '../../../test'
Expand All @@ -16,29 +9,22 @@ import { LifeCycleEventType } from '../lifeCycle'
import type { RequestCompleteEvent } from '../requestCollection'
import { TraceIdentifier } from '../tracing/tracer'
import { validateAndBuildRumConfiguration } from '../configuration'
import { PageState } from '../contexts/pageStateHistory'
import { RumPerformanceEntryType } from '../../browser/performanceCollection'
import { startResourceCollection } from './resourceCollection'

describe('resourceCollection', () => {
let setupBuilder: TestSetupBuilder
let trackResources: boolean
let findAllSpy: jasmine.Spy<jasmine.Func>
let wasInPageStateDuringPeriodSpy: jasmine.Spy<jasmine.Func>

beforeEach(() => {
trackResources = true
setupBuilder = setup().beforeBuild(({ lifeCycle, sessionManager, pageStateHistory, configuration }) => {
findAllSpy = spyOn(pageStateHistory, 'findAll')
wasInPageStateDuringPeriodSpy = spyOn(pageStateHistory, 'wasInPageStateDuringPeriod')
startResourceCollection(lifeCycle, { ...configuration, trackResources }, sessionManager, pageStateHistory)
})
})

afterEach(() => {
resetExperimentalFeatures()
})

it('should create resource from performance entry', () => {
const { lifeCycle, rawRumEvents } = setupBuilder.build()

Expand Down Expand Up @@ -196,27 +182,6 @@ describe('resourceCollection', () => {
})
})

it('should collect page states on resources when ff resource_page_states enabled', () => {
addExperimentalFeatures([ExperimentalFeature.RESOURCE_PAGE_STATES])
const { lifeCycle, rawRumEvents } = setupBuilder.build()
const mockPageStates = [{ state: PageState.ACTIVE, startTime: 0 as RelativeTime }]
const mockXHR = createCompletedRequest()
const mockPerformanceEntry = createPerformanceEntry(RumPerformanceEntryType.RESOURCE)

findAllSpy.and.returnValue(mockPageStates)

lifeCycle.notify(LifeCycleEventType.REQUEST_COMPLETED, mockXHR)
lifeCycle.notify(LifeCycleEventType.PERFORMANCE_ENTRIES_COLLECTED, [mockPerformanceEntry])

const rawRumResourceEventFetch = rawRumEvents[0].rawRumEvent as RawRumResourceEvent
const rawRumResourceEventEntry = rawRumEvents[1].rawRumEvent as RawRumResourceEvent

expect(findAllSpy.calls.first().args).toEqual([mockXHR.startClocks.relative, mockXHR.duration])
expect(findAllSpy.calls.mostRecent().args).toEqual([mockPerformanceEntry.startTime, mockPerformanceEntry.duration])
expect(rawRumResourceEventFetch._dd.page_states).toEqual(jasmine.objectContaining(mockPageStates))
expect(rawRumResourceEventEntry._dd.page_states).toEqual(jasmine.objectContaining(mockPageStates))
})

it('should not have a duration if a frozen state happens during the request and no performance entry matches', () => {
const { lifeCycle, rawRumEvents } = setupBuilder.build()
const mockXHR = createCompletedRequest()
Expand All @@ -229,24 +194,6 @@ describe('resourceCollection', () => {
expect(rawRumResourceEventFetch.resource.duration).toBeUndefined()
})

it('should not collect page states on resources when ff resource_page_states disabled', () => {
const { lifeCycle, rawRumEvents } = setupBuilder.build()
const mockPageStates = [{ state: PageState.ACTIVE, startTime: 0 as RelativeTime }]
const mockXHR = createCompletedRequest()
const mockPerformanceEntry = createPerformanceEntry(RumPerformanceEntryType.RESOURCE)

findAllSpy.and.returnValue(mockPageStates)

lifeCycle.notify(LifeCycleEventType.REQUEST_COMPLETED, mockXHR)
lifeCycle.notify(LifeCycleEventType.PERFORMANCE_ENTRIES_COLLECTED, [mockPerformanceEntry])

const rawRumResourceEventFetch = rawRumEvents[0].rawRumEvent as RawRumResourceEvent
const rawRumResourceEventEntry = rawRumEvents[1].rawRumEvent as RawRumResourceEvent

expect(rawRumResourceEventFetch._dd.page_states).not.toBeDefined()
expect(rawRumResourceEventEntry._dd.page_states).not.toBeDefined()
})

it('should create resource from completed fetch request', () => {
if (isIE()) {
pending('No IE support')
Expand Down
Loading

0 comments on commit 05769ac

Please sign in to comment.