Review of key-value feature #584

2023-08-29T03:46:09-05:00

Thomasdezeeuw commented

2023-08-29 03:46:09 -05:00

(Migrated from github.com)

Discussion for https://github.com/rust-lang/log/issues/328.

KodrAus (Migrated from github.com) reviewed

2023-08-29 04:15:16 -05:00

src/kv/key.rs

					
				@ -49,6 +49,8 @@ impl<'k> Key<'k> {

				        self.key

				    }

				    // REVIEW: do we still want this? Isn't most of log's API based on borrowed

It has some niche use, which at this stage I think makes it a fair candidate for removal. If someone discovers a need for it we can bring it back.

src/kv/source.rs

					
				@ -374,6 +374,12 @@ mod std_support {

				    }

				}

				// REVIEW: I'm not a 100% convinced that AsmAp and AsList are pulling their

These also seem fair to remove and consider adding later, or adding in a utilities crate 👍

These also seem fair to remove and consider adding later, or adding in a utilities crate :+1:

src/kv/value.rs

					
				@ -83,6 +83,11 @@ macro_rules! as_sval {

				/// - Using the `ToValue` trait.

				/// - Using the standard `From` trait.

				///

				// REVIEW: maybe add a section about which conversion method to use and when? Or

Good call 👍

The preference would be for capture_*, then from_*. The reason being if you Value::capture_display(&42) you'll get a number, but if you use Value::from_display(&42) you'll get a string. I'd be open to some different naming that made this more obvious. We may consider also only adding them for Display and Debug since serde and sval are naturally structured. We could also consider offering just a single capture method that is what capture_display currently is, and using that by default in the macros. The goal is that by default if you log a boolean or number you should get a boolean or number, and you shouldn't have to implement any non-standard-library traits like ToValue to log something.

I'm open to suggestions though. What do you think?

Good call :+1: The preference would be for `capture_*`, then `from_*`. The reason being if you `Value::capture_display(&42)` you'll get a number, but if you use `Value::from_display(&42)` you'll get a string. I'd be open to some different naming that made this more obvious. We may consider also only adding them for `Display` and `Debug` since `serde` and `sval` are naturally structured. We could also consider offering just a single `capture` method that is what `capture_display` currently is, and using that by default in the macros. The goal is that by default if you log a boolean or number you should get a boolean or number, and you shouldn't _have_ to implement any non-standard-library traits like `ToValue` to log something. I'm open to suggestions though. What do you think?

src/kv/value.rs

Good point 👍 I'd be up for removing them.

Good point :+1: I'd be up for removing them.

src/kv/value.rs

					
				@ -635,6 +646,9 @@ pub trait Visit<'v> {

				    /// more specific methods that aren't overridden.

				    /// The `Value` may be formatted using its `fmt::Debug` or `fmt::Display` implementation,

				    /// or serialized using its `sval::Value` or `serde::Serialize` implementation.

Good point 👍 There's probably some precedence in the standard library to look at.

Good point :+1: There's probably some precedence in the standard library to look at.

src/kv/value.rs

That's a good question. The design here is that you shouldn't need separate methods, because you don't know what framework a user logged their structured data with, and as a consumer it doesn't matter to you. They pick the one that best suits them and you just pick the one that best suits you. Value will bridge them for you.

The drawback is it's really non-obvious how to work with it. You try to visit a Value to see what's in it and you get a method that gives you... a Value. This could just be an issue of documentation. The API itself doesn't give you a lot of clues about how to use it. That could be a reason to add separate methods, and examples alongside them on how to forward visit_any into them.

The only thing we'd need to worry about is whether Visitor needs to be object safe. I think it doesn't? In which case it's fine to add separate methods.

That's a good question. The design here is that you shouldn't need separate methods, because you don't know what framework a user logged their structured data with, and as a consumer it doesn't matter to you. They pick the one that best suits them and you just pick the one that best suits you. `Value` will bridge them for you. The drawback is it's really non-obvious how to work with it. You try to visit a `Value` to see what's in it and you get a method that gives you... a `Value`. This could just be an issue of documentation. The API itself doesn't give you a lot of clues about how to use it. That could be a reason to add separate methods, and examples alongside them on how to forward `visit_any` into them. The only thing we'd need to worry about is whether `Visitor` needs to be object safe. I think it doesn't? In which case it's fine to add separate methods.

Thomasdezeeuw (Migrated from github.com) reviewed

2023-08-29 07:55:22 -05:00

src/kv/key.rs

					
				@ -49,6 +49,8 @@ impl<'k> Key<'k> {

				        self.key

				    }

				    // REVIEW: do we still want this? Isn't most of log's API based on borrowed

Done in https://github.com/rust-lang/log/pull/585.

src/kv/source.rs

					
				@ -374,6 +374,12 @@ mod std_support {

				    }

				}

				// REVIEW: I'm not a 100% convinced that AsmAp and AsList are pulling their

Done in https://github.com/rust-lang/log/pull/585.

src/kv/value.rs

Done in https://github.com/rust-lang/log/pull/585.

Thomasdezeeuw (Migrated from github.com) reviewed

2023-08-29 08:19:57 -05:00

src/kv/value.rs

					
				@ -83,6 +83,11 @@ macro_rules! as_sval {

				/// - Using the `ToValue` trait.

				/// - Using the standard `From` trait.

				///

				// REVIEW: maybe add a section about which conversion method to use and when? Or

Does it make sense to change the macros to always call log::ToValue::to_value($value)? Then at least for the primitive types users don't have to worry about it. For non-primitive types users will have to use as_{debug,display,serde,sval}. Then, once specialisation is stablised (and log's MSRV is updated), we could even add impl<T: {fmt::Display,fmt::Debug,serde::Serialize,sval::Value}> ToValue for T { .. }.

As for the capture_* vs. from_*, can we drop the 'static requirement of the type? If maybe we can merge them (or at least some methods).

Does it make sense to change the macros to always call `log::ToValue::to_value($value)`? Then at least for the primitive types users don't have to worry about it. For non-primitive types users will have to use `as_{debug,display,serde,sval}`. Then, once specialisation is stablised (and log's MSRV is updated), we could even add `impl<T: {fmt::Display,fmt::Debug,serde::Serialize,sval::Value}> ToValue for T { .. }`. As for the `capture_*` vs. `from_*`, can we drop the `'static` requirement of the type? If maybe we can merge them (or at least some methods).

src/kv/value.rs

					
				@ -635,6 +646,9 @@ pub trait Visit<'v> {

				    /// more specific methods that aren't overridden.

				    /// The `Value` may be formatted using its `fmt::Debug` or `fmt::Display` implementation,

				    /// or serialized using its `sval::Value` or `serde::Serialize` implementation.

I can't find much precedence in std lib. Using rg --fixed-strings -e '-> Cow<' all I could find was IntoCow::into_cow as a testing trait.

Maybe to_cow_str?

I can't find much precedence in std lib. Using `rg --fixed-strings -e '-> Cow<'` all I could find was `IntoCow::into_cow` as a testing trait. Maybe `to_cow_str`?

src/kv/value.rs

That's a good question. The design here is that you shouldn't need separate methods, because you don't know what framework a user logged their structured data with, and as a consumer it doesn't matter to you. They pick the one that best suits them and you just pick the one that best suits you. Value will bridge them for you.

The drawback is it's really non-obvious how to work with it. You try to visit a Value to see what's in it and you get a method that gives you... a Value. This could just be an issue of documentation. The API itself doesn't give you a lot of clues about how to use it. That could be a reason to add separate methods, and examples alongside them on how to forward visit_any into them.

I saw the Visitor trait as a sort of callback mechanism where you ask the type "what is your concrete type?", where the "most accurate" method gets called. However currently I have to do another layer of matching in visit_any to determine if serde or sval is used. Making this separate methods avoid that and you can just fall back to using fmt::Display in visit_any.

The only thing we'd need to worry about is whether Visitor needs to be object safe. I think it doesn't? In which case it's fine to add separate methods.

I don't think Visitor needs to be object safe, but I rarely use dynamic objects like that. I can imagine case for object safe Log trait, but for Visitor it's much harder.

If it's every needed to I recently ran into askama::DynTemplate, which I found an interesting work around.

