-
-
Notifications
You must be signed in to change notification settings - Fork 165
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
base: lcian/feat/logs-types
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## lcian/feat/logs-types #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.
@@ -2127,7 +2127,8 @@ pub struct Log { | |||
/// The log body/message (required). | |||
pub body: String, | |||
/// The ID of the Trace in which this log happened (required). | |||
pub trace_id: TraceId, | |||
#[serde(default, skip_serializing_if = "Option::is_none")] | |||
pub trace_id: Option<TraceId>, |
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.
Making this optional because conceptually the TraceId
should be filled when applying scope to the log, which is done after the top-level API is called.
if let Some(res) = func(log) { | ||
log = res | ||
} else { | ||
return None; | ||
} |
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.
if let Some(res) = func(log) { | |
log = res | |
} else { | |
return None; | |
} | |
log = func(log)?; |
} | ||
|
||
#[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.
|
||
if send_default_pii { | ||
if !log.attributes.contains_key("user.id") { | ||
if let Some(id) = self.user().and_then(|user| user.id.as_ref()) { |
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.
maybe move the if let Some(user) = self.user()
one level up, and then check for the contains_key
within that block.
I think that might be a bit more efficient. Though I’m unsure if self.user()
returns a reference or a clone.
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