🔊 Report remote configuration sync metadata on configuration telemetry - #5001
🔊 Report remote configuration sync metadata on configuration telemetry#5001mormubis wants to merge 4 commits into
Conversation
| // version. Skipping the write here is also what keeps `firstApplied` alive across refetches. | ||
| // TODO: compare on `ETag` instead once the CDN exposes it through | ||
| // `Access-Control-Expose-Headers`. The stringify compare is key-order sensitive, which holds | ||
| // only because both sides come from `JSON.parse` of the same CDN payload. |
There was a problem hiding this comment.
This early return is the whole sync_id semantic, and it's also what keeps firstApplied from being wiped when the background fetch pulls the same config back. Worth knowing before touching the comparison.
| * readable cross-origin without the CDN having to expose it explicitly. It carries an HTTP-date, | ||
| * so the value is only accurate to the second. | ||
| */ | ||
| function parseLastModified(response: Response): number | undefined { |
There was a problem hiding this comment.
last-modified is the only useful header we can read here. etag and x-amz-version-id are not CORS-safelisted and the CDN doesn't send Access-Control-Expose-Headers, so they come back null in the browser even though curl sees them. That's why there's no version_id in this PR.
| if (cacheResult.status === 'hit') { | ||
| return applyRemoteConfiguration(initConfiguration, cacheResult.config, supportedContextManagers, metrics) | ||
| return { | ||
| // Stamping is synchronous, so it always lands before the background sync above can write. |
There was a problem hiding this comment.
doBackgroundCacheSync starts the fetch before this line, so it looks racy. It isn't, the stamp is synchronous and the fetch can't resolve before the next microtask. I left the order alone to keep the diff small, but I can move the sync call below if it reads better.
Bundles Sizes Evolution
|
|
✅ All CI checks and tests passed. 🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 9febebe | Docs | View more details | Give us feedback! |
9c2e781 to
5514ed4
Compare
5514ed4 to
9febebe
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9febebe9c3
ℹ️ 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".
| import type { RemoteConfiguration } from './remoteConfiguration' | ||
|
|
||
| export const CACHE_VERSION = 2 | ||
| export const CACHE_VERSION = 3 |
There was a problem hiding this comment.
issue: Existing installations have version-2 entries containing a usable configuration, but this version bump makes read() classify every one as an error and delete it. On the first page load after upgrading, clients using required: true will not start RUM at all, while other clients temporarily run without their cached remote overrides until another navigation; accept and migrate the previous shape (or at least apply its config without telemetry metadata) to avoid this rollout-wide gap.
Useful? React with 👍 / 👎.
| if (cached.status === 'hit' && JSON.stringify(cached.config) === JSON.stringify(config)) { | ||
| return | ||
| } |
There was a problem hiding this comment.
issue: When the CDN republishes the same JSON payload with a new Last-Modified value, this early return discards that new publish timestamp and retains the previous lastModified, lastSynced, firstApplied, and syncId. Those sessions are consequently attributed to the old publication and the new propagation latency is never measured, even though the newly collected lastModified value provides a way to distinguish this fetch from the cached publication.
Useful? React with 👍 / 👎.
| return applyRemoteConfiguration(initConfiguration, cacheResult.config, supportedContextManagers, metrics) | ||
| return { | ||
| // Stamping is synchronous, so it always lands before the background sync above can write. | ||
| metadata: configurationCache.stampFirstApplied(cacheResult), |
There was a problem hiding this comment.
issue: This persists firstApplied before the cached configuration is resolved and validated by doInit. For example, if a dynamic applicationId is unavailable on the first load, validation rejects the configuration and no RUM session starts, but the cache is still marked as applied; if that value becomes available on a later load, telemetry reports the failed load's timestamp rather than when the configuration actually took effect.
Useful? React with 👍 / 👎.
1 participant
Motivation
The remote configuration telemetry RFC wants to measure how long a published config takes to take effect on a device. Two segments:
last_synced - last_modifiedfor CDN to fetch, andfirst_applied - last_syncedfor fetch to apply. We have no visibility on either today.The schema already merged in the events format repo and is synced here. This is the browser side.
Changes
The metadata goes in the existing
dd_rc_<id>localStorage entry instead of a separate key, so a version and its metadata can't drift apart on a partial write.CACHE_VERSIONis bumped to 3 andfetchedAtbecomes ametadataobject withlastModified,lastSynced,firstAppliedandsyncId.sync_iddoesn't follow the RFC. The RFC regenerates it on non-304 fetches, but we send noIf-None-Matchand never see a 304. Insteadwrite()compares the fetched payload with the cached one and skips the write when they match. It keys on the config changing instead of the transport, and it's also what keepsfirstAppliedfrom being wiped by a background refetch.The metadata is threaded through
doInitintoserializeRumConfigurationinstead of module state. RUM and Logs read the same cache entry once #4884 lands, so a singleton would report the wrong product's numbers.config_idis not included, we already sendremote_configuration_id.version_idis not included either,x-amz-version-idis not readable from JS.Nothing is reported on the sync path (
sync: trueor the legacyremoteConfigurationId). Fetch and apply happen in the same tick there, sofirst_applied - last_syncedwould always be ~0. I can add it if you prefer having the CDN to fetch half for those setups. What do you think?Test instructions
Needs a real published remote configuration ID.
yarn dev-server startremoteConfiguration: { id: '<rc-id>' },proxy: '/proxy',telemetrySampleRate: 100andtelemetryConfigurationSampleRate: 100localStorage.getItem('dd_rc_<rc-id>')hasversion: 3and ametadatawithlastModified,lastSyncedandsyncId, nofirstAppliedyetfirstAppliedis set,lastSyncedandsyncIdunchangedagent-browser tab newto flush, thenyarn dev-server intake telemetry-configuration-events | jq '.telemetry.configuration.remote_configuration'First load reports nothing, second reports the four fields:
{"last_modified":1777557229000,"last_synced":1787930428705, "first_applied":1787930438546,"sync_id":"476d2ee9-fb31-4c30-824a-5410d4850ad0"}Checklist