> That's a good question. The design here is that you shouldn't need separate methods, because you don't know what framework a user logged their structured data with, and as a consumer it doesn't matter to you. They pick the one that best suits them and you just pick the one that best suits you. `Value` will bridge them for you. > > The drawback is it's really non-obvious how to work with it. You try to visit a `Value` to see what's in it and you get a method that gives you... a `Value`. This could just be an issue of documentation. The API itself doesn't give you a lot of clues about how to use it. That could be a reason to add separate methods, and examples alongside them on how to forward `visit_any` into them. I saw the `Visitor` trait as a sort of callback mechanism where you ask the type "what is your concrete type?", where the "most accurate" method gets called. However currently I have to do another layer of matching in `visit_any` to determine if serde or sval is used. Making this separate methods avoid that and you can just fall back to using `fmt::Display` in `visit_any`. > The only thing we'd need to worry about is whether `Visitor` needs to be object safe. I think it doesn't? In which case it's fine to add separate methods. I don't think `Visitor` needs to be object safe, but I rarely use dynamic objects like that. I can imagine case for object safe `Log` trait, but for `Visitor` it's much harder. If it's every needed to I recently ran into [`askama::DynTemplate`](https://docs.rs/askama/latest/askama/trait.DynTemplate.html), which I found an interesting work around.

KodrAus (Migrated from github.com) reviewed

2023-10-08 19:39:22 -05:00

src/kv/value.rs

to determine if serde or sval is used

I think this is the part you shouldn't need to do in visit_any. In std_logger's JSON format for instance, your visit_any implementation could be:

fn visit_any(&mut self, value: kv::Value) -> Result<(), kv::Error> {
        Ok(serde_json::to_writer(&mut self.0, &value)?)
}

where self.0 is impl std::io::Write. You don't need to know whether the value comes from serde or sval, or even need to override any of the other Visitor methods. You'll get the most accurate structured representation no matter what the value was captured with.

