WIP: funcr: split out PseudoStruct into separate package

[pohly]

This makes it possible to reference it in other LogSinks which want to provide the same functionality without pulling in the funcr source code.

Doing the same with current master results in "go mod vendor" including "funcr.go" under "vendor".

[pohly]

I'm leaning towards reusing the funcr.PseudoStruct for structured errors in klog instead of adding a new top-level type (i.e. this replaces PR #164).

This PR makes such a reuse a bit nicer (no need to vendor funcr.go in Kubernetes).

[thockin]

why funcr/types rather than putting this in logr or logr/x or similar?

[pohly]

Because then we would need to discuss whether this is the right name and type for this kind of thing and how it relates to the upcoming slog changes - I was trying that in #164.

Moving the type inside funcr sidesteps all of that while still making it easier to support such a feature in Kubernetes (smaller vendor changes). It's a purely pragmatic choice...

[thockin]

The problem I have with a more structured form is that all of the Logger interfaces take []interface{}. Pseudostruct was added so that callers could choose to render things like the saved values as a pseudo-struct. If we normalize on anything but []interface{}, then we are forcing that specific use-case to allocate and/or copy data around.

For example, funcr saves the WithValues() values in a []interface{} and offers a hook before rendering them. That gets called on EVERY SINGLE log line. If a new logr.PseudoStruct is a []KeyValuePair, then that hook needs to make a new slice and populate it, render that, then discard the results (or maybe cache it until it changes, I guess, but that is scary). That's a lot of work to do on every log line.

So instead we save it as that struct. That's an API break, so it needs a new hook. Then any situation which DOESN'T use that hook for this specific purpose has to unpack the slice into a new []interface{} in order to call Info/Error.

It's a loss either way. funcr could detect which hook you set up and store the data that way natively, I guess. Yuck. As I said elsewhere, I am not keen on major changes here, with slog on the horizon.

It feels very weird to have k8s vendor funcr/types and not use funcr itself.

If all you want is the type definition and we are NOT going to make it part of logr proper, why not just copy it into klog?

[pohly]

As I said elsewhere, I am not keen on major changes here, with slog on the horizon.

That's why I am proposing this PR here as the interim solution until we can use slog: it's no change for funcr and would enable the implementation of kubernetes/klog#357.

If all you want is the type definition and we are NOT going to make it part of logr proper, why not just copy it into klog?

Because then if we start to use klog.PseudoStruct in, say, client-go and someone builds a binary which uses client-go and funcr, funcr will not recognize the special meaning of klog.PseudoStruct (because it's not the funcr.PseudoStruct type that it knows and supports) and render it poorly.

It doesn't matter for Kubernetes, but it still looks like a missed opportunity to avoid diverging behavior between loggers to me.

[thockin]

Because then if we start to use klog.PseudoStruct in, say, client-go and someone builds a binary which uses client-go and funcr, funcr will not recognize the special meaning of klog.PseudoStruct

s/funcr/zapr/ or glogr or zerologr ... ?

Are we going to ask them to defend on funcr/types? What I means is - we either expect users to be able to choose any implementation of logr and get decent behavior, or else we are biasing the discussion - k8s works with klog of funcr, but anything else...may produce bad results. Is that OK?

[pohly]

If these other implementations want to offer similar functionality, then yes, they would have to depend on funcr/types.

We have four choices right now:

• standardize on a type in logr
• let each LogSink implementation provide its own, incompatible type
• make it easier for other LogSink implementations to support the type defined by funcr
• do nothing

We ruled out the first choice because it raises questions for which we don't have answers.

To me, the second choice seems worse than the the third:

• some compatibility between implementations is better than none
• because funcr is part of logr, other implementations can use the type without adding any new dependency and (with this PR) pulling more code into their vendor directory than strictly necessary
• zapr and klogr would also be incompatible with each other unless I create an entirely new module with just a type definition in it - I cannot put the type into zapr and use it in klogr, nor the other way around.

The fourth choice means that klogr and zapr wouldn't provide functionality that was considered useful for funcr before and now also has a specific use case in klog, or do it by depending on the funcr package instead of a lighter funcr/types. I wouldn't mind waiting for slog, but I don't like that we hold back implementing functionality out of concerns for compatibility when the cat is already of the bag.

[thockin]

I have not had ANY time for logr in the last couple months. I don't really have time now, either, but I am trying to clear some backlog :)

We ruled out the first choice because it raises questions for which we don't have answers.

I don't think we really ruled this out - there was some discussion of using a different format, but I don't think it's concluded. I think I'd be OK with something like logr/x with docs that these are OPTIONAL extensions for implementations to converge upon, with type PseudoStruct (happy to have a better name :) as the first such.

[pohly]

I think we agreed to not extend the main go-logr API (which would include such a new package) while it was unclear how slog evolves. That is still a valid concern: slog has https://godocs.io/golang.org/x/exp/slog#GroupValue which is conceptually very similar.

Let's see how that turns out, perhaps we can use it instead of a go-logr specific type and then get interoperability with slog.Handler implementations.

[pohly]

The recommendation is that logr.LogSink implementations should also support slog types because they might be wrapped by slogr. Therefore we don't need to promote PseudoStruct to a first-class logr type because client code can use slog.GroupValue instead.