🔊 add telemetry for unexpected session id changes#3815
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
✅ Tests 🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage 🔗 Commit SHA: 15e4867 | Docs | Was this helpful? Give us feedback! |
b0c39b0 to
d7ddf16
Compare
Bundles Sizes Evolution
🚀 CPU Performance
🧠 Memory Performance
|
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Integrated commit sha: d7ddf16 Co-authored-by: BenoitZugmeyer <[email protected]>
d7ddf16 to
4bdf1b4
Compare
4bdf1b4 to
c3337fb
Compare
|
/to-staging |
|
View all feedbacks in Devflow UI.
Commit c3337fba16 will soon be integrated into staging-37.
Commit c3337fba16 has been merged into staging-37 in merge commit bc8c148d83. Check out the triggered pipeline on Gitlab 🦊 If you need to revert this integration, you can use the following command: |
Integrated commit sha: c3337fb Co-authored-by: BenoitZugmeyer <[email protected]>
| } else { | ||
| const newSessionState = toSessionState(changed.value) | ||
| if (newSessionState.id !== initialSessionState.id) { | ||
| addTelemetryDebug('Session cookie changed', { |
There was a problem hiding this comment.
💬 suggestion: I think there is still some amount of short session that are due to cookie set with different domain or config resulting to having multiple _dd_s cookies.
What about adding all the _dd_s cookies from cookieStore.getAll('_dd_s') as these will include the domain and other options the cookie were set with and it could be useful to understand why the session was shorter than expected
| } | ||
|
|
||
| const sessionAge = dateNow() - sessionCreatedTime | ||
| if (sessionAge > 14 * ONE_MINUTE) { |
There was a problem hiding this comment.
❓ question: why not using the session expiration delay?
There was a problem hiding this comment.
I want to prevent a few false positives at the cost of losing a bit of logs. I'm fearing that if we are too close to the actual limit, we could face clock issues. I'm not sure that Date.now() returns the exact same value across tabs, so the cookie could be expired in some tab and still be considered within the 15 minutes limit in another. With a 1 minute margin of error, we get rid of this potential issue.
Do you think that could have a negative impact on the data?
There was a problem hiding this comment.
I just wanted to understand the intent.
IMO, that should be fine to get telemetry data, if we want to later include that in a potential fix, it could be interesting to explore a bit more this hypothesis around different Date.now() values across tabs.
Also, skip session state when there is no id (i.e. the cookie is expired) because it's noisy and isn't a problematic state.
|
/to-staging |
|
View all feedbacks in Devflow UI.
Commit 15e4867676 will soon be integrated into staging-37.
Commit 15e4867676 has been merged into staging-37 in merge commit 9fafe56e0c. Check out the triggered pipeline on Gitlab 🦊 If you need to revert this integration, you can use the following command: |
Integrated commit sha: 15e4867 Co-authored-by: BenoitZugmeyer <[email protected]>
* 🔊 add telemetry for unexpected session id changes * 👌 add cookie values in the telemetry log Also, skip session state when there is no id (i.e. the cookie is expired) because it's noisy and isn't a problematic state.
* 🔊 add telemetry for unexpected session id changes * 👌 add cookie values in the telemetry log Also, skip session state when there is no id (i.e. the cookie is expired) because it's noisy and isn't a problematic state.
3 participants
Motivation
Explore a quickfix for #incident-42289
Changes
This PR adds some telemetry to validate whether it is possible to detect an unexpected cookie change based on
cookieStore. This change is gated by theshort_session_investigationexperimental feature flagTest instructions
Using my playwright script to stress test browsers and increase the chance of cookie synchronization issues, the telemetry log is showing up when there is an issue.
Checklist