Conversation
📚 Documentation Check Results📦
|
Clippy Allow Annotation ReportComparing clippy allow annotations between branches:
Summary by Rule
Annotation Counts by File
Annotation Stats by Crate
About This ReportThis report tracks Clippy allow annotations for specific rules, showing how they've changed in this PR. Decreasing the number of these annotations generally improves code quality. |
🔒 Cargo Deny Results📦
|
🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage (details) 🔗 Commit SHA: 01e38e3 | Docs | Datadog PR Page | Give us feedback! |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1896 +/- ##
==========================================
- Coverage 71.55% 71.50% -0.06%
==========================================
Files 431 434 +3
Lines 69158 69547 +389
==========================================
+ Hits 49489 49729 +240
- Misses 19669 19818 +149
🚀 New features to boost your workflow:
|
Artifact Size Benchmark Reportaarch64-alpine-linux-musl
aarch64-unknown-linux-gnu
libdatadog-x64-windows
libdatadog-x86-windows
x86_64-alpine-linux-musl
x86_64-unknown-linux-gnu
|
anais-raison
changed the title
Aaalibaba42
left a comment
Aaalibaba42
left a comment
There was a problem hiding this comment.
I'm not learnt in the data-flow etc, I mostly just commented on the code, which do have a few smells but nothing blocking I'd say
| let chunks = trace_utils::collect_trace_chunks(traces, false).map_err(|e| { | ||
| TraceExporterError::Deserialization(DecodeError::InvalidFormat(e.to_string())) | ||
| })?; | ||
| match chunks { | ||
| tracer_payload::TraceChunks::V04(traces) => { | ||
| Ok(tracer_payload::TraceChunks::V1(traces)) | ||
| } | ||
| other => Ok(other), | ||
| } | ||
| } |
There was a problem hiding this comment.
You call collect_trace_chunks saying false on use_v05_format, so it will always return TraceChunks::V04(traces), the other arm should be unreachable iiuc. I don't know even what's the point of this all or if the collect_trace_chunks function should change to take the traces and the enum of formats directly, it seems to do the same logic twice somewhat
There was a problem hiding this comment.
Plus in terms of "design", passing true/false to a function is without context is not great, I had to look up the function in my editor to know what/why etc, I'd be keen to make use_v05_format of collect_trace_chunk an enum anyway (but maybe that's out of scope for this PR idk).
| fn new() -> Self { | ||
| Self { | ||
| seen: HashMap::new(), | ||
| next_id: 1, |
There was a problem hiding this comment.
You could probably derive that from the size of the hashmap (I'm not saying it would be more readable, or better, but just to have the option thought of)
There was a problem hiding this comment.
[EDIT: Ok I get what you meant, sorry for the noise] Are you talking about next_id?
| fn encode_array_element<W: RmpWrite, T: TraceData>( | ||
| writer: &mut W, | ||
| value: &AttributeArrayValue<T>, | ||
| table: &mut StringTable, | ||
| ) -> Result<(), ValueWriteError<W::Error>> { | ||
| match value { | ||
| AttributeArrayValue::String(s) => { | ||
| write_uint8(writer, AnyValueKey::String as u8)?; | ||
| table.write_interned(writer, s.borrow())?; | ||
| } | ||
| AttributeArrayValue::Boolean(b) => { | ||
| write_uint8(writer, AnyValueKey::Bool as u8)?; | ||
| write_bool(writer, *b).map_err(ValueWriteError::InvalidDataWrite)?; | ||
| } | ||
| AttributeArrayValue::Integer(i) => { | ||
| write_uint8(writer, 4u8)?; // Int64 | ||
| write_sint(writer, *i)?; | ||
| } | ||
| AttributeArrayValue::Double(d) => { | ||
| write_uint8(writer, AnyValueKey::Double as u8)?; | ||
| write_f64(writer, *d)?; | ||
| } | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
I'm not against this per-se, but if it needs to be a function and not a lambda, I'd write it out of the function's body. Like to me either it's an auxiliary function and it's not in the body, or it's a lambda and it can be there. Except if you have good reasons not to do it like that.
There was a problem hiding this comment.
I kind of like this pattern. This keeps the function scoped, not need to wonder if it's called elsewhere, and having an actual function is not much worse than a closure.
The only thing gained by making this a closure would probably be a shorter declaration thanks to inference
There was a problem hiding this comment.
I guess it's just a point of style, I agree it's not a problem in any way, it's just feels icky idk
There was a problem hiding this comment.
It's a matter of taste, but I kinda agree with @paullegranddc here. If it's a local re-usable block but doesn't actually need to capture any environment, I personally use the fn syntax that I find more uniform (you also don't have to worry about making it mut or a reference etc.). For that matter I've seen struct, enum or even trait defined locally. I don't think it's wrong in any way, when justified.
There was a problem hiding this comment.
Just to pile on, I agree with @paullegranddc and @yannham. The scoping here makes the intentions clear.
| /// - `trace_id` is not encoded in the span (it belongs to the chunk). | ||
| /// - `error` is encoded as a boolean. | ||
| /// - String values use streaming string interning via `StringTable`. | ||
| #[inline(always)] |
There was a problem hiding this comment.
Why ? It doesn't look like your typical function that benefits from being inline, I'd leave that decision to the compiler except if you have good reasons not to.
There was a problem hiding this comment.
I agree, especially given that this function is quite big. FWIW, my "runbook" for inline annotations:
- if it's a very small function (like, getter or setter, no more than a few operations really), and it's a
pubitem that might be re-used from other crates:#[inline](it often doesn't change much the decision of the compiler but enable cross-crate inlining, which is otherwise not possible by default, and is usually the actual motivation for#inlineannotations) - if it's a small-ish function, that is very performance critical (e.g. called repeatedly in a hot loop) and I have thoroughly measured with benchmarks that inlining it shows clear, reproducible improvement:
#[inline(always)]might be considered. But inlining can be a pessimization, whether it is or not can heavily depends on the callsites as well (and change over time), so it's risky business.
There was a problem hiding this comment.
Agreed. I think our msgpack encoder / decoders had a lot of inline annotations in the past (and probably some still hanging around) and most of them made no difference.
| pub(crate) struct StringTable { | ||
| seen: HashMap<String, u32>, | ||
| next_id: u32, | ||
| } | ||
|
|
||
| impl StringTable { | ||
| fn new() -> Self { | ||
| Self { | ||
| seen: HashMap::new(), | ||
| next_id: 1, | ||
| } | ||
| } |
There was a problem hiding this comment.
Could be
| pub(crate) struct StringTable { | |
| seen: HashMap<String, u32>, | |
| next_id: u32, | |
| } | |
| impl StringTable { | |
| fn new() -> Self { | |
| Self { | |
| seen: HashMap::new(), | |
| next_id: 1, | |
| } | |
| } | |
| pub(crate) struct StringTable { | |
| seen: HashMap<String, u32>, | |
| } | |
| impl StringTable { | |
| fn new() -> Self { | |
| let mut seen = HashMap::new(); | |
| seen.insert(String::new(), 0); | |
| Self { | |
| seen, | |
| } | |
| } | |
| } |
Then you can remove the special empty string caee in write_interned and do
let id = self.seen.len();
self.seen.insert(s.to_string(), id);There was a problem hiding this comment.
when I started my review you hadn't posted yours I think 😄
| /// Origin tag from `_dd.origin` meta on the root span. | ||
| origin: Option<String>, |
There was a problem hiding this comment.
Is it possible to borrow the origin as an &str instead of copying it?
| } | ||
| } |
There was a problem hiding this comment.
Can we break out of the loop here if we encountered a root span?
This seems to take the "last root span" in the chunk if there are multiple which i don't think is really deliberate
There was a problem hiding this comment.
@ajgajg1134 - Weren't we discussing something similar in the agent recently? A scenario where we encounter multiple root spans?
| // Chunk-level attributes come from the root span (parent_id == 0). | ||
| if span.parent_id == 0 { |
There was a problem hiding this comment.
This is not complete.
Span chunks can have a remote parent, in which case the top level is a span where the parent exists but is not in the chunk. In which case we need to promote it's attributes
You can look for _dd.top_level I think, as it is tagged before in the pipeline https://github.com/DataDog/libdatadog/blob/main/libdd-trace-utils/src/span/trace_utils.rs#L16
Btw the agent this is not obvious when looking at the agent v04 to v1 conversion in the agent, but that does happen when the converter iterates over all spans.
It put fields to be promoted in a SpanConvertedFields and then promotes these fields to chunk level attributes latter.
(Example here on how this work for the sampling priority:
// Collect promoted fields
// https://github.com/DataDog/datadog-agent/blob/paullgdc/trace/add_agent_telemetry_batching/pkg/proto/pbgo/trace/idx/span.go#L1651-L1657
// handlePromotedMetricsFields processes promoted metrics fields that have dedicated span fields
// If we fail to parse a value we don't use the promoted value, but the original string will still be in the span attributes
func (s *InternalSpan) handlePromotedMetricsFields(key uint32, val float64, convertedFields *SpanConvertedFields) {
if s.Strings.Get(key) == "_sampling_priority_v1" {
convertedFields.SamplingPriority = int32(val)
}
}
// promotes them https://github.com/DataDog/datadog-agent/blob/paullgdc/trace/add_agent_telemetry_batching/pkg/proto/pbgo/trace/idx/span.go#L931
// ApplyPromotedFields applies span promoted fields to the chunk, copying chunk promoted fields to chunkConvertedFields
func (c *InternalTraceChunk) ApplyPromotedFields(convertedFields *SpanConvertedFields, chunkConvertedFields *ChunkConvertedFields) {
// ...
c.Priority = int32(convertedFields.SamplingPriority)
// ...
}
)
| /// Promoted fields extracted from spans and written at the chunk level. | ||
| struct ChunkAttrs { |
There was a problem hiding this comment.
I think you are missing the sampling mechanism (_dd.p.dm)
| ) -> Result<(), ValueWriteError<W::Error>> { | ||
| let mut table = StringTable::new(); | ||
|
|
||
| // Top-level map contains only the chunks array for now. |
There was a problem hiding this comment.
What about the payload promoted fields?
- env
- apm mode
- hostname
- app version
- git commit sha
| tracer_payload::TraceChunks::V04(traces) => { | ||
| Ok(tracer_payload::TraceChunks::V1(traces)) | ||
| } | ||
| other => Ok(other), |
There was a problem hiding this comment.
This shouldn't be reachable, right, so it should error.
| write_map_len(writer, fields)?; | ||
|
|
||
| // 128-bit trace ID as 16-byte big-endian binary. | ||
| write_uint8(writer, ChunkKey::TraceId as u8)?; |
There was a problem hiding this comment.
| write_uint8(writer, ChunkKey::TraceId as u8)?; | |
| write_uint(writer, ChunkKey::TraceId)?; |
The write methods will use the smallest possible encoding anyway, and you can remove all the casts.
There was a problem hiding this comment.
The casts are there because ChunkKey is a repr(u8) enum I think no, so using write_uint will nto change anything?
I guess I would prefer something like this pattern though. There is no need for an enum because we never match back
mod chunk_key {
const PRIORITY: u8 = 1;
// other keys
const TRACE_ID: u8 = 6;
}
| /// The string table is scoped per payload: each `to_vec` / `write_to_slice` call starts with a | ||
| /// fresh table so deduplication is payload-local. | ||
| pub(crate) struct StringTable { | ||
| seen: HashMap<String, u32>, |
There was a problem hiding this comment.
It's probably for much later, I'm just thinking out loud here. I wonder if we can make the string table slightly more performant with the following scheme, if all the strings have the same lifetime:
- allocate the strings in a local bump arena
- have seen be a
Hashmap<&'self str, u32>(it's self referential but easy to do with something likeourboros)
Something like this:
#[ouroboros::self_referencing]
struct StringTable {
/// Preallocates space where strings are stored.
arena: Arena<u8>,
/// Deduplicate strings.
#[borrows(arena)]
#[covariant]
map: HashMap<&'this str, u32>,
}The gain would be faster string allocation and de-allocation. This is only if the encoding is perf sensitive.
There was a problem hiding this comment.
I agree it doesn't have to be in this PR, but I think this is worth exploring deeper. String allocation is traditionally the biggest drag on performance. @anais-raison Can you create a ticket for the epic to investigate later?
| let id = self.next_id; | ||
| self.next_id += 1; | ||
| self.seen.insert(s.to_string(), id); |
There was a problem hiding this comment.
| let id = self.next_id; | |
| self.next_id += 1; | |
| self.seen.insert(s.to_string(), id); | |
| self.seen.insert(s.to_string(), self.next_id); | |
| self.next_id += 1; |
| /// # Errors | ||
| /// Returns a `ValueWriteError` if the underlying writer fails. | ||
| pub fn write_to_slice<T: TraceData, S: AsRef<[Span<T>]>>( | ||
| slice: &mut &mut [u8], |
There was a problem hiding this comment.
Out of curiosity, why does it need to be &mut &mut here?
| ) -> Vec<u8> { | ||
| let mut buf = ByteBuf::with_capacity(capacity as usize); | ||
| #[allow(clippy::expect_used)] | ||
| encode_payload(&mut buf, traces).expect("infallible: the error is std::convert::Infallible"); |
There was a problem hiding this comment.
I think there's a way to unwrap an infallible without actually needing a panicking function, although the second match is a bit confusing:
let unwrapped = match result {
Ok(x) => x,
Err(e) => match e {},
}It's not uncommon to have a little helper unwrap_infallible in some crates to do that for you.
There was a problem hiding this comment.
Additionally, since you don't do anything with the result, you could also do something like:
let _ = encode_payalod with a comment explaining why you ignore the result. Might slightly nicer than the allow(clippy) and the expect and error message that will never happen.
There was a problem hiding this comment.
+1 we should try to avoid panics unless absolutely necessary (like acquiring mutex locks, for example)
| } | ||
|
|
||
| /// Returns the number of bytes the V1 payload for `traces` would occupy. | ||
| pub fn to_len<T: TraceData, S: AsRef<[Span<T>]>>(traces: &[S]) -> u32 { |
There was a problem hiding this comment.
Nitpick: I find the name of this function a bit obscure.
|
|
||
| for link in span_links.iter() { | ||
| let trace_id_128 = ((link.trace_id_high as u128) << 64) | link.trace_id as u128; | ||
| let link_len = 1 /* trace_id (always) */ |
There was a problem hiding this comment.
| let link_len = 1 /* trace_id (always) */ | |
| let link_len = 1 // trace_id (always) |
| write_uint8(writer, SpanKey::SpanEvents as u8)?; | ||
| rmp::encode::write_array_len(writer, span_events.len() as u32)?; | ||
|
|
||
| for event in span_events.iter() { |
There was a problem hiding this comment.
I think &[_] implements IntoIter<Item = &_>?
| for event in span_events.iter() { | |
| for event in span_events { |
| rmp::encode::write_array_len(writer, span_events.len() as u32)?; | ||
|
|
||
| for event in span_events.iter() { | ||
| let event_len = 2 /* time_unix_nano, name */ |
There was a problem hiding this comment.
| let event_len = 2 /* time_unix_nano, name */ | |
| let event_len = 2 // time_unix_nano, name |
| match self { | ||
| TraceExporterOutputFormat::V04 => "/v0.4/traces", | ||
| TraceExporterOutputFormat::V05 => "/v0.5/traces", | ||
| TraceExporterOutputFormat::V1 => "/v1.0/traces", |
There was a problem hiding this comment.
I believe the builder is going to fail validation if input is v0.4 and output is v1.0. Is that intentional? With this PR we should have no issue serializing to v1.0, right?
| #[default] | ||
| V04, | ||
| V05, | ||
| V1, |
There was a problem hiding this comment.
Can we include an integration test that does a snapshot test with the test agent? I assume you'll have to bump the version of the test agent to pick up the V1 support.
7 participants
What does this PR do?
Implements the v0.4 to V1 encoder for the Trace Exporter.
Key additions:
ID
Motivation
APMSP-2808
Additional Notes
How to test the change?
Added unit tests.