-
Notifications
You must be signed in to change notification settings - Fork 11
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
Fix meta struct serialization. #927
base: main
Are you sure you want to change the base?
Conversation
c74fc0b
to
2beef4c
Compare
* Now 'meta_struct' is defined as HashMap<T, Bytes> so it holds a a slice from the buffer rather than copying the bytes in anther container. * Bytes implements serialization so it's always serialized by serde as a byte array.
2beef4c
to
1c85b90
Compare
BenchmarksComparisonBenchmark execution time: 2025-03-14 12:57:46 Comparing candidate commit 1c85b90 in PR branch Found 0 performance improvements and 2 performance regressions! Performance is the same for 50 metrics, 2 unstable metrics. scenario:credit_card/is_card_number/378282246310005
CandidateCandidate benchmark detailsGroup 1
Group 2
Group 3
Group 4
Group 5
Group 6
Group 7
Group 8
Group 9
Group 10
Group 11
Group 12
Group 13
BaselineOmitted due to size. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #927 +/- ##
==========================================
- Coverage 72.62% 72.59% -0.03%
==========================================
Files 334 334
Lines 50480 50469 -11
==========================================
- Hits 36659 36638 -21
- Misses 13821 13831 +10
🚀 New features to boost your workflow:
|
Artifact Size Benchmark Reportaarch64-alpine-linux-musl
aarch64-apple-darwin
aarch64-unknown-linux-gnu
i686-alpine-linux-musl
i686-unknown-linux-gnu
libdatadog-x64-windows
libdatadog-x86-windows
x86_64-alpine-linux-musl
x86_64-apple-darwin
x86_64-unknown-linux-gnu
|
@@ -270,6 +273,16 @@ impl fmt::Debug for Bytes { | |||
} | |||
} | |||
|
|||
#[cfg(feature = "serde")] | |||
impl Serialize for Bytes { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it's covered in trace-utils, but Is this something we should explicitly test in tinybytes as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for jumping on this.
Could you also update https://github.com/DataDog/libdatadog/blob/main/data-pipeline/examples/send-traces-with-stats.rs#L32 to generate synthetic meta structs and make sure it works round-trip?
What does this PR do?
Fix 'meta_struct' serialization field so it's serialized as a byte array, for that the new type is defined as
HashMap<T, Bytes>
so it holds a a slice from the incoming buffer rather than copying the bytes. Additionally Bytes implementsserde::Serialization
trait so it's properly serialized when called from serde.