-
-
Notifications
You must be signed in to change notification settings - Fork 166
feat(logs): add ability to capture and send logs #823
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #823 +/- ##
==========================================
- Coverage 72.94% 72.61% -0.34%
==========================================
Files 63 63
Lines 7477 7598 +121
==========================================
+ Hits 5454 5517 +63
- Misses 2023 2081 +58 🚀 New features to boost your workflow:
|
); | ||
} | ||
|
||
// TODO: set OS (and Rust?) context |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cannot do this at the moment because sentry-contexts
adds those through an event processor, which logs don't go through.
} | ||
|
||
#[cfg(feature = "UNSTABLE_logs")] | ||
fn set_log_default_attributes(&self, log: &mut Log) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is what all other SDKs are doing? IMO it might be a bit wasteful to copy the same fields into the log event for every single log event.
In the likely case that users are not logging any attributes, this will allocate a hashmap, and clone at least 4 * 2
Strings into it. That is quite some overhead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's true. Unfortunately this is what the spec mandates, every single log should have these default attributes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you see any way of optimizing it? @Swatinem
Maybe we could pre populate a Map with some of the default attributes and store it somewhere to reuse it, but we would still need to clone it for every log.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about using a more optimized string type (like my own https://docs.rs/smol_buf/latest/smol_buf/) that can avoid clones in favor of refcounting or small string optimization.
That would cut down on memory usage and allocations a bit. Though the hashmap might still be heavyweight, and hard to avoid.
Not sure these things are worth the effort though? Considering that those would appear everywhere in public types, so it would likely cause some pain for users as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I don't know.
I was also thinking of optimizations using Cow
s, providing special fields to store the default attributes that would take in a Cow
. This way we wouldn't clone.
But the special fields would need to be pub
anyway, because the struct lives in sentry-types
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can merge it as is for now and we can always think about optimizing in the future.
However I agree it kinda sucks we need to attach this to every single log.
Co-authored-by: Arpad Borsos <[email protected]>
|
Log
s with a methodcapture_log
on theHub
that takes in aLog
.UNSTABLE_logs
flag. This is needed because we will use a thread to batch and flush logs, which would not work in e.g. WASM environments.UNSTABLE
and make it justlogs
in a future release.enable_logs
was added to enable sending them.Note that this is not the final API.
The final API will come in a follow-up PR and will be similar to that of
tracing
, using macros for each level and taking in message and attributes.Example:
sentry::logger::warn!(my.attribute = 42, "structured warning: {}", "something");
Closes #798
Comes after #821