fix(metrics): Apply global tags to transaction metrics #3615
Conversation
| @@ -115,7 +115,8 @@ pub struct CustomMeasurementConfig { | |||
| /// - Delay metrics extraction for indexed transactions. | |||
| /// - 4: Adds support for `RuleConfigs` with string comparisons. | |||
| /// - 5: No change, bumped together with [`MetricExtractionConfig::MAX_SUPPORTED_VERSION`]. | |||
| const TRANSACTION_EXTRACT_MAX_SUPPORTED_VERSION: u16 = 5; | |||
| /// - 6: Bugfix to make transaction metrics extraction apply globally defined tag mappings. | |||
| const TRANSACTION_EXTRACT_MAX_SUPPORTED_VERSION: u16 = 6; | |||
There was a problem hiding this comment.
We need to bump this version because old Relays will not apply global tags to legacy transaction metrics.
| @@ -324,7 +325,7 @@ impl MetricExtractionConfig { | |||
| /// The latest version for this config struct. | |||
| /// | |||
| /// This is the maximum version supported by this Relay instance. | |||
| pub const MAX_SUPPORTED_VERSION: u16 = 3; | |||
| pub const MAX_SUPPORTED_VERSION: u16 = 4; | |||
There was a problem hiding this comment.
Unfortunately both versions need to be bumped in tandem: If I only bump TRANSACTION_EXTRACT_MAX_SUPPORTED_VERSION, old Relays will still set the metrics_extracted flag to true when they extract on-demand metrics, which means the crucial legacy transaction metrics (duration etc.) will not be extracted.
| /// transaction events. | ||
| fn extract_metrics( | ||
| /// Extract transaction metrics. | ||
| fn extract_transaction_metrics( |
There was a problem hiding this comment.
Apologies for the large diff. The rewrite of this function is trying to achieve:
- Fix the bug referenced in the PR title.
- Make tandem version bumps unnecessary in the future.
- Simplify: This function was originally written as a general purpose function, but since it's only called from
process_transactionswe can introduce some early returns.
| let generic_config = { | ||
| let config = match &state.project_state.config.metric_extraction { | ||
| ErrorBoundary::Ok(ref config) if config.is_supported() => config, | ||
| _ => return Ok(()), |
There was a problem hiding this comment.
Generic project config is no longer optional. event_metrics_extraced will only be set if both configs are present, allowing us to bump the two extraction versions individually in the future.
|
|
||
| let extractor = TransactionExtractor { | ||
| config: tx_config, | ||
| generic_config: Some(&generic_config), |
There was a problem hiding this comment.
This is the actual bug fix: Tag definitions are applied from both global and project config, not just project config.
| "d:transactions/measurements.fcp@millisecond": "outlier", | ||
| "d:transactions/duration@millisecond": "inlier", | ||
| "d:transactions/measurements.lcp@millisecond": "inlier", | ||
| } |
There was a problem hiding this comment.
Without the bug fix, this dict would be empty.
There was a problem hiding this comment.
I copied this from a snapshot in sentry, so the integration test also verifies that the config that sentry provides is applied correctly.
relay-server/Cargo.toml
Outdated
| @@ -141,5 +141,6 @@ insta = { workspace = true } | |||
| relay-event-schema = { workspace = true, features = ["jsonschema"] } | |||
| relay-protocol = { workspace = true, features = ["test"] } | |||
| relay-test = { workspace = true } | |||
| serde_yaml = { workspace = true } | |||
| return Ok(()); | ||
| }) | ||
| } | ||
| let generic_config = { |
There was a problem hiding this comment.
| let generic_config = { | |
| let combined_config = { |
| let Some(event) = state.event.value() else { | ||
| return Ok(()); | ||
| }; |
There was a problem hiding this comment.
Can we move this to the top after state.event_metrics_extracted?
|
@jjbayer watch out we had a self hosted release and the changelog is messed up |
Prerequisite to getsentry/sentry#71004: Global tag mappings were not yet being applied to transaction metrics.
Beside the bug fix, this PR also bumps the metrics extraction version(s), and attempts to simplify the extraction function for transactions.