🎨 [PANA-5260] Consolidate recorder object id tracking code#4049
Conversation
Bundles Sizes Evolution
🚀 CPU Performance
🧠 Memory Performance
|
|
✅ Tests 🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage 🔗 Commit SHA: f63d749 | Docs | Datadog PR Page | Was this helpful? Give us feedback! |
| export interface ItemIds<ItemType, ItemId extends number> { | ||
| clear(): void | ||
| get(this: void, item: ItemType): ItemId | undefined | ||
| getOrInsert(this: void, item: ItemType): ItemId |
There was a problem hiding this comment.
This method was called assign() for the old NodeIds and getIdForEvent() for the old EventIds, but it had the same behavior in both cases. I've renamed it to use the same name as the standard Map method that does essentially the same thing. I think this name makes the behavior a bit clearer than either of the previous names.
| function createIdMap<ItemType, ItemId extends number>(firstId: ItemId): ItemIds<ItemType, ItemId> { | ||
| return createItemIds(() => new Map<ItemType, ItemId>(), firstId) | ||
| } | ||
|
|
||
| function createWeakIdMap<ItemType extends object, ItemId extends number>(firstId: ItemId): ItemIds<ItemType, ItemId> { | ||
| return createItemIds(() => new WeakMap<ItemType, ItemId>(), firstId) | ||
| } |
There was a problem hiding this comment.
We need two variations here because strings cannot be stored in a WeakMap, since they're value types.
gonzalezreal
left a comment
There was a problem hiding this comment.
Looks good!
Just one question: What triggers clearing the ID mappings? (especially for StringIds which, as far as I understand, don't auto-GC)
Motivation
To correlate references to the same object in different records, the recording code needs to assign ids to these objects. Today, we track ids for two kinds of objects:
Nodeobjects.)Eventobjects.)The objects that manage these two kinds of ids are defined in an almost identical way; the only real difference is the naming scheme of the methods.
The new data format will add to the duplication by tracking ids for two new kinds of objects:
stringvalues.)CSSStyleSheetobjects.)The new data format also requires the capability to clear these mappings, so the implementation of the tracking code will get slightly more complicated.
Instead of following the existing pattern and duplicating nearly identical code in four places, let's build a single implementation that can handle id tracking for any kind of object or value.
Changes
This PR replaces the existing
EventIdsandNodeIdsimplementations with simple wrappers around a shared generic type,ItemIds. It also implements the new id tracking facilities that the new data format will need by addingStringIdsandStyleSheetIdswrappers.As the number of id collections grows,
createRecordingScope()is starting to require a large number of function arguments. Since these are simple data structures with very little behavior, there isn't much benefit in using dependency injection for them. So,createRecordingScope()now just creates the id collections internally, simplifying its interface.The
NodeIdscollection was a special case in that it had a unique method,NodeIds#areAssignedForNodeAndAncestors(). This is only called in one place,trackMutations.ts, so I've moved the implementation there as a free function, and moved the tests totrackMutations.spec.ts.Test instructions
This is pure refactoring; there are no functional changes. So, testing that recording continues to work with the browser SDK extension is enough.
Checklist