ref: Use FiniteF64 for measurements #4828
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
loewenheim
force-pushed
the
sebastian/measurements-finite
branch
from
461866d to
9284fea
Compare
Dav1dde
approved these changes
Jun 24, 2025
loewenheim
commented
Jun 24, 2025
| @@ -643,80 +641,4 @@ mod tests { | |||
| ] | |||
| "###); | |||
| } | |||
|
|
|||
| #[test] | |||
| fn skip_nonfinite_float() { | |||
There was a problem hiding this comment.
This test can't even be expressed now.
| @@ -134,6 +134,19 @@ impl<T> Annotated<T> { | |||
| }) | |||
| } | |||
|
|
|||
| /// Tries to convert a `U` value into `Annotated<T>`. | |||
| pub fn try_from<U>(value: U) -> Self | |||
There was a problem hiding this comment.
This function's signature is kinda wild, but it's pretty useful for creating Annotated<FiniteF64>.
loewenheim
added a commit
that referenced
this pull request
Jun 24, 2025
Some fields of `SpanKafkaMessage` were previously deserialized as `serde_json::Value`, which was partly caused by having to parse and validate `f64` values in `measurements`. As of #4828 that validation is no longer necessary. Consequently, we can now use `RawValue` everywhere. The `inner_produce_protobuf_span` function needs to match on `serde_json::Value`, but we can deserialize the raw values there. The same applies for setting the sample rates in `backfill_data`. This change also removes some unnecessary `Cow`s in `SpanKafkaMessage` where nothing was ever changed or inserted.
loewenheim
added a commit
that referenced
this pull request
Jun 24, 2025
Some fields of `SpanKafkaMessage` were previously deserialized as `serde_json::Value`, which was partly caused by having to parse and validate `f64` values in `measurements`. As of #4828 that validation is no longer necessary. Consequently, we can now use `RawValue` everywhere. The `inner_produce_protobuf_span` function needs to match on `serde_json::Value`, but we can deserialize the raw values there. The same applies for setting the sample rates in `backfill_data`. This change also removes some unnecessary `Cow`s in `SpanKafkaMessage` where nothing was ever changed or inserted.
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jun 30, 2025
Some fields of `SpanKafkaMessage` were previously deserialized as `serde_json::Value`, which was partly caused by having to parse and validate `f64` values in `measurements`. As of #4828 that validation is no longer necessary. Consequently, we can now use `RawValue` everywhere. The `inner_produce_protobuf_span` function needs to match on `serde_json::Value`, but we can deserialize the raw values there. The same applies for setting the sample rates in `backfill_data`. #skip-changelog
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
FiniteF64type is intended forf64values that can't be ±∞ or NaN. It's currently only used for metrics, but measurements ought to work the same way. Therefore, this PR tries to useFiniteF64in measurements.FiniteF64torelay-protocolso that all parts of Relay that need it have access to it.FiniteF64:FromValue,IntoValue,ProcessValue,Empty,PartialOrd<f64>,PartialEq<f64>, andAddAssign. The only notable thing here is that in order to implementProcessValueI added a new methodprocess_finite_f64to theProcessortrait, as simply reusingprocess_f64wouldn't be sound.I've hit a roadblock with the statistical functions in
normalize/utils.rs. Specifically, calculate_cdf_score returns NaN when thep50parameter is 0, making it unsound to use withFiniteF64.How do we deal with this? Do we wrap the calculation in
Optionand then discard anyNone(inf or NaN) values we get? As far as I understand, such values never make it to Kafka anyway because of this check instore.rs.Closes #4817. Closes RELAY-109.