-
Notifications
You must be signed in to change notification settings - Fork 8
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
[Data-Pipeline] [APMSP-1240] Add concentrator #570
[Data-Pipeline] [APMSP-1240] Add concentrator #570
Conversation
BenchmarksComparisonBenchmark execution time: 2024-08-22 13:04:11 Comparing candidate commit b80115d in PR branch Found 0 performance improvements and 1 performance regressions! Performance is the same for 49 metrics, 2 unstable metrics. scenario:benching deserializing traces from msgpack to their internal representation
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. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #570 +/- ##
==========================================
+ Coverage 71.73% 72.90% +1.17%
==========================================
Files 238 241 +3
Lines 32941 34364 +1423
==========================================
+ Hits 23631 25054 +1423
Misses 9310 9310
|
401f75c
to
fabcd74
Compare
33cd1cf
to
6778da6
Compare
89bc718
to
c750b0d
Compare
c750b0d
to
1a295e5
Compare
@@ -410,6 +412,13 @@ pub fn compute_top_level_span(trace: &mut [pb::Span]) { | |||
} | |||
} | |||
|
|||
pub fn has_top_level(span: &pb::Span) -> bool { |
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.
These public functions should have unit tests
} | ||
|
||
pub fn add_span(&mut self, span: &pb::Span) -> Result<()> { | ||
if !(trace_utils::has_top_level(span) |
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.
minor: I think this if statement is complex enough that it should be moved into a separate function for readability / maintainability purposes. Something along the lines of:
fn should_ignore_span(span: &Span, compute_stats_by_span_kind: bool) -> bool {
!(trace_utils::has_top_level(span)
|| trace_utils::is_measured(span)
|| (self.compute_stats_by_span_kind && compute_stats_for_span_kind))
|| trace_utils::is_partial_snapshot(span)
}
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.
Looks good in general.
pub is_synthetics_request: bool, | ||
pub peer_tags: Vec<Tag>, | ||
pub is_trace_root: bool, | ||
} |
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.
This is not for this PR, but maybe the AggregationKey
should cache a precomputed hash value (since it's immutable anyway), and implement Hash
by itself.
c6b0466
to
60be546
Compare
1133a4e
to
ea7bee7
Compare
Change concentrator name to be consistent with the trace agent
ea7bee7
to
a517d6b
Compare
What does this PR do?
Implement the stats concentrator from the trace agent, to allow stats computation in the data pipeline.
Motivation
Is required to allow stats computation in libdatadog