fix(trace-utils): v05 events and span links serialization #980
+199
−14
Conversation
This file contains 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
BenchmarksComparisonBenchmark execution time: 2025-03-27 14:59:30 Comparing candidate commit 798fba6 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 52 metrics, 2 unstable metrics. 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. |
paullegranddc
changed the title
hoolioh
requested changes
Mar 28, 2025
Comment on lines
+149
to
+150
| if !span.span_links.is_empty() { | ||
| let serialized_span_links = serde_json::to_string(&span.span_links)?; |
There was a problem hiding this comment.
I think we should check here for the 25kb limit on the tag value, right?
There was a problem hiding this comment.
Yes, anything beyond 25KB limits is dropped. the implementation should try to stuff as many as links possible under the limit and drop the others.
There used to be dropped_attributes_count to count the dropped attributes in case of oversized span links which i didn't see our implementation.
| if !span.span_links.is_empty() { | ||
| let serialized_span_links = serde_json::to_string(&span.span_links)?; | ||
| meta.insert( | ||
| dict.get_or_insert(&tinybytes::BytesString::from("span_links"))?, |
There was a problem hiding this comment.
In the RFC there is some discussion about "span_links" or "_dd.span_links". Did they finally choose the former?
There was a problem hiding this comment.
No, it's a mistake the key should definitely be "_dd.span_links" from other implementations
ganeshnj
requested changes
Mar 28, 2025
Comment on lines
+35
to
+45
| ///This structure is a wrapper around a slice of span events | ||
| /// | ||
| /// It is meant to overrdide the default serialization, so we can serialize attributes | ||
| /// differently from the original impl. | ||
| /// Span events are serialized to JSON and added to "meta" when serializing to v0.5 | ||
| /// | ||
| /// The main difference with messagepacck serialization is that attributes with any types | ||
| /// are supposed to be mapped to their natural JSON representation. | ||
| /// | ||
| /// Sadly, I haven't found a good way of overriding the default Serialize behavior, other | ||
| /// than just doing it for the whole data structures that embed it. |
There was a problem hiding this comment.
Suggested change
| ///This structure is a wrapper around a slice of span events | |
| /// | |
| /// It is meant to overrdide the default serialization, so we can serialize attributes | |
| /// differently from the original impl. | |
| /// Span events are serialized to JSON and added to "meta" when serializing to v0.5 | |
| /// | |
| /// The main difference with messagepacck serialization is that attributes with any types | |
| /// are supposed to be mapped to their natural JSON representation. | |
| /// | |
| /// Sadly, I haven't found a good way of overriding the default Serialize behavior, other | |
| /// than just doing it for the whole data structures that embed it. | |
| /// This structure is a wrapper around a slice of span events | |
| /// | |
| /// It is meant to override the default serialization, so we can serialize attributes | |
| /// differently from the original impl. | |
| /// Span events are serialized to JSON and added to "meta" when serializing to v0.5 | |
| /// | |
| /// The main difference with messagepack serialization is that attributes with any types | |
| /// are supposed to be mapped to their natural JSON representation. |
you can skip the justification and personalization.
| @@ -82,8 +209,47 @@ mod tests { | |||
| )]), | |||
| metrics: HashMap::from([(BytesString::from("metrics_field"), 1.1)]), | |||
| meta_struct: HashMap::new(), | |||
| span_links: vec![], | |||
| span_events: vec![], | |||
| span_links: vec![SpanLinkBytes { | |||
There was a problem hiding this comment.
/suggestion
- would be nice to have test case for multiple span links
- a complex attributes (remember we do some flattening for array and maps), I'm not sure at this point we have them already or we have to do ourselves
eg
{"key": [[1,2], ["3", "4"]]} to {"key.0.0": "1", "key.0.1": 2, "key.1.0": 3, "key.1.1":4}
Comment on lines
+149
to
+150
| if !span.span_links.is_empty() { | ||
| let serialized_span_links = serde_json::to_string(&span.span_links)?; |
There was a problem hiding this comment.
Yes, anything beyond 25KB limits is dropped. the implementation should try to stuff as many as links possible under the limit and drop the others.
There used to be dropped_attributes_count to count the dropped attributes in case of oversized span links which i didn't see our implementation.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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?
V05 format specifies fallback options for serializing events and span links inside of the meta field.
span_links fallback format spec
event fallback format spec
This PR should be fixing it
Motivation
What inspired you to submit this pull request?
Additional Notes
Anything else we should know when reviewing?
How to test the change?
Describe here in detail how the change can be validated.