> to determine if serde or sval is used I think this is the part you shouldn't need to do in `visit_any`. In `std_logger`'s JSON format for instance, your [`visit_any`](https://github.com/Thomasdezeeuw/std-logger/blob/eff67f4c33b89c8debd325c1ad79724e6cd6af2c/src/format/json.rs#L169-L173) implementation could be: ```rust fn visit_any(&mut self, value: kv::Value) -> Result<(), kv::Error> { Ok(serde_json::to_writer(&mut self.0, &value)?) } ``` where `self.0` is `impl std::io::Write`. You don't need to know whether the value comes from `serde` or `sval`, or even need to override any of the other `Visitor` methods. You'll get the most accurate structured representation no matter what the value was captured with.

Thomasdezeeuw (Migrated from github.com) reviewed

2023-10-09 04:12:18 -05:00

src/kv/value.rs

to determine if serde or sval is used

I think this is the part you shouldn't need to do in visit_any. In std_logger's JSON format for instance, your visit_any implementation could be:
fn visit_any(&mut self, value: kv::Value) -> Result<(), kv::Error> {
        Ok(serde_json::to_writer(&mut self.0, &value)?)
}
where self.0 is impl std::io::Write. You don't need to know whether the value comes from serde or sval, or even need to override any of the other Visitor methods. You'll get the most accurate structured representation no matter what the value was captured with.

So you're saying that kv::Value implements serde::Deserialize regardless of the source of the actual value (i.e. a sval value would be deserialised correctly?). And vice versa?

If so, it does essentially mean that any logging implementation that want to support structured value needs to use either serde or sval.

> > to determine if serde or sval is used > > I think this is the part you shouldn't need to do in `visit_any`. In `std_logger`'s JSON format for instance, your [`visit_any`](https://github.com/Thomasdezeeuw/std-logger/blob/eff67f4c33b89c8debd325c1ad79724e6cd6af2c/src/format/json.rs#L169-L173) implementation could be: > > ```rust > fn visit_any(&mut self, value: kv::Value) -> Result<(), kv::Error> { > Ok(serde_json::to_writer(&mut self.0, &value)?) > } > ``` > > where `self.0` is `impl std::io::Write`. You don't need to know whether the value comes from `serde` or `sval`, or even need to override any of the other `Visitor` methods. You'll get the most accurate structured representation no matter what the value was captured with. So you're saying that `kv::Value` implements `serde::Deserialize` regardless of the source of the actual value (i.e. a `sval` value would be deserialised correctly?). And vice versa? If so, it does essentially mean that any logging implementation that want to support structured value needs to use either `serde` or `sval`.

KodrAus (Migrated from github.com) reviewed

2023-10-18 15:24:15 -05:00

src/kv/value.rs

So you're saying that kv::Value implements serde::Deserialize regardless of the source of the actual value (i.e. a sval value would be deserialised correctly?). And vice versa?

That's essentially right 👍 It always implements serde::Serialize and the Serialize implementation will always use the least-lossy representation of the data you've captured.

If so, it does essentially mean that any logging implementation that want to support structured value needs to use either serde or sval.

For simple values like numbers, strings, and booleans you don't need to. If you want to log complex values like maps or sequences then you'll need to use one of them. If you're happy to treat complex values as text then you can also always use std::fmt for them.

> So you're saying that kv::Value implements serde::Deserialize regardless of the source of the actual value (i.e. a sval value would be deserialised correctly?). And vice versa? That's essentially right :+1: It always implements `serde::Serialize` and the `Serialize` implementation will always use the least-lossy representation of the data you've captured. > If so, it does essentially mean that any logging implementation that want to support structured value needs to use either serde or sval. For simple values like numbers, strings, and booleans you don't need to. If you want to log complex values like maps or sequences then you'll need to use one of them. If you're happy to treat complex values as text then you can also always use `std::fmt` for them.

KodrAus (Migrated from github.com) reviewed

2023-10-18 15:26:55 -05:00

src/kv/value.rs

					
				@ -635,6 +646,9 @@ pub trait Visit<'v> {

				    /// more specific methods that aren't overridden.

				    /// The `Value` may be formatted using its `fmt::Debug` or `fmt::Display` implementation,

				    /// or serialized using its `sval::Value` or `serde::Serialize` implementation.

to_cow_str sounds totally fine to me here.

`to_cow_str` sounds totally fine to me here.

KodrAus (Migrated from github.com) reviewed

2023-10-18 15:34:29 -05:00

src/kv/value.rs

					
				@ -83,6 +83,11 @@ macro_rules! as_sval {

				/// - Using the `ToValue` trait.

				/// - Using the standard `From` trait.

				///

				// REVIEW: maybe add a section about which conversion method to use and when? Or

Does it make sense to change the macros to always call log::ToValue::to_value($value)?

The idea was to try discourage libraries from implementing ToValue themselves so it doesn't become yet another widespread piece of trivia that library authors need to keep on top of. Defaulting to Display + 'static means a lot of library types like Uuid can be captured, but the drawback of throwing lifetime errors at you if you try log a non-'static value would be unpleasant.

For what it's worth, I would be ok with using ToValue by default in addition to adding some easy syntax for Display/Debug/Serialize/Value. If we did that then we could remove the capture_* methods altogether; they only exist to support downcasting to tell if we're logging numbers/booleans etc.

Then, once specialisation is stablised

🥲

> Does it make sense to change the macros to always call log::ToValue::to_value($value)? The idea was to try discourage libraries from implementing `ToValue` themselves so it doesn't become yet another widespread piece of trivia that library authors need to keep on top of. Defaulting to `Display + 'static` means a lot of library types like `Uuid` can be captured, but the drawback of throwing lifetime errors at you if you try log a non-`'static` value would be unpleasant. For what it's worth, I would be ok with using `ToValue` by default in addition to adding some easy syntax for `Display`/`Debug`/`Serialize`/`Value`. If we did that then we could remove the `capture_*` methods altogether; they only exist to support downcasting to tell if we're logging numbers/booleans etc. > Then, once specialisation is stablised 🥲

KodrAus commented

2023-10-18 15:36:42 -05:00

(Migrated from github.com)

Thanks for all the time you've spent digging through this so far @Thomasdezeeuw! I think we're getting towards something that feels appropriate for the log crate without losing capability.

Thanks for all the time you've spent digging through this so far @Thomasdezeeuw! I think we're getting towards something that feels appropriate for the `log` crate without losing capability.

👍 1

Thomasdezeeuw (Migrated from github.com) reviewed

2023-10-19 08:38:31 -05:00

src/kv/value.rs

					
				@ -83,6 +83,11 @@ macro_rules! as_sval {

				/// - Using the `ToValue` trait.

				/// - Using the standard `From` trait.

				///

				// REVIEW: maybe add a section about which conversion method to use and when? Or

Does it make sense to change the macros to always call log::ToValue::to_value($value)?

The idea was to try discourage libraries from implementing ToValue themselves so it doesn't become yet another widespread piece of trivia that library authors need to keep on top of. Defaulting to Display + 'static means a lot of library types like Uuid can be captured, but the drawback of throwing lifetime errors at you if you try log a non-'static value would be unpleasant.

👍 makes sense

For what it's worth, I would be ok with using ToValue by default in addition to adding some easy syntax for Display/Debug/Serialize/Value. If we did that then we could remove the capture_* methods altogether; they only exist to support downcasting to tell if we're logging numbers/booleans etc.

We can keep using the as_display/as_debug macros, or shorten them to /?, but perhaps that a different discussion.

Then, once specialisation is stablised

🥲

Indeed.

> > Does it make sense to change the macros to always call log::ToValue::to_value($value)? > > The idea was to try discourage libraries from implementing `ToValue` themselves so it doesn't become yet another widespread piece of trivia that library authors need to keep on top of. Defaulting to `Display + 'static` means a lot of library types like `Uuid` can be captured, but the drawback of throwing lifetime errors at you if you try log a non-`'static` value would be unpleasant. :+1: makes sense > For what it's worth, I would be ok with using `ToValue` by default in addition to adding some easy syntax for `Display`/`Debug`/`Serialize`/`Value`. If we did that then we could remove the `capture_*` methods altogether; they only exist to support downcasting to tell if we're logging numbers/booleans etc. We can keep using the `as_display`/`as_debug` macros, or shorten them to <nothing>/`?`, but perhaps that a different discussion. > > Then, once specialisation is stablised > > 🥲 Indeed.

src/kv/value.rs

That makes sense. Then I think we should add some docs to the log::kv::value::Visit trait and leave it as is.

That makes sense. Then I think we should add some docs to the `log::kv::value::Visit` trait and leave it as is.

Thomasdezeeuw (Migrated from github.com) reviewed

2023-10-19 12:36:25 -05:00

src/kv/value.rs

					
				@ -635,6 +646,9 @@ pub trait Visit<'v> {

				    /// more specific methods that aren't overridden.

				    /// The `Value` may be formatted using its `fmt::Debug` or `fmt::Display` implementation,

				    /// or serialized using its `sval::Value` or `serde::Serialize` implementation.

Created https://github.com/rust-lang/log/pull/592.

Thomasdezeeuw (Migrated from github.com) reviewed

2023-10-19 13:05:01 -05:00

src/kv/value.rs

https://github.com/rust-lang/log/pull/593

KodrAus commented

2024-01-08 15:59:55 -06:00

(Migrated from github.com)

Just checking back in on this, is the main blocker we've got deciding whether to default to capturing using ToValue and supporting some tracing-like syntax for changing it?

Just checking back in on this, is the main blocker we've got deciding whether to default to capturing using `ToValue` and supporting some `tracing`-like syntax for changing it?

Thomasdezeeuw commented

2024-01-09 03:42:26 -06:00

(Migrated from github.com)

Just checking back in on this, is the main blocker we've got deciding whether to default to capturing using ToValue and supporting some tracing-like syntax for changing it?

Yes, I think those are the two main things from this pr.

> Just checking back in on this, is the main blocker we've got deciding whether to default to capturing using `ToValue` and supporting some `tracing`-like syntax for changing it? Yes, I think those are the two main things from this pr.

KodrAus commented

2024-01-25 22:09:44 -06:00

(Migrated from github.com)

I'm doing some work on this now; removing the Value::capture_* and downcasting methods, and improving the overall documentation for the kv module.

It looks like we already default to using ToValue for key-values, so nothing needs to change there. That just leaves some nicer macro support for tweaking how values are captured.

Are you happy with that plan @Thomasdezeeuw?

I'm doing some work on this now; removing the `Value::capture_*` and downcasting methods, and improving the overall documentation for the `kv` module. It looks like we already default to using `ToValue` for key-values, so nothing needs to change there. That just leaves some nicer macro support for tweaking how values are captured. Are you happy with that plan @Thomasdezeeuw?

KodrAus commented

2024-01-26 02:09:53 -06:00

(Migrated from github.com)