Conversation
rum-events-format was unexpectedly updated. This commit reverts the submodule and generated files to the main branch.
This commit makes it easier to change large functions signatures like `makeRumPublicApi`, `makePreStartRum`, etc.
No need to start the telemetry again in `startLogs` and `startRum`.
This simplifies the code a bit.
…n-manager-with-telemetry
Replace polling-based waitFor with the cleaner collectAsyncCalls pattern which waits for a specific number of spy calls and fails fast on extra calls. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
thomas-lebeau
changed the title
![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: fc052aa79e
ℹ️ 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".
| trackingConsentState.observable.subscribe(() => { | ||
| if (trackingConsentState.isGranted()) { | ||
| sessionStore.expandOrRenewSession() | ||
| } else { | ||
| sessionStore.expire(false) |
There was a problem hiding this comment.
The consent change handler is registered only inside the expandOrRenewSession callback. If the session store is locked (e.g., Chromium cookie locking) and that callback is delayed, a trackingConsentState.update(NOT_GRANTED) during that window will be missed, so sessionStore.expire(false) never runs and the session can stay active despite revoked consent. This is a regression from the previous synchronous subscription and can leak data in the lock-wait scenario.
Useful? React with 👍 / 👎.
|
✅ Tests 🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage 🔗 Commit SHA: 1183c6f | Docs | Datadog PR Page | Was this helpful? Give us feedback! |
Bundles Sizes Evolution
🚀 CPU Performance
🧠 Memory Performance
|
If consent is revoked while waiting for the session store lock (e.g., Chromium cookie locking), the session should be expired instead of proceeding with initialization. This prevents a race condition where consent revocation during the lock-wait period would be missed.
thomas-lebeau
force-pushed
the
thomas.lebeau/asyc-session-manager-with-telemetry-merge-main
branch
from
37c3f07 to
3b4ec11
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b4ec110a4
ℹ️ 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".
| startRumSessionManager(configuration, trackingConsentState, (newSessionManager) => { | ||
| sessionManager = newSessionManager | ||
| tryStartRum() |
There was a problem hiding this comment.
Because startRumSessionManager is now async, the onGrantedOnce callback can run while consent is granted, but the session manager can become ready later (e.g., cookie lock delay). If consent is revoked during that window, this callback still calls tryStartRum() once the manager resolves, and tryStartRum doesn’t re-check consent, so RUM can start and collect despite NOT_GRANTED. Consider checking trackingConsentState.isGranted() before calling tryStartRum() (or cancelling the pending start on revoke) to avoid starting RUM after a mid-init revoke.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
fixed by hoisting startTrackingConsentContext (which discard telemetry using hooks if consent is not granted)
| startLogsSessionManager(configuration, trackingConsentState, (newSessionManager) => { | ||
| sessionManager = newSessionManager | ||
| tryStartLogs() |
There was a problem hiding this comment.
The async startLogsSessionManager callback unconditionally calls tryStartLogs() once the session manager resolves. If consent is granted at init but revoked before the session manager is ready (e.g., cookie lock delay), this path still starts Logs even though consent is currently not granted, because tryStartLogs doesn’t re-check consent. Add a consent check (or cancel the pending start on revoke) before calling tryStartLogs() to prevent starting Logs in a revoked-consent state.
Useful? React with 👍 / 👎.
Tests in 'buffers API calls before starting RUM' section expected synchronous doStartRum calls but session manager initialization is now async. Added proper async/await with collectAsyncCalls to wait for the async callbacks. Fixes race conditions on slower browsers (Chrome 63, Edge 80, Firefox 67).
Tests in 'trackViews mode' section expected synchronous or partially async doStartRum calls but session manager initialization is now fully async. Added proper async/await with collectAsyncCalls to wait for all async callbacks to complete. Fixes race conditions on slower browsers (Edge 80).
Introduced a new function to reset buffered and ongoing session store operations, ensuring a clean state when stopping the session manager. This change enhances session management unt test reliability.
Move startTrackingConsentContext from startRum to preStartRum to align with the pattern of initializing consent-dependent contexts earlier in the startup sequence. This removes the trackingConsentState parameter from startRum since it's no longer needed there.
Move startTrackingConsentContext from startLogs to preStartLogs to align with the pattern of initializing consent-dependent contexts earlier in the startup sequence. This removes the trackingConsentState parameter from startLogs since it's no longer needed there.
thomas-lebeau
force-pushed
the
thomas.lebeau/asyc-session-manager-with-telemetry-merge-main
branch
from
411583d to
1183c6f
Compare
| }) | ||
| }) | ||
|
|
||
| describe('tracking consent', () => { |
There was a problem hiding this comment.
Moved to an e2e test
3 participants
Motivation
Important
Base branch for merging is v7
Prepare for the upcoming new session manager that will use modern async APIs such as the native Lock API and CookieStore API. These APIs are inherently asynchronous, so the session manager initialization needs to be async to support them.
Also ensure telemetry respects user consent by deferring initialization until consent is granted.
Changes
onGrantedOncecollectAsyncCallsTest instructions
yarn test:unityarn test:e2eChecklist