Skip to content

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

Merged
0xnm merged 1 commit into
feature/sdkv2from
nogorodnikov/rumm-2177/use-tlv-format
May 16, 2022
Merged

RUMM-2177: Use TLV format for data storage#931
0xnm merged 1 commit into
feature/sdkv2from
nogorodnikov/rumm-2177/use-tlv-format

Conversation

@0xnm

@0xnm 0xnm commented May 12, 2022

Copy link
Copy Markdown
Member

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

@mariusc83 mariusc83 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@maxep maxep left a comment

Copy link
Copy Markdown
Member

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.

@maxep maxep left a comment

Copy link
Copy Markdown
Member

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.

4 participants