feat(transactions): Extract computed measurements [INGEST-1530] #1373
Conversation
relay-general/src/store/normalize.rs
Outdated
| .and_then(Annotated::value) | ||
| { | ||
| if stall_total_time.unit.value() | ||
| == Some(&MetricUnit::Duration(DurationUnit::MilliSecond)) |
There was a problem hiding this comment.
This strict check means that we do not set stall_percentage if stall_total_time has unit None. Ideally, get_measurement_unit would run as part of store normalization such that we don't have to think about this special case.
There was a problem hiding this comment.
Please ensure that this is considered in #1366. It's still very common for SDKs to skip the unit and it's also not required.
What would be the downside of omitting this check?
There was a problem hiding this comment.
If the SDK sends seconds instead of milliseconds for some reason, we divide seconds by milliseconds. But I'll relax the condition to accept None as well.
relay-general/src/store/normalize.rs
Outdated
| 100.0 * (stall_total_time / transaction_duration_ms), | ||
| ), | ||
| unit: Annotated::new(MetricUnit::Fraction(FractionUnit::Percent)), |
There was a problem hiding this comment.
@wmak I multiply by 100 and set the unit Percent here, because that is what the name percentage seems to imply. But looking at https://github.com/getsentry/sentry/blob/3b3cb01fd16e9ccce104a077efa29a89d8253891/src/sentry/search/events/datasets/discover.py#L1222-L1225, the product actually expects a ratio between 0 and 1?
def _resolve_measurements_stall_percentage(self, _: str) -> SelectType:
return self._resolve_aliased_division(
"measurements.stall_total_time", "transaction.duration", MEASUREMENTS_STALL_PERCENTAGE
)There was a problem hiding this comment.
The definition in search fields makes this a ratio like all others without a multiplication by 100. The product type of "percentage" is actually 0-1. See: https://github.com/getsentry/sentry/blob/8b13ba0e01744d88632d6b8cff185f5644b40c0a/src/sentry/search/events/fields.py#L414-L426
PseudoField(
MEASUREMENTS_STALL_PERCENTAGE,
MEASUREMENTS_STALL_PERCENTAGE,
expression=[
"if",
[
["greater", ["transaction.duration", 0]],
["divide", ["measurements.stall_total_time", "transaction.duration"]],
None,
],
],
result_type="percentage",
),There was a problem hiding this comment.
ACK, will change the unit.
There was a problem hiding this comment.
+1 Jan is correct here, but there's no strong reason for 0-1 or 0-100 it was just an arbitrary decision. So if there was a good reason to we could likely update to 0-100.
There was a problem hiding this comment.
@wmak thanks, let's keep it 0-1.
The following measurements were removed from metrics extraction under the assumption that they could be computed on the fly: frames_slow_rate := measurements.frames_slow / measurements.frames_total frames_frozen_rate := measurements.frames_frozen / measurements.frames_total stall_percentage := measurements.stall_total_time / transaction.duration They cannot, because for example p75(a/b) is not the same as p75(a)/p75(b). This reverts #33725. Relay part implemented here: getsentry/relay#1373
Extract the following derived mobile measurements, and write them back into the transaction:
These new measurements will be available in both the metrics and the transactions dataset.
The addition to the measurement allowlist is done in getsentry/sentry#37330.