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(breakdown): Account root span in span op breakdown #1213
Conversation
586a6ac
to
8dfb47d
Compare
CI lint job is failing due to psf/black#2966. |
let spans = match event.spans.value() { | ||
None => return None, | ||
Some(spans) => spans, | ||
let mut spans = match event.spans.value() { |
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.
instead of allocating a new vector here, can you extract the loop body into a function, and call it for each entry in event.spans
+ the root span?
@@ -82,6 +84,37 @@ fn get_op_time_spent(mut intervals: Vec<TimeWindowSpan>) -> Option<f64> { | |||
Some(op_time_spent) | |||
} | |||
|
|||
fn span_from_trace_context(event: &Event) -> Option<Annotated<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.
Instead of doing this I would create a trait that abstracts over spans and trace contexts, called SpanLike
. That's probably useful across the codebase.
Main reason is re-allocation of strings
4d51c3d
to
0539f79
Compare
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.
requesting changes from above... we don't have a ton of leeway wrt performance and will likely need to make similar improvements across the entire codebase at some point
@dashed could you give the requested changes a look and update or close this PR? Thanks! |
This is an alternative to #1212.
For span operation breakdowns, we should account for the root span. The root span was intentionally omitted during the initial breakdown implementation, since it would've "blow up" the
span.total.time
field. This was reflected on the client-side implementation of the span operation breakdowns for the span view; of which the breakdown implementation in Relay was based upon.We can do this today since the
span.total.time
field is currently un-used in the product today. Its usage in the product was removed in: