feat(ourlogs): Always set observed time for logs #4559
Conversation
Observed time nanos should be set by the collector in OTel. In the case of Sentry the collector will always be Relay. We may want to allow passing observed time nanos in the future, but we can address that after we move to using EAP items consumers since they rely on 'received' instead of observed time.
| let observed_time = our_log.observed_timestamp_nanos.value().unwrap(); | ||
| assert!(*observed_time > 0); | ||
| assert!(*observed_time >= before_test); | ||
| assert!(*observed_time <= after_test); |
There was a problem hiding this comment.
I looked briefly but didn't see any prior examples of mocking Utc::now(), open to suggestions here.
There was a problem hiding this comment.
Unfortunately as good as it gets, our testing capatbilities for time are really limited at the moment.
relay-ourlogs/src/ourlog.rs
Outdated
| @@ -29,6 +30,10 @@ pub fn otel_to_sentry_log(otel_log: OtelLog) -> OurLog { | |||
|
|
|||
| let mut attribute_data = Object::new(); | |||
|
|
|||
| // We ignore the passed observed time since Relay always acts as the collector in Sentry. | |||
| // We may change this in the future with forwarding Relays. | |||
| let observed_time_unix_nano = Utc::now().timestamp_nanos_opt().unwrap_or(0) as u64; | |||
There was a problem hiding this comment.
What is the precision you need on this, generally we'd use UnixTimestamp::now() here, which has an as_secs(). This limits your precision to per second, but that seems like it should be good enough for the received/observed timestamp?
Feel free to add an as_nanos() to the UnixTimestamp or just doing the multiplication here.
| let observed_time = our_log.observed_timestamp_nanos.value().unwrap(); | ||
| assert!(*observed_time > 0); | ||
| assert!(*observed_time >= before_test); | ||
| assert!(*observed_time <= after_test); |
There was a problem hiding this comment.
Unfortunately as good as it gets, our testing capatbilities for time are really limited at the moment.
relay-common/src/time.rs
Outdated
| pub fn as_nanos(self) -> u64 { | ||
| self.0 * 1_000_000_000 | ||
| } |
There was a problem hiding this comment.
@Dav1dde this what you had in mind? I like it on the UnixTimestamp.
tests/integration/test_ourlogs.py
Outdated
| @@ -77,7 +77,6 @@ def test_ourlog_extraction( | |||
| "project_id": 42, | |||
| "retention_days": 90, | |||
| "timestamp_nanos": int(start.timestamp() * 1e9), | |||
| "observed_timestamp_nanos": int(end.timestamp() * 1e9), | |||
There was a problem hiding this comment.
We have helpers for this, so you can assert timestamps in tests, e.g. time_within_delta() should work here.
These assert/time helpers have a custom eq logic implemented which allows fuzzy-ish time assertions.
There was a problem hiding this comment.
Realized we don't have support for nanos, added it in #4562
For: #4559 Make millies/micros/nanos assertable with our `time_*` assertion helpers.
Summary
Observed time nanos should be set by the collector in OTel. In the case of Sentry the collector will always be Relay. We may want to allow passing observed time nanos in the future, but we can address that after we move to using EAP items consumers since they rely on 'received' instead of observed time.