fix(spans): Extract metrics from transaction spans #3273
Conversation
jjbayer
changed the title
| struct NormalizeSpanConfig<'a> { | ||
| /// The time at which the event was received in this Relay. | ||
| pub received_at: DateTime<Utc>, | ||
| received_at: DateTime<Utc>, |
| #[cfg(test)] | ||
| #[cfg(feature = "processing")] | ||
| impl<'a, Group: TryFrom<ProcessingGroup>> ProcessEnvelopeState<'a, Group> { | ||
| fn simple(event_json: &str, group: ProcessingGroup, project_state: ProjectState) -> Self { |
There was a problem hiding this comment.
Just a helper method to create a state for testing.
There was a problem hiding this comment.
I don't mind having it, but usually we just have free standing functions instead
| return; | ||
| } | ||
|
|
||
| let mut add_span = |span: Annotated<Span>| { |
There was a problem hiding this comment.
I used the opportunity to upgrade this helper to a real function.
| /// Returns an untracked envelope that does not report to outcomes or test store. | ||
| #[cfg(test)] | ||
| pub fn silent(envelope: Box<Envelope>, group: ProcessingGroup) -> Self { | ||
| Self::standalone(envelope, Addr::custom().0, Addr::custom().0, group) |
There was a problem hiding this comment.
| Self::standalone(envelope, Addr::custom().0, Addr::custom().0, group) | |
| Self::standalone(envelope, Addr::dummy(), Addr::dummy(), group) |
| #[cfg(test)] | ||
| #[cfg(feature = "processing")] | ||
| impl<'a, Group: TryFrom<ProcessingGroup>> ProcessEnvelopeState<'a, Group> { | ||
| fn simple(event_json: &str, group: ProcessingGroup, project_state: ProjectState) -> Self { |
There was a problem hiding this comment.
I don't mind having it, but usually we just have free standing functions instead
|
Requires change: Metrics extraction has to happen before dynamic sampling of the transaction. |
There was a problem hiding this comment.
LGTM.
Is it worth adding an integration test with multiple relays to ensure metrics from a transaction span are only extracted once (1x transaction, 1x span)? Since we're making changes around span<>transaction conversion, to ensure we don't break anything in the future. I'm not sure if that'd cover uncovered use cases, so your call.
Co-authored-by: Iker Barriocanal <[email protected]>
Added span metrics to |
Since #3273, we accidentally emit the metric `d:transactions/measurements.score.total@ratio` twice for every transaction. This happens because the metric is not only extracted from transactions, but also from spans: https://github.com/getsentry/relay/blob/0b8687c03b3eaf23db368733754d17c446bcb482/relay-dynamic-config/src/defaults.rs#L459-L461 As a quick fix, extract that metric only if the span is not a segment span extracted from a transaction. Long-term, we should stop extracting that _transaction_ metric from spans, and base performance score computation on `"d:spans/webvital.score.total@ratio` instead.
Because span metrics extraction happens as part of transaction metrics extraction, at which point we did not convert the transaction into a span yet, span metrics were so far only extracted from child spans of transactions and standalone spans.
With this change, the transaction span gets its metrics extracted as well.
ref: #2494