Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 97 additions & 3 deletions kv.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
use log::kv::value::Error as ValueError;
use slog::{Record, Serializer};
use slog::{Record, Serializer, KV};

use std::fmt::Arguments;

struct Visitor<'s> {
serializer: &'s mut dyn Serializer,
Expand Down Expand Up @@ -60,7 +62,7 @@ impl<'s> log::kv::value::Visit<'s> for KeyVisit<'s> {
visit_to_emit!(&(dyn std::error::Error + 'static): visit_error -> emit_error);
}

impl slog::KV for SourceKV<'_> {
impl KV for SourceKV<'_> {
fn serialize(&self, _record: &Record, serializer: &mut dyn Serializer) -> slog::Result {
// Unfortunately, there isn't a way for use to pass the original error through.
self.0
Expand All @@ -79,4 +81,96 @@ fn to_value_err(err: slog::Error) -> ValueError {
}
}

// TODO: support going the other way
/// Create a [`log::kv::Source`] for the key-value pairs for a slog record.
pub(crate) fn get_kv_source<'a>(
record: &'a slog::Record<'a>,
logger_kv: &'a slog::OwnedKVList,
) -> std::io::Result<Vec<(String, OwnedValue)>> {
Copy link
Collaborator

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 this Vec I'd be keen to try figure out what forces it.

Copy link
Collaborator

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'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.

Copy link
Contributor Author

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.

let mut serialized_source = LogSerializer(vec![]);

record.kv().serialize(record, &mut serialized_source)?;
logger_kv.serialize(record, &mut serialized_source)?;
Ok(serialized_source.0)
}

/// A wrapper around [`log::kv::Value`], that owns the data included.
///
/// In particular this is necessary for strings, and large integers (u128, and i128), because the
/// `Value` type itself only supports references, which must survive for the lifetime of the
/// visitor.
pub(crate) enum OwnedValue {
Value(log::kv::Value<'static>),
Str(String),
U128(Box<u128>),
I128(Box<i128>),
}

impl log::kv::value::ToValue for OwnedValue {
fn to_value(&self) -> log::kv::Value<'_> {
use OwnedValue::*;

match self {
Value(v) => v.to_value(),
Str(s) => s.to_value(),
U128(v) => v.to_value(),
I128(v) => v.to_value(),
}
}
}

struct LogSerializer(Vec<(String, OwnedValue)>);

impl LogSerializer {
fn add(&mut self, key: slog::Key, val: OwnedValue) -> slog::Result {
self.0.push((key.into(), val));
Ok(())
}
}

macro_rules! emit_to_value {
($f:ident : $t:ty) => {
fn $f(&mut self, key: slog::Key, val: $t) -> slog::Result {
self.add(key, OwnedValue::Value(val.into()))
}
};
}

impl Serializer for LogSerializer {
fn emit_arguments(&mut self, key: slog::Key, val: &Arguments<'_>) -> slog::Result {
self.add(key, OwnedValue::Str(val.to_string()))
}

emit_to_value!(emit_usize: usize);
emit_to_value!(emit_isize: isize);
emit_to_value!(emit_bool: bool);
emit_to_value!(emit_char: char);
emit_to_value!(emit_u8: u8);
emit_to_value!(emit_i8: i8);
emit_to_value!(emit_u16: u16);
emit_to_value!(emit_i16: i16);
emit_to_value!(emit_u32: u32);
emit_to_value!(emit_i32: i32);
emit_to_value!(emit_f32: f32);
emit_to_value!(emit_f64: f64);

fn emit_u128(&mut self, key: slog::Key, val: u128) -> slog::Result {
self.add(key, OwnedValue::U128(Box::new(val)))
}

fn emit_i128(&mut self, key: slog::Key, val: i128) -> slog::Result {
self.add(key, OwnedValue::I128(Box::new(val)))
}

fn emit_str(&mut self, key: slog::Key, val: &str) -> slog::Result {
self.add(key, OwnedValue::Str(val.to_string()))
}

fn emit_unit(&mut self, key: slog::Key) -> slog::Result {
use log::kv::ToValue;
self.add(key, OwnedValue::Value(().to_value()))
}

fn emit_none(&mut self, key: slog::Key) -> slog::Result {
self.emit_unit(key)
}
}
38 changes: 16 additions & 22 deletions lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pardon my ignorance about log, but why aren't we passing non-structured args anymore? Does log not support mixing the two?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 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.

.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(())
}
Expand Down