feat(metrics): Metrics extraction config, custom tags [INGEST-421] [INGEST-334] [INGEST-336] #1141
Conversation
Rename metrics extracted from measurements (e.g. measurements.lcp) to plural in order to to be consistent with the rest of the product. This PR only fixes the mock data source of the Metrics API, the actual extraction is fixed as part of getsentry/relay#1141.
relay-server/src/actors/envelopes.rs
Outdated
| } | ||
| let config = match state.project_state.config.transaction_metrics { | ||
| Some(ErrorBoundary::Ok(ref config)) => config, | ||
| None | Some(ErrorBoundary::Err(_)) => return Ok(()), |
There was a problem hiding this comment.
nit: Is this equivalent to _ => return Ok(())?
There was a problem hiding this comment.
yes, i have the tendency to avoid _ but it's not healthy in this case
| } | ||
|
|
||
| let mut push_metric = move |metric: Metric| { | ||
| if config.extract_metrics.contains(&metric.name) { |
There was a problem hiding this comment.
nit: If the config check is the last thing we do for every metric, I would refactor to something like
extract_transaction_metrics(&event, &mut target); // no config!
target.retain(|x| config.extract_metrics.contains(x));Although that opens the question how to handle tags. So probably this is fine the way it is.
There was a problem hiding this comment.
i want to early-return if there's no metrics to extract too, i think if i have the config handling in this function i can also unit-test it better
jjbayer
left a comment
jjbayer
left a comment
There was a problem hiding this comment.
There's an import error, and I assume the project config is missing from the relevant integration tests, but conceptually this looks good!
As in #1141, move session extraction to the metrics_extraction module.
Counterpart to getsentry/sentry#30189