refactor(logs): cache default attributes and add OS attributes #842
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #842 +/- ##
==========================================
+ Coverage 73.82% 74.04% +0.22%
==========================================
Files 64 64
Lines 7762 7778 +16
==========================================
+ Hits 5730 5759 +29
+ Misses 2032 2019 -13 🚀 New features to boost your workflow:
|
sentry-core/src/client.rs
Outdated
| if let Some(default_attributes) = self.default_log_attributes.clone() { | ||
| for (key, val) in default_attributes.into_iter() { | ||
| log.attributes.entry(key).or_insert(val); |
There was a problem hiding this comment.
I would iterate over default_log_attributes and then clone each key/val separately. This should be cheaper as it avoids having to clone/deallocate the container itself.
There was a problem hiding this comment.
Okay, I thought this would be better because it's just a single call to clone but maybe I'm wrong (and the compiler does whatever it wants anyways). I'll do it like you said.
There was a problem hiding this comment.
Still reminds me that splitting the SDK up into multiple crates was a really bad decision :-D
I think the sentry-core and sentry split is useful, maybe it would be better for all integrations to live in sentry.
There was a problem hiding this comment.
But I guess this is a topic for v1 😄
There was a problem hiding this comment.
Fwiw this problem of fetching the OS context applies to other SDKs such as Java, there we will add a new method to the Integration interface to process logs.
I like this approach we're using here more as it seems like a one-off thing and doesn't require API changes, especially as they wouldn't be aligned across SDKs.
We can always revisit in the future.
os.nameandos.versionattributes to the log, which are mandatory for native SDKs according to the spec, and personally I think they could be useful.sentry-corewithout making breaking changes or exposing additional API surface is the one I'm proposing, which still sucks.