Skip to content

Commit

Permalink
♻️ [RUMF-1529] use an enum for experimental features
Browse files Browse the repository at this point in the history
  • Loading branch information
BenoitZugmeyer committed Mar 28, 2023
1 parent 28d26bb commit 22b1acf
Show file tree
Hide file tree
Showing 28 changed files with 153 additions and 83 deletions.
4 changes: 2 additions & 2 deletions packages/core/src/browser/pageExitObservable.spec.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { createNewEvent, restorePageVisibility, setPageVisibility } from '../../test/specHelper'
import { resetExperimentalFeatures, updateExperimentalFeatures } from '../domain/configuration'
import { resetExperimentalFeatures, updateExperimentalFeatures, ExperimentalFeature } from '../domain/configuration'
import type { Subscription } from '../tools/observable'
import type { PageExitEvent } from './pageExitObservable'
import { PageExitReason, createPageExitObservable } from './pageExitObservable'
Expand All @@ -20,7 +20,7 @@ describe('createPageExitObservable', () => {
})

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

Expand Down
4 changes: 2 additions & 2 deletions packages/core/src/browser/pageExitObservable.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { isExperimentalFeatureEnabled } from '../domain/configuration'
import { isExperimentalFeatureEnabled, ExperimentalFeature } from '../domain/configuration'
import { Observable } from '../tools/observable'
import { includes, noop, objectValues } from '../tools/utils'
import { addEventListeners, addEventListener, DOM_EVENT } from './addEventListener'
Expand All @@ -18,7 +18,7 @@ export interface PageExitEvent {

export function createPageExitObservable(): Observable<PageExitEvent> {
const observable = new Observable<PageExitEvent>(() => {
const pagehideEnabled = isExperimentalFeatureEnabled('pagehide')
const pagehideEnabled = isExperimentalFeatureEnabled(ExperimentalFeature.PAGEHIDE)
const { stop: stopListeners } = addEventListeners(
window,
[DOM_EVENT.VISIBILITY_CHANGE, DOM_EVENT.FREEZE, DOM_EVENT.PAGE_HIDE],
Expand Down
20 changes: 16 additions & 4 deletions packages/core/src/domain/configuration/configuration.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import type { RumEvent } from '../../../../rum-core/src'
import { display } from '../../tools/display'
import type { InitConfiguration } from './configuration'
import { validateAndBuildConfiguration } from './configuration'
import { isExperimentalFeatureEnabled, updateExperimentalFeatures } from './experimentalFeatures'
import { ExperimentalFeature, isExperimentalFeatureEnabled, updateExperimentalFeatures } from './experimentalFeatures'

describe('validateAndBuildConfiguration', () => {
const clientToken = 'some_client_token'
Expand All @@ -11,9 +11,21 @@ describe('validateAndBuildConfiguration', () => {
updateExperimentalFeatures([])
})

it('updates experimental feature flags', () => {
validateAndBuildConfiguration({ clientToken, enableExperimentalFeatures: ['foo'] })
expect(isExperimentalFeatureEnabled('foo')).toBeTrue()
describe('experimentalFeatures', () => {
const TEST_FEATURE_FLAG = 'foo' as ExperimentalFeature

beforeEach(() => {
;(ExperimentalFeature as any).FOO = TEST_FEATURE_FLAG
})

afterEach(() => {
delete (ExperimentalFeature as any).FOO
})

it('updates experimental feature flags', () => {
validateAndBuildConfiguration({ clientToken, enableExperimentalFeatures: ['foo'] })
expect(isExperimentalFeatureEnabled(TEST_FEATURE_FLAG)).toBeTrue()
})
})

describe('validate init configuration', () => {
Expand Down
12 changes: 9 additions & 3 deletions packages/core/src/domain/configuration/configuration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@ import type { CookieOptions } from '../../browser/cookie'
import { getCurrentSite } from '../../browser/cookie'
import { catchUserErrors } from '../../tools/catchUserErrors'
import { display } from '../../tools/display'
import { assign, isPercentage, ONE_KIBI_BYTE, ONE_SECOND } from '../../tools/utils'
import { assign, isPercentage, objectHasValue, ONE_KIBI_BYTE, ONE_SECOND } from '../../tools/utils'
import type { RawTelemetryConfiguration } from '../telemetry'
import { updateExperimentalFeatures } from './experimentalFeatures'
import { ExperimentalFeature, updateExperimentalFeatures } from './experimentalFeatures'
import type { TransportConfiguration } from './transportConfiguration'
import { computeTransportConfiguration } from './transportConfiguration'

Expand Down Expand Up @@ -112,7 +112,13 @@ export function validateAndBuildConfiguration(initConfiguration: InitConfigurati
}

// Set the experimental feature flags as early as possible, so we can use them in most places
updateExperimentalFeatures(initConfiguration.enableExperimentalFeatures)
if (Array.isArray(initConfiguration.enableExperimentalFeatures)) {
updateExperimentalFeatures(
initConfiguration.enableExperimentalFeatures.filter((flag): flag is ExperimentalFeature =>
objectHasValue(ExperimentalFeature, flag)
)
)
}

return assign(
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import type { BuildEnvWindow } from '../../../test/specHelper'
import { startsWith } from '../../tools/utils'
import type { InitConfiguration } from './configuration'
import { createEndpointBuilder } from './endpointBuilder'
import { resetExperimentalFeatures, updateExperimentalFeatures } from './experimentalFeatures'
import { ExperimentalFeature, resetExperimentalFeatures, updateExperimentalFeatures } from './experimentalFeatures'

describe('endpointBuilder', () => {
const clientToken = 'some_client_token'
Expand Down Expand Up @@ -126,7 +126,7 @@ describe('endpointBuilder', () => {
})

it('should contain flush reason when ff collect_flush_reason is enabled', () => {
updateExperimentalFeatures(['collect_flush_reason'])
updateExperimentalFeatures([ExperimentalFeature.COLLECT_FLUSH_REASON])
expect(createEndpointBuilder(initConfiguration, 'rum', []).build('xhr', 'batch_bytes_limit')).toContain(
'flush_reason%3Abatch_bytes_limit'
)
Expand Down
4 changes: 2 additions & 2 deletions packages/core/src/domain/configuration/endpointBuilder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { normalizeUrl } from '../../tools/urlPolyfill'
import { generateUUID } from '../../tools/utils'
import type { InitConfiguration } from './configuration'
import { INTAKE_SITE_AP1, INTAKE_SITE_US1 } from './intakeSites'
import { isExperimentalFeatureEnabled } from './experimentalFeatures'
import { ExperimentalFeature, isExperimentalFeatureEnabled } from './experimentalFeatures'

// replaced at build time
declare const __BUILD_ENV__SDK_VERSION__: string
Expand Down Expand Up @@ -104,7 +104,7 @@ function buildEndpointParameters(
retry: RetryInfo | undefined
) {
const tags = [`sdk_version:${__BUILD_ENV__SDK_VERSION__}`, `api:${api}`].concat(configurationTags)
if (flushReason && isExperimentalFeatureEnabled('collect_flush_reason')) {
if (flushReason && isExperimentalFeatureEnabled(ExperimentalFeature.COLLECT_FLUSH_REASON)) {
tags.push(`flush_reason:${flushReason}`)
}
if (retry) {
Expand Down
26 changes: 15 additions & 11 deletions packages/core/src/domain/configuration/experimentalFeatures.spec.ts
Original file line number Diff line number Diff line change
@@ -1,39 +1,43 @@
import type { ExperimentalFeature } from './experimentalFeatures'
import {
updateExperimentalFeatures,
isExperimentalFeatureEnabled,
resetExperimentalFeatures,
} from './experimentalFeatures'

const TEST_FEATURE_FLAG_ONE = 'foo' as ExperimentalFeature
const TEST_FEATURE_FLAG_TWO = 'bar' as ExperimentalFeature

describe('experimentalFeatures', () => {
afterEach(() => {
resetExperimentalFeatures()
})

it('initial state is empty', () => {
expect(isExperimentalFeatureEnabled('foo')).toBeFalse()
expect(isExperimentalFeatureEnabled('bar')).toBeFalse()
expect(isExperimentalFeatureEnabled(TEST_FEATURE_FLAG_ONE)).toBeFalse()
expect(isExperimentalFeatureEnabled(TEST_FEATURE_FLAG_TWO)).toBeFalse()
})

it('should define enabled experimental features', () => {
updateExperimentalFeatures(['foo'])
expect(isExperimentalFeatureEnabled('foo')).toBeTrue()
expect(isExperimentalFeatureEnabled('bar')).toBeFalse()
updateExperimentalFeatures([TEST_FEATURE_FLAG_ONE])
expect(isExperimentalFeatureEnabled(TEST_FEATURE_FLAG_ONE)).toBeTrue()
expect(isExperimentalFeatureEnabled(TEST_FEATURE_FLAG_TWO)).toBeFalse()
})

it('should allow to be shared between products', () => {
updateExperimentalFeatures(['foo'])
updateExperimentalFeatures(['bar'])
updateExperimentalFeatures([TEST_FEATURE_FLAG_ONE])
updateExperimentalFeatures([TEST_FEATURE_FLAG_TWO])

expect(isExperimentalFeatureEnabled('foo')).toBeTrue()
expect(isExperimentalFeatureEnabled('bar')).toBeTrue()
expect(isExperimentalFeatureEnabled(TEST_FEATURE_FLAG_ONE)).toBeTrue()
expect(isExperimentalFeatureEnabled(TEST_FEATURE_FLAG_TWO)).toBeTrue()
})

it('should support some edge cases', () => {
updateExperimentalFeatures(['foo'])
updateExperimentalFeatures([TEST_FEATURE_FLAG_ONE])
updateExperimentalFeatures(undefined)
updateExperimentalFeatures([])
updateExperimentalFeatures([11 as any])

expect(isExperimentalFeatureEnabled('foo')).toBeTrue()
expect(isExperimentalFeatureEnabled(TEST_FEATURE_FLAG_ONE)).toBeTrue()
})
})
35 changes: 22 additions & 13 deletions packages/core/src/domain/configuration/experimentalFeatures.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,27 @@
* For NPM setup, this feature flag singleton is shared between RUM and Logs product.
* This means that an experimental flag set on the RUM product will be set on the Logs product.
* So keep in mind that in certain configurations, your experimental feature flag may affect other products.
*
* FORMAT:
* All feature flags should be snake_cased
*/
import { includes } from '../../tools/utils'
import { display } from '../../tools/display'
// We want to use a real enum (i.e. not a const enum) here, to be able to check whether an arbitrary
// string is an expected feature flag
// eslint-disable-next-line no-restricted-syntax
export enum ExperimentalFeature {
PAGEHIDE = 'pagehide',
SHADOW_DOM_DEBUG = 'shadow_dom_debug',
FEATURE_FLAGS = 'feature_flags',
RESOURCE_DURATIONS = 'resource_durations',
RESOURCE_PAGE_STATES = 'resource_page_states',
CLICKMAP = 'clickmap',
COLLECT_FLUSH_REASON = 'collect_flush_reason',
SANITIZE_INPUTS = 'sanitize_inputs',
}

let enabledExperimentalFeatures: Set<string> | undefined
let enabledExperimentalFeatures: Set<ExperimentalFeature> | undefined

export function updateExperimentalFeatures(enabledFeatures: string[] | undefined): void {
export function updateExperimentalFeatures(enabledFeatures: ExperimentalFeature[] | undefined): void {
// Safely handle external data
if (!Array.isArray(enabledFeatures)) {
return
Expand All @@ -19,17 +33,12 @@ export function updateExperimentalFeatures(enabledFeatures: string[] | undefined
enabledExperimentalFeatures = new Set(enabledFeatures)
}

enabledFeatures
.filter((flag) => typeof flag === 'string')
.forEach((flag: string) => {
if (includes(flag, '-')) {
display.warn(`please use snake case for '${flag}'`)
}
enabledExperimentalFeatures!.add(flag)
})
enabledFeatures.forEach((flag) => {
enabledExperimentalFeatures!.add(flag)
})
}

export function isExperimentalFeatureEnabled(featureName: string): boolean {
export function isExperimentalFeatureEnabled(featureName: ExperimentalFeature): boolean {
return !!enabledExperimentalFeatures && enabledExperimentalFeatures.has(featureName)
}

Expand Down
1 change: 1 addition & 0 deletions packages/core/src/domain/configuration/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,5 +12,6 @@ export {
updateExperimentalFeatures,
resetExperimentalFeatures,
getExperimentalFeatures,
ExperimentalFeature,
} from './experimentalFeatures'
export * from './intakeSites'
10 changes: 7 additions & 3 deletions packages/core/src/domain/console/consoleObservable.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { find, jsonStringify } from '../../tools/utils'
import { ConsoleApiName } from '../../tools/display'
import { callMonitored } from '../../tools/monitor'
import { sanitize } from '../../tools/sanitize'
import { isExperimentalFeatureEnabled } from '../configuration'
import { ExperimentalFeature, isExperimentalFeatureEnabled } from '../configuration'

export interface ConsoleLog {
message: string
Expand Down Expand Up @@ -70,10 +70,14 @@ function buildConsoleLog(params: unknown[], api: ConsoleApiName, handlingStack:

function formatConsoleParameters(param: unknown) {
if (typeof param === 'string') {
return isExperimentalFeatureEnabled('sanitize_inputs') ? sanitize(param) : param
return isExperimentalFeatureEnabled(ExperimentalFeature.SANITIZE_INPUTS) ? sanitize(param) : param
}
if (param instanceof Error) {
return formatErrorMessage(computeStackTrace(param))
}
return jsonStringify(isExperimentalFeatureEnabled('sanitize_inputs') ? sanitize(param) : param, undefined, 2)
return jsonStringify(
isExperimentalFeatureEnabled(ExperimentalFeature.SANITIZE_INPUTS) ? sanitize(param) : param,
undefined,
2
)
}
4 changes: 2 additions & 2 deletions packages/core/src/domain/telemetry/telemetry.spec.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import type { StackTrace } from '@datadog/browser-core'
import { callMonitored } from '../../tools/monitor'
import type { Configuration } from '../configuration'
import type { Configuration, ExperimentalFeature } from '../configuration'
import {
resetExperimentalFeatures,
updateExperimentalFeatures,
Expand Down Expand Up @@ -74,7 +74,7 @@ describe('telemetry', () => {
})

it('should contains feature flags', () => {
updateExperimentalFeatures(['foo'])
updateExperimentalFeatures(['foo' as ExperimentalFeature])
const { notifySpy } = startAndSpyTelemetry()
callMonitored(() => {
throw new Error('message')
Expand Down
1 change: 1 addition & 0 deletions packages/core/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ export {
DefaultPrivacyLevel,
EndpointBuilder,
isExperimentalFeatureEnabled,
ExperimentalFeature,
updateExperimentalFeatures,
resetExperimentalFeatures,
serializeConfiguration,
Expand Down
10 changes: 7 additions & 3 deletions packages/core/src/tools/contextManager.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { isExperimentalFeatureEnabled } from '../domain/configuration'
import { ExperimentalFeature, isExperimentalFeatureEnabled } from '../domain/configuration'
import { computeBytesCount, deepClone, jsonStringify } from './utils'
import type { Context, ContextValue } from './context'
import { sanitize } from './sanitize'
Expand Down Expand Up @@ -40,12 +40,16 @@ export function createContextManager(computeBytesCountImpl = computeBytesCount)
getContext: () => deepClone(context),

setContext: (newContext: Context) => {
context = isExperimentalFeatureEnabled('sanitize_inputs') ? sanitize(newContext) : deepClone(newContext)
context = isExperimentalFeatureEnabled(ExperimentalFeature.SANITIZE_INPUTS)
? sanitize(newContext)
: deepClone(newContext)
bytesCountCache = undefined
},

setContextProperty: (key: string, property: any) => {
context[key] = isExperimentalFeatureEnabled('sanitize_inputs') ? sanitize(property) : deepClone(property)
context[key] = isExperimentalFeatureEnabled(ExperimentalFeature.SANITIZE_INPUTS)
? sanitize(property)
: deepClone(property)
bytesCountCache = undefined
},

Expand Down
6 changes: 4 additions & 2 deletions packages/core/src/tools/error.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { isExperimentalFeatureEnabled } from '../domain/configuration'
import { ExperimentalFeature, isExperimentalFeatureEnabled } from '../domain/configuration'
import type { StackTrace } from '../domain/tracekit'
import { computeStackTrace } from '../domain/tracekit'
import { callMonitored } from './monitor'
Expand Down Expand Up @@ -70,7 +70,9 @@ export function computeRawError({
handling,
}: RawErrorParams): RawError {
if (!stackTrace || (stackTrace.message === undefined && !(originalError instanceof Error))) {
const sanitizedError = isExperimentalFeatureEnabled('sanitize_inputs') ? sanitize(originalError) : originalError
const sanitizedError = isExperimentalFeatureEnabled(ExperimentalFeature.SANITIZE_INPUTS)
? sanitize(originalError)
: originalError
return {
startClocks,
source,
Expand Down
4 changes: 2 additions & 2 deletions packages/core/src/tools/limitModification.spec.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { resetExperimentalFeatures, updateExperimentalFeatures } from '../domain/configuration'
import { ExperimentalFeature, resetExperimentalFeatures, updateExperimentalFeatures } from '../domain/configuration'
import type { Context } from './context'
import { limitModification } from './limitModification'

Expand Down Expand Up @@ -158,7 +158,7 @@ describe('limitModification', () => {
})

it('should call sanitize on newly provided values', () => {
updateExperimentalFeatures(['sanitize_inputs'])
updateExperimentalFeatures([ExperimentalFeature.SANITIZE_INPUTS])
const object: Context = { bar: { baz: 42 } }

const modifier = (candidate: any) => {
Expand Down
8 changes: 6 additions & 2 deletions packages/core/src/tools/limitModification.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { isExperimentalFeatureEnabled } from '../domain/configuration'
import { ExperimentalFeature, isExperimentalFeatureEnabled } from '../domain/configuration'
import type { Context } from './context'
import { sanitize } from './sanitize'
import { deepClone, getType } from './utils'
Expand All @@ -20,7 +20,11 @@ export function limitModification<T extends Context, Result>(
const originalType = getType(originalValue)
const newType = getType(newValue)
if (newType === originalType) {
set(object, path, isExperimentalFeatureEnabled('sanitize_inputs') ? sanitize(newValue) : newValue)
set(
object,
path,
isExperimentalFeatureEnabled(ExperimentalFeature.SANITIZE_INPUTS) ? sanitize(newValue) : newValue
)
} else if (originalType === 'object' && (newType === 'undefined' || newType === 'null')) {
set(object, path, {})
}
Expand Down
7 changes: 5 additions & 2 deletions packages/logs/src/boot/logsPublicApi.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import type { Context, InitConfiguration, User } from '@datadog/browser-core'
import {
ExperimentalFeature,
isExperimentalFeatureEnabled,
assign,
BoundedBuffer,
Expand Down Expand Up @@ -120,10 +121,12 @@ export function makeLogsPublicApi(startLogsImpl: StartLogs) {
createLogger: monitor((name: string, conf: LoggerConfiguration = {}) => {
customLoggers[name] = new Logger(
(...params) => handleLogStrategy(...params),
isExperimentalFeatureEnabled('sanitize_inputs') ? sanitize(name) : name,
isExperimentalFeatureEnabled(ExperimentalFeature.SANITIZE_INPUTS) ? sanitize(name) : name,
conf.handler,
conf.level,
isExperimentalFeatureEnabled('sanitize_inputs') ? (sanitize(conf.context) as object) : conf.context
isExperimentalFeatureEnabled(ExperimentalFeature.SANITIZE_INPUTS)
? (sanitize(conf.context) as object)
: conf.context
)

return customLoggers[name]!
Expand Down
Loading

0 comments on commit 22b1acf

Please sign in to comment.