⚗️ ✨ [RUMF-1530] send replay metadata as json #2125
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2125 +/- ##
==========================================
+ Coverage 93.56% 93.64% +0.07%
==========================================
Files 179 180 +1
Lines 5985 6011 +26
Branches 1342 1347 +5
==========================================
+ Hits 5600 5629 +29
+ Misses 385 382 -3
... and 2 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
BenoitZugmeyer
force-pushed
the
benoit/send-replay-metadata-as-json
branch
from
8146e37 to
f065346
Compare
BenoitZugmeyer
force-pushed
the
benoit/send-replay-metadata-as-json
branch
from
5ffb0ea to
04c4b04
Compare
packages/rum/src/domain/segmentCollection/buildReplayPayload.spec.ts
Outdated
Show resolved
Hide resolved
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
This commit introduces an experimental feature flag for sending replay payloads as JSON. It restores a few unit and e2e tests, and adjusts the way we read metadata in tests to work in both ways.
| }) | ||
| done() | ||
| const requests = await readSentRequests(1) | ||
| expect(requests[0].segment).toEqual(jasmine.any(Object)) |
There was a problem hiding this comment.
question: this is supposed to be binary data no?
thought: I am surprise it matches the Object predicate?
There was a problem hiding this comment.
readSentRequests is using readReplayPayload, which is a helper to read the FormData that is sent and decode/deserialize it.
There was a problem hiding this comment.
praise: much nicer with async/await than done
| }) | ||
| }) | ||
|
|
||
| describe('without replay_json_payload experimental flag', () => { |
There was a problem hiding this comment.
suggestion(not-blocking): if possible maybe add disableExperimentalFeatures([ExperimentalFeature.REPLAY_JSON_PAYLOAD]) or a reset to make the label of the describe explicit and no relying on default value on the reset of the previous tests
There was a problem hiding this comment.
All tests rely on a default state. This default state needs to be reset at the end of each test, else everything would be impredictable, especially because we are using a randomized test execution order.
| } | ||
| } | ||
|
|
||
| return readJsonBlob((payload.data as FormData).get('event') as Blob) as Promise<BrowserSegmentMetadataAndSegmentSizes> |
There was a problem hiding this comment.
question: why not cast in the resolve function in readJsonBlob?
There was a problem hiding this comment.
You mean, something like this?
function readJsonBlob<T>() {
return new Promise<T>(...)
}The end result is indeed the same, but the cast would be implicit. I'd rather have readJsonBlob(): Promise<unknown>, which provide no guarantee about the returned type, and then let the caller take the responsibility to ducktype it or cast it. But I agree the syntax is a bit more verbose.
There was a problem hiding this comment.
this would not work?
resolve(deserialized as BrowserSegmentMetadataAndSegmentSizes)There was a problem hiding this comment.
readJsonBlob() is used two times with different return types
BenoitZugmeyer
changed the title
Motivation
Send Replay metadata as JSON instead of individual multipart entries to make it easier to extend.
Changes
Reading metadata from Replay payloads will need to be asynchronous, because we are using a
Blobto set the entry content type toapplication/json. Thus, a large part of the PR focuses on preparing the tests for this change.Then, we actually send the metadata as JSON
Finally, we add the
compressed_segment_sizeto avoid needing custom backend code.Testing
I have gone over the contributing documentation.