♻️ Make session manager injectable and isolate for v7 rewrite#4157
♻️ Make session manager injectable and isolate for v7 rewrite#4157thomas-lebeau merged 10 commits intov7from
Conversation
Bundles Sizes Evolution
🚀 CPU Performance
🧠 Memory Performance
|
|
✅ Tests 🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage (details) 🔗 Commit SHA: 8e76c0c | Docs | Datadog PR Page | Was this helpful? Give us feedback! |
b9b9897 to
e356ec1
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e356ec178a
ℹ️ 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".
Move the stub-vs-real session manager branching from createPreStartStrategy to the caller (rumPublicApi/logsPublicApi) via a factory parameter. This decouples boot-level tests from session internals (cookies, storage). - Unify stub signatures to callback pattern (config, consent, onReady) - Accept startSessionManagerImpl in createPreStartStrategy - Callers use ternary: canUseEventBridge() ? stub : real - Boot tests inject mock factories, removing stopSessionManager cleanup
Gut the real session manager implementations (core, rum, logs) and skip their unit tests. This ensures all non-session-manager tests use mocks, isolating the session manager work for the upcoming rewrite.
The gutting confirmed all non-session-manager tests use mocks. Now restore the real implementations so the full test suite passes (3174 tests, 0 failures).
…iguration Move the isWorkerEnvironment check from configuration (sessionStoreStrategyType) to the public APIs, using session manager stubs in worker environments. This allows the Logs SDK to load in worker environments by aligning worker handling with the event bridge pattern.
- Force memory persistence at the storage level for workers - Block RUM entirely in workers with explicit warning - Let Logs use the real session manager (storage handles memory) - Skip DOM event tracking in session manager for workers
Mirror the RUM pattern: when no event bridge is available and no session store strategy type exists, warn and bail out instead of crashing on `sessionStoreStrategyType!` dereference.
e356ec1 to
bbbd93c
Compare
- Remove isWorkerEnvironment check that forced the session manager stub in worker contexts; only canUseEventBridge() now triggers the stub - Add session_id assertions to service worker E2E tests to verify workers produce proper session IDs with the real session manager
…nager - Guard against missing sessionStoreStrategyType directly in startSessionManager with an early return and warning, removing the non-null assertion - Remove duplicate "no storage available" guards from preStartLogs and preStartRum - Update tests accordingly
- Move the isWorkerEnvironment check from selectSessionStoreStrategyType into normalizePersistenceList so it only applies as a default when no explicit sessionPersistence is configured by the user
| }) | ||
| trackVisibility(configuration, () => sessionStore.expandSession()) | ||
| trackResume(configuration, () => sessionStore.restartSession()) | ||
| if (!isWorkerEnvironment) { |
There was a problem hiding this comment.
Enabling the session manager to work on service workers seem to increase the complexity.
💬 suggestion: Should we move the trackActivity/trackVisibility/trackResume to an upper level, an call sessionManager.expandOrRenewSession?
There was a problem hiding this comment.
The complexity was already here, it has just been moved from preStartRum and preStartLogs to the session Manager (we were using a stub session Manager instead of the real one)
Moving that back one level up will risk duplicating the logic (LOGS vs RUM, even though RUM does not work in worker env). I think it's fine, so far it's just one if statement but we will need to keep an eye on this.
In v8, is we manage to reconcile session manager and only have one instance, we might want to do differently then.
I will keep note on this though, maybe we can be smarter and guard by feature instead of isWorkerEnvironment (i.e. document is present,...)
Motivation
Prepare the session manager layer for a from-scratch rewrite in v7. The existing session manager internals (cookies, storage, core session store) are tightly coupled to boot-level tests, making it hard to replace the implementation without breaking unrelated tests.
This PR decouples all non-session-manager code from session internals, so the session manager can be rewritten in isolation.
Changes
1. Injectable session manager factory
Move the stub-vs-real session manager branching from
createPreStartStrategyto the caller (rumPublicApi/logsPublicApi) via a factory parameter:graph LR A[rumPublicApi / logsPublicApi] -->|ternary: bridge ? stub : real| B[factory] B -->|passed as param| C[createPreStartStrategy] C -->|calls factory| D[session manager instance]sessionPersistence: 'memory'to work without storage. In worker environments, Logs uses the real session manager with memory persistence (we can revisit that when we star using CookieStore as this seems supported in service workers). RUM is explicitly blocked with a warning.createPreStartStrategyreceives one factory, just calls it — no branching logiccanUseEventBridge() ? stub : real2. Prove isolation via gut-then-restore
To verify that all non-session-manager tests use mocks (not real implementations), the session manager bodies were temporarily gutted and their unit tests skipped (
xdescribe). All 3071 remaining tests passed with zero failures, proving the isolation is complete. The implementations and tests were then fully restored.3. Mock infrastructure
preStartRum.spec,preStartLogs.spec) inject mock factories — no morestopSessionManager()cleanup or cookie manipulationrumPublicApi.spec,logsPublicApi.spec) inject mock session managers via optional parameter onmakeRumPublicApi/makeLogsPublicApimockRumSessionManager/mockLogsSessionManagerupdated with factory wrappers4. Worker environment support
Use a defense-in-depth strategy for worker environments, handling workers at multiple layers:
selectSessionStoreStrategyTypeforcesSessionPersistence.MEMORYin workers — cookies and localStorage are unavailable in worker contextstrackActivity,trackVisibility,trackResume) in workers — no DOM events to listen topreStartRumwith an explicit warning — RUM features (DOM observation, user interactions) are not applicable in workersNew exports from
@datadog/browser-core:selectSessionStoreStrategyType,SessionStoreStrategyTypestartSessionManagernow takessessionStoreStrategyTypeas a direct parameter.Checklist