Get structured logging API ready for stabilization

[KodrAus]

Closes #149
Closes #328
Closes #357
Closes #436
Closes #584

Based on the review in #584

The plan is to:

• Deprecate Value::capture_* and Value::downcast_* methods. All deprecated APIs are gated under the old kv_unstable features and will be removed once we remove those old unstable features.
• Fill in the documentation for structured logging support so people can actually use it.
• Add some macro syntax for capturing attributes as debug or display instead of the default ToValue (key:? = value).
• Only pull in value-bag as a dependency if either serde or sval is also needed. In cases where you just want basic structured logging, we should do it in a dependency-free way. This will just mean adding a simple ValueInner enum.

I'll cc once this is ready for review, to save a bit of notification noise.

Once this PR is complete, we should be able to stabilize structured logging support in the log crate.

[KodrAus]

We currently have kv::source::Visitor and kv::value::Visit. They're both visitor types (separate because they serve separate purposes), but should probably still be named consistently. @Thomasdezeeuw what do you think? Should we rename either source::Visitor or value::Visit? My preference would be to rename value::Visit to value::Visitor.

[Thomasdezeeuw]

We currently have kv::source::Visitor and kv::value::Visit. They're both visitor types (separate because they serve separate purposes), but should probably still be named consistently. @Thomasdezeeuw what do you think? Should we rename either source::Visitor or value::Visit? My preference would be to rename value::Visit to value::Visitor.

I think Visitor is more common? So I would go with that.

I would understand possible confusing between the two traits, so maybe we need to rename one of the two? What about renaming source::Visitor to KeyValuePairs or just KeyValues, to indicate its a collection of key-value pairs?

[KodrAus]

I would understand possible confusing between the two traits, so maybe we need to rename one of the two?

That's a fair point. Perhaps we could keep the names verb-y and use VisitSource and VisitValue?

[Thomasdezeeuw]

That's a fair point. Perhaps we could keep the names verb-y and use VisitSource and VisitValue?

Sounds good 👍

[KodrAus]

Ok, I think the last remaining thing here is macro syntax. We could adopt exactly tracing's syntax of:

key = <modifier> arg

where <modifier> may be ? for fmt::Debug or % for fmt::Display. I'm not a huge fan of leaning on sigils, or on prefixed modifiers here, because it leaves us almost no room for extensibility, but does have the benefit of being what's already there. I was thinking something like:

key = arg : <modifier>

where <modifier> may be blank for fmt::Display, ? for fmt::Debug, or the literal text serde or sval for those respective libraries. In practice, it would look something like:

• data: captured using ToValue
• data:: captured using fmt::Display
• data:?: captured using fmt::Debug
• data:serde: captured using serde::Serialize
• data:sval: captured using sval::Value

We could use a simple lowering scheme so data:? lowered to data:debug and data: lowered to data:display, so it would be less easy to miss the presence of the :.

Another potential position could be on the key name:

key : <modifier> = arg

That would more easily let us expected arg to be a full expression in the future.

Any thoughts?

[Thomasdezeeuw]

key = arg : <modifier>

where <modifier> may be blank for fmt::Display, ? for fmt::Debug, or the literal text serde or sval for those respective libraries. In practice, it would look something like:

I like this, feels similar to write! (and print!, etc.) macros.

* `data:serde`: captured using `serde::Serialize`

* `data:sval`: captured using `sval::Value`

Small thing is versioning, in case serde/sval ever publish a v2, how do we want to handle that? We can go serdeV1 or serde1 or something else with a version in it, but if a v2 never comes it will just be annoying. So maybe serde/sval means v1, and serde2/sval2 could v2?

We could use a simple lowering scheme so data:? lowered to data:debug and data: lowered to data:display, so it would be less easy to miss the presence of the :.

Because we'll be copying the standard formatting found in the std lib, I don't think will become an issue.

Another potential position could be on the key name:

key : <modifier> = arg

That would more easily let us expected arg to be a full expression in the future.

How would be limiting ourselves using the above arg : <modifier>? I suppose we won't be able to support full expressions?

[KodrAus]

