Skip to content
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

👷‍♀️ [RUM-8295] Change anonymous id format to uuid #3306

Merged
merged 3 commits into from
Feb 6, 2025

Conversation

cy-moi
Copy link
Contributor

@cy-moi cy-moi commented Jan 30, 2025

Motivation

Change anonymous id format to uuid so we achieve:

  1. Fewer collisions when >100M device concurrencies
  2. Aligned with other ids and with mobile sdk

Changes

Use generateUUID() for anonymous id generation
Update e2e tests and unit tests to test effectively.

Testing

  • Local
  • Staging
  • Unit
  • End to end

I have gone over the contributing documentation.

@cy-moi
Copy link
Contributor Author

cy-moi commented Jan 30, 2025

/to-staging

@dd-devflow
Copy link
Contributor

dd-devflow bot commented Jan 30, 2025

Devflow running: /to-staging

View all feedbacks in Devflow UI.


2025-01-30 16:38:24 UTC ℹ️ Branch Integration: starting soon, median merge time is 10m45s

Commit 4f471c612f will soon be integrated into staging-05.


2025-01-30 16:52:23 UTC ℹ️ Branch Integration: This commit was successfully integrated

Commit 4f471c612f has been merged into staging-05 in merge commit 233d688db1.

Check out the triggered pipeline on Gitlab 🦊

If you need to revert this integration, you can use the following command: /code revert-integration -b staging-05

dd-mergequeue bot added a commit that referenced this pull request Jan 30, 2025
@cy-moi cy-moi marked this pull request as ready for review January 30, 2025 17:56
@cy-moi cy-moi requested a review from a team as a code owner January 30, 2025 17:56
@@ -103,33 +101,3 @@ describe('session in cookie strategy', () => {
})
})
})
describe('session in cookie strategy when opt-out anonymous user tracking', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ question: ‏ Why removing these tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, this one is wrongly removed. I put them back.

@thomas-lebeau thomas-lebeau self-assigned this Jan 31, 2025
@cy-moi cy-moi force-pushed the congyao/RUM-8295-change-anonymous-id branch from 4f471c6 to 9ddb7cc Compare January 31, 2025 09:35
@codecov-commenter
Copy link

codecov-commenter commented Jan 31, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.96%. Comparing base (b7fcfcb) to head (89ceca7).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3306      +/-   ##
==========================================
- Coverage   92.96%   92.96%   -0.01%     
==========================================
  Files         297      297              
  Lines        7851     7849       -2     
  Branches     1791     1791              
==========================================
- Hits         7299     7297       -2     
  Misses        552      552              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

Bundles Sizes Evolution

📦 Bundle Name Base Size Local Size 𝚫 𝚫% Status
Rum 146.74 KiB 146.68 KiB -68 B -0.05%
Logs 51.19 KiB 51.13 KiB -67 B -0.13%
Rum Slim 105.49 KiB 105.43 KiB -67 B -0.06%
Worker 24.50 KiB 24.50 KiB 0 B 0.00%
🚀 CPU Performance
Action Name Base Average Cpu Time (ms) Local Average Cpu Time (ms) 𝚫
addglobalcontext 0.002 0.002 -0.000
addaction 0.049 0.036 -0.012
addtiming 0.001 0.001 -0.000
adderror 0.067 0.051 -0.016
startstopsessionreplayrecording 0.007 0.012 0.004
startview 0.369 0.378 0.009
logmessage 0.024 0.020 -0.003
🧠 Memory Performance
Action Name Base Consumption Memory (bytes) Local Consumption Memory (bytes) 𝚫 (bytes)
addglobalcontext 26.98 KiB 27.90 KiB 939 B
addaction 56.46 KiB 55.90 KiB -579 B
addtiming 29.48 KiB 31.99 KiB 2.51 KiB
adderror 63.74 KiB 63.30 KiB -458 B
startstopsessionreplayrecording 25.96 KiB 26.54 KiB 592 B
startview 418.79 KiB 421.90 KiB 3.12 KiB
logmessage 63.65 KiB 61.84 KiB -1860 B

🔗 RealWorld

@@ -78,16 +80,6 @@ describe('rum sessions', () => {

expect((await findSessionCookie())?.aid).toEqual(anonymousId)
})

createTest('generated when cookie is cleared')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is removed as renewSession is basically putting the id back manually. So the test is not much meaningful. Plus, we should not restart a session when the cookie cleared by third party.

Comment on lines 59 to 61
await expireSession()
await browser.execute(() => {
window.DD_RUM!.stopSession()
})
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid calling manual expireSession here to test the anonymous id logic fully.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the expireSession helper could be improve to actually expire the session.
I think with playwright we could use await page.clock.runFor(SESSION_TIME_OUT_DELAY)

maybe you can add a not and we can revert this change when we have migrated to playwright fully

@cy-moi cy-moi merged commit 5c8d3d2 into main Feb 6, 2025
19 checks passed
@cy-moi cy-moi deleted the congyao/RUM-8295-change-anonymous-id branch February 6, 2025 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants