feat(metrics): Extract transaction metrics in external relays [INGEST-1477] #1344
Conversation
Move all non-processing imports out of the processing cfg config in the envelopes actor.
The `metrics_extracted` header must only be used on items that had transactions extracted. This means that relay must not extract metrics from items with such header, to not duplicate metrics.
Use the function to get the config, instead of accessing the struct's property directly.
The transaction item was reused to keep the item's headers. However, no additional payload was being set and this was causing missing data such as missing breakdowns.
relay-server/src/actors/envelopes.rs
Outdated
| .metric_conditional_tagging | ||
| .as_slice(); | ||
|
|
||
| let (event, _) = self.event_from_json_payload(item, Some(EventType::Transaction))?; |
There was a problem hiding this comment.
I don't think this should be necessary. Don't you have access to state.event here?
relay-server/src/actors/envelopes.rs
Outdated
| @@ -346,6 +346,8 @@ struct ProcessEnvelopeState { | |||
| /// extracted. | |||
| event: Annotated<Event>, | |||
|
|
|||
| transaction_item: Option<Item>, | |||
There was a problem hiding this comment.
you can probably store item headers only here, not sure if it's feasible though
relay-server/src/actors/envelopes.rs
Outdated
| .metric_conditional_tagging | ||
| .as_slice(); | ||
|
|
||
| let (event, _) = self.event_from_json_payload(item, Some(EventType::Transaction))?; |
There was a problem hiding this comment.
Why parse the event again here?
relay-server/src/actors/envelopes.rs
Outdated
| let event_type = state.event_type().unwrap_or_default(); | ||
| let mut event_item = Item::new(ItemType::from_event_type(event_type)); | ||
| let mut event_item = match state.event_type().unwrap_or_default() { | ||
| EventType::Transaction => state.transaction_item.take().unwrap(), |
There was a problem hiding this comment.
There's usually a better way than unwrap(). E.g. store event_item on the state instead of event_type and transaction_item, and then get the type from the item.
| #[cfg(feature = "processing")] | ||
| impl TransactionMetricsConfig { | ||
| pub fn is_enabled(&self) -> bool { | ||
| self.version > 0 && self.version <= EXTRACT_MAX_VERSION |
There was a problem hiding this comment.
Quick thought: We could actually remove the self.version > 0 check and set EXTRACT_MAX_VERSION = 0. That way, we would not need any changes on the sentry side. Or am I missing something crucial?
There was a problem hiding this comment.
Actually, let's keep it as-is to be consistent with session metrics extraction.
relay-server/src/actors/envelopes.rs
Outdated
| let mut event_item = match state.event_type().unwrap_or_default() { | ||
| EventType::Transaction => state.transaction_item.take().unwrap(), | ||
| ty => Item::new(ItemType::from_event_type(ty)), | ||
| }; |
There was a problem hiding this comment.
I assume we reuse the item here because we do not want to lose the metrics_extracted information. But this could have side effects (I don't know what else is in that item header). So it might be safer to explicitly transfer the metrics_extracted information to the new item, as we do with sample_rates a few lines below.
Two minor tweaks to #1344, based on comments on that PR: - Do not parse the event payload twice. - Store a boolean on the envelope state, rather than the full event item.
Just like we did for session metrics extraction, add a version to the project config protocol such that older Relays will stop extracting metrics when the version supplied by Sentry is too high. This is a prerequisite for getsentry/relay#1344.
Currently, transaction metrics are only extracted in processing relays (the function call to extract metrics is surrounded by an
if_processing!). Even if we extract metrics in external relays, we don't want to extract metrics multiple times from a transaction, causing metric (and billing) duplication.The implemented approach in this PR is to extract metrics in the very first relay in the chain, mark the transaction as extracted, and not extract metrics from that transaction in subsequent relays. The simplest way to accomplish that is using the existing
metrics_extractedheader in items that had metrics extracted. The approach and the header are reused from the metric extraction from sessions, there's nothing new here.Implementation-wise, removing that
if_processingtriggers a lot of changes to make current processing-only features non-processing too. On the other hand, we store a new flagtransaction_metrics_extractedonProcessEnvelopeStateto make it available to the whole state processing function and transfer it to the outgoing event. Ideally,process_stateis refactored not to throw every piece of data we need to the envelope state object, but that's out of the scope of this PR.