♻️ [RUM-8123] Introduce a hook to assemble events #3289

amortemousque · 2025-01-16T19:47:18Z

Motivation

The overall goal is to create a Modular SDK to simplify contributions and enhance extensibility.

This PR introduces a hook system, initially used to decouple event assembly. Each collection module can now independently handle the assembly of its own properties.

Changes

Introduce a Hooks system
Use the Assemble hook in viewCollection to manage the assembly of view properties
Use the Assemble hook in urlContext to manage the assembly of url properties

Testing

Local
Staging
Unit
End to end

I have gone over the contributing documentation.

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

codecov-commenter · 2025-01-16T19:54:59Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.67%. Comparing base (c1b94b4) to head (0eb4e78).
Report is 9 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3289      +/-   ##
==========================================
- Coverage   93.68%   93.67%   -0.02%     
==========================================
  Files         288      289       +1     
  Lines        7617     7633      +16     
  Branches     1739     1741       +2     
==========================================
+ Hits         7136     7150      +14     
- Misses        481      483       +2

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

cit-pr-commenter · 2025-01-17T13:53:40Z

Bundles Sizes Evolution

📦 Bundle Name	Base Size	Local Size	𝚫	𝚫%	Status
Rum	145.66 KiB	146.04 KiB	392 B	+0.26%	✅
Logs	51.09 KiB	51.09 KiB	0 B	0.00%	✅
Rum Slim	104.45 KiB	104.82 KiB	377 B	+0.35%	✅
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.062	0.027
addtiming	0.001	0.001	-0.000
adderror	0.054	0.106	0.052
startstopsessionreplayrecording	0.012	0.010	-0.002
startview	0.606	0.386	-0.220
logmessage	0.026	0.028	0.002

🧠 Memory Performance

Action Name	Base Consumption Memory (bytes)	Local Consumption Memory (bytes)	𝚫 (bytes)
addglobalcontext	26.92 KiB	27.68 KiB	779 B
addaction	54.10 KiB	55.24 KiB	1.14 KiB
addtiming	26.43 KiB	24.97 KiB	-1500 B
adderror	60.58 KiB	56.94 KiB	-3730 B
startstopsessionreplayrecording	25.60 KiB	25.43 KiB	-169 B
startview	413.60 KiB	419.60 KiB	5.99 KiB
logmessage	58.94 KiB	57.69 KiB	-1281 B

🔗 RealWorld

nulrich · 2025-01-22T15:10:00Z

packages/rum-core/src/hooks.ts

+    ): ReturnType<HookCallbackMap[K]> {
+      const hookCallbacks = callbacks[hookName] || []
+      const results = hookCallbacks.map((callback) => callback(param))
+      return combine(...(results as [object, object])) as ReturnType<HookCallbackMap[K]>


💬 suggestion: ‏The result looks nice. I'm just a bit concerned about debugging the Assembly Hook for overridden props if we connect many sources to it because we can't easily know the order of declarations. Maybe we could warn user when a prop is being overridden?

Yeah, I remember we talked about this a bit too, @amortemousque. What do you think of the idea of requiring all hooks to be registered in a single place and specifying them with an array that makes the order clear?

It doesn't look like you actually use the unregister callback returned from register() anywhere, but if there's a need to dynamically add and remove these hooks individually, an alternative approach that might work would be to support enabling and disabling them. That way, they'd keep their initially-defined order, but you'd still be able to turn them on and off when needed.

I agree! We discussed this earlier with @sethfowler-datadog, and I experimented with centralized registration. However, this approach adds some boilerplate and coupling, as modules would need to expose their hooks. Self-registration offers better decoupling.

In my view, the assemble hook will be used by contexts and collections, which are explicitly called in startRum. This makes the order relatively easy to visualize. For now, I suggest waiting to see if this becomes an issue before making improvements. Wdyt?

For now, I suggest waiting to see if this becomes an issue before making improvements. Wdyt?

That'd be fine with me, if you prefer to take that direction!

... this approach adds some boilerplate and coupling, as modules would need to expose their hooks. Self-registration offers better decoupling.

FWIW, my perspective is that centralized registration does not add any additional coupling; it just turns implicit dependencies, which are hard to reason about, into explicit dependencies, which are easier to reason about. Another way to think about it is that this isn't coupling at all, but composition: we start with small pieces of functionality that can be reasoned about independently, and compose them together in an explicit way to form larger pieces of functionality. You end up with a hierarchical structure, which is nice for all the reasons that structured programming in general is nice.

That said, programming is all about tradeoffs, and there are pros and cons to every solution; I'm totally fine with a dynamic registration approach if you prefer it. As you say, we can always revisit the issue later.

sethfowler-datadog

I love the direction this takes us in!

sethfowler-datadog · 2025-01-22T16:28:46Z

packages/rum-core/src/hooks.ts

+    ): ReturnType<HookCallbackMap[K]> {
+      const hookCallbacks = callbacks[hookName] || []
+      const results = hookCallbacks.map((callback) => callback(param))
+      return combine(...(results as [object, object])) as ReturnType<HookCallbackMap[K]>


Yeah, I remember we talked about this a bit too, @amortemousque. What do you think of the idea of requiring all hooks to be registered in a single place and specifying them with an array that makes the order clear?

It doesn't look like you actually use the unregister callback returned from register() anywhere, but if there's a need to dynamically add and remove these hooks individually, an alternative approach that might work would be to support enabling and disabling them. That way, they'd keep their initially-defined order, but you'd still be able to turn them on and off when needed.

packages/rum-core/src/hooks.ts

BenoitZugmeyer · 2025-01-21T08:58:22Z

packages/rum-core/src/hooks.ts

+}
+export type Hooks = ReturnType<typeof startHooks>
+
+export function startHooks() {


💬 suggestion: ‏I would rename it createHooks as it doesn't start anything, it just creates a data structure

BenoitZugmeyer · 2025-01-23T12:48:51Z

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

+          service: configuration.service,
+          version: configuration.version,


💬 suggestion: ‏it might be less confusing / more explicit to have service and version declared in a single place in trackViews.ts:

const service = viewOptions?.service || configuration.service const version = viewOptions?.version || configuration.version

so we can remove those two lines from assembly

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

packages/rum-core/src/hooks.ts

BenoitZugmeyer · 2025-01-23T12:57:13Z

packages/rum-core/src/domain/view/viewCollection.ts

@@ -33,11 +38,27 @@ export function startViewCollection(
      processViewUpdate(view, configuration, featureFlagContexts, recorderApi, pageStateHistory)
    )
  )
