Support forwarding structured data to log implementation #26
Conversation
|
LGTM, but again: @Techcable I'm out of touch with slog. :) |
There was a problem hiding this comment.
Right now, this PR has some significant conflicts with your other PR #25 that I just merged.
Do you think you could do the rebase onto the master branch? It seems you are more familiar with log's kv system than I am... 😄
I'll need to look into this a little bit more myself.
I added some basic tests for slog2log in master (tests/slog2log.rs) in commits 4a2f3be...252b6bb . These tests will need to be updated to test for support for structured data now (I think I can handle this part 😉 ).
| */ | ||
| log::logger().log( | ||
| &log::Record::builder() | ||
| .args(format_args!("{}", lazy)) |
There was a problem hiding this comment.
Pardon my ignorance about log, but why aren't we passing non-structured args anymore? Does log not support mixing the two?
There was a problem hiding this comment.
We are, on line 269.
I had to move it there, because I conditionally (at compile time) call a key_values on the record_build, but due to rust-lang/rust#92698, the call to format_args! has to be in the same statement that we call build() in.
I can add a comment explaining that.
Done |
| pub(crate) fn get_kv_source<'a>( | ||
| record: &'a slog::Record<'a>, | ||
| logger_kv: &'a slog::OwnedKVList, | ||
| ) -> std::io::Result<Vec<(String, OwnedValue)>> { |
There was a problem hiding this comment.
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 this Vec I'd be keen to try figure out what forces it.
There was a problem hiding this comment.
Ah I see the lifetimes of keys and values simply don't play nicely between slog's Serializer and log's Visitors. In log, we require an explicit lifetime for keys and values so that methods like get() 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.
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.
4 participants
See: #328