♻️ [PANA-5982] Make the serialization code more configurable and testable#4185
Conversation
|
✅ Tests 🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage (details) 🔗 Commit SHA: 30f61f6 | Docs | Datadog PR Page | Was this helpful? Give us feedback! |
Bundles Sizes Evolution
🚀 CPU Performance
🧠 Memory Performance
|
b999f89 to
a80cfd8
Compare
7d1e657 to
30f61f6
Compare
| serializeInTransaction(kind, emitRecord, emitStats, scope, (transaction: SerializationTransaction) => { | ||
| const defaultPrivacyLevel = transaction.scope.configuration.defaultPrivacyLevel | ||
|
|
||
| // We are sure that Documents are never ignored, so this function never returns null. | ||
| const node = serializeNode(document, defaultPrivacyLevel, transaction)! | ||
|
|
||
| const record: BrowserFullSnapshotRecord = { | ||
| data: { | ||
| node, | ||
| initialOffset: { | ||
| left: getScrollX(), | ||
| top: getScrollY(), | ||
| }, | ||
| }, | ||
| type: RecordType.FullSnapshot, | ||
| timestamp, | ||
| } | ||
| transaction.add(record) | ||
| }) | ||
| } |
There was a problem hiding this comment.
This is just the old serializeDocument() function, merged into the code block that called it in startFullSnapshots.ts. The boundary between "driver" code and serialization code is now at the serializeInTransaction() call, for both full snapshots and incremental snapshots.
| scope, | ||
| (transaction: SerializationTransaction) => processMutations(timestamp, mutations, transaction) | ||
| ) | ||
| } |
There was a problem hiding this comment.
This entire file is just the old contents of trackMutation.ts, minus the top-level driver. There are no changes in the actual implementation code; I'm just moving things around.
| node = serializeDocument(document, transaction) | ||
| } | ||
| ) | ||
| serializeFullSnapshot(0 as TimeStamp, SerializationKind.INITIAL_FULL_SNAPSHOT, document, emitRecord, noop, scope) |
There was a problem hiding this comment.
Other than the code in startFullSnapshots.ts that became serializeFullSnapshot(), this test helper was the only code that called serializeDocument(). I've reworked it to use startFullSnapshot() instead.
| function recordMutation( | ||
| mutation: () => void, | ||
| options: { | ||
| mutationBeforeTrackingStarts?: () => void | ||
| scope?: RecordingScope | ||
| skipFlush?: boolean | ||
| } = {} | ||
| ): { | ||
| mutationTracker: MutationTracker | ||
| serializedDocument: DocumentNode & SerializedNodeWithId | ||
| } { | ||
| const scope = options.scope || getRecordingScope() | ||
|
|
||
| const serializedDocument = takeFullSnapshotForTesting(scope) | ||
|
|
||
| if (options.mutationBeforeTrackingStarts) { | ||
| options.mutationBeforeTrackingStarts() | ||
| } | ||
|
|
||
| const mutationTracker = trackMutation(document, emitRecordCallback, emitStatsCallback, scope, serializeMutations) | ||
| registerCleanupTask(() => { | ||
| mutationTracker.stop() | ||
| }) | ||
|
|
||
| mutation() | ||
|
|
||
| appendElement('<div></div>', sandbox) | ||
| if (!options.skipFlush) { | ||
| mutationTracker.flush() | ||
| } | ||
|
|
||
| return { mutationTracker, serializedDocument } | ||
| } |
There was a problem hiding this comment.
I wish the diff formatted this change better; it's better just to look at this as a new function. recordMutation() is the core helper that all of the other tests now use. It supports various configuration options for a few tests that do slightly weirder things, but at its core, the flow is:
- It takes a full snapshot.
- It starts mutation tracking, which adds a
MutationObserverto the DOM. From this point, everything that happens will be processed by the mutation serialization code. - It runs the
mutation()callback provided by the test, which changes something in the DOM. - It calls
MutationTracker#flush()to make sure that all of the changes are serialized. - It returns the mutation tracker (to let the test make assertions about the mutations) and the full snapshot (since this is needed to "play back" the recorded mutations and reconstruct the document to make assertions).
| function defaultSerializeFullSnapshotCallback(): SerializeFullSnapshotCallback { | ||
| return isExperimentalFeatureEnabled(ExperimentalFeature.USE_CHANGE_RECORDS) | ||
| ? serializeFullSnapshotAsChange | ||
| : serializeFullSnapshot | ||
| } |
There was a problem hiding this comment.
defaultSerializeMutationsCallback() will follow this exact same pattern as soon as I add serializeMutationsAsChange.ts; look for that in an upcoming PR!
| @@ -0,0 +1,113 @@ | |||
| import type { RecordingScope } from '../recordingScope' | |||
| import { createRecordingScopeForTesting } from '../test/recordingScope.specHelper' | |||
| import { idsAreAssignedForNodeAndAncestors, sortAddedAndMovedNodes } from './serializeMutations' | |||
There was a problem hiding this comment.
Everything in this file has been moved from trackMutation.spec.ts. There are no changes; I just moved it because the code it tests has been moved to serializeMutations.ts.
Motivation
Change records are a new, experimental data format for session replay data that's designed to record the same information in a more compact, more compressible form.
We've already landed support for recording full snapshots as Change records behind an experiment flag. The next step is to introduce support for incremental snapshots, as well. This requires making improvements to a number of different aspects of the recording code. This PR is the fourth in a series that will work toward the goal of supporting incremental snapshots; for the previous PR, see #4165.
This PR focuses on making testing and verification of the serialization code easier by making it simpler to switch between the V1 and Change serializers.
Up to now, the only mechanism we had for switching between the serialization implementations was to change the feature flag. This doesn't cause problems for testing the full snapshot code because that code already cleanly separates the "driver" (
startFullSnapshots.ts) from the serialization code (serializeNode.tsand friends). The feature flag just tells the driver which serialization implementation to use; it doesn't affect the behavior of the serialization implementations themselves. Most tests are written against the serialization implementations directly, so they don't need to manipulate the feature flag. For the few tests that involve the driver, it's enough to simply change the feature flag and start it, because the driver takes a full snapshot immediately at startup.Problem 1: The incremental snapshot code did not provide such a clean separation. The same file,
trackMutation.ts, contains both the driver and the serialization code, and there's not a clean interface between the two.Problem 2: We need a bit more flexibility to test the incremental snapshot code than we needed for full snapshots; to verify that the V1 and Change implementations produce the same results, we need to allow both implementations to run simultaneously so that they can both handle the same sequence of events. This makes relying totally on a feature flag awkward even in the driver; in testing contexts, it would really be much better if we could configure the driver to use one implementation or the other at startup.
Problem 3: Even once we resolve the other problems, the tests for incremental snapshots in
trackMutation.spec.tsneed refactoring. The tests are complex and have multiple steps, and they're currently written in a style which makes it very hard to swap between implementations or to compare one implementation against the other.Changes
To address problem 1: To make it easier to select a specific serialization implementation, with or without the driver, I've made the following changes:
trackMutation.tshas been split into two parts: the code that serializes aMutationRecord[], which now lives inserializeMutations.ts, and the driver, which remains intrackMutation.ts.trackMutation.spec.tshave been split as well: most tests remain intrackMutation.spec.ts, because they test both the serialization logic and the driver, but some tests that involve only serialization helper functions have moved toserializeMutations.spec.ts.To address problem 2: To eliminate the need to manipulate the feature flag to test the different variations of the serialization code, I've made the following changes:
trackMutation.tscan now be configured to use a particular serialization implementation by passing in aSerializeMutationsCallback. If you don't one pass one, the default implementation is picked by thedefaultSerializeMutationsCallback()helper function. (It's trivial now since there's only one, but soon there will be two!)startFullSnapshots.tscan be configured by passing in aSerializeFullSnapshotCallback, with the default selected bydefaultSerializeFullSnapshotCallback()if you don't provide one.startFullSnapshots.tsintoserializeFullSnapshot.tsandserializeFullSnapshotAsChange.ts; the new files have been designed to match the pattern established byserializeMutations.ts. The goal here is to ensure that the code for full snapshots and incremental snapshots follows a similar structure.serializeFullSnapshot.tshas absorbed the contents ofserializeDocument.ts, sinceserializeFullSnapshot.tsis the only non-test caller ofserializeDocument.ts.To address problem 3: To make it easier to run the tests in
trackMutation.spec.tsagainst both serialization algorithms, I've restructured the tests so that each test no longer takes its own full snapshot, creates its own mutation tracker, and handles its own flushing. Instead, all of this functionality has moved into a helper calledrecordMutation(). Every test now just callsrecordMutation(), passing in a callback that actually performs the mutation we're interested in, and then makes assertions against the results. Note that the tests haven't changed; they've just been refactored.Test instructions
These changes are pure refactoring, and should not change the behavior of the code at all except in testing scenarios. So, it should be enough to test that recording still works on staging with the browser SDK extension, using the Live Replay tab.
Checklist