-
Notifications
You must be signed in to change notification settings - Fork 60
fix(analytics-browser): defer session_start and attribution when optOut is "true" #1509
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Defer session start and web attribution initialization when
|
|
Link the design doc here for reference https://amplitude.atlassian.net/wiki/spaces/GOV/pages/3566829692/optOut+not+playing+nicely+with+autocapture.sessions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When optOut=true, don’t start a session (do not even set sessionId). Invoke the session logic later when optOut=false.
Hi @daniel-graham-amplitude, according to the design, we should not event set session id when opt out. But the current implementation save it to cookie storage which doesn't align with the design.
I have a local test with the test sever
- visit
http://localhost:5173/opt-out/index.html?ampSessionId=123456789first - then go to
http://localhost:5173/opt-out/index.html - click "Turn off opt out" and a new session with the session id 123456789 is started
I'd expect we start the session with the session id at the time when i click "turn off opt out". wdyt?
|
@Mercy811 it doesn't set This was to cover a case that Jin had flagged, he pointed out that a customer could call |
It makes sense to me to let the SDK remember customer's setSessionId call and ampDeviceId from url. But if they just call amplitude.init() and setOptOut(false) later, shouldn't the session id be the time setOptOut(false) is called instead of amplitude.init() is called like the design doc
|
Summary
Currently, if you set "optOut=true" on init, and you have autocapture enabled (sessions, attribution) it will still track "session_start" and "identify" (for attribution) but it will be dropped because it's optOut=true; and will never be called again because the session has already started.
This pull request makes it so that if a session is started while optOut, the session creation will be deferred until later when optOut becomes "false".
When optOut becomes false (either via
setOptOut(false)or calling init again withoptOut != true) than it will call session_start, identify (attribution) and page_viewedChecklist
Note
Medium Risk
Touches session lifecycle, opt-out state transitions, and initialization ordering; bugs here could cause missing or duplicated session/attribution/page-view events when users toggle opt-out.
Overview
When initialized with
optOut: true, the browser SDK now defers starting a session (and the resultingsession_start/ configuredidentify) by storing adeferredSessionIdand only applying it once opt-out is turned off.It also defers web attribution and page view tracking while opted out by skipping attribution initialization and delaying page-view plugin registration until
setOptOut(false)(or a subsequentinitwith opt-out disabled) triggers new opt-out listeners.Core changes add a small async opt-out listener mechanism to
Timeline, persistdeferredSessionIdin browser config/user session storage, and adjustsetOptOutordering to update config before notifying listeners; tests and a new test-server page cover the new opt-out/opt-in behavior.Written by Cursor Bugbot for commit 4ee8599. This will update automatically on new commits. Configure here.