-
Notifications
You must be signed in to change notification settings - Fork 26
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
Support forwarding structured data to log implementation #26
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -251,31 +251,25 @@ impl slog::Drain for StdLog { | |
|
||
let lazy = LazyLogString::new(info, logger_values); | ||
/* | ||
* TODO: Support `log` crate key_values here. | ||
* | ||
* This requires the log/kv_unstable feature here. | ||
* | ||
* Not supporting this feature is backwards compatible | ||
* and it shouldn't break anything (because we've never had), | ||
* but is undesirable from a feature-completeness point of view. | ||
* | ||
* However, this is most likely not as powerful as slog's own | ||
* notion of key/value pairs, so I would humbly suggest using `slog` | ||
* directly if this feature is important to you ;) | ||
* | ||
* This avoids using the private log::__private_api_log api function, | ||
* which is just a thin wrapper around a `RecordBuilder`. | ||
*/ | ||
log::logger().log( | ||
&log::Record::builder() | ||
.args(format_args!("{}", lazy)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pardon my ignorance about There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are, on line 269. I had to move it there, because I conditionally (at compile time) call a I can add a comment explaining that. |
||
.level(level) | ||
.target(target) | ||
.module_path_static(Some(info.module())) | ||
.file_static(Some(info.file())) | ||
.line(Some(info.line())) | ||
.build(), | ||
); | ||
let mut record_builder = log::Record::builder(); | ||
record_builder | ||
.level(level) | ||
.target(target) | ||
.module_path_static(Some(info.module())) | ||
.file_static(Some(info.file())) | ||
.line(Some(info.line())); | ||
#[cfg(feature = "kv_unstable")] | ||
let source = kv::get_kv_source(info, logger_values)?; | ||
#[cfg(feature = "kv_unstable")] | ||
record_builder.key_values(&source); | ||
|
||
// Due to https://github.com/rust-lang/rust/issues/92698, it is necessary to wait until we | ||
// actually call `.build()` to use `format_args`. And we split that out so that we can | ||
// conditionally call `key_values` based on the kv_unstable feature flag | ||
log::logger().log(&record_builder.args(format_args!("{}", lazy)).build()); | ||
|
||
Ok(()) | ||
} | ||
|
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.
You've clearly spent some time figuring all this integration out (nice work by the way! Especially given the
log
key-value APIs are basically undocumented at this point), but if it's desirable to avoid thisVec
I'd be keen to try figure out what forces it.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.
Ah I see the lifetimes of keys and values simply don't play nicely between
slog
'sSerializer
andlog
'sVisitor
s. Inlog
, we require an explicit lifetime for keys and values so that methods likeget()
can return borrowed data, so you aren't really left with much alternative. If this became a major issue we could consider alternative strategies to re-use allocations where possible and things like that.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, the lifetimes don't quite match up. Ideally, slog would use a similar lifetime strategy. That might actually avoid needing allocations in some existing slog adapters. But that would be a breaking change for slog, and probably wouldn't be worth the churn.