Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

♻️ [RUM-250] introduce a DeflateEncoder #2376

Merged
merged 5 commits into from Aug 22, 2023
Merged

Conversation

BenoitZugmeyer
Copy link
Member

Motivation

This PR introduce a higher level API for the deflate Worker. The goal is to reuse it to compress RUM and Logs data. For now, it is only used for Replay.

This PR also makes sure to leverage this new encoder object to simplify the segment collection logic.

Changes

  • previously, the Segment required an initial record to be instanciated. The motivation behind this was to make sure no empty Segment can be created, but it required to have two different ways to add a record. This PR changes this so the flow of adding a record is easier to follow and factorize. To make sure no Segment are flushed, a runtime assertion has been added.

  • move deflate-related modules in a folder to be collocated

  • introduce a DeflateEncoder and use it in the segmentCollection

Testing

  • Local
  • Staging
  • Unit
  • End to end

I have gone over the contributing documentation.

With this change, we always follow the same codepath when adding a
record to a segment. This will be useful in the next commit to adjust
`addRecord` so it takes a callback parameter.
@BenoitZugmeyer BenoitZugmeyer requested review from a team as code owners August 11, 2023 13:57
@BenoitZugmeyer BenoitZugmeyer marked this pull request as draft August 11, 2023 13:57
@BenoitZugmeyer
Copy link
Member Author

/to-staging

@dd-devflow
Copy link

dd-devflow bot commented Aug 11, 2023

🚂 Branch Integration: starting soon, merge in < 0s

commit 2f2913b48c will soon be integrated into staging-32.

This build is going to start soon! (estimated merge in less than 0s)

you can cancel this operation by commenting your pull request with /to-staging -c!

@dd-devflow
Copy link

dd-devflow bot commented Aug 11, 2023

🚂 Branch Integration: this commit was successfully integrated

Commit 2f2913b48c has been merged into staging-32 in merge commit 3f5189748c.

Check out the triggered pipeline on Gitlab 🦊

Copy link
Member Author

BenoitZugmeyer commented Aug 11, 2023

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@BenoitZugmeyer BenoitZugmeyer marked this pull request as ready for review August 11, 2023 16:13
@@ -8,7 +8,8 @@
"moduleResolution": "node",
"resolveJsonModule": true,
"target": "ES2015",
"types": ["node", "jasmine", "@wdio/globals/types", "ajv"]
"types": ["node", "jasmine", "@wdio/globals/types", "ajv"],
"allowJs": true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ question: is it needed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is, because:

  • e2e tests are importing @datadog/browser-rum/test
  • packages/rum/test/index.ts is importing mockWorker
  • mockWorker now imports string2buf from deflate.js
  • deflate.js is using export statements which are not supported in nodejs for .js files.

So we need tsc to transpile JS files as well. Moving string2buf in a TS file could also work, but I prefered to change the TS config because we might want to keep pako sources as close to the original as possible to ease its upgrade, but this could be challenged of course.

packages/rum/src/domain/deflate/deflateEncoder.spec.ts Outdated Show resolved Hide resolved
Co-authored-by: Bastien Caudan <1331991+bcaudan@users.noreply.github.com>
@BenoitZugmeyer BenoitZugmeyer merged commit 52d20c1 into main Aug 22, 2023
17 checks passed
@BenoitZugmeyer BenoitZugmeyer deleted the benoit/deflate-writer branch August 22, 2023 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants