ref(store): Normalize units in event payload [INGEST-1638] #1488
Conversation
| // Default | ||
| _ => None, | ||
| } | ||
| } |
There was a problem hiding this comment.
This function was moved 1:1 from transaction metrics extraction.
relay-general/src/store/normalize.rs
Outdated
|
|
||
| let stated_unit = measurement.unit.value().copied(); | ||
| let default_unit = get_metric_measurement_unit(name); | ||
| if let (Some(default), Some(stated)) = (default_unit, stated_unit) { |
There was a problem hiding this comment.
It seems default is now a keyword in Rust. Although still experimental, see https://github.com/rust-lang/rfcs/blob/master/text/1210-impl-specialization.md. Can we use another name?
| if let (Some(default), Some(stated)) = (default_unit, stated_unit) { | |
| if let (Some(expected), Some(actual)) = (default_unit, stated_unit) { |
| measurement | ||
| .unit | ||
| .set_value(Some(stated_unit.or(default_unit).unwrap_or_default())) |
There was a problem hiding this comment.
If stated_unit is None and default_unit is Some we should also throw a mismatch error, and same for the opposite case of Some and None. If both are None, then it's fine.
There was a problem hiding this comment.
👍 to prioritizing stated_unit here.
There was a problem hiding this comment.
FWIW I copied this logic from
Let's break down the use cases:
- If stated_unit is None and default_unit is Some, there is no mismatch, because we allow SDKs to omit the unit for builtin measurements.
- If stated_unit is Some and default_unit is None, we are dealing with a custom measurement for which we don't have a default. In this case, we accept any unit, so there is also no mismatch.
Note the subtle difference between None (unit not set) and Some(MetricUnit::None) (this is a unitless measurement).
Now that I think of it, get_metric_measurement_unit should probably use the allowlist passed down from Sentry to determine default units. I'll leave that for a follow-up PR.
There was a problem hiding this comment.
Note the subtle difference between None (unit not set) and Some(MetricUnit::None) (this is a unitless measurement).
Yep, I messed that one hehe. And I wasn't aware of omitting the unit was allowed. So all good then 👍.
Now that I think of it, get_metric_measurement_unit should probably use the allowlist passed down from Sentry to determine default units. I'll leave that for a follow-up PR.
I would not do that for now. At the moment, that'd mean using the project config, and I think the confusion of doing that is bigger vs the work and frequency to add/update new built-in metrics.
Co-authored-by: Iker Barriocanal <[email protected]>
The allowlist introduced in #1483 strictly checks the measurement unit. For that to work, we need to normalize the event unit first. Until now, unit normalization happened in metrics extraction. This PR moves it into (light) normalization.