Skip to content
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

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

- Remove unused item types. ([#1211](https://github.com/getsentry/relay/pull/1211))
- Pin click dependency in requirements-dev.txt. ([#1214](https://github.com/getsentry/relay/pull/1214))
- Account root span in span op breakdown. ([#1213](https://github.com/getsentry/relay/pull/1213))

## 22.3.0

Expand Down
72 changes: 63 additions & 9 deletions relay-general/src/store/normalize/breakdowns.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@ use std::ops::Deref;

use serde::{Deserialize, Serialize};

use crate::protocol::{Breakdowns, Event, Measurement, Measurements, Timestamp};
use crate::protocol::{
Breakdowns, Context, ContextInner, Event, Measurement, Measurements, Span, Timestamp,
};
use crate::types::Annotated;

#[derive(Clone, Debug)]
Expand Down Expand Up @@ -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>> {
Copy link
Member

@untitaker untitaker Mar 29, 2022

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

let contexts = match event.contexts.value() {
Some(contexts) => contexts,
None => return None,
};

let trace_context = match contexts.get("trace").map(Annotated::value) {
Some(Some(trace_context)) => trace_context,
_ => return None,
};

match trace_context {
ContextInner(Context::Trace(trace_context)) => {
let op_name = match trace_context.op.value() {
None => return None,
Some(op_name) => op_name,
};

// Use the trace context to construct the root span that is sufficient for the operation breakdown.
let root_span = Annotated::new(Span {
timestamp: event.timestamp.clone(),
start_timestamp: event.start_timestamp.clone(),
op: Annotated::new(op_name.to_string()),
..Default::default()
});
Some(root_span)
}
_ => None,
}
}

/// Emit breakdowns that are derived using information from the given event.
pub trait EmitBreakdowns {
fn emit_breakdowns(&self, event: &Event) -> Option<Measurements>;
Expand All @@ -105,11 +138,15 @@ impl EmitBreakdowns for SpanOperationsConfig {
return None;
}

let spans = match event.spans.value() {
None => return None,
Some(spans) => spans,
let mut spans = match event.spans.value() {
Copy link
Member

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?

None => vec![],
Some(spans) => spans.to_vec(),
};

if let Some(root_span) = span_from_trace_context(event) {
spans.push(root_span);
}

// Generate span operation breakdowns
let mut intervals = HashMap::new();

Expand Down Expand Up @@ -278,7 +315,7 @@ pub fn normalize_breakdowns(event: &mut Event, breakdowns_config: &BreakdownsCon
#[cfg(test)]
mod tests {
use super::*;
use crate::protocol::{EventType, Span, SpanId, SpanStatus, TraceId};
use crate::protocol::{Contexts, EventType, Span, SpanId, SpanStatus, TraceContext, TraceId};
use crate::types::Object;
use chrono::{TimeZone, Utc};

Expand Down Expand Up @@ -366,7 +403,24 @@ mod tests {

let mut event = Event {
ty: EventType::Transaction.into(),
start_timestamp: Annotated::new(Utc.ymd(2020, 1, 1).and_hms_nano(0, 0, 0, 0).into()),
timestamp: Annotated::new(Utc.ymd(2020, 1, 1).and_hms_nano(10, 0, 0, 0).into()),
spans: spans.into(),
contexts: Annotated::new(Contexts({
let mut contexts = Object::new();
contexts.insert(
"trace".to_owned(),
Annotated::new(ContextInner(Context::Trace(Box::new(TraceContext {
trace_id: Annotated::new(TraceId(
"4c79f60c11214eb38604f4ae0781bfb2".into(),
)),
span_id: Annotated::new(SpanId("fa90fdead5f74053".into())),
op: Annotated::new("db.oracle".to_owned()),
..Default::default()
})))),
);
contexts
})),
..Default::default()
};

Expand Down Expand Up @@ -399,16 +453,16 @@ mod tests {
span_ops_breakdown.insert(
"ops.db".to_owned(),
Annotated::new(Measurement {
// 2 hours in milliseconds
value: Annotated::new(7_200_000.0),
// 10 hours in milliseconds
value: Annotated::new(36_000_000.0),
}),
);

span_ops_breakdown.insert(
"total.time".to_owned(),
Annotated::new(Measurement {
// 4 hours and 10 microseconds in milliseconds
value: Annotated::new(14_400_000.01),
// 12 hours and 10 microseconds in milliseconds
value: Annotated::new(43_200_000.01),
}),
);

Expand Down
22 changes: 17 additions & 5 deletions tests/integration/test_envelope.py
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,16 @@ def test_ops_breakdowns(mini_sentry, relay_with_processing, transactions_consume
transaction_item = generate_transaction_item()
transaction_item.update(
{
"start_timestamp": 0,
"timestamp": 5000,
"contexts": {
"trace": {
"trace_id": "4C79F60C11214EB38604F4AE0781BFB2",
"span_id": "FA90FDEAD5F74052",
"type": "trace",
"op": "db",
}
},
"spans": [
{
"description": "GET /api/0/organizations/?member=1",
Expand Down Expand Up @@ -241,7 +251,7 @@ def test_ops_breakdowns(mini_sentry, relay_with_processing, transactions_consume
"parent_span_id": "8f5a2b8768cafb4f",
"span_id": "bd429c44b67a3eb7",
"start_timestamp": 500.001,
"timestamp": 600.002003,
"timestamp": 600.002005,
"trace_id": "ff62a8b040f340bda5d830223def1d81",
},
{
Expand All @@ -267,14 +277,16 @@ def test_ops_breakdowns(mini_sentry, relay_with_processing, transactions_consume
assert "breakdowns" in event, event
assert event["breakdowns"] == {
"span_ops": {
"ops.db": {"value": 5000000.0},
"ops.http": {"value": 2000000.0},
"ops.resource": {"value": 100001.003},
"total.time": {"value": 2200001.003},
"ops.resource": {"value": pytest.approx(100001.005)},
"total.time": {"value": pytest.approx(7200001.005)},
},
"span_ops_2": {
"ops.db": {"value": 5000000.0},
"ops.http": {"value": 2000000.0},
"ops.resource": {"value": 100001.003},
"total.time": {"value": 2200001.003},
"ops.resource": {"value": pytest.approx(100001.005)},
"total.time": {"value": pytest.approx(7200001.005)},
},
}

Expand Down
6 changes: 4 additions & 2 deletions tests/integration/test_metrics.py
Original file line number Diff line number Diff line change
Expand Up @@ -472,7 +472,9 @@ def test_transaction_metrics(
}

transaction = generate_transaction_item()
transaction["timestamp"] = timestamp.isoformat()
end_timestamp = datetime.now(tz=timezone.utc)
transaction["timestamp"] = end_timestamp.isoformat()
transaction["start_timestamp"] = (end_timestamp - timedelta(seconds=10)).isoformat()
transaction["measurements"] = {
"foo": {"value": 1.2},
"bar": {"value": 1.3},
Expand All @@ -493,7 +495,7 @@ def test_transaction_metrics(
assert event["breakdowns"] == {
"span_ops": {
"ops.react.mount": {"value": 9.910106},
"total.time": {"value": 9.910106},
"total.time": {"value": 10009.910106},
}
}

Expand Down