✨ Add remote configuration support for logs fields - #4884
Conversation
| return options.remoteConfigurationProxy | ||
| } | ||
| const id = getRemoteConfigurationId(options)! | ||
| return buildEndpointUrl({ |
There was a problem hiding this comment.
The generic fetch doesn't validate whether the response has a rum or logs section. That's intentional — each SDK wraps it with its own guard. RUM checks rum || profiling before proceeding; Logs just skips applying if logs is absent. What do you think?
|
Bundles Sizes Evolution
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 756c9d22f0
ℹ️ 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".
756c9d2 to
61208b1
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62c3b5273e
ℹ️ 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".
| "properties": { | ||
| "forwardErrorsToLogs": { | ||
| "type": "boolean", | ||
| "description": "Whether to forward console.error calls as Datadog log events" |
There was a problem hiding this comment.
Correct the documented effect of forwardErrorsToLogs
issue: When this schema is used to present or generate Logs remote configuration, it tells operators that this field controls console.error. The runtime contract in packages/browser-logs/src/domain/configuration.ts instead says it controls uncaught exceptions and network errors, while console.error requires forwardConsoleLogs. A configuration created from this description therefore will not collect the advertised console calls and may unintentionally change unrelated error collection.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is just internal types.
| const cacheResult = cache.read() | ||
|
|
||
| // Background sync — update the cache for the next page load | ||
| fetchRemoteConfiguration(initConfiguration) |
There was a problem hiding this comment.
Exclude background configuration fetches from network logs
issue: On the non-blocking path with forwardErrorsToLogs enabled (the default), this request is issued after startBufferingData() has instrumented fetch and remains observable when startNetworkErrorCollection() starts. The remote-configuration endpoint is not recognized by isIntakeUrl, so a rejected request or 5xx response is emitted as a customer Fetch error log. A remote-configuration outage therefore pollutes the customer's Logs data with an SDK-internal request; this fetch needs to be marked or excluded from network-error collection.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 913742dbfd
ℹ️ 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".
| remoteConfigurationId: getRemoteConfigurationId(initConfiguration)!, | ||
| }) | ||
|
|
||
| const cacheResult = cache.read() |
There was a problem hiding this comment.
Fall back from the cache path in service workers
issue: When Logs runs in a supported service-worker context with remoteConfiguration: { id }, localStorage is unavailable, so cache.read() always reports an error and the background write is silently ignored. As a result, non-required overrides are never applied on any worker restart, while required: true prevents Logs from starting on every attempt. Use worker-compatible storage or fall back to the live-fetch path when persistent cache storage is unavailable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
we can try to use Cookie Store as a fallback for LS in service-worker or (even worse) Indexed DB
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f58a48f9da
ℹ️ 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".
Motivation
The previous PR (#4877) intentionally excluded
logs.*fields from remote configuration becausebrowser-logsis a separate package with no dependency onbrowser-rum-core, where all the remote config infrastructure lived. This PR solves that.Changes
To make remote configuration available to
browser-logs, the shared infrastructure had to move tobrowser-core. Three things were extracted:createConfigurationCache) — made generic<T>so it works for any config shapeRumSdkConfig/RemoteConfiguration) — previously inbrowser-rum-core, now generated intobrowser-corefetchRemoteConfiguration,buildEndpoint,getRemoteConfigurationId) — generic, no SDK-specific guardsbrowser-rum-corere-exports everything frombrowser-core, so there's no public API breakage.On the logs side, the implementation follows the same two-function pattern as RUM:
getLogsRemoteConfiguration— reads from cache, kicks off a background sync, returns the resolved init config (orundefinedifrequiredand cache miss)fetchAndApplyLogsRemoteConfiguration— fetches live and applies the resultpreStartLogsuses the sameisSyncLoadingbranching aspreStartRum: sync path forremoteConfigurationId, cache path forremoteConfiguration: { id }.The three logs fields now supported:
forwardErrorsToLogs,forwardConsoleLogs,forwardReports.No double-fetch. When both RUM and Logs are initialized with the same remote config ID, only one HTTP request goes out — even when loaded as separate CDN bundles.
fetchRemoteConfigurationdeduplicates concurrent calls for the same endpoint using awindow-level registry (window.__ddRcInflight), so the promise is shared across bundle boundaries.Test instructions
logs.forwardErrorsToLogs: falseDD_LOGSwithremoteConfiguration: { id: '<your-id>' }(cache path — takes effect on the second page load after the background sync populates the cache)console.error('test')Checklist