+
+  hooks.register(HookNames.Assemble, ({ startTime, eventType }): PartialRumEvent | undefined => {


🥜 nitpick: ‏you can remove the return type

Suggested change

hooks.register(HookNames.Assemble, ({ startTime, eventType }): PartialRumEvent | undefined => {

hooks.register(HookNames.Assemble, ({ startTime, eventType }) => {

I don't know why but it is required for the typechecking to work properly.

Weird. Do you have an example where there is an issue with the return type but not without the return type?

BenoitZugmeyer · 2025-01-23T12:57:36Z

packages/rum-core/src/domain/contexts/urlContexts.ts

@@ -69,6 +72,18 @@ export function startUrlContexts(
    }
  }

+  hooks.register(HookNames.Assemble, ({ startTime, eventType }): PartialRumEvent | undefined => {


🥜 nitpick: ‏same here

Suggested change

hooks.register(HookNames.Assemble, ({ startTime, eventType }): PartialRumEvent | undefined => {

hooks.register(HookNames.Assemble, ({ startTime, eventType }) => {

amortemousque requested a review from a team as a code owner January 16, 2025 19:47

amortemousque commented Jan 16, 2025

View reviewed changes

packages/rum-core/src/domain/assembly.ts Outdated Show resolved Hide resolved

amortemousque marked this pull request as draft January 16, 2025 19:53

Add hooks

675aaab

amortemousque force-pushed the aymeric/introduce-hook-system branch 4 times, most recently from d66a0b9 to 1192e1d Compare January 21, 2025 17:09

Use assembly hooks for view and url context

222c207

amortemousque force-pushed the aymeric/introduce-hook-system branch from 1192e1d to 222c207 Compare January 22, 2025 09:23

Fix typescript compat

c70139a

amortemousque force-pushed the aymeric/introduce-hook-system branch from e1cef02 to c70139a Compare January 22, 2025 10:41

amortemousque marked this pull request as ready for review January 22, 2025 13:23

Fix typo

cb68523

nulrich approved these changes Jan 22, 2025

View reviewed changes

sethfowler-datadog reviewed Jan 22, 2025

View reviewed changes

Merge branch 'main' into aymeric/introduce-hook-system

8bcec50

BenoitZugmeyer reviewed Jan 23, 2025

View reviewed changes

amortemousque added 2 commits January 23, 2025 14:43

Rename and remove unnecessary code

b7d8fb0

Set service and version in startViews

0eb4e78

amortemousque force-pushed the aymeric/introduce-hook-system branch from 02f9fb2 to 0eb4e78 Compare January 23, 2025 19:13

BenoitZugmeyer approved these changes Jan 24, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

♻️ [RUM-8123] Introduce a hook to assemble events #3289

♻️ [RUM-8123] Introduce a hook to assemble events #3289

amortemousque commented Jan 16, 2025 •

edited

Loading

codecov-commenter commented Jan 16, 2025 •

edited

Loading

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

edited

Loading

nulrich Jan 22, 2025

sethfowler-datadog Jan 22, 2025

amortemousque Jan 22, 2025

sethfowler-datadog Jan 23, 2025

sethfowler-datadog left a comment

sethfowler-datadog Jan 22, 2025

BenoitZugmeyer Jan 21, 2025 •

edited

Loading

BenoitZugmeyer Jan 23, 2025

BenoitZugmeyer Jan 23, 2025

amortemousque Jan 23, 2025

BenoitZugmeyer Jan 23, 2025

BenoitZugmeyer Jan 23, 2025

		service: configuration.service,
		version: configuration.version,

	hooks.register(HookNames.Assemble, ({ startTime, eventType }): PartialRumEvent \| undefined => {
	hooks.register(HookNames.Assemble, ({ startTime, eventType }) => {

♻️ [RUM-8123] Introduce a hook to assemble events #3289

Are you sure you want to change the base?

♻️ [RUM-8123] Introduce a hook to assemble events #3289

Conversation

amortemousque commented Jan 16, 2025 • edited Loading

Motivation

Changes

Testing

codecov-commenter commented Jan 16, 2025 • edited Loading

Codecov Report

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

Bundles Sizes Evolution

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

sethfowler-datadog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenoitZugmeyer Jan 21, 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

amortemousque commented Jan 16, 2025 •

edited

Loading

codecov-commenter commented Jan 16, 2025 •

edited

Loading

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

edited

Loading

BenoitZugmeyer Jan 21, 2025 •

edited

Loading