crypto: Include event timestamp in decryption failure logs #3194
Conversation
| // a max year of 285427, so this can overflow for very large timestamps. (The | ||
| // Y10K problem!) | ||
| match OffsetDateTime::from_unix_timestamp_nanos(nanos_since_epoch) { | ||
| Err(_) => "<out of range>".to_owned(), |
There was a problem hiding this comment.
It's a bad API smell that the utility function decides what to do in case of errors. Instead, could we have this method return Option<String>, and in this branch it could return None?
(or more concisely:
OffsetDateTime::from_unix_timestamp_nanos(nanos_since_epoch).ok()?.format(&...)There was a problem hiding this comment.
I take your point, but I feel like that is somewhat in conflict with its intended use; if we use this function in several span.record calls, each of them is going to end up with an identical .unwrap_or("<out_of_range>) incantation and pretty soon we'll want another utility function to reduce the duplication.
Maybe the duplication is ok though?
There was a problem hiding this comment.
I think the duplication would be OK, I also get the function is an internal utility. But having it return Option could also make it so that we avoid the unwrap below, so I'm tempted to insist just a bit on returning an Option here, even though overall I don't have a very strong opinion.
| // Y10K problem!) | ||
| match OffsetDateTime::from_unix_timestamp_nanos(nanos_since_epoch) { | ||
| Err(_) => "<out of range>".to_owned(), | ||
| Ok(dt) => dt.format(&Iso8601::<ISO8601_WITH_MILLIS>).unwrap(), |
There was a problem hiding this comment.
We try to avoid unwrap() without a SAFETY: comment that explains why it's safe to unwrap here. Can you add that? (Or consider returning None instead incase of error/none?)
bnjbvr
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3194 +/- ##
==========================================
- Coverage 83.84% 83.83% -0.01%
==========================================
Files 232 232
Lines 23919 23925 +6
==========================================
+ Hits 20054 20058 +4
- Misses 3865 3867 +2 ☔ View full report in Codecov by Sentry. |
|
Have hopefully addressed the review comments. |
Co-authored-by: Benjamin Bouvier <[email protected]> Signed-off-by: Richard van der Hoff <[email protected]>
Turns out this is quite useful information when analysing decryption failures.