Add basic decoding for v0.4 msgpack traces#545
Merged
ekump merged 10 commits intoAug 6, 2024
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #545 +/- ##
==========================================
+ Coverage 70.44% 71.27% +0.83%
==========================================
Files 214 219 +5
Lines 28884 29918 +1034
==========================================
+ Hits 20346 21323 +977
- Misses 8538 8595 +57
🚀 New features to boost your workflow:
|
BenchmarksComparisonBenchmark execution time: 2024-08-06 12:54:44 Comparing candidate commit 87c03a4 in PR branch Found 1 performance improvements and 1 performance regressions! Performance is the same for 41 metrics, 1 unstable metrics. scenario:benching deserializing traces from msgpack to their internal representation
scenario:tags/replace_trace_tags
CandidateCandidate benchmark detailsGroup 1
Group 2
Group 3
Group 4
Group 5
Group 6
Group 7
Group 8
Group 9
Group 10
Group 11
BaselineOmitted due to size. |
ekump
force-pushed
the
ekump/APMSP-1214-removing-serde-from-deserialization
branch
4 times, most recently
from
9d56098 to
066807b
Compare
ekump
force-pushed
the
ekump/APMSP-1214-removing-serde-from-deserialization
branch
from
066807b to
97aadee
Compare
Contributor
paullegranddc
left a comment
paullegranddc
left a comment
There was a problem hiding this comment.
I don't quite understand what the custom deserializer brings.
The goal is to capture &str instead of String for span fields in the future right?
But from what I know this can be done with serde, using &'a str references for strings, and #[serde::borrow] annotations right?
Comment on lines
+8
to
+13
| pub enum Number { | ||
| U8(u8), | ||
| U32(u32), | ||
| U64(u64), | ||
| I8(i8), | ||
| I32(i32), | ||
| I64(i64), | ||
| F64(f64), | ||
| } |
Contributor
There was a problem hiding this comment.
I don't really see why you need this runtime representation of how the message pack is encoded?
You could make the code simpler by having two read function
- read_number_signed<T: TryFrom>() -> Result
- read_number_unsigned<T: TryFrom>() -> Result
What would these functions do is:
- Widening the decoded numbers to i64 or u64, since these type would be big enough to represent any signed or unsigned number using the existing TryFrom trait implementation for these, then call T::try_from to cast to the desired output.
See this playground for a small implementation of this idea https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=2dfcc2b984bcebb4395179a62087c97f
Contributor
Author
There was a problem hiding this comment.
If I understand your suggestion correctly we'd change Number to be:
pub enum Number {
Signed(i64),
Unsigned(u64),
Float(f64),
}The existing read_number function wouldn't be simplified much because we still need to handle all the different Marker types. It would wind up looking like:
pub fn read_number(buf: &mut &[u8]) -> Result<Number, DecodeError> {
match rmp::decode::read_marker(buf)
.map_err(|_| DecodeError::InvalidFormat("Unable to read marker for number".to_owned()))?
{
Marker::FixPos(val) => Ok(Number::Unsigned(val as u64)),
Marker::FixNeg(val) => Ok(Number::Signed(val as i64)),
Marker::U8 => Ok(Number::Unsigned(
buf.read_data_u8().map_err(|_| DecodeError::IOError)? as u64,
)),
Marker::U16 => Ok(Number::Unsigned(
buf.read_data_u16().map_err(|_| DecodeError::IOError)? as u64,
)),
Marker::U32 => Ok(Number::Unsigned(
buf.read_data_u32().map_err(|_| DecodeError::IOError)? as u64,
)),
Marker::U64 => Ok(Number::Unsigned(
buf.read_data_u64().map_err(|_| DecodeError::IOError)?,
)),
Marker::I8 => Ok(Number::Signed(
buf.read_data_i8().map_err(|_| DecodeError::IOError)? as i64,
)),
Marker::I16 => Ok(Number::Signed(
buf.read_data_i16().map_err(|_| DecodeError::IOError)? as i64,
)),
Marker::I32 => Ok(Number::Signed(
buf.read_data_i32().map_err(|_| DecodeError::IOError)? as i64,
)),
Marker::I64 => Ok(Number::Signed(
buf.read_data_i64().map_err(|_| DecodeError::IOError)?,
)),
Marker::F32 => Ok(Number::Float(
buf.read_data_f32().map_err(|_| DecodeError::IOError)? as f64,
)),
Marker::F64 => Ok(Number::Float(
buf.read_data_f64().map_err(|_| DecodeError::IOError)?,
)),
_ => Err(DecodeError::InvalidType("Invalid number type".to_owned())),
}
}A bigger problem arises when we implement the TryFrom traits and need to downcast.
The following will silently truncate a u64 value to u32:
impl TryFrom<Number> for u32 {
type Error = DecodeError;
fn try_from(value: Number) -> Result<Self, Self::Error> {
match value {
Number::Unsigned(val) => Ok(val as u32),
_ => Err(DecodeError::InvalidConversion(format!(
"unable to convert {} to u32",
value
))),
}
}
}To raise an error when we can't downcast we wind up with:
impl TryFrom<Number> for u32 {
type Error = DecodeError;
fn try_from(value: Number) -> Result<Self, Self::Error> {
match value {
Number::Unsigned(val) if val <= u32::MAX as u64 => Ok(val as u32),
_ => Err(DecodeError::InvalidConversion(format!(
"unable to convert {} to u32",
value
))),
}
}
}I lean slightly towards preferring the existing implementation as it seems a bit clearer to me. @hoolioh you originally implemented this. Do you have any thoughts?
And, apologies if I misunderstood your suggestion.
Contributor
The idea is to to have this a a building block to gradually introduce future performance optimizations. We're evaluating different mechanisms for reducing allocation overhead: string interning, implementing a string arena, memory pools, etc. Also we would want to test different ways of representing the data. In essence this will give us the flexibility to test those different approaches easily. |
Contributor
That makes sense |
| match decode::read_marker(buf) | ||
| .map_err(|_| DecodeError::InvalidFormat("Unable to read marker for map".to_owned()))? | ||
| { | ||
| Marker::FixMap(len) => { |
Contributor
There was a problem hiding this comment.
Does the V04 spec impose to have less than 16 entries, wouldn't it be more future proof to support Map16 ?
Contributor
Author
There was a problem hiding this comment.
That's a good question. I'm not aware of anything that says v04 can't have maps with > 16 entries. @hoolioh - Are you aware of anything preventing larger maps from being sent from the client libraries?
I've updated the decoder to also support Map16.
ekump
force-pushed
the
ekump/APMSP-1214-removing-serde-from-deserialization
branch
3 times, most recently
from
8e81eb8 to
9007960
Compare
ekump
force-pushed
the
ekump/APMSP-1214-removing-serde-from-deserialization
branch
2 times, most recently
from
654cd8a to
1b29d18
Compare
bantonsson
reviewed
Aug 2, 2024
Contributor
bantonsson
left a comment
bantonsson
left a comment
There was a problem hiding this comment.
This looks good to me overall, and the performance numbers are promising (-20% even without much in the way of optimizations)
| } | ||
|
|
||
| #[inline] | ||
| fn read_string(buf: &mut &[u8]) -> Result<String, DecodeError> { |
Contributor
There was a problem hiding this comment.
Can't this function just use read_string_ref, and then bump the buf like the code in fill_span?
Contributor
Author
There was a problem hiding this comment.
Yes, that works. Good idea!
|
|
||
| match key { | ||
| SpanKey::Service => { | ||
| let (value, next) = read_string_ref(buf)?; |
Contributor
There was a problem hiding this comment.
Could this use read_string?
Contributor
There was a problem hiding this comment.
This and all the other ones below that directly convert the &str to a String can use read_string.
ekump
force-pushed
the
ekump/APMSP-1214-removing-serde-from-deserialization
branch
from
2737928 to
58cbd82
Compare
ekump
force-pushed
the
ekump/APMSP-1214-removing-serde-from-deserialization
branch
from
3548fb9 to
4421efd
Compare
bantonsson
reviewed
Aug 6, 2024
Contributor
bantonsson
left a comment
bantonsson
left a comment
There was a problem hiding this comment.
The performance regression is real and can be fixed.
|
|
||
| match key { | ||
| SpanKey::Service => { | ||
| let (value, next) = read_string_ref(buf)?; |
Contributor
There was a problem hiding this comment.
This and all the other ones below that directly convert the &str to a String can use read_string.
* Add methods to get string and &str. * Add Number abstraction layer in order to decode integer and floats. * Implement decoder for Span. * Implement decoder for meta attributes. * Implement decoder for metrics attributes. * Implement decoder for SpanLinks. * Add tests.
- introduce enums for span_link and span keys and switch from long if statements to matches to codify all possible enumerations. - include trace payload schema version in namespace for decoder. - break up decoder into multiple files to be easier to follow.
ekump
force-pushed
the
ekump/APMSP-1214-removing-serde-from-deserialization
branch
from
4421efd to
87c03a4
Compare
Contributor
Author
@bantonsson - Not only fix, but looks like improve upon:
I think the |
bantonsson
approved these changes
Aug 6, 2024
hoolioh
added a commit
that referenced
this pull request
Aug 21, 2024
* Replace the use of rmp_serde with a custom decoder for decoding v04 traces in msgpack format. * Introduce fuzz testing for trace_utils This is a precursor to reducing the number of String allocations that occur when processing traces. --------- Co-authored-by: Julio Gonzalez <julio.gonzalez@datadoghq.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Deserialize msgpack payloads to PB without using serde.
Originally authored by @hoolioh this PR replaces the use of serde to decode msgpack v04 trace payloads
Motivation
This is part of a larger effort to reduce memory allocations when sending traces through the data-pipeline. After we stop using serde we will move on to improving the internal representation of traces to use references rather than allocating new strings.
Additional Notes
The introduction of the deserialization process in trace-utils integrations tests will be introduced in a follow-up PR.
How to test the change?
unit and fuzz tests included.