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-7704] Remove anonymous user feature flag for v6 #3216

Merged
merged 4 commits into from
Jan 3, 2025

Conversation

cy-moi
Copy link
Contributor

@cy-moi cy-moi commented Dec 13, 2024

Motivation

Remove the anonymous id feature flag to prepare for v6 release

Changes

Removed feature flags everywhere
Fixed unit and e2e tests of session storage to include anonymous id

Testing

  • Local
  • Staging
  • Unit
  • End to end

I have gone over the contributing documentation.

@cy-moi cy-moi changed the title [RUM-7704] Remove anonymous user feature flag for v6 💥 [RUM-7704] Remove anonymous user feature flag for v6 Dec 13, 2024
@cy-moi cy-moi marked this pull request as ready for review December 13, 2024 14:16
@cy-moi cy-moi requested a review from a team as a code owner December 13, 2024 14:16
@cy-moi cy-moi force-pushed the congyao/RUM-7704-remove-device-id-ff branch from 7c36d46 to c5d3f2a Compare December 17, 2024 09:43
@codecov-commenter
Copy link

codecov-commenter commented Dec 17, 2024

Codecov Report

Attention: Patch coverage is 95.65217% with 1 line in your changes missing coverage. Please review.

Project coverage is 93.52%. Comparing base (6e392f4) to head (67de507).
Report is 42 commits behind head on v6.

Files with missing lines Patch % Lines
packages/core/src/domain/session/sessionStore.ts 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##               v6    #3216      +/-   ##
==========================================
+ Coverage   93.51%   93.52%   +0.01%     
==========================================
  Files         286      286              
  Lines        7552     7552              
  Branches     1718     1717       -1     
==========================================
+ Hits         7062     7063       +1     
+ Misses        490      489       -1     

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

Copy link

cit-pr-commenter bot commented Dec 17, 2024

Bundles Sizes Evolution

📦 Bundle Name Base Size Local Size 𝚫 𝚫% Status
Rum 153.33 KiB 153.43 KiB 103 B +0.07%
Logs 50.97 KiB 51.14 KiB 174 B +0.33%
Rum Slim 103.30 KiB 103.40 KiB 103 B +0.10%
Worker 25.16 KiB 25.16 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.094 0.067 -0.027
addtiming 0.001 0.001 -0.000
adderror 0.061 0.057 -0.004
startstopsessionreplayrecording 1.111 1.093 -0.018
startview 0.454 0.444 -0.009
logmessage 0.027 0.034 0.007
🧠 Memory Performance
Action Name Base Consumption Memory (bytes) Local Consumption Memory (bytes) 𝚫 (bytes)
addglobalcontext 27.90 KiB 26.88 KiB -1047 B
addaction 51.48 KiB 50.25 KiB -1260 B
addtiming 26.35 KiB 26.07 KiB -289 B
adderror 55.75 KiB 56.69 KiB 965 B
startstopsessionreplayrecording 25.27 KiB 25.17 KiB -100 B
startview 412.26 KiB 413.79 KiB 1.53 KiB
logmessage 55.27 KiB 58.44 KiB 3.16 KiB

🔗 RealWorld

@cy-moi cy-moi force-pushed the congyao/RUM-7704-remove-device-id-ff branch from 815a996 to 73db602 Compare December 23, 2024 17:28
@cy-moi cy-moi force-pushed the congyao/RUM-7704-remove-device-id-ff branch from 73db602 to 8e6e650 Compare December 23, 2024 18:18
packages/core/src/domain/session/sessionManager.spec.ts Outdated Show resolved Hide resolved
@@ -592,6 +592,7 @@ describe('startSessionManager', () => {

describe('tracking consent', () => {
it('expires the session when tracking consent is withdrawn', () => {
spyOn(Math, 'random').and.callFake(() => 0)
Copy link
Member

Choose a reason for hiding this comment

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

💬 suggestion: ‏add a comment to explain why you need this

packages/core/src/domain/session/sessionStore.ts Outdated Show resolved Hide resolved
@@ -126,7 +129,7 @@ export function startSessionStore<TrackingType extends string>(

function synchronizeSession(sessionState: SessionState) {
if (isSessionInExpiredState(sessionState)) {
sessionState = getExpiredSessionState(sessionState)
sessionState = getExpiredSessionState(sessionState, !!configuration.trackAnonymousUser)
Copy link
Member

Choose a reason for hiding this comment

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

🥜 nitpick: ‏ ‏it could be simpler to pass the configuration object to getExpiredState

test/e2e/scenario/sessionStore.scenario.ts Outdated Show resolved Hide resolved
test/e2e/scenario/sessionStore.scenario.ts Outdated Show resolved Hide resolved
@cy-moi cy-moi force-pushed the congyao/RUM-7704-remove-device-id-ff branch from 9d99746 to 67de507 Compare January 3, 2025 10:53
@cy-moi cy-moi merged commit ecb2fb1 into v6 Jan 3, 2025
15 checks passed
@cy-moi cy-moi deleted the congyao/RUM-7704-remove-device-id-ff branch January 3, 2025 13:53
@@ -592,6 +592,7 @@ describe('startSessionManager', () => {

describe('tracking consent', () => {
it('expires the session when tracking consent is withdrawn', () => {
spyOn(Math, 'random').and.callFake(() => 0) // mock random for anonymous uuid generation
Copy link
Member

Choose a reason for hiding this comment

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

💬 suggestion: ‏I don't think the anonymous id value is tested in this test. Maybe remove this line?

@@ -16,8 +17,8 @@ const DURATION = 123456
const PRODUCT_KEY = 'product'
const FIRST_ID = 'first'
const SECOND_ID = 'second'

const EXPIRED_SESSION: SessionState = { isExpired: '1' }
const EXPIRED_SESSION: SessionState = { isExpired: '1', anonymousId: '0' }
Copy link
Member

Choose a reason for hiding this comment

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

💬 suggestion:

Suggested change
const EXPIRED_SESSION: SessionState = { isExpired: '1', anonymousId: '0' }
const EXPIRED_SESSION: SessionState = { isExpired: '1', anonymousId: jasmine.any(String) as string }

@@ -136,6 +142,7 @@ describe('session store', () => {

describe('initialize session', () => {
it('when session not in store, should initialize a new session', () => {
spyOn(Math, 'random').and.callFake(() => 0)
Copy link
Member

Choose a reason for hiding this comment

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

💬 suggestion: ‏remove this line, use jasmine.any(String)

@@ -454,6 +461,7 @@ describe('session store', () => {

describe('reinitialize session', () => {
it('when session not in store, should reinitialize the store', () => {
spyOn(Math, 'random').and.callFake(() => 0)
Copy link
Member

Choose a reason for hiding this comment

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

💬 suggestion: ‏remove this line, use jasmine.any(String)

cy-moi added a commit that referenced this pull request Jan 3, 2025
@cy-moi cy-moi restored the congyao/RUM-7704-remove-device-id-ff branch January 3, 2025 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants