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

🔥 Cleanup experimental features #2768

Merged
merged 5 commits into from
May 22, 2024
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
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
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