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

RUMM-2177: Use TLV format for data storage #931

Merged
merged 1 commit into from
May 16, 2022

Conversation

0xnm
Copy link
Contributor

@0xnm 0xnm commented May 12, 2022

What does this PR do?

This change aligns the binary format used to write data to the disk with format implemented in DataDog/dd-sdk-ios#841.

For every event received we are going to write meta + event in the TLV (Type-Length-Value) format, where the format itself for each block (meta or event) has the following structure:

    +-  2 bytes -+-   4 bytes  -+- n bytes  -|
   |  block type | data size (n) |    data   |
    +------------+---------------+-----------+

It is safe to merge it without cleaning existing data, because this PR targets SDK v2 branch, where the change later will be done to use SDK instance ID in the feature folder naming, so data in the SDK v2 read/write paths won't overlap with data from existing SDK (but during the current development existing data should be erased from emulator).

Tests are verified against the latest testable commit.

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • Make sure you discussed the feature or bugfix with the maintaining team in an Issue
  • Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)

@0xnm 0xnm marked this pull request as ready for review May 12, 2022 14:52
@0xnm 0xnm requested a review from a team as a code owner May 12, 2022 14:52
@0xnm 0xnm requested a review from maxep May 12, 2022 14:52
Copy link
Collaborator

@mariusc83 mariusc83 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@maxep maxep left a comment

Choose a reason for hiding this comment

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

Nice work here!!

I think we could benefit from adding more flexibility on reading phase. We discussed that we should expect a pair of valid meta+event data but that would be when metadata is present. As we don't have yet needs for metadata, I dont think we should add it now. On iOS, we currently support stream with only events.

In short, when reading a block we would have these conditions:

  1. If we encounter an event, keep it
  2. If we encounter metadata, read next block and expect an event. Then we can decode both.

Copy link
Member

@maxep maxep left a comment

Choose a reason for hiding this comment

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

👍

@0xnm 0xnm merged commit f293a75 into feature/sdkv2 May 16, 2022
@0xnm 0xnm deleted the nogorodnikov/rumm-2177/use-tlv-format branch May 16, 2022 09:51
@xgouchet xgouchet added this to the 1.16.0 milestone Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants