✨ [RUM-7019] Add setAccount API and account in RUM events #3242

nulrich · 2024-12-24T16:49:34Z

Motivation

Adding a standard way to set an account.

Changes

Adding public API in RUM and LOGS to set Account context.
Enforce the Account id in setAccount.
Assembly don't send account to intake if the id property is missing.

Testing

Local
Staging
Unit
End to end

I have gone over the contributing documentation.

codecov-commenter · 2024-12-24T16:51:13Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.71%. Comparing base (9fbe626) to head (1e33c1d).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3242      +/-   ##
==========================================
+ Coverage   93.68%   93.71%   +0.02%     
==========================================
  Files         288      290       +2     
  Lines        7617     7651      +34     
  Branches     1739     1745       +6     
==========================================
+ Hits         7136     7170      +34     
  Misses        481      481

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

…count

cit-pr-commenter · 2025-01-07T14:56:09Z

Bundles Sizes Evolution

📦 Bundle Name	Base Size	Local Size	𝚫	𝚫%	Status
Rum	145.66 KiB	146.24 KiB	595 B	+0.40%	✅
Logs	51.09 KiB	51.63 KiB	561 B	+1.07%	✅
Rum Slim	104.45 KiB	105.00 KiB	562 B	+0.53%	✅
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.035	0.046	0.012
addtiming	0.001	0.001	0.000
adderror	0.044	0.072	0.028
startstopsessionreplayrecording	0.007	0.019	0.011
startview	0.462	0.538	0.077
logmessage	0.020	0.027	0.008

🧠 Memory Performance

Action Name	Base Consumption Memory (bytes)	Local Consumption Memory (bytes)	𝚫 (bytes)
addglobalcontext	27.57 KiB	27.07 KiB	-515 B
addaction	56.55 KiB	55.21 KiB	-1369 B
addtiming	25.51 KiB	26.31 KiB	816 B
adderror	59.36 KiB	59.35 KiB	-11 B
startstopsessionreplayrecording	26.77 KiB	25.85 KiB	-937 B
startview	416.06 KiB	419.91 KiB	3.85 KiB
logmessage	63.47 KiB	60.18 KiB	-3372 B

🔗 RealWorld

nulrich · 2025-01-07T15:58:37Z

/to-staging

dd-devflow · 2025-01-07T15:58:49Z

Devflow running: `/to-staging`

View all feedbacks in Devflow UI.

2025-01-07 15:58:49 UTC ℹ️ Branch Integration: starting soon, median merge time is 11m40s

Commit b9b91aa4ff will soon be integrated into staging-02.

2025-01-07 15:59:21 UTC 🚨 Branch Integration: This merge request has conflicts

The source pull request has conflicts with the base branch. Please resolve them first.

…count

sethfowler-datadog

Some minor nits, but everything looks great!

packages/core/src/domain/context/contextUtils.ts

sethfowler-datadog · 2025-01-08T12:11:25Z

packages/core/src/domain/account/account.ts

+import type { Context } from '../../tools/serialisation/context'
+import { sanitizeContext } from '../context/contextUtils'
+
+export function sanitizeAccount(newAccount: Context) {


The return value of this function is not just a Context, but an Account, right? It might make sense to update the function signature to capture that.

sanitizeAccount and sanitizeUser are manipulating Context and are used by Context Manager functions that expect a Context:

accountContextManager.setContext(sanitizeAccount(newUser))

If I update the signature I will have to cast the result. But maybe I could rename to sanitizeAccountContext ?

packages/logs/src/boot/logsPublicApi.spec.ts

packages/logs/src/boot/logsPublicApi.ts

packages/logs/src/domain/assembly.ts

packages/rum-core/src/boot/rumPublicApi.ts

sethfowler-datadog · 2025-01-08T12:35:52Z

packages/rum-core/src/boot/rumPublicApi.ts

+    getAccount: monitor(() => accountContextManager.getContext()),
+
+    setAccountProperty: monitor((key, property) => {
+      const sanitizedProperty = sanitizeAccount({ [key]: property })[key]


It's unfortunate that this logic has to be repeated twice. It'd be nice if the context manager itself could be configured with a sanitizer, and it'd just handle the work internally. Not a blocker for this PR, but might be nice to consider as a followup.

sethfowler-datadog · 2025-01-08T12:47:29Z

packages/rum-core/src/domain/assembly.ts

+        if (!isEmptyObject(commonContext.account)) {
+          if (commonContext.account.id) {
+            ;(serverRumEvent.account as Mutable<RumEvent['account']>) = commonContext.account as Account &
+              Context & { id: string }


I'm guessing the as Account type assertion is losing the type narrowing you just did with the if (commonContext.account.id) check, and that's why you need & { id: string }. Out of curiosity, does the assignment type check if you just get rid of the whole type assertion?

The type of commonContext.account is Account so even with the narrowing it is still "just" an Account and we must enforce { id: string }.

But because there's overlap between Account and Context, I could do:

if (commonContext.account.id) { ;(serverRumEvent.account as Mutable<RumEvent['account']>) = commonContext.account as { id: string } } else {

packages/core/src/domain/account/account.types.ts

amortemousque · 2025-01-08T17:05:11Z

packages/core/src/domain/account/account.types.ts

@@ -0,0 +1,5 @@
+export interface Account {
+  id?: string | undefined


❓ question: Shouldn't the id be required?‏

In theory yes 😅 But the issue is that Context Manager is manipulating Context so we loose the original type when passing the User or Account object. Until now it was fine because both Context and User have no required properties.

I could require the id property but we would have to cast the context, and maybe getting an account without an id.

export function buildCommonContext( globalContextManager: ContextManager, userContextManager: ContextManager, acccountContextManager: ContextManager ): CommonContext { return { view: { referrer: document.referrer, url: window.location.href, }, context: globalContextManager.getContext(), user: userContextManager.getContext(), account: acccountContextManager.getContext() as Account, } }

That's why I preferred not requiring the Id and checking it in the assembly. Hope it makes sense.

Following @BenoitZugmeyer comment, we now use Context type in the CommonContext for account property. So now the Account type have a required id property.

amortemousque · 2025-01-08T17:14:39Z

packages/rum-core/src/domain/assembly.ts

+            ;(serverRumEvent.account as Mutable<RumEvent['account']>) = commonContext.account as Account &
+              Context & { id: string }
+          } else {
+            display.warn("The account object is missing the 'id' property, it will not be sent to the intake.")


💬 suggestion: ‏We likely don’t want to print a warning every time an event is assembled. Wouldn’t this check be more appropriate in sanitizeContext or checkContext?

amortemousque · 2025-01-08T21:32:22Z

packages/rum-core/src/boot/rumPublicApi.ts

+   * @param property Value of the property
+   *
+   */
+  setAccountProperty: (key: any, property: any) => void


❓ question: key: string be more accurate? I know we also have any for user API but might worth reconsider.‏

BenoitZugmeyer · 2025-01-13T16:36:40Z

packages/logs/src/boot/logsPublicApi.ts

+  /**
+   * Set account information to all events, stored in `@account`
+   */
+  setAccount: (newAccount: Account & { id: string }) => void


💬 suggestion: ‏maybe using a proper type here would be nicer for the user

Suggested change

setAccount: (newAccount: Account & { id: string }) => void

setAccount: (newAccount: Account) => void

I understand that you don't want account.id to be required in the type we use internally, since we cannot enforce it. I think it's fine to use Context instead of Account internally (see this patch).

💯 and we enforce the type just after in the assembly anyway. We should though how the context management could handle sanitization like @sethfowler-datadog proposed, but also typing.

…count

datadog-datadog-prod-us1 · 2025-01-17T11:05:51Z

Datadog Report

Branch report: nicolas.ulrich/set-account
Commit report: 1e33c1d
Test service: browser-sdk

❌ 16 Failed (0 Known Flaky), 17373 Passed, 345 Skipped, 1m 53.93s Total Time
❄️ 1 New Flaky

❌ Failed Tests (16)

This report shows up to 5 failed tests.

report observable should compute stack for intervention - Chrome 63.0.3239.84 (Windows 10) - Details

Expand for error

 TypeError: Cannot read property 'unsubscribe' of undefined
     at UserContext.afterEach (webpack:///packages/core/src/domain/report/reportObservable.spec.ts:27:25 <- reportObservable.spec.3761986749.js:1311:29)
     at <Jasmine>

report observable should notify csp_violation - Chrome 63.0.3239.84 (Windows 10) - Details

Expand for error

 TypeError: Cannot read property 'unsubscribe' of undefined
     at UserContext.afterEach (webpack:///packages/core/src/domain/report/reportObservable.spec.ts:27:25 <- reportObservable.spec.3761986749.js:1311:29)
     at <Jasmine>

report observable should notify deprecation reports - Chrome 63.0.3239.84 (Windows 10) - Details

Expand for error

 TypeError: Cannot read property 'unsubscribe' of undefined
     at UserContext.afterEach (webpack:///packages/core/src/domain/report/reportObservable.spec.ts:27:25 <- reportObservable.spec.3761986749.js:1311:29)
     at <Jasmine>

report observable should notify intervention reports - Chrome 63.0.3239.84 (Windows 10) - Details

Expand for error

 TypeError: Cannot read property 'unsubscribe' of undefined
     at UserContext.afterEach (webpack:///packages/core/src/domain/report/reportObservable.spec.ts:27:25 <- reportObservable.spec.3761986749.js:1311:29)
     at <Jasmine>

report observable should compute stack for intervention - Firefox 67.0 (Windows 10) - Details

Expand for error

 TypeError: consoleSubscription is undefined in reportObservable.spec.3761986749.js (line 1311)
 @webpack:///packages/core/src/domain/report/reportObservable.spec.ts:27:5 <- reportObservable.spec.3761986749.js:1311:9
 <Jasmine>

New Flaky Tests (1)

collect a rage click - action collection - Last Failure

Expand for error

 Expected 3 to be 1.      
       actual expected
       
       31

nulrich force-pushed the nicolas.ulrich/set-account branch from fd6582e to afda0b9 Compare January 6, 2025 17:25

Add setAccount API

4c268b7

nulrich force-pushed the nicolas.ulrich/set-account branch from afda0b9 to 4c268b7 Compare January 7, 2025 09:20

nulrich added 2 commits January 7, 2025 11:18

Merge remote-tracking branch 'origin/main' into nicolas.ulrich/set-ac…

eaaf40c

…count

Update rum-events-format

b9b91aa

Merge remote-tracking branch 'origin/main' into nicolas.ulrich/set-ac…

b78a3ef

…count

nulrich changed the title ~~Add setAccount API~~ ✨ [RUM-7019] Add setAccount API and account in RUM events Jan 7, 2025

nulrich marked this pull request as ready for review January 7, 2025 16:38

nulrich requested a review from a team as a code owner January 7, 2025 16:38

sethfowler-datadog approved these changes Jan 8, 2025

View reviewed changes

amortemousque reviewed Jan 8, 2025

View reviewed changes

nulrich added 4 commits January 9, 2025 11:25

Clean comments and wording

a93cf36

Update checkContext to avoid as Context

7aa6a7d

Clean Account tests

967bcef

Move Account in account.ts

844b2f1

BenoitZugmeyer reviewed Jan 13, 2025

View reviewed changes

nulrich added 3 commits January 14, 2025 17:41

Use Context type for Account in CommonContext

7ca18e7

Merge remote-tracking branch 'origin/main' into nicolas.ulrich/set-ac…

7abdf3a

…count

Enforce string type for Account context keys

1e33c1d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ [RUM-7019] Add setAccount API and account in RUM events #3242

✨ [RUM-7019] Add setAccount API and account in RUM events #3242

nulrich commented Dec 24, 2024 •

edited

Loading

codecov-commenter commented Dec 24, 2024 •

edited

Loading

cit-pr-commenter bot commented Jan 7, 2025 •

edited

Loading

nulrich commented Jan 7, 2025

dd-devflow bot commented Jan 7, 2025 •

edited

Loading

sethfowler-datadog left a comment

sethfowler-datadog Jan 8, 2025

nulrich Jan 9, 2025

sethfowler-datadog Jan 8, 2025

sethfowler-datadog Jan 8, 2025

nulrich Jan 9, 2025 •

edited

Loading

amortemousque Jan 8, 2025

nulrich Jan 9, 2025

nulrich Jan 14, 2025

amortemousque Jan 8, 2025

amortemousque Jan 8, 2025

BenoitZugmeyer Jan 13, 2025

nulrich Jan 14, 2025

datadog-datadog-prod-us1 bot commented Jan 17, 2025 •

edited

Loading

		@@ -0,0 +1,5 @@
		export interface Account {
		id?: string \| undefined

	setAccount: (newAccount: Account & { id: string }) => void
	setAccount: (newAccount: Account) => void

✨ [RUM-7019] Add setAccount API and account in RUM events #3242

Are you sure you want to change the base?

✨ [RUM-7019] Add setAccount API and account in RUM events #3242

Conversation

nulrich commented Dec 24, 2024 • edited Loading

Motivation

Changes

Testing

codecov-commenter commented Dec 24, 2024 • edited Loading

Codecov Report

cit-pr-commenter bot commented Jan 7, 2025 • edited Loading

Bundles Sizes Evolution

nulrich commented Jan 7, 2025

dd-devflow bot commented Jan 7, 2025 • edited Loading

Devflow running: /to-staging

sethfowler-datadog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nulrich Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

datadog-datadog-prod-us1 bot commented Jan 17, 2025 • edited Loading

Datadog Report

❌ Failed Tests (16)

New Flaky Tests (1)

nulrich commented Dec 24, 2024 •

edited

Loading

codecov-commenter commented Dec 24, 2024 •

edited

Loading

cit-pr-commenter bot commented Jan 7, 2025 •

edited

Loading

dd-devflow bot commented Jan 7, 2025 •

edited

Loading

Devflow running: `/to-staging`

nulrich Jan 9, 2025 •

edited

Loading

datadog-datadog-prod-us1 bot commented Jan 17, 2025 •

edited

Loading