Review of key-value feature

[Thomasdezeeuw]

Discussion for #328.

[KodrAus]

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?

[Thomasdezeeuw]

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

[KodrAus]

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

🥲

[Thomasdezeeuw]

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.

[KodrAus]

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.

[KodrAus]

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]

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]

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]

I've opened #613 for this. I'll cc there once it's ready for review, but wanted to point it out here too.

[Thomasdezeeuw]

Are you happy with that plan @Thomasdezeeuw?

Yes, sounds good 👍