feat(metrics): Allow ingestion of custom metric units [INGEST-1131] #1256
Conversation
|
I'm not going to fix up breakdown computation with support for custom units in this PR. To do this in a type-safe fashion, I want to consistently pass |
|
We agreed in acceptance criteria that violations of the well-known units should be logged. This still needs to be added along with a test case before merging. |
relay-common/src/constants.rs
Outdated
| "ratio" => Self::Fraction(FractionUnit::Ratio), | ||
| "percent" => Self::Fraction(FractionUnit::Percent), | ||
|
|
||
| "" | "unit" | "none" => Self::None, |
There was a problem hiding this comment.
Had a discussion offline about if "unit" should pattern matched to Self::None. The reasoning that this was put in place was to track the unit type () that exists in different languages. Does this still make sense? I would rather remove it.
Also worthwhile syncing to see if we can better match OpenTelemetry in general here, particularly around defining dimensionless units, and opting into custom units (like recording X number of Y - for ex with a counter for network packets, 100 (measure) packets (unit)).
https://opentelemetry.io/docs/reference/specification/metrics/semantic_conventions/#instrument-units
Units should follow the Unified Code for Units of Measure (need more clarification in #705).
- Instruments for utilization metrics (that measure the fraction out of a total) are dimensionless and SHOULD use the default unit 1 (the unity).
- Instruments that measure an integer count of something SHOULD only use annotations with curly braces to give additional meaning without the leading default unit (1). For example, use {packets}, {errors}, {faults}, etc.
There was a problem hiding this comment.
Thanks, agreed to remove "unit" from here. This has not been used actively, so we can safely remove it.
Other than that, this PR merely moves the existing unit definition and derives new traits. We can ideally discuss units in a new DACI or GitHub issue. Units are definitely not ideal yet, and we're also not able to support X per Y units (e.g. frames per second). Note that this is different from fractions.
| let default_unit = get_metric_measurement_unit(name); | ||
| if let (Some(default), Some(stated)) = (default_unit, stated_unit) { | ||
| if default != stated { | ||
| relay_log::error!("unit mismatch on measurements.{}: {}", name, stated); |
There was a problem hiding this comment.
I wonder if we can bring the SDK name into the error message, so we immediately know where this came from.
There was a problem hiding this comment.
The SDK is already on the context since we're in envelope processing.
relay/relay-server/src/actors/envelopes.rs
Lines 1867 to 1875 in 2ca65bc
Generally, we should aim for central configuration rather than overloading code with error reporting boilerplate.
Instructions and example for changelogFor changes exposed to the Python package, please add an entry to For changes to the Relay server, please add an entry to
To the changelog entry, please add a link to this PR (consider a more descriptive message): - Allow ingestion of custom metric units. ([#1256](https://github.com/getsentry/relay/pull/1256))If none of the above apply, you can opt out by adding #skip-changelog to the PR description. |
Introduces
MetricUnitinmeasurements[].uniton the event protocol. This allows SDKs to submit units for custom measurements, as well as override units on well-known measurements defined in the past. Metric extraction uses these units when supplied, falling back to the predefined units in their absence.Follow-ups:
Durationfor type safetyThis connects to: