Build out initial key values API #324

2019-03-13T16:37:05-05:00

KodrAus commented

2019-03-13 16:37:05 -05:00

(Migrated from github.com)

Introduces an initial structured logging API that's feature gated by the opt-in kv_unstable. APIs behind this feature gate aren't considered stable and may get broken between patch releases that happen to include them.

This implementation requires rustc 1.21.0 for making it nicer to treat a closure as a fn pointer, and for making it nicer to capture a constant local with a lifetime that's wider than its local scope. Since the feature gate is opt-in we aren't going to break anyone currently on compilers before 1.21.0. If we want to make this API available by default then we'll have to bump to 1.21.0 (which seems ok to me).

There is a lot of new code in here, so I appreciate that reviews might take time and it could be a lot of back-and-forth before we're happy. The core pieces are:

trait Source: A source for key-value pairs. Like a repeatable iterator for key-value pairs.
trait Visitor: The mutable part of the repeatable iterator for key-value pairs. It's driven by a Source.
struct Key: The key in a key-value pair. A container around a string that gives us room to abstract over ownership and add an optional index.
struct Value: The value in a key-value pair. A container around an erased fragment of data where its structure is retained by an internal serialization contract. Right now you can only format a value using its debug implementation, but the machinery is there for structured backends (I ended up needing it to implement impl<T: ToValue> ToValue for Option<T>).
struct KeyValueError: An error working with a key-value pair that lets us propagate errors serializing a key-value pair naturally. It's expected that consumers will be producing these.

The implementation is based on the structured logging RFC with a key difference:

The Source trait is object-safe. That means we can't add convenient non-object-safe methods like for_each to it even though I've found those super useful in practice and I think we should add them eventually. Making Source object-safe saves some clumsy machinery though and we can work around providing non-object-safe methods:

Non object-safe convenient methods that implementors will want to override like get and to_owned can be made object-safe. For instance, get could be changed from fn get<Q: ToKey>(&self, key: Q) -> Option<Value> to fn get(&self, key: Key) -> Option<Value>.
Non-chaining convenient methods like for_each and as_map can be added as inherent impls on dyn Source and/or to an extension trait.
Chaining convenient methods like chain and filter can be added by an extension trait.

Once we've merged I'd like to work on support in env_logger and integration with slog and tokio-trace and use those as drivers for the next round.

r? @sfackler

For #149 Introduces an initial structured logging API that's feature gated by the opt-in `kv_unstable`. APIs behind this feature gate aren't considered stable and may get broken between patch releases that happen to include them. This implementation requires rustc `1.21.0` for making it nicer to treat a closure as a fn pointer, and for making it nicer to capture a constant local with a lifetime that's wider than its local scope. Since the feature gate is opt-in we aren't going to break anyone currently on compilers before `1.21.0`. If we want to make this API available by default then we'll have to bump to `1.21.0` (which seems ok to me). There is a _lot_ of new code in here, so I appreciate that reviews might take time and it could be a lot of back-and-forth before we're happy. The core pieces are: - `trait Source`: A source for key-value pairs. Like a repeatable iterator for key-value pairs. - `trait Visitor`: The mutable part of the repeatable iterator for key-value pairs. It's driven by a `Source`. - `struct Key`: The key in a key-value pair. A container around a string that gives us room to abstract over ownership and add an optional index. - `struct Value`: The value in a key-value pair. A container around an erased fragment of data where its structure is retained by an internal serialization contract. Right now you can only format a value using its debug implementation, but the machinery is there for structured backends (I ended up needing it to implement `impl<T: ToValue> ToValue for Option<T>`). - `struct KeyValueError`: An error working with a key-value pair that lets us propagate errors serializing a key-value pair naturally. It's expected that consumers will be producing these. The implementation is based on [the structured logging RFC](https://github.com/rust-lang-nursery/log/blob/master/rfcs/0296-structured-logging.md) with a key difference: The `Source` trait is object-safe. That means we can't add convenient non-object-safe methods like `for_each` to it even though I've found those super useful in practice and I think we should add them eventually. Making `Source` object-safe saves some clumsy machinery though and we can work around providing non-object-safe methods: - Non object-safe convenient methods that implementors will want to override like `get` and `to_owned` can be made object-safe. For instance, `get` could be changed from `fn get<Q: ToKey>(&self, key: Q) -> Option<Value>` to `fn get(&self, key: Key) -> Option<Value>`. - Non-chaining convenient methods like `for_each` and `as_map` can be added as inherent impls on `dyn Source` and/or to an extension trait. - Chaining convenient methods like `chain` and `filter` can be added by an extension trait. Once we've merged I'd like to work on support in `env_logger` and integration with `slog` and `tokio-trace` and use those as drivers for the next round. r? @sfackler

KodrAus commented

2019-03-13 16:45:41 -05:00

(Migrated from github.com)

~~Note: CI won't pass yet, I have to tweak it to only run on 1.21.0+ for the new structured API.~~

Also missing in this first round:

Docs. I haven't bothered writing nice docs when things are likely to change a lot.
A bunch of Source trait impls for things like BTreeMaps.
A bunch of ToValue trait impls for things like SocketAddrs.

~~Note: CI won't pass yet, I have to tweak it to only run on `1.21.0+` for the new structured API.~~ Also missing in this first round: - Docs. I haven't bothered writing nice docs when things are likely to change a lot. - A bunch of `Source` trait impls for things like `BTreeMap`s. - A bunch of `ToValue` trait impls for things like `SocketAddrs`.

KodrAus (Migrated from github.com) reviewed

2019-03-24 17:05:33 -05:00

src/key_values/value/any.rs

pub types in this module are currently pub(super) in visibility.

`pub` types in this module are currently `pub(super)` in visibility.

KodrAus commented

2019-03-25 18:01:56 -05:00

(Migrated from github.com)

r? @dtolnay

KodrAus commented

2019-03-25 18:17:41 -05:00

(Migrated from github.com)

Sorry, I don't mean to put you on the spot to feel compelled to invest your time into reviewing this large new API. If you don't have the bandwidth/interest that's no problem :) It'd be great to at least get a sanity check from somebody on this new code since we the Libs team will be supporting it at some point.

If we'd like to try minimise the amount of bandwidth needed from the wider Libs team to flesh this feature out I'm also ok to continue landing stuff behind this unstable feature-gate and defer a thorough review to a point where everything is implemented and there's experimental integration from structured frameworks in the ecosystem.

Sorry, I don't mean to put you on the spot to feel compelled to invest your time into reviewing this large new API. If you don't have the bandwidth/interest that's no problem :) It'd be great to at least get a sanity check from somebody on this new code since we the Libs team will be supporting it at some point. If we'd like to try minimise the amount of bandwidth needed from the wider Libs team to flesh this feature out I'm also ok to continue landing stuff behind this unstable feature-gate and defer a thorough review to a point where everything is implemented and there's experimental integration from structured frameworks in the ecosystem.

KodrAus (Migrated from github.com) reviewed

2019-03-27 16:19:43 -05:00

src/key_values/error.rs

An alternative here could be to make this error like fmt::Error, so it doesn't carry any context on its own and is easy to create.

An alternative here could be to make this error like `fmt::Error`, so it doesn't carry any context on its own and is easy to create.

dtolnay commented

2019-04-01 21:46:16 -05:00

(Migrated from github.com)

I would be happy to review but I just got home from 2 weeks of travel and have a substantial backlog. Will aim to get you some feedback within the next 14 days.

❤️ 1

KodrAus (Migrated from github.com) reviewed

2019-04-09 00:59:17 -05:00

.travis.yml

					
				@ -10,0 +13,4 @@

				        - cargo test --verbose --features "kv_unstable std"

				    - rust: stable

				      script:

				        - rustup target add thumbv6m-none-eabi

I ended up rolling in this tweak to our CI so we do the embedded build as a separate task like the kv_unstable one, but it does mean we only check it on stable instead of also on beta and nightly. I can revert this change if we don't like it.

I ended up rolling in this tweak to our CI so we do the embedded build as a separate task like the `kv_unstable` one, but it does mean we only check it on stable instead of also on beta and nightly. I can revert this change if we don't like it.

dtolnay (Migrated from github.com) reviewed

2019-04-17 02:38:37 -05:00

dtolnay (Migrated from github.com) left a comment

I haven't gotten a chance to look at key_values/value/*.rs at all yet, but here are a few comments on the rest. My first impression is that this seems promising and very nicely put together! I would be on board with landing something like this.

I haven't gotten a chance to look at *key_values/value/\*.rs* at all yet, but here are a few comments on the rest. My first impression is that this seems promising and very nicely put together! I would be on board with landing something like this.

src/key_values/error.rs

Returning None is the default behavior so we can leave out this method.

I don't like doing best-effort implementations of Error::description. It is deprecated and people should learn not to use it. We need to keep the method here for now to support old compilers, but I would prefer to return some not particularly meaningful string like "key_value error" instead.

I don't like doing best-effort implementations of Error::description. It is deprecated and people should learn not to use it. We need to keep the method here for now to support old compilers, but I would prefer to return some not particularly meaningful string like `"key_value error"` instead.

Could you see whether you can set your editor to end every line with a newline? Without that, this isn't a "text file" as defined by POSIX and behaves badly with some cli commands.

https://unix.stackexchange.com/questions/18743/whats-the-point-in-adding-a-new-line-to-the-end-of-a-file
https://stackoverflow.com/questions/729692/why-should-text-files-end-with-a-newline

Could you see whether you can set your editor to end every line with a newline? Without that, this isn't a "text file" as defined by POSIX and behaves badly with some cli commands. https://unix.stackexchange.com/questions/18743/whats-the-point-in-adding-a-new-line-to-the-end-of-a-file https://stackoverflow.com/questions/729692/why-should-text-files-end-with-a-newline

src/key_values/key.rs

Would it make sense to provide impl ToKey for str instead?

Would it make sense to provide `impl ToKey for str` instead?

Spelling: conversion

You wrote:

A container around a string that gives us room to abstract over ownership and add an optional index.

And in the RFC I see:

The Key type could be extended to hold an optional index into a source. This could be used to retrieve a specific key-value pair more efficiently than scanning. There's some subtlety to consider though when the sources of keys are combined in various ways that might invalidate a previous index.

I know this isn't designed yet but is there anything you would be able to describe about what use cases you would like for the index to enable and roughly what sort of code would they want to write using indices?

You wrote: > A container around a string that gives us room to abstract over ownership and add an optional index. And in the RFC I see: > The `Key` type could be extended to hold an optional index into a source. This could be used to retrieve a specific key-value pair more efficiently than scanning. There's some subtlety to consider though when the sources of keys are combined in various ways that might invalidate a previous index. I know this isn't designed yet but is there anything you would be able to describe about what use cases you would like for the index to enable and roughly what sort of code would they want to write using indices?

src/key_values/mod.rs

I count 7 public things under key_values:

Key
Value
KeyValueError
Source
Visitor
ToKey
ToValue

I don't think organizing these 7 things across 4 modules (key_values, key_values::key, key_values::value, key_values::source) with half of them present in 3 of the 4 modules, and 14 total import paths, is valuable. I think it would be sufficient to expose all 7 from a single module.

Is this structured under the expectation that this API will grow much larger? If so, I would still like to avoid the case of the same types repeated in 3 modules. What are some types that we might later consider adding here?

Currently the public paths are:

key_values::Key
key_values::key::Key
key_values::source::Key
key_values::Value
key_values::value::Value
key_values::source::Value
key_values::KeyValueError
key_values::value::KeyValueError
key_values::source::KeyValueError
key_values::Source
key_values::source::Source
key_values::source::Visitor
key_values::key::ToKey
key_values::value::ToValue

I count 7 public things under key_values: - `Key` - `Value` - `KeyValueError` - `Source` - `Visitor` - `ToKey` - `ToValue` I don't think organizing these 7 things across 4 modules (key_values, key_values::key, key_values::value, key_values::source) with half of them present in 3 of the 4 modules, and 14 total import paths, is valuable. I think it would be sufficient to expose all 7 from a single module. Is this structured under the expectation that this API will grow much larger? If so, I would still like to avoid the case of the same types repeated in 3 modules. What are some types that we might later consider adding here? Currently the public paths are: - `key_values::Key` - `key_values::key::Key` - `key_values::source::Key` - `key_values::Value` - `key_values::value::Value` - `key_values::source::Value` - `key_values::KeyValueError` - `key_values::value::KeyValueError` - `key_values::source::KeyValueError` - `key_values::Source` - `key_values::source::Source` - `key_values::source::Visitor` - `key_values::key::ToKey` - `key_values::value::ToValue`

src/lib.rs

					
				@ -307,6 +307,9 @@ use std::sync::atomic::ATOMIC_USIZE_INIT;

				mod macros;

Alternative name: mod kv. Do you think this is recognizable and unique and memorable enough?

src/lib.rs

					
				@ -725,6 +728,37 @@ pub struct Record<'a> {

				    module_path: Option<&'a str>,

It would be great to visit the Source here and format the pairs into the Debug output.

Please add a comment that the reason this wrapper exists is that putting key_values: &Source inside Record would prevent using derive(Debug).

Please add a comment that the reason this wrapper exists is that putting `key_values: &Source` inside Record would prevent using derive(Debug).

src/lib.rs

For the extremely common case of no key-values, I wonder whether there would be a measurable performance difference from using a (private) Source impl whose visit fn does nothing, unlike Option's which needs to check for Some every time.

dtolnay (Migrated from github.com) reviewed

2019-04-17 03:38:18 -05:00

src/key_values/value/any.rs

This type looks like "&mut dyn Backend, but with extra helpers". It sounds like we'll want Backend in the public API eventually, so consider putting the helpers directly on dyn Backend instead:

impl dyn Backend + '_ {
    pub(crate) fn value<T>(&mut self, v: T) -> Result<(), KeyValueError>
    where
        T: ToValue,
    {...}

    pub fn debug<T>(&mut self, v: T) -> Result<(), KeyValueError>
    where
        T: Debug,
    {...}
}

We can seal the Backend trait for now if we'd like to be free to add mandatory methods.

This type looks like "&mut dyn Backend, but with extra helpers". It sounds like we'll want Backend in the public API eventually, so consider putting the helpers directly on dyn Backend instead: ```rust impl dyn Backend + '_ { pub(crate) fn value<T>(&mut self, v: T) -> Result<(), KeyValueError> where T: ToValue, {...} pub fn debug<T>(&mut self, v: T) -> Result<(), KeyValueError> where T: Debug, {...} } ``` We can seal the Backend trait for now if we'd like to be free to add mandatory methods.

This seems sketchy as the Rust calling convention for those two function types might be arbitrarily different. I think this is in the realm of "probably works but who knows". It would be good to have a compiler person take a look.

fn(FromAny, &T) -> Result<(), KeyValueError>
fn(FromAny, &Void) -> Result<(), KeyValueError>

Making them something like unsafe extern "C" fn(FromAny, *const T) -> Result<(), KeyValueError> seems like it would be sufficient to avoid undefined behavior but I don't know how much worse that makes the rest of the API.

This seems sketchy as the Rust calling convention for those two function types might be arbitrarily different. I think this is in the realm of "probably works but who knows". It would be good to have a compiler person take a look. - `fn(FromAny, &T) -> Result<(), KeyValueError>` - `fn(FromAny, &Void) -> Result<(), KeyValueError>` Making them something like `unsafe extern "C" fn(FromAny, *const T) -> Result<(), KeyValueError>` seems like it would be sufficient to avoid undefined behavior but I don't know how much worse that makes the rest of the API.

src/key_values/value/backend.rs

This is equivalent to:

self.fmt(format_args!("None"))

I think it would be clearer to just write that, rather than leaving it up to how Option::None::<()> gets formatted.

This is equivalent to: ```rust self.fmt(format_args!("None")) ``` I think it would be clearer to just write that, rather than leaving it up to how Option::None::\<()\> gets formatted.

KodrAus (Migrated from github.com) reviewed

2019-04-17 19:16:29 -05:00

src/key_values/key.rs

So this all came out of a possible Source::get(Key) -> Option<Value> method, where for sources like a BTreeMap<String, Value> the implementation could do an indexed lookup, but for Vec<(String, Value)> it would need to scan. It could be a useful thing for a logging framework that wraps log and has some well-known structured pairs like a timestamp or correlation id that consumers want to poke at.

How you would get a hold of an index, and how useful that index would be if you, say, concat two Sources together is where I stopped the design train and left it as a possible extension. For the well-known pairs use-case you could also just write a Source like:

struct MyFrameworkSource<'s> {
    timestamp: Timestamp,
    correlation: Uuid,
    the_rest: Option<&'s dyn Source>,
}

All that's to say it's something I didn't want to absolutely rule out as an enhancement but it's still airy-fairy.

So this all came out of a possible `Source::get(Key) -> Option<Value>` method, where for sources like a `BTreeMap<String, Value>` the implementation could do an indexed lookup, but for `Vec<(String, Value)>` it would need to scan. It could be a useful thing for a logging framework that wraps `log` and has some well-known structured pairs like a timestamp or correlation id that consumers want to poke at. How you would get a hold of an index, and how useful that index would be if you, say, concat two `Source`s together is where I stopped the design train and left it as a possible extension. For the well-known pairs use-case you could also just write a `Source` like: ```rust struct MyFrameworkSource<'s> { timestamp: Timestamp, correlation: Uuid, the_rest: Option<&'s dyn Source>, } ``` All that's to say it's something I didn't want to absolutely rule out as an enhancement but it's still airy-fairy.

KodrAus (Migrated from github.com) reviewed

2019-04-17 19:19:24 -05:00

src/lib.rs

					
				@ -307,6 +307,9 @@ use std::sync::atomic::ATOMIC_USIZE_INIT;

				mod macros;

I think I was using the name kv at one point, and only switched it to key_values so the module name was the same as the Record::key_values method. When I first starting exploring slog, I found its abbreviated names hard to follow, but those were type names not module names.

I'm on board with kv.

I think I was using the name `kv` at one point, and only switched it to `key_values` so the module name was the same as the `Record::key_values` method. When I first starting exploring `slog`, I found its abbreviated names hard to follow, but those were type names not module names. I'm on board with `kv`.

KodrAus (Migrated from github.com) reviewed

2019-04-17 19:21:06 -05:00

src/lib.rs

This sounds like a good excuse to bootstrap some micro-benchmarks for the log crate, so I'll come back to this one later!

This sounds like a good excuse to bootstrap some micro-benchmarks for the `log` crate, so I'll come back to this one later!

KodrAus (Migrated from github.com) reviewed

2019-04-17 19:48:36 -05:00

src/key_values/value/any.rs

Yeh, this was the weird API I was hoping to get some more eyes on at the All Hands earlier this year.

It's the same approach that std::fmt::Argument uses, but that's unstable. It's also similar to the std::task::RawWakerVTable which is up for stabilization now, but includes some of the extra type safety @eddyb suggested in the stabilization PR. The impression I've gotten is that this approach is ok, but is well off the beaten track and not something you should reach for if you don't have to.

As for why we need it, there's a case where you're bridging between log and another framework where you effectively need:

&'kv T ->  Value<'kv> where T: ToValue || Debug

If your bridging crate doesn't own the type T and it doesn't satisfy either of those bounds on its own then you're stuck. You can't wrap &'kv T in a newtype because you need the data within Value to live for 'kv. That's where Value::from_any comes in. It lets you create a Value<'kv> from any arbitrary &'kv T, regardless of the traits T implements.

Additionally, when we come to integrating other serialization frameworks like serde into log, we can do it by erasing into the same Any mechanism, so there's no matching required.

This approach comes with drawbacks though:

It is kinda sketchy and will probably surprise any future contributors that come across it.
Once we make from_any public, I can't think of any alternative internal implementation of Value that could support that &'kv T -> Value<'kv> method.
Supporting owned values in the future using this erased approach is much trickier.

This PR doesn't make the from_any method public though, so right now there's no reason to use this erased approach over an internal trait object. I just copied it from my reference implementation. For implementation simplicity I'd be on board with replacing this with the more obvious trait-object-based implementation.

Yeh, this was the weird API I was hoping to get some more eyes on at the All Hands earlier this year. It's the same approach that [`std::fmt::Argument` uses](https://github.com/rust-lang/rust/blob/master/src/libcore/fmt/mod.rs#L240-L267), but that's unstable. It's also similar to the `std::task::RawWakerVTable` which is up for stabilization now, but includes some of the extra type safety @eddyb suggested [in the stabilization PR](https://github.com/rust-lang/rust/pull/59739#discussion_r272793688). The impression I've gotten is that this approach is _ok_, but is well off the beaten track and not something you should reach for if you don't have to. As for why we need it, there's a case where you're bridging between `log` and another framework where you effectively need: ``` &'kv T -> Value<'kv> where T: ToValue || Debug ``` If your bridging crate doesn't own the type `T` and it doesn't satisfy either of those bounds on its own then you're stuck. You can't wrap `&'kv T` in a newtype because you need the data within `Value` to live for `'kv`. That's where `Value::from_any` comes in. It lets you create a `Value<'kv>` from any arbitrary `&'kv T`, regardless of the traits `T` implements. Additionally, when we come to integrating other serialization frameworks like `serde` into `log`, we can do it by erasing into the same `Any` mechanism, so there's no matching required. This approach comes with drawbacks though: - It _is_ kinda sketchy and will probably surprise any future contributors that come across it. - Once we make `from_any` public, I can't think of any alternative internal implementation of `Value` that could support that `&'kv T -> Value<'kv>` method. - Supporting owned values in the future using this erased approach is much trickier. This PR doesn't make the `from_any` method public though, so right now there's no reason to use this erased approach over an internal trait object. I just copied it from my reference implementation. For implementation simplicity I'd be on board with replacing this with the more obvious trait-object-based implementation.

KodrAus (Migrated from github.com) reviewed

2019-04-17 20:07:44 -05:00

src/key_values/value/any.rs

The idea behind the FromAny type (and the rest of the any API here) is to try really hard not to make Value look like a serialization contract. The reason is mostly philosophical. The exercise of building out the sval framework convinced me that building a serialization API is tricky, and design decisions can seem arbitrary but have significant impacts on usability and interoperability (sval has some really wacky code to integrate with serde), so log should avoid owning that story and defer to existing libraries.

As an example, the way I imagine we'd add serde support to Value would be:

impl<'v> Value<'v> {
    /// Capture a type that implements `serde::Serialize` as a `Value.
    pub fn from_serde(v: &'v impl Serialize) -> Self {
        Self::from_any(v, |from, v| from.serde(v)) 
    }
}

impl<'v> Serialize for Value<'v> {
    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
    where
        S: Serializer,
    {
        //  an implementation of the internal `Backend` trait
        let mut backend = SerdeBackend::new(serializer);
        self.visit(&mut backend)?;
        
        backend.into_result()
    }
}

impl<'b> FromAny<'b> {
    pub fn serde(self, v: impl Serialize) -> Result<(), KeyValuesError> {
        self.0.serde(v)
    }
}

trait Backend {
    /// Visit a `serde::Serialize`. All backends have to support visiting a `Serialize`.
    fn serde(&mut self, v: &erased_serde::Serialize) -> Result<(), KeyValuesError>;
}

Internally, we can't get away with not having some contract for frameworks to conform to, but we can avoid ever exposing it directly. Instead, we can implement traits like std::fmt::Debug, serde::Serialize, andsval::Value on log::Value as the way you can inspect the underlying structure instead of implementing a trait.

But as someone with a lot of experience working on serialization APIs I'd welcome any input you've got here!

The idea behind the `FromAny` type (and the rest of the `any` API here) is to try really hard not to make `Value` look like a serialization contract. The reason is mostly philosophical. The exercise of building out the `sval` framework convinced me that building a serialization API is tricky, and design decisions can seem arbitrary but have significant impacts on usability and interoperability (`sval` has some really wacky code to integrate with `serde`), so `log` should avoid owning that story and defer to existing libraries. As an example, the way I imagine we'd add `serde` support to `Value` would be: ```rust impl<'v> Value<'v> { /// Capture a type that implements `serde::Serialize` as a `Value. pub fn from_serde(v: &'v impl Serialize) -> Self { Self::from_any(v, |from, v| from.serde(v)) } } impl<'v> Serialize for Value<'v> { fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error> where S: Serializer, { // an implementation of the internal `Backend` trait let mut backend = SerdeBackend::new(serializer); self.visit(&mut backend)?; backend.into_result() } } impl<'b> FromAny<'b> { pub fn serde(self, v: impl Serialize) -> Result<(), KeyValuesError> { self.0.serde(v) } } trait Backend { /// Visit a `serde::Serialize`. All backends have to support visiting a `Serialize`. fn serde(&mut self, v: &erased_serde::Serialize) -> Result<(), KeyValuesError>; } ``` Internally, we can't get away with not having some contract for frameworks to conform to, but we can avoid ever exposing it directly. Instead, we can implement traits like `std::fmt::Debug`, `serde::Serialize`, and`sval::Value` on `log::Value` as the way you can inspect the underlying structure instead of implementing a trait. But as someone with a lot of experience working on serialization APIs I'd welcome any input you've got here!

KodrAus commented

2019-04-17 20:09:59 -05:00

(Migrated from github.com)

Thanks @dtolnay for a thorough review! I'm working through your feedback and have left some of my thoughts on the value API. I appreciate the fresh perspective.

Thanks @dtolnay for a thorough review! I'm working through your feedback and have left some of my thoughts on the `value` API. I appreciate the fresh perspective.

KodrAus (Migrated from github.com) reviewed

2019-04-18 01:03:17 -05:00

src/key_values/value/any.rs

Alrighty, in https://github.com/rust-lang-nursery/log/pull/324/commits/ee05032759cf2fd6c04b6501b8c35b8e2e3942a7 I've refactored this to remove this FromAny infrastructure and use a pair of internal traits Visit and Visitor to preserve the structure of primitive values.

Alrighty, in https://github.com/rust-lang-nursery/log/pull/324/commits/ee05032759cf2fd6c04b6501b8c35b8e2e3942a7 I've refactored this to remove this `FromAny` infrastructure and use a pair of internal traits `Visit` and `Visitor` to preserve the structure of primitive values.

KodrAus commented

2019-04-24 22:23:24 -05:00

(Migrated from github.com)

@dtolnay Alrighty, this should be ready for another look now.

Thomasdezeeuw commented

2019-04-25 18:14:06 -05:00

(Migrated from github.com)

@KodrAus Quick note: I'm missing ToValue implementations for usize and isize. More feedback to follow, looking good so far!

@KodrAus Quick note: I'm missing `ToValue` implementations for `usize` and `isize`. More feedback to follow, looking good so far!

Thomasdezeeuw commented

2019-04-25 18:37:25 -05:00

(Migrated from github.com)

I've added support for this to my logging implementation to test this pr, it's available here: https://github.com/Thomasdezeeuw/std-logger/pull/14.

As mentioned in a previous comment. I'm missing ToValue for usize and isize.

It would be nice to detect if the record has any key value pairs, this would avoid needless
formatting. Could be as simple as changing key_values to return Option<&dyn Source>.

Could KeyValueError be renamed to kv::Error (where the kv is the module), this would match types like fmt::Error and io::Error. This does mean that without the module name in front of it the name loses all meaning.

I'm missing a Value::from_display method that uses the fmt::Display implementation in printing/formatting. I've also noticed that the fmt::Display implementation for Value actually uses the fmt::Debug implementation via FmtVisitor.

I'm also missing a lot of docs. For example for the kv module: what the types and traits and how they work (together). But this can be future work.

I've added support for this to my logging implementation to test this pr, it's available here: https://github.com/Thomasdezeeuw/std-logger/pull/14. As mentioned in a previous comment. I'm missing `ToValue` for `usize` and `isize`. It would be nice to detect if the record has any key value pairs, this would avoid needless formatting. Could be as simple as changing `key_values` to return `Option<&dyn Source>`. Could `KeyValueError` be renamed to `kv::Error` (where the `kv` is the module), this would match types like `fmt::Error` and `io::Error`. This does mean that without the module name in front of it the name loses all meaning. I'm missing a `Value::from_display` method that uses the `fmt::Display` implementation in printing/formatting. I've also noticed that the `fmt::Display` implementation for `Value` actually uses the `fmt::Debug` implementation via `FmtVisitor`. I'm also missing a lot of docs. For example for the `kv` module: what the types and traits and how they work (together). But this can be future work.

👍 1

KodrAus commented

2019-04-25 21:18:27 -05:00

(Migrated from github.com)

Thanks for the feedback @Thomasdezeeuw!

missing ToValue for usize and isize.

We should definitely add these 👍

It would be nice to detect if the record has any key value pairs, this would avoid needless
formatting. Could be as simple as changing key_values to return Option<&dyn Source>.

We could look at a convenience method on Source that can tell if there are any pairs present so you could avoid formatting. I think it would be good to avoid exposing a Option<&dyn Source> though so we avoid the confusion of None vs Some(&[]).

Could KeyValueError be renamed to kv::Error

That's the name I'd prefer too, but used KeyValueError instead so that it's consistent with the explicitly named errors that already exist in the log crate. I'd happily change it to kv::Error, and maybe consider re-exporting it as log::KeyValueError at the crate root (or not).

I'm missing a Value::from_display method

That's a good idea too!

fmt::Display implementation for Value actually uses the fmt::Debug implementation via FmtVisitor.

This actually shouldn't be an issue, because we can forward a fmt::Display impl through a fmt::Debug one, so whatever fmt trait you use for a particular value is the one that'll be used in both the Value as Debug and Value as Display impls.

Thanks for the feedback @Thomasdezeeuw! > missing ToValue for usize and isize. We should definitely add these :+1: > It would be nice to detect if the record has any key value pairs, this would avoid needless formatting. Could be as simple as changing key_values to return Option<&dyn Source>. We could look at a convenience method on `Source` that can tell if there are any pairs present so you could avoid formatting. I think it would be good to avoid exposing a `Option<&dyn Source>` though so we avoid the confusion of `None` vs `Some(&[])`. > Could KeyValueError be renamed to kv::Error That's the name I'd prefer too, but used `KeyValueError` instead so that it's consistent with the explicitly named errors that already exist in the `log` crate. I'd happily change it to `kv::Error`, and maybe consider re-exporting it as `log::KeyValueError` at the crate root (or not). > I'm missing a Value::from_display method That's a good idea too! > fmt::Display implementation for Value actually uses the fmt::Debug implementation via FmtVisitor. This actually shouldn't be an issue, because we can forward a `fmt::Display` impl through a `fmt::Debug` one, so whatever `fmt` trait you use for a particular value is the one that'll be used in both the `Value as Debug` and `Value as Display` impls.

Thomasdezeeuw commented

2019-04-27 14:10:23 -05:00

(Migrated from github.com)

I think it would be good to avoid exposing a Option<&dyn Source> though so we avoid the confusion of None vs Some(&[]).

I'm not sure I understand, when do we want to use Some(&[])? And what confusion would it cause?

If the key_values field in Record is changed to Option<&dyn Source> we can just return that right? Then RecordBuilder, and any log messages with key/values, can just default to using None.

> I think it would be good to avoid exposing a `Option<&dyn Source>` though so we avoid the confusion of `None` vs `Some(&[])`. I'm not sure I understand, when do we want to use `Some(&[])`? And what confusion would it cause? If the `key_values` field in `Record` is changed to `Option<&dyn Source>` we can just return that right? Then `RecordBuilder`, and any log messages with key/values, can just default to using `None`.

KodrAus commented

2019-04-27 18:32:40 -05:00

(Migrated from github.com)

Ah sorry that wasn’t very clear. The confusion is assuming that if key_values returns some then there are more than zero pairs to work with. That’s not necessarily going to be the case, so having to jump through the extra hoops of matching on an Option doesn’t seem worth it.

Ah sorry that wasn’t very clear. The confusion is assuming that if `key_values` returns some then there are more than zero pairs to work with. That’s not necessarily going to be the case, so having to jump through the extra hoops of matching on an `Option` doesn’t seem worth it.

dtolnay (Migrated from github.com) approved these changes

2019-04-28 16:41:57 -05:00

dtolnay (Migrated from github.com) left a comment

Nice, this looks terrific.

KodrAus commented

2019-04-28 23:31:30 -05:00

(Migrated from github.com)

🎉

@Thomasdezeeuw The intention now is that we can continue developing off this base without having to break it, but you never know. So in case that does need to happen I'll try remember to give you a CC in any PRs I see that would result in breakage!

Any more input you have in the form of issues and PRs will be most welcome! I've created a tracking issue we can use to follow progress.

:tada: @Thomasdezeeuw The intention now is that we can continue developing off this base without having to break it, but you never know. So in case that does need to happen I'll try remember to give you a CC in any PRs I see that would result in breakage! Any more input you have in the form of issues and PRs will be most welcome! I've created [a tracking issue](https://github.com/rust-lang-nursery/log/issues/328) we can use to follow progress.

Thomasdezeeuw commented

2019-04-29 04:11:28 -05:00

(Migrated from github.com)

The confusion is assuming that if key_values returns some then there are more than zero pairs to work with. That’s not necessarily going to be the case, so having to jump through the extra hoops of matching on an Option doesn’t seem worth it.

But we could ensure that when key_values is some there are one or more key values. Then we won't have that problem.

> The confusion is assuming that if `key_values` returns some then there are more than zero pairs to work with. That’s not necessarily going to be the case, so having to jump through the extra hoops of matching on an `Option` doesn’t seem worth it. But we could ensure that when `key_values` is some there are one or more key values. Then we won't have that problem.

Sign in to join this conversation.

No reviewers

dtolnay