Compatibility hazard with Key

[Kixunil]

I noticed that Key is a type alias or a newtype depending on used features. This is a compatibility hazard.

For example:

Crate A:

serializer.emit_arguments("foo", &format_args!("bar"))

Crate B:

With featrue dynamic_keys

serializer.emit_arguments(Key::from(compute_key_string()), &format_args!("baz"))

Crate C depends on both A and B -> crate A will break.

One can defend against this by always using Key::from, but it's possible to forget and clippy complains about useless conversion.
I think the best course of action is to use a newtype for static keys too. It will be breaking for crates that don't defend but it's probably more predictable and easier to handle than the situation above.

[Techcable]

I 100% agree, this is part of the reason tests were broken with feature="dynamic-keys" (until I fixed them)

Unfortunately, making this change would break backward compatibility :(

[dpc]

Eeehhhh... I bolted on this feature and now I regret it. :D

The problem is that dynamic keys are an opt-in and probably less used feature here, so we would be breaking the wrong side of users, I think.

I can't figure out a sane way to reconcile things without breaking anyone.

It seems to me that instead of wrapping a &'static str into a newtype, we should just gradually make everyone use dynamic_keys, and eventually have it enabled by default as a no-op and use Key everywhere unconditionally.

To ease the pain, I think what we could do is the release a version in which if the dynamic_keys is not enabled, we somehow generate a compiler warning that says we are really sorry, but there's no other way and in the future we will have to make a potentially breaking change, and the best way to protect yourself is to manually enable dynamic_keys in your software and make sure it still compiles, and if it does keep dynamic_keys enabled, and if it doesn't fix it or report to libraries that need to fix it.

[Techcable]

Eeehhhh... I bolted on this feature and now I regret it. :D

The problem is that dynamic keys are an opt-in and probably less used feature here, so we would be breaking the wrong side of users, I think.

I can't figure out a sane way to reconcile things without breaking anyone.

It seems to me that instead of wrapping a &'static str into a newtype, we should just gradually make everyone use dynamic_keys, and eventually have it enabled by default as a no-op and use Key everywhere unconditionally.dynamic_keys enabled, and if it doesn't fix it or report to libraries that need to fix it.

I mean we could always put it off till slog 3.0 🤷

Slog 3.0 doesn't have to be a huge rewrite or anything, it could just be:

1. serde by default
2. Key as newtype

Then we release a beta and call it Slog 3.0 😉

[dpc]

Big releases are a PITA for the ecosystem, because they require all the things to switch to new major version.

There's a question - is it even worth it? Seems that log and tracing added support for key-values, so the only thing that slog has is "contextual" logging. And tracing can do async well, and has spans and so on.

If one was to build slog 3.0 it should probably start from the beginning as a log/tracing addons that add contextual part (Loggers) and rethink everything from scratch accordingly, IMO.

[Kixunil]

I was thinking about this for a while and the only somewhat reasonable approach I could figure out is:

• For each method taking key add another one with a different name (e.g. suffix _new) and taking newtyped Key as argument
• The default implementation converts the argument to current type - &'static str without dynamic_keys (newtyped Key is just struct Key(&'static str); in this case), Key with dynamic_keys
• Deprecate old methods with explanation to use the others (could also make deprecation conditional on feature and suggest using the feature instead of calling other methods.

Maybe there's a way to use semver trick to cleanup the names but I couldn't figure out how.

[Techcable]

I was thinking about this for a while and the only somewhat reasonable approach I could figure out is:

* For each method taking key add another one with a different name (e.g. suffix `_new`) and taking newtyped `Key` as argument

* The default implementation converts the argument to current type - `&'static str` without `dynamic_keys` (newtyped `Key` is just `struct Key(&'static str);` in this case), `Key` with `dynamic_keys`

* Deprecate old methods with explanation to use the others (could also make deprecation conditional on feature and suggest using the feature instead of calling other methods.

Unfortunately, think something like this is what we're probably going to have to go with :(

We would have to change pretty much every method in the slog::Serializer trait, which seems like a lot of renaming.

Most users use slog macros instead of calling serializer methods directly, so slog consumers aren't hurt very much by this, mostly slog backends.

make deprecation conditional on feature and suggest using the feature instead of calling other methods.

Yes. I think something like using feature flags is probably the "cleanest" way to go.

Right now, we have the new type Key behind a feature="dynamic-keys".

Ideally, we would eventually invert this behavior. We would use a new type key by default, with the old behavior behind a feature flag.

In slog 2.8 we could give nasty deprecation warnings for anyone using non-newtype keys. We could warn that newtype keys are becoming the new default and the old usage is deprecated. We could then suggest using feature="newtype-keys" to silence the deprecation warnings.

Assuming they use the macros, clients are mostly unaffected by this change, it's implementers that will struggle with it.

In slog 2.9 we could make feature="newtype-keys" the behavior the default, and the old behavior behind feature="legacy-keys".

Maybe there's a way to use semver trick to cleanup the names but I couldn't figure out how.

Yeah. I've been thinking about @dpc saying about ecosystem breakage in "slog 3.0" and he's unfortunately right.

From the motivation from the semver-trick crate:

The upgrade of libc from 0.1 to 0.2 in 2015 was known as the "libcpocalypse".
[....]
For longer dependency chains this is a huge ordeal and requires coordinated effort across dozens of developers. During the most recent libcpocalypse, Servo found themselves coordinating an upgrade of 52 libraries over a period of three months (servo/servo#8608).

Ouch.

Is it possible to use the semver trick with different versions traits and an adapter impls?

So like we have something like impl<T> slog2::Serializer for T where T: slog3::Serializer { .... }

This would allow slog2 clients to use slog3 serializer, at least in theory. We could also do it the other way around if we want, allowing slog3 clients to use slog2 serializers.

Because of Rust's coherence rules, this could only work one way, not both.

I guess the only way we could find out is by prototyping this type of semver compatibility hack and seeing what breaks ...

In an ideal world, we'd want client updates from 2.8 -> 3.0 to be completely seamless with only a little breakage for logging backends.....

I don't think there's any easy solution to this problem.