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.

Sign up for GitHub

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

Jump to bottom

ffi: Add support for serializing msgpack map into KV-pair IR format. #467

Merged
merged 17 commits into from
Jul 17, 2024
Merged

ffi: Add support for serializing msgpack map into KV-pair IR format. #467

merged 17 commits into from
Jul 17, 2024

Conversation

LinZhihao-723
Copy link
Member

Description

This PR implements the method to serialize msgpack maps using CLP key-value pair IR format. It includes the following component changes:

  1. The core serialization method, which contains the traversal of a msgpack map.
  2. Updates on CLP IR protocol to add support for the key-value pair format.
  3. Serialization helpers that convert primitive values into encoded CLP IR byte sequences.

With this PR, the ffi should have all the functionality needed to serialize structured log events.

Validation performed

  1. Add new unit tests to ensure the serialization method can successfully serialize msgpack maps that we support. Notice that deserialization cannot be validated until we have the deserializer formally implemented.
  2. Using beta branch code to validate that the serialized data can be successfully deserialized and converted back to the original structured log event records.

@kirkrodrigues kirkrodrigues requested a review from gibber9809 July 3, 2024 01:27
gibber9809
gibber9809 requested changes Jul 3, 2024
View reviewed changes
components/core/src/clp/ffi/ir_stream/Serializer.cpp Outdated Show resolved Hide resolved
components/core/src/clp/ffi/ir_stream/Serializer.cpp Show resolved Hide resolved
components/core/src/clp/ffi/ir_stream/Serializer.cpp Show resolved Hide resolved
components/core/src/clp/ffi/ir_stream/Serializer.cpp Show resolved Hide resolved
components/core/src/clp/ffi/ir_stream/utils.hpp Outdated
(std::is_same_v<encoded_variable_t, clp::ir::eight_byte_encoded_variable_t>
|| std::is_same_v<encoded_variable_t, clp::ir::four_byte_encoded_variable_t>)
);
std::string logtype;
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: could we re-use the same buffer for clp logtype instead of recreating it each time this function is called?

components/core/src/clp/ffi/ir_stream/Serializer.cpp Show resolved Hide resolved
components/core/tests/test-ir_encoding_methods.cpp Outdated Show resolved Hide resolved
kirkrodrigues
kirkrodrigues reviewed Jul 7, 2024
View reviewed changes
components/core/src/clp/ffi/ir_stream/encoding_methods.hpp Outdated Show resolved Hide resolved
components/core/src/clp/ffi/ir_stream/Serializer.cpp Outdated Show resolved Hide resolved
components/core/src/clp/ffi/ir_stream/utils.cpp Outdated Show resolved Hide resolved
components/core/src/clp/ffi/ir_stream/Serializer.cpp Outdated Show resolved Hide resolved
components/core/src/clp/ffi/ir_stream/Serializer.cpp Show resolved Hide resolved
components/core/src/clp/ffi/ir_stream/Serializer.cpp Outdated Show resolved Hide resolved
components/core/src/clp/ffi/ir_stream/Serializer.cpp Show resolved Hide resolved
components/core/src/clp/ffi/ir_stream/Serializer.cpp Outdated Show resolved Hide resolved
components/core/src/clp/ffi/ir_stream/Serializer.cpp Outdated Show resolved Hide resolved
components/core/src/clp/ffi/ir_stream/utils.cpp Outdated Show resolved Hide resolved
kirkrodrigues
kirkrodrigues requested changes Jul 17, 2024
View reviewed changes
Copy link
Member

@kirkrodrigues kirkrodrigues left a comment

Choose a reason for hiding this comment

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

A few more suggestions to polish the code. Do you plan to add support for the unsigned long value as well?

components/core/src/clp/ffi/ir_stream/encoding_methods.hpp Outdated Show resolved Hide resolved
components/core/src/clp/ffi/ir_stream/encoding_methods.hpp Outdated Show resolved Hide resolved
components/core/src/clp/ffi/ir_stream/protocol_constants.hpp Outdated Show resolved Hide resolved
components/core/src/clp/ffi/ir_stream/protocol_constants.hpp Outdated Show resolved Hide resolved
components/core/tests/test-ir_encoding_methods.cpp Outdated Show resolved Hide resolved
components/core/src/clp/ffi/ir_stream/Serializer.cpp Outdated Show resolved Hide resolved
components/core/src/clp/ffi/ir_stream/Serializer.cpp Outdated Show resolved Hide resolved
components/core/src/clp/ffi/ir_stream/Serializer.cpp Outdated Show resolved Hide resolved
components/core/src/clp/ffi/ir_stream/Serializer.cpp Outdated Show resolved Hide resolved
components/core/src/clp/ffi/ir_stream/Serializer.cpp Outdated Show resolved Hide resolved
kirkrodrigues
kirkrodrigues approved these changes Jul 17, 2024
View reviewed changes
Copy link
Member

@kirkrodrigues kirkrodrigues left a comment

Choose a reason for hiding this comment

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

For the PR title, how about:

ffi: Add support for serializing msgpack map into KV-pair IR format.

@LinZhihao-723 LinZhihao-723 changed the title ffi: Add support for serializing msgpack map using key-value pair IR format. ffi: Add support for serializing msgpack map into KV-pair IR format. Jul 17, 2024
@LinZhihao-723 LinZhihao-723 enabled auto-merge (squash) July 17, 2024 06:22
@LinZhihao-723 LinZhihao-723 requested a review from gibber9809 July 17, 2024 06:23
@LinZhihao-723 LinZhihao-723 merged commit eebbf1e into y-scope:main Jul 17, 2024
13 checks passed
jackluo923 pushed a commit to jackluo923/clp that referenced this pull request Dec 4, 2024
@LinZhihao-723
ffi: Add support for serializing msgpack map into KV-pair IR format. (y…
3fd34cb 
…-scope#467)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gibber9809 gibber9809 gibber9809 approved these changes

@kirkrodrigues kirkrodrigues kirkrodrigues approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@LinZhihao-723 @kirkrodrigues @gibber9809