So maybe serde/sval means v1, and serde2/sval2 could v2?

That seems like a good scheme to me 👍 I spelled this out explicitly in value-bag, but it's not really meant to be user-facing. I think it's reasonable to make the "unversioned" ident refer to the current stable versions in log.

How would be limiting ourselves using the above arg : ? I suppose we won't be able to support full expressions?

We'd just need to wrap full expressions in parenthesis or braces, because the : character can't appear directly after an expression in a macro rule.

[KodrAus]

This is quite a big change but I think it's ready for a review now. Some things to call out:

• The macro syntax I ended up with was key:modifier = value, so key:? = value for debug for instance. This was easier to implement and didn't require breaking any existing users.
• Should we make the kv::source and kv::value modules private now there's no types in them that aren't exported in the root kv module? I think we probably should.

[Thomasdezeeuw]

I like it.

The only concern I have is that people find the modified on the key odd/unexpected, when coming from the std::fmt implementation. That might be worth calling out in the documentation.

[Thomasdezeeuw]

Why the double reference?

[KodrAus]

I left a comment in the source on why we do this. It's just so we can capture T: ?Sized while still erasing it behind a dyn Trait. Passing in the double reference in just helps the lifetimes work out.

[KodrAus]

The

err:error = err

way of capturing an error using the std::error::Error trait is maybe something we can improve on someday 🙂

[KodrAus]

I think the last thing remaining here is what to do about the _unstable suffix on our features. I was thinking making them just shims for the unsuffixed versions, but continuing to accept them so that we don't break every current user of the feature. What do you think @Thomasdezeeuw?

[Thomasdezeeuw]

I think the last thing remaining here is what to do about the _unstable suffix on our features. I was thinking making them just shims for the unsuffixed versions, but continuing to accept them so that we don't break every current user of the feature. What do you think @Thomasdezeeuw?

We can, but we a also have other breakages, e.g. the removal of the as_{display,debug, etc.} macros. Some of the API on Value has changes, which is also breaking.

So I'm wondering if we should keep some of the old API, if possible, if kv_unstable is used? Marking it all as deprecated show it at least generates warnings (I don't know if that is possible for features). Then e.g. 6 months after the stable release we can remove the _unstable feature and all it's related API? This would give people some time to migrate their code.

[Thomasdezeeuw]

We could start using cargo-hack for this at some point (for another pr).

[Thomasdezeeuw]

To bad there is only compile_error!, not compile_warn! otherwise it would be nice to add that.

[Thomasdezeeuw]

Actually on second though... we're going to break a lot of (unstable) code already by removing the as_serde macros, do we just rip the bandaid in one go and break everything? Or should we add those macros back if [kv_unstable] is used (and mark them as #[deprecated])?

[KodrAus]

I think this is a reasonable thing to do 👍

Unfortunately you can’t mark them as deprecated but we can also re-export the renamed visitor traits under their old names when the old _unstable features are enabled.

[KodrAus]

How does this sound as a plan for stabilization:

1. Keep all old APIs when the kv_unstable feature is enabled, deprecating wherever possible.
2. Set a time period where we’ll remove the kv_unstable feature altogether and all those deprecated items. If we keep rough tabs on public code using the old features then we could get some idea of when would be a good time to do this.

The idea being new code and any maintained in that time period should move from the unstable to the stable API without much fuss, but there will be a point where the bandaid comes off completely. The only unfortunate part is we can’t deprecate old trait names so we don’t have a good way of making users of them aware things have changed.

[Thomasdezeeuw]

How does this sound as a plan for stabilization:

Sounds good to me 👍

[KodrAus]

In this latest round of changes I've restored the old APIs under their respective *_unstable feature flags and added support for shorthand capturing, so instead of having to write info!(a = a; "") you can write info!(a; "").

[KodrAus]

Ok, I think this is ready for its last review now. Thank you so much for all the time you've invested in helping push this forwards so far @Thomasdezeeuw 🙏 We'd never have reached this point without you.

[Thomasdezeeuw]

Nice! Let's merge.

I'm gonna go over the API with https://github.com/Enselic/cargo-public-api/ see what, if any, difference there are