ref: Use Duration to compute breakdowns and span durations [INGEST-1131] #1260
Conversation
| Some(get_duration_millis(start_timestamp, end_timestamp)) | ||
| } | ||
| SatisfactionMetric::Duration => Some(relay_common::signed_duration_to_millis( | ||
| end_timestamp - start_timestamp, |
There was a problem hiding this comment.
I think this was intentionally a saturating_sub. But this sort of normalization behavior I do think we want to have in a centralized function.
There was a problem hiding this comment.
Check the doc comment and implementation of signed_duration_to_millis. It has that normalization behavior now, so we no longer need to perform a saturating sub here.
There was a problem hiding this comment.
ah okay. hmm. it kinda does mean that the duration can underflow now before it is clamped. I am not sure how this is handled by chrono but probably not likely enough
There was a problem hiding this comment.
The subtraction is Sub<DateTime<Utc>> for DateTime<Utc> which is runs signed_duration_since internally. That method is documented with:
This does not overflow or underflow at all.
| (delta / 1_000_000.00).abs() | ||
| pub fn duration(&self) -> Duration { | ||
| // Cannot fail since durations are ordered in the constructor | ||
| (self.end - self.start).to_std().unwrap_or_default() |
There was a problem hiding this comment.
nit: We could also make TimeWindowSpan store a timestamp and a duration to make the unwrap here unnecessary.
jan-auer
force-pushed
the
feat/breakdowns-duration
branch
from
9679448
to
a38e2c8
Compare
* master: feat(sampling): Support custom tags and more contexts (#1268) deps: Update symbolic to pull in Unreal parser fixes (#1266) Bring in CLA Lite (#1257) release: 0.8.11 ref(profiling): Normalize all profiles (#1250) feat(profiling): Add a profile data category and count profiles in an envelope to apply rate limits (#1259) fix: Raise a new InvalidCompression Outcome for invalid Unreal compression (#1237) chore: Update logo for dark or light theme (#1262) ref: Use Duration to compute breakdowns and span durations (#1260)
Instead of using raw
f64, this PR switches to usingDurationfor computing span durations and breakdowns. This allows to pass the values around without the additional knowledge in which unit they are. Currently, this was only described in comments.As part of this refactor, I'm simplifying the actual computation algorithm. There are two repeated changes:
It was actually not necessary to perform this refactor just for introducing custom units because breakdowns are always created as milliseconds within Relay.
This is a follow-up to #1256
#skip-changelog