🎨 [PANA-5053] Separate DOM and virtual attribute serialization

[sethfowler-datadog]

Motivation

The current DOM serialization algorithm produces two kinds of attributes on elements:

1. Real DOM attributes, like foo="bar" in <div foo="bar">.
2. "Virtual" attributes representing metadata about the element. For example, rr_scrollLeft and rr_scrollTop are used to capture information about the initial scroll position of an element.

Many of these virtual attributes are primarily of interest in full snapshots; later, when the values change incrementally, we represent them using dedicated records. For example, later changes to rr_scrollLeft and rr_scrollTop are represented as BrowserIncrementalSnapshotRecord records with IncrementalSource.Scroll. (Speculating on the reasoning for the different representation in full snapshots, I'd guess this was done because there's no easy way to bundle multiple changes into a single, atomic full snapshot record except to include them in the DOM snapshot itself. Incremental snapshots can bundle together many changes at once, so they don't have the same issue.)

The new DOM serialization algorithm doesn't have a special record type for full snapshots; everything uses a representation comparable to our current incremental snapshots. For that reason, the new algorithm doesn't need these virtual attributes; it makes more sense to encode these values in the same way we do today for incremental snapshots.

To make it easier to share code between the current DOM serialization algorithm and the new one, let's separate the code that serializes real DOM attributes from the code that serializes virtual attributes.

Changes

This PR:

• Renames safeAttrs to attrs in serializeAttributes() as a minor opportunistic refactor. I'm not a big fan of the name safeAttrs since it implies that everything in safeAttrs satisfies our privacy rules and is safe to record, but in fact that's not true until the end of the function.
• Splits serializeAttributes() into two functions, serializeDOMAttributes() and serializeVirtualAttributes(). (serializeAttributes() survives as a trivial wrapper around these two functions.)
• Tweaks the types in a few places to more accurately reflect the kinds of values that serializeDOMAttributes() can now return.
• Removes the call to getValidTagName() from both functions since it contains a regular expression which it would be wasteful to invoke in two places. getValidTagName() doesn't really provide any benefit since we are only comparing the tag name with exact known strings in this code; we can just use Element#tagName directly.
• Updates the tests for serializeAttributes() to call serializeDOMAttributes() or serializeVirtualAttributes(), as appropriate. (I wrote the tests with this in mind, so making the switch is easy.)

Checklist

• Tested locally
• Tested on staging
• Added unit tests for this change.
• Added e2e/integration tests for this change.

[yannickadam]

What worries me is the removal of getValidTagName. I know it's probably a stretch nowadays, but if one of our customer still serves their application using application/xhtml+xml, wouldn't the tag names be lower case?

[sethfowler-datadog]

What worries me is the removal of getValidTagName. I know it's probably a stretch nowadays, but if one of our customer still serves their application using application/xhtml+xml, wouldn't the tag names be lower case?

Yes, indeed that is still possible. (I double checked the behavior with this site, which I am linking for posterity because it's incredibly hard to find any site that's served using application/xhtml+xml. 😄 )

We are unfortunately already checking only for uppercase tag names in many places in the code, so I don't think the SDK will behave correctly on XHTML sites today, but I think it's worth keeping the things that work today working. We can do that without the full weight of getValidTagName(), though. I'll update the PR.

[cit-pr-commenter]

Bundles Sizes Evolution

📦 Bundle Name	Base Size	Local Size	𝚫	𝚫%	Status
Rum	164.31 KiB	164.31 KiB	0 B	0.00%	✅
Rum Profiler	4.84 KiB	4.84 KiB	0 B	0.00%	✅
Rum Recorder	19.74 KiB	19.88 KiB	+142 B	+0.70%	✅
Logs	56.12 KiB	56.12 KiB	0 B	0.00%	✅
Flagging	944 B	944 B	0 B	0.00%	✅
Rum Slim	121.58 KiB	121.58 KiB	0 B	0.00%	✅
Worker	23.63 KiB	23.63 KiB	0 B	0.00%	✅

🚀 CPU Performance

Action Name	Base CPU Time (ms)	Local CPU Time (ms)	𝚫%
addglobalcontext	N/A	0.0102	N/A
addaction	N/A	0.0224	N/A
adderror	N/A	0.0406	N/A
addtiming	N/A	0.0066	N/A
startview	N/A	0.0052	N/A
startstopsessionreplayrecording	N/A	0.0018	N/A
logmessage	N/A	0.0292	N/A

🧠 Memory Performance

Action Name	Base Memory Consumption	Local Memory Consumption	𝚫
addglobalcontext	N/A	24.98 KiB	N/A
addaction	N/A	47.97 KiB	N/A
addtiming	N/A	25.17 KiB	N/A
adderror	N/A	54.72 KiB	N/A
startstopsessionreplayrecording	N/A	24.49 KiB	N/A
startview	N/A	430.21 KiB	N/A
logmessage	N/A	42.48 KiB	N/A

🔗 RealWorld

[datadog-official]

⚠️ Tests

⚠️ Warnings

❄️ 1 New flaky test detected

cookie getCurrentSite returns the eTLD+1 for foo.bar.baz.example.com from Safari 12.1.2 (Mac OS 10.14.6) (Datadog) (Fix with Cursor)

Expected 'foo.bar.baz.example.com' to be 'example.com'.
<Jasmine>
webpack:///packages/core/src/browser/cookie.spec.ts:24:61 <- /tmp/_karma_webpack_596170/commons.js:48598:108
<Jasmine>

ℹ️ Info

🧪 All tests passed

🎯 Code Coverage
• Patch Coverage: 100.00%
• Overall Coverage: 92.66% (+0.01%)

View detailed report

<sub>This comment will be updated automatically if new data arrives.
* Fix with Cursor requires Datadog plugin ≥v2.17.0
🔗 Commit SHA: 5f2f0df | Docs | Datadog PR Page | Was this helpful? Give us feedback!</sub>