♻️ [js-core] Schema-driven configuration validation across browser-core, browser-logs, and browser-rum-core#4798
♻️ [js-core] Schema-driven configuration validation across browser-core, browser-logs, and browser-rum-core#4798thomas-lebeau wants to merge 14 commits into
Conversation
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 81e0734 | Docs | Datadog PR Page | Give us feedback! |
Bundles Sizes Evolution
|
2d8531b to
0e608e6
Compare
9c41061 to
629c7b1
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 629c7b13dc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| sessionPersistence: { | ||
| type: 'enum', | ||
| values: ['cookie', 'local-storage', 'memory'] as const, | ||
| multiple: true, | ||
| strict: false, | ||
| }, |
There was a problem hiding this comment.
Don't default invalid sessionPersistence to cookies
With strict: false on this public option, an explicit unrecognized sessionPersistence value from plain JavaScript (for example a typo) is dropped by schema validation and becomes undefined, so selectSessionStoreStrategyType() falls back to the default cookie/memory strategy. Before this change the raw invalid value reached the strategy selector and no session store was selected, so this can start tracking with cookies even though the caller explicitly configured an invalid persistence mode; make this field strict or otherwise preserve the invalid value so it does not silently default.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Technically a breaking change, but I don't think anyone willingly use invalid session persistence to prevent the SDK from starting. I think it's fine
| export const DEFAULT_REQUEST_ERROR_RESPONSE_LENGTH_LIMIT = 32 * ONE_KIBI_BYTE | ||
| export const LOGS_SCHEMA = { | ||
| ...BROWSER_CORE_SCHEMA, | ||
| forwardErrorsToLogs: { type: 'boolean', default: true, strict: false }, |
There was a problem hiding this comment.
Preserve forwardErrorsToLogs !== false semantics
For plain JavaScript users who pass a falsy non-boolean value such as 0 or '', this strict: false boolean coercion turns forwardErrorsToLogs off. The previous implementation, and the test that was removed, treated every value except the literal false as enabled via initConfiguration.forwardErrorsToLogs !== false, so these configurations now silently stop forwarding uncaught/runtime/network errors. This option needs the old !== false behavior rather than generic !!value coercion.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Also technically a small breaking change but this makes the behaviour more predictable. For example forwardErrorsToLogs: 0 is probably not expected to actually forward errors to logs
| strict: false, | ||
| default: DEFAULT_PROPAGATOR_TYPES, |
There was a problem hiding this comment.
Don't default invalid propagatorTypes to active headers
When an allowedTracingUrls object has invalid propagatorTypes (for example a typo array like ['traceContext'] or a non-array value), this nested strict: false field falls back to DEFAULT_PROPAGATOR_TYPES, so the SDK injects Datadog/tracecontext headers for that URL. Previously invalid propagators produced no tracing headers or caused the option to be ignored, which avoids unexpected CORS preflights and unintended trace propagation; keep invalid propagatorTypes ignored instead of defaulting them to active headers.
Useful? React with 👍 / 👎.
629c7b1 to
ec72176
Compare
Move buildCookieOptions and CookieConfiguration out of configuration.ts into cookie.ts where they logically belong. - CookieConfiguration now lives in cookie.ts with optional boolean fields - buildCookieOptions accepts CookieConfiguration instead of InitConfiguration - configuration/index.ts re-exports buildCookieOptions from cookie.ts - cookie.spec.ts covers all four buildCookieOptions scenarios
Introduces a declarative, type-safe configuration engine in a new @datadog/js-core/configuration sub-path entry point. Field types: StringField, PercentageField, BooleanField, SiteField, MatchOptionField, EnumField, UnionField, SchemaField, FunctionField. Key features: - InferredConfig<S> derives the output type from the schema definition, making it impossible for the interface to drift from the schema - strict: false mode for backward-compatible validation fallback - FunctionField with signature phantom field for precise type inference - ConfigurationSchema as an interface for better type ergonomics - Per-field auto-generated error messages via display.error Also registers the package in tsconfig path aliases and eslint side-effect allowlist.
Replaces the hand-written Configuration interface and per-field validators with a declarative BROWSER_CORE_SCHEMA. Configuration is now inferred via InferredConfig<typeof BROWSER_CORE_SCHEMA> — impossible to drift. Key changes: - validateAndBuildConfiguration is now a thin wrapper: passes display.error to the schema engine so all field errorMessages surface as display.error calls - trackingConsent uses the object-form enum (INVALID on explicit invalid value) - source uses the array-form enum (defaults to 'browser' on invalid/missing) - sessionPersistence uses enum + multiple: true, normalising 'cookie' → ['cookie'] - allowedTrackingOrigins uses match-option + multiple: true - The three cookie flags (useSecureSessionCookie, usePartitionedCrossSiteSessionCookie, trackSessionAcrossSubdomains) are now first-class schema fields; cookieOptions is removed from Configuration — sessionInCookie.ts calls buildCookieOptions() directly - BROWSER_CORE_SCHEMA exported so browser-logs and browser-rum-core can extend it - normalizePersistenceList simplified — sessionPersistence is always SessionPersistence[] | undefined after schema validation - isAllowedTrackingOrigins moved to the init layer (preStartLogs/preStartRum)
Replaces hand-written LogsConfiguration validation with a declarative LOGS_SCHEMA that extends BROWSER_CORE_SCHEMA. - forwardErrorsToLogs: boolean field with default true - forwardConsoleLogs / forwardReports: enum fields with allowAll: true - Removes validateAndBuildForwardOption helper (schema handles it) - isAllowedTrackingOrigins check moved to preStartLogs init layer
Replaces hand-written RumConfiguration validation with a declarative RUM_SCHEMA that extends BROWSER_CORE_SCHEMA. Key changes: - allowedTracingUrls / excludedActivityUrls: union + multiple fields, single string/regex now normalises to array instead of erroring - propagatorTypes: invalid values fall back to defaults - trackFeatureFlagsForEvents: invalid items fall back to [] - RumConfiguration type inferred from schema with manual overrides for complex fields (beforeSend, plugins, allowedTracingUrls) - isAllowedTrackingOrigins check moved to preStartRum init layer
- Cast DEFAULT_CONFIG to Configuration in allowedTrackingOrigins.spec.ts - Refactor sessionContext.spec.ts: replace beforeEach with setup() factory to avoid shared mutable state across tests - Fix recordingScope.specHelper.ts: spread configuration instead of casting - Fix trackInput/trackMutation specs: recreate scope instead of mutating configuration.defaultPrivacyLevel in place - Export DEFAULT_REQUEST_ERROR_RESPONSE_LENGTH_LIMIT from networkErrorCollection and update e2e import path accordingly
Register src/entries/configuration.ts as a typedoc entry point and add the generated API Extractor report for the js-core configuration API.
- Document Optionality, Multiple, Strict, and all FieldDef variants (StringField, PercentageField, BooleanField, SiteField, MatchOptionField, EnumField, UnionField, SchemaField, FunctionField) - Document ConfigurationSchema, InferredConfig, and validateAndBuildConfiguration - Regenerate configuration.api.md to reflect the newly documented exports (removes "(undocumented)" tags)
ec72176 to
34b7bf5
Compare
- Split InferredConfig into required and optional key sets so fields without `required: true` or a `default` become optional properties instead of always-present `| undefined` properties - Lets consumers building configuration object literals by hand (e.g. test fixtures) omit fields instead of writing `field: undefined` - Update API report and add a compile-time test verifying required fields stay required while optional fields can be omitted
BeltranBulbarellaDD
left a comment
There was a problem hiding this comment.
LTGM. Do you think we could add js-core to the Bundles Sizes Evolution commenter?
|
|
|
||
| const DATADOG_SITE_REGEX = /(datadog|ddog|datad0g|dd0g)/ | ||
|
|
||
| function validateInnerSchema( |
There was a problem hiding this comment.
suggestion: This could probably be factorized with validateAndBuildConfiguration
| case 'boolean': | ||
| return `"${key}" must be a boolean` | ||
| case 'site': | ||
| return `"${key}" must be a valid Datadog site. More details: https://docs.datadoghq.com/getting_started/site/.` |
There was a problem hiding this comment.
suggestion: use MORE_DETAILS and DOCS_ORIGIN constants
| import type { RawNetworkLogsEvent } from '../../rawLogsEvent.types' | ||
| import type { LogsConfiguration } from '../configuration' | ||
|
|
||
| const DEFAULT_REQUEST_ERROR_RESPONSE_LENGTH_LIMIT = 32 * ONE_KIBI_BYTE |
There was a problem hiding this comment.
suggestion: import the constant from the related source file
| startSessionReplayRecordingManually: | ||
| initConfiguration.startSessionReplayRecordingManually !== undefined | ||
| ? !!initConfiguration.startSessionReplayRecordingManually | ||
| : sessionReplaySampleRate === 0, | ||
| traceSampleRate: initConfiguration.traceSampleRate ?? 100, | ||
| config.startSessionReplayRecordingManually ?? config.sessionReplaySampleRate === 0, |
There was a problem hiding this comment.
suggestion: remove this fallback, it's not needed with v7. Rationale: if sessionReplaySampleRate is 0, session replay will be disabled, so startSessionReplayRecordingManually is irrelevant.
| } | ||
| export type Configuration = InferredConfig<typeof BROWSER_CORE_SCHEMA> | ||
|
|
||
| export function serializeConfiguration(initConfiguration: InitConfiguration) { |
There was a problem hiding this comment.
question: did you explore generating the serialized version from the schema?
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 636fc571a7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| betaEnableViewUpdates: { type: 'boolean', default: false }, | ||
| enablePrivacyForActionName: { type: 'boolean', default: true }, | ||
| propagateTraceBaggage: { type: 'boolean', default: true }, | ||
| startSessionReplayRecordingManually: { type: 'boolean', default: false, strict: false }, |
There was a problem hiding this comment.
Preserve the replay manual-start default
When sessionReplaySampleRate is omitted or explicitly 0, this schema default now builds startSessionReplayRecordingManually: false; the previous builder derived the default from the replay sample rate and returned true whenever replay sampling was disabled. Because the default replay sample rate is still 0, a default RUM init now flips the value propagated to view/session-replay logic and changes the start_session_replay_recording_manually event field and pre-start recorder behavior. Please keep the conditional default after schema validation instead of hard-coding false.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
it's fine, with deterministic sampling we don't need the conditional default anymore
…ma validation - Move DOCS_ORIGIN and MORE_DETAILS constants into js-core/util/display.ts and re-export them from browser-core/tools/display.ts and js-core/entries/util.ts, so the site validation error message in entries/configuration.ts reuses the same strings instead of duplicating the URL and prefix inline - Extract validateSchemaFields() out of validateAndBuildConfiguration() and reuse it (with an optional display) for nested `type: 'schema'` fields, removing the near-duplicate validateInnerSchema() implementation - Update the generated util.api.md report for the newly exported constants
…alse - Schema-driven fields only support static defaults, so the previous conditional default (true when sessionReplaySampleRate is 0, false otherwise) can no longer be resolved in post-processing - Change the default to false regardless of sessionReplaySampleRate, and update the TSDoc/@DefaultValue and tests accordingly
…rom source - The test redefined DEFAULT_REQUEST_ERROR_RESPONSE_LENGTH_LIMIT locally instead of importing the constant already exported by networkErrorCollection.ts, risking the two values drifting apart
636fc57 to
b139b2d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b5c9b6295
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| : validateField(field, rawValue, display) | ||
|
|
||
| if (validated === undefined) { | ||
| if (rawValue !== undefined && rawValue !== null && rawValue !== '') { |
There was a problem hiding this comment.
Don't treat explicit empty consent as absent
When a non-required field with a default fails validation, this condition treats explicit null and '' the same as an omitted value, so trackingConsent: null or trackingConsent: '' now falls through to the schema default granted. Before this change, those explicit invalid consent values were rejected by the TrackingConsent check, so RUM/Logs could not start collecting data under an invalid privacy setting; keep explicit empty values invalid for enum fields such as trackingConsent instead of defaulting them.
Useful? React with 👍 / 👎.
| }, | ||
|
|
||
| // Arrays | ||
| excludedActivityUrls: { type: 'match-option', multiple: true, default: [] as MatchOption[] }, |
There was a problem hiding this comment.
Keep ignoring invalid excluded activity matchers
For excludedActivityUrls arrays that contain a conditional or otherwise invalid entry, such as [isDev && /sockjs/] when isDev is false, this strict matcher schema now rejects the whole RUM configuration. The previous builder only checked that the option was an array and matchList() ignored non-matchers at use time, so these pages continued collecting RUM data; preserve that tolerant filtering here instead of aborting init for one bad item.
Useful? React with 👍 / 👎.
| trackLongTasks: { type: 'boolean', default: true, strict: false }, | ||
| trackViewsManually: { type: 'boolean', default: false, strict: false }, | ||
| betaEnableViewUpdates: { type: 'boolean', default: false }, | ||
| enablePrivacyForActionName: { type: 'boolean', default: true }, |
There was a problem hiding this comment.
Keep action privacy tolerant of non-boolean values
If plain JavaScript or remote configuration supplies a non-boolean value for enablePrivacyForActionName (for example a dynamic cookie value of 'false'), strict validation now aborts the entire RUM init. Before this change the option used initConfiguration.enablePrivacyForActionName !== false, so the same malformed optional privacy flag kept action-name privacy enabled and allowed RUM to start; preserve that non-false fallback instead of taking down collection.
Useful? React with 👍 / 👎.
Motivation
Configuration validation in
browser-core,browser-logs, andbrowser-rum-corewas previously done imperatively — each package had its own ad-hoc validation logic with repetitive checks, inconsistent error handling, and no shared abstraction.This PR introduces a schema-driven configuration system in
@datadog/js-corethat declaratively describes fields, their types, defaults, and validation rules. Browser packages then derive theirConfigurationtypes and validation functions from these schemas.Changes
@datadog/js-core: Addconfigurationentry point withvalidateAndBuildConfiguration,InferredConfig, and field definition types (StringField,BooleanField,PercentageField,SiteField,MatchOptionField,EnumField,CustomField). Includes anINVALIDsentinel value that custom validators can return to fail the whole configuration.browser-core: Replace imperativevalidateAndBuildConfigurationand manualConfigurationinterface withBROWSER_CORE_SCHEMA+InferredConfig<typeof BROWSER_CORE_SCHEMA>. Cookie-option fields (useSecureSessionCookie, etc.) are moved into the schema.buildCookieOptionsnow takes a typedCookieConfigurationrather than raw init config.browser-logs: ReplaceLogsConfigurationinterface and imperative validation withLOGS_SCHEMAthat spreadsBROWSER_CORE_SCHEMAand addsforwardErrorsToLogs,forwardConsoleLogs,forwardReports, andrequestErrorResponseLengthLimit.browser-rum-core: Same pattern —RUM_SCHEMAspreadsBROWSER_CORE_SCHEMAand adds all RUM-specific fields.Test instructions
Checklist