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.

Sign up for GitHub

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-1097] revamp internal configuration - core #1216

Merged
merged 3 commits into from
Dec 14, 2021
Merged

♻ [RUMF-1097] revamp internal configuration - core #1216

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f11e098
♻ [RUMF-1097][core] expose a new function to validate and build core …
BenoitZugmeyer Dec 8, 2021
85fd72f
♻️ [RUMF-1097] inline "commonInit" in logs and rum init
BenoitZugmeyer Dec 8, 2021
e86a9ef
Merge branch 'main' into benoit/revamp-configuration-core
BenoitZugmeyer Dec 14, 2021
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
15 changes: 1 addition & 14 deletions packages/core/src/boot/init.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
import { buildConfiguration, InitConfiguration } from '../domain/configuration'
import { setDebugMode, startInternalMonitoring } from '../domain/internalMonitoring'
import { setDebugMode } from '../domain/internalMonitoring'
import { catchUserErrors } from '../tools/catchUserErrors'
import { updateExperimentalFeatures } from '../domain/configuration/experimentalFeatures'

export function makePublicApi<T>(stub: T): T & { onReady(callback: () => void): void } {
const publicApi = {
Expand Down Expand Up @@ -46,14 +44,3 @@ export interface BuildEnv {
buildMode: BuildMode
sdkVersion: string
}

export function commonInit(initConfiguration: InitConfiguration, buildEnv: BuildEnv) {
updateExperimentalFeatures(initConfiguration.enableExperimentalFeatures)
const configuration = buildConfiguration(initConfiguration, buildEnv)
const internalMonitoring = startInternalMonitoring(configuration)

return {
configuration,
internalMonitoring,
}
}
101 changes: 99 additions & 2 deletions packages/core/src/domain/configuration/configuration.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
import { RumEvent } from '../../../../rum-core/src/rumEvent.types'
import { BuildEnv, BuildMode } from '../../boot/init'
import { display } from '../../tools/display'
import { buildConfiguration } from './configuration'
import { buildConfiguration, InitConfiguration, validateAndBuildConfiguration } from './configuration'
import { isExperimentalFeatureEnabled, updateExperimentalFeatures } from './experimentalFeatures'

describe('configuration', () => {
describe('buildConfiguration', () => {
const clientToken = 'some_client_token'
const buildEnv: BuildEnv = {
buildMode: BuildMode.RELEASE,
Expand Down Expand Up @@ -61,3 +62,99 @@ describe('configuration', () => {
})
})
})

describe('validateAndBuildConfiguration', () => {
const clientToken = 'some_client_token'
const buildEnv: BuildEnv = {
buildMode: BuildMode.RELEASE,
sdkVersion: 'some_version',
}

beforeEach(() => {
updateExperimentalFeatures([])
})

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

describe('validate init configuration', () => {
let displaySpy: jasmine.Spy<typeof display.error>

beforeEach(() => {
displaySpy = spyOn(display, 'error')
})

it('requires the InitConfiguration to be defined', () => {
expect(validateAndBuildConfiguration((undefined as unknown) as InitConfiguration, buildEnv)).toBeUndefined()
expect(displaySpy).toHaveBeenCalledOnceWith('Client Token is not configured, we will not send any data.')
})

it('requires clientToken to be defined', () => {
expect(validateAndBuildConfiguration(({} as unknown) as InitConfiguration, buildEnv)).toBeUndefined()
expect(displaySpy).toHaveBeenCalledOnceWith('Client Token is not configured, we will not send any data.')
})

it('requires sampleRate to be a percentage', () => {
expect(
validateAndBuildConfiguration(({ clientToken, sampleRate: 'foo' } as unknown) as InitConfiguration, buildEnv)
).toBeUndefined()
expect(displaySpy).toHaveBeenCalledOnceWith('Sample Rate should be a number between 0 and 100')
})
})

describe('cookie options', () => {
it('should not be secure nor crossSite by default', () => {
const configuration = validateAndBuildConfiguration({ clientToken }, buildEnv)!
expect(configuration.cookieOptions).toEqual({ secure: false, crossSite: false })
})

it('should be secure when `useSecureSessionCookie` is truthy', () => {
const configuration = validateAndBuildConfiguration({ clientToken, useSecureSessionCookie: true }, buildEnv)!
expect(configuration.cookieOptions).toEqual({ secure: true, crossSite: false })
})

it('should be secure and crossSite when `useCrossSiteSessionCookie` is truthy', () => {
const configuration = validateAndBuildConfiguration({ clientToken, useCrossSiteSessionCookie: true }, buildEnv)!
expect(configuration.cookieOptions).toEqual({ secure: true, crossSite: true })
})

it('should have domain when `trackSessionAcrossSubdomains` is truthy', () => {
const configuration = validateAndBuildConfiguration(
{ clientToken, trackSessionAcrossSubdomains: true },
buildEnv
)!
expect(configuration.cookieOptions).toEqual({ secure: false, crossSite: false, domain: jasmine.any(String) })
})
})

describe('beforeSend', () => {
it('should be undefined when beforeSend is missing on user configuration', () => {
const configuration = validateAndBuildConfiguration({ clientToken }, buildEnv)!
expect(configuration.beforeSend).toBeUndefined()
})

it('should return the same result as the original', () => {
const beforeSend = (event: RumEvent) => {
if (event.view.url === '/foo') {
return false
}
}
const configuration = validateAndBuildConfiguration({ clientToken, beforeSend }, buildEnv)!
expect(configuration.beforeSend!({ view: { url: '/foo' } }, {})).toBeFalse()
expect(configuration.beforeSend!({ view: { url: '/bar' } }, {})).toBeUndefined()
})

it('should catch errors and log them', () => {
const myError = 'Ooops!'
const beforeSend = () => {
throw myError
}
const configuration = validateAndBuildConfiguration({ clientToken, beforeSend }, buildEnv)!
const displaySpy = spyOn(display, 'error')
expect(configuration.beforeSend!(null, {})).toBeUndefined()
expect(displaySpy).toHaveBeenCalledWith('beforeSend threw an error:', myError)
})
})
})
40 changes: 37 additions & 3 deletions packages/core/src/domain/configuration/configuration.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import { BuildEnv } from '../../boot/init'
import { CookieOptions, getCurrentSite } from '../../browser/cookie'
import { catchUserErrors } from '../../tools/catchUserErrors'
import { objectHasValue, ONE_KILO_BYTE, ONE_SECOND } from '../../tools/utils'
import { display } from '../../tools/display'
import { isPercentage, objectHasValue, ONE_KILO_BYTE, ONE_SECOND } from '../../tools/utils'
import { updateExperimentalFeatures } from './experimentalFeatures'
import { computeTransportConfiguration, TransportConfiguration } from './transportConfiguration'

export const DefaultPrivacyLevel = {
Expand Down Expand Up @@ -95,12 +97,44 @@ export type Configuration = typeof DEFAULT_CONFIGURATION &
TransportConfiguration & {
cookieOptions: CookieOptions

service?: string
beforeSend?: BeforeSendCallback
service: string | undefined
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using service: string | undefined instead of service?: string to ensure that the service property is set ( slightly stricter typings).

Copy link
Contributor

@bcaudan bcaudan Dec 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it allow or prevent something compared to previous state?
Should we do the same for actionNameAttribute?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it change something?

Yes, it ensures that the property is set. For example:

type Configuration = { foo?: string }

let configuration: Configuration = {} // no error. We may have forgot to set `foo` from `initConfiguration`


type Configuration = { foo: string | undefined }

let configuration: Configuration = {} // error
let configuration: Configuration = { foo: initConfiguration.foo } // ok

Should we do the same for actionNameAttribute?

This will be changed in the RUM PR. This is a bit weird to change it now, since we don't want to set it in the core validateAndBuildConfiguration function.

Copy link
Contributor

@bcaudan bcaudan Dec 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I think I get it, the idea is to be sure that for the internal configuration object, we set all the needed fields even if they are not set in the customer facing configuration.

beforeSend: BeforeSendCallback | undefined

actionNameAttribute?: string
}

export function validateAndBuildConfiguration(
initConfiguration: InitConfiguration,
buildEnv: BuildEnv
): Configuration | undefined {
if (!initConfiguration || !initConfiguration.clientToken) {
display.error('Client Token is not configured, we will not send any data.')
return
}

// Set the experimental feature flags as early as possible so we can use them in most places
updateExperimentalFeatures(initConfiguration.enableExperimentalFeatures)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to set experimental feature flags as early as possible. This is the earliest I can do, since we are checking if initConfiguration isn't falsy just above. This deviates a bit from the purpose of the function though, let me know what you think.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good to me


const configuration: Configuration = {
beforeSend:
initConfiguration.beforeSend && catchUserErrors(initConfiguration.beforeSend, 'beforeSend threw an error:'),
cookieOptions: buildCookieOptions(initConfiguration),
service: initConfiguration.service,
...computeTransportConfiguration(initConfiguration, buildEnv),
...DEFAULT_CONFIGURATION,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following PRs will follow the same strategy to define default configuration, let me know what you think!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we trim the default configuration used here as well?
or is it planned for later?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is planned for later, when I move RUM-only options to the RUM package.

}

if (initConfiguration.sampleRate !== undefined) {
if (!isPercentage(initConfiguration.sampleRate)) {
display.error('Sample Rate should be a number between 0 and 100')
return
}
configuration.sampleRate = initConfiguration.sampleRate
}
Comment on lines +127 to +133
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following PRs will follow the same strategy to validate and set the configuration, let me know what you think!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sound good!
Maybe we could move the validation, next to the clientToken validation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to collocate validation and definition, to avoid doing initConfiguration.sampleRate !== undefined twice. I tried to move this block above, but this prevent to use DEFAULT_CONFIGURATION as we do currently, resulting on something a bit more verbose:

  let sampleRate
  if (initConfiguration.sampleRate !== undefined) {
    if (!isPercentage(initConfiguration.sampleRate)) {
      display.error('Sample Rate should be a number between 0 and 100')
      return
    }
    sampleRate = initConfiguration.sampleRate
  } else {
    sampleRate = DEFAULT_SAMPLE_RATE
  }

  let configuration: Configuration = {
    cookieOptions: buildCookieOptions(initConfiguration),
    service: initConfiguration.service,
    ...computeTransportConfiguration(initConfiguration, buildEnv),
    sampleRate,
  }


return configuration
}

export function buildConfiguration(initConfiguration: InitConfiguration, buildEnv: BuildEnv): Configuration {
const configuration: Configuration = {
beforeSend:
Expand Down
1 change: 1 addition & 0 deletions packages/core/src/domain/configuration/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,5 @@ export {
BeforeSendCallback,
DefaultPrivacyLevel,
buildConfiguration,
validateAndBuildConfiguration,
} from './configuration'
5 changes: 4 additions & 1 deletion packages/core/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@ export {
DEFAULT_CONFIGURATION,
Configuration,
InitConfiguration,
buildConfiguration,
buildCookieOptions,
validateAndBuildConfiguration,
BeforeSendCallback,
DefaultPrivacyLevel,
} from './domain/configuration'
Expand All @@ -14,8 +16,9 @@ export {
export { trackConsoleError } from './domain/error/trackConsoleError'
export { trackRuntimeError } from './domain/error/trackRuntimeError'
export { computeStackTrace, StackTrace } from './domain/tracekit'
export { BuildEnv, BuildMode, defineGlobal, makePublicApi, commonInit } from './boot/init'
export { BuildEnv, BuildMode, defineGlobal, makePublicApi } from './boot/init'
export {
startInternalMonitoring,
InternalMonitoring,
MonitoringMessage,
monitored,
Expand Down
9 changes: 7 additions & 2 deletions packages/logs/src/boot/startLogs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import {
areCookiesAuthorized,
combine,
commonInit,
Configuration,
Context,
createEventRateLimiter,
Expand All @@ -15,6 +14,9 @@ import {
canUseEventBridge,
getEventBridge,
getRelativeTime,
updateExperimentalFeatures,
buildConfiguration,
startInternalMonitoring,
} from '@datadog/browser-core'
import { trackNetworkError } from '../domain/trackNetworkError'
import { Logger, LogsMessage, StatusType } from '../domain/logger'
Expand All @@ -29,7 +31,10 @@ export interface LogsInitConfiguration extends InitConfiguration {
}

export function startLogs(initConfiguration: LogsInitConfiguration, errorLogger: Logger) {
const { configuration, internalMonitoring } = commonInit(initConfiguration, buildEnv)
updateExperimentalFeatures(initConfiguration.enableExperimentalFeatures)
const configuration = buildConfiguration(initConfiguration, buildEnv)
const internalMonitoring = startInternalMonitoring(configuration)

const errorObservable = new Observable<RawError>()

if (initConfiguration.forwardErrorsToLogs !== false) {
Expand Down
9 changes: 7 additions & 2 deletions packages/rum-core/src/boot/rumPublicApi.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import {
clocksNow,
timeStampNow,
display,
commonInit,
Configuration,
InternalMonitoring,
callMonitored,
Expand All @@ -21,6 +20,9 @@ import {
RelativeTime,
canUseEventBridge,
areCookiesAuthorized,
updateExperimentalFeatures,
buildConfiguration,
startInternalMonitoring,
} from '@datadog/browser-core'
import { LifeCycle } from '../domain/lifeCycle'
import { ParentContexts } from '../domain/parentContexts'
Expand Down Expand Up @@ -123,7 +125,10 @@ export function makeRumPublicApi<C extends RumInitConfiguration>(
return
}

const { configuration, internalMonitoring } = commonInit(initConfiguration, buildEnv)
updateExperimentalFeatures(initConfiguration.enableExperimentalFeatures)
const configuration = buildConfiguration(initConfiguration, buildEnv)
const internalMonitoring = startInternalMonitoring(configuration)

if (!configuration.trackViewsManually) {
doStartRum(initConfiguration, configuration, internalMonitoring)
} else {
Expand Down