rust-lang / log

Logging implementation for Rust
https://docs.rs/log
Apache License 2.0
2.2k stars 252 forks source link

Simple zero-dependency visitor for kv::Value #440

Closed KodrAus closed 2 years ago

KodrAus commented 3 years ago

Motivation

Users want to be able to visit simple primitives captured in kv::Values but currently resort to chaining calls like:

if let Some(v) = value.to_i64() {
    ..
} else if let Some(v) = value.to_u64() {
    ..
} ..

This is flaky and not fun to write. The goal of the current design is that if you're serializing values you probably already have a serialization framework handy (either std::fmt, serde, or sval) that you'll want to use.

There are reasons to want to visit values without pulling one of these in though. For example, https://github.com/Thomasdezeeuw/std-logger/pull/29#discussion_r553903486 wants to write custom formatting logic to make use of IoSlices, and others don't want the additional mental overhead in learning another framework.

Proposal

Add a simple zero-dependency visit API for log::kv::Value that can be used instead of chaining to_* methods or learning a full serialization framework. It intentionally won't support complex values, but should be enough for simple cases to get off the ground with. A new trait, kv::value::Visitor will be added with the following methods:

pub mod value {
    pub trait Visitor<'v> {
        fn visit_i64(&mut self, v: i64) -> Result<(), Error> { self.visit_any(v.to_value()) }
        fn visit_u64(&mut self, v: u64) -> Result<(), Error> { self.visit_any(v.to_value()) }
        fn visit_bool(&mut self, v: bool) -> Result<(), Error> { self.visit_any(v.to_value()) }
        fn visit_str(&mut self, v: &str) -> Result<(), Error> { self.visit_any(v.to_value()) }
        fn visit_borrowed_str(&mut self, v: &'v str) -> Result<(), Error> { self.visit_str(v) }
        fn visit_error(&mut self, v: &(dyn std::error::Error + 'static)) -> Result<(), Error> { self.visit_any(Value::from_dyn_error(v)) }
        fn visit_borrowed_error(&mut self, v: &'v (dyn std::error::Error + 'static)) -> Result<(), Error> { self.visit_error(v) }

        // NOTE: This is the only required method
        fn visit_any(&mut self, v: Value) -> Result<(), Error>;
    }
}

with a default implementation looking something like:

impl<'v> value::Visitor<'v> for MyVisitor {
    fn visit_any(&mut self, v: Value) -> Result<(), Error> {
        write!(self.buf, "{}", v)?;
        Ok(())
    }
}

A Value can be visited using an inherent visit method:

impl<'v> Value<'v> {
    pub fn visit(&self, visitor: impl Visitor<'v>) -> Result<(), Error> {
        // Here we know what kind of value we're looking at,
        // so can call the most specific method on `value::Visitor`
    }
}

Other methods can be overridden to customize their behavior.

Migrating from value::Visitor to sval or serde

If callers want to switch from value::Visitor to sval or serde to support more complex serialization needs, the change will be from calling Value::visit to either <Value as sval::Value>::stream or <Value as serde::Serialize>::serialize.

Questions

Why not add these to source::Visitor?

The source::Visitor trait is centred around visit_pair, which lets us build higher-level methods on top, like get. If we add other methods to source::Visitor it may become confusing how the trait should be implemented to support these higher-level methods.

Why does visit_any take a Value?

The Value type is a carrier for a bunch of trait implementations, fmt::Debug, fmt::Display, and possibly sval::Value, and serde::Serialize. By passing in a Value here we give the caller flexibility to decide what trait they want to use without adding a lot of complexity to the value::Visitor trait.

KodrAus commented 3 years ago

If we wanted to re-introduce the Fill API which would be needed to do zero-cost bridging between log and tracing then this Visitor type could be used in place of that Slot, which is really just a value::Visitor.

KodrAus commented 3 years ago

I've updated the proposed trait based on the design that ended up coming out of value-bag in https://github.com/sval-rs/value-bag/pull/16

All it really does is add a 'v lifetime to the value::Visitor trait, so that it becomes possible to capture borrowed strings across calls if you need. This 'v lifetime will be the same as the 'kvs in Visitor::visit_pair<'kvs> so it ends up being threaded all the way through from a borrow of the Source to visitors of the Values.

KodrAus commented 3 years ago

Now that we've got 0.4.14 out we can make progress on this :tada:

Thomasdezeeuw commented 2 years ago

@KodrAus what parts are missing to implement this?

KodrAus commented 2 years ago

Hey @Thomasdezeeuw :wave:

The implementation work was actually all there, so I've opened up #467 that surfaces the visitor API.