support key/value checking for arbitrary functions

[pohly]

The slog.Logger API is just one of possible many other high-level APIs which accept key/value pairs. Another example is go-logr/logr.Logger and some custom wrapper functions in Kubernetes.

It would be great if sloglint could also warn about incorrect parameters in those other methods.

Disclaimer: I'm the maintainer of logcheck, a linter which does similar checking for go-logr APIs, and of logging in Kubernetes. Long-term sloglint could replace or supplant logcheck.

[pohly]

go vet handles wrappers around fmt.Printf and fmt.Print by detecting calls inside wrappers: https://pkg.go.dev/golang.org/x/tools/go/analysis/passes/printf#hdr-Analyzer_printf

Perhaps something similar can be done here?

Alternatively, support special comment tags?

[tmzane]

Hi, did you mean loggercheck? I decided to focus on slog-specific checks to not overlap with this linter too much. Sure, some sloglint checks can be applied to different logging libraries, but it seems a bit late by now to make it a generic logger checker.

Speaking about checking slog wrappers, that would be a great addition and I have plans to implement it later. Detecting calls inside wrappers seems smart, I like that it requires zero configuration, but I also worry about false positives. I'll take a look at go vet's implementation, thank you for the suggestion. We could also implement it as a list of custom functions in the linter config, like I did here. Would such API work for you?

[pohly]

Hi, did you mean loggercheck?

No, https://github.com/kubernetes-sigs/logtools/tree/main/logcheck. Some of its functionality is specific to the migration to structured, contextual logging in Kubernetes, therefore I have not tried to make it more general or get it included in golangci-lint.

make it a generic logger checker

The intention wasn't to add any special support for other loggers. But when wrappers can be marked or detected, then other logging libraries can enable sloglint.

For example, there's https://github.com/kubernetes/klog/blob/2086216a5034ba7c4e3a4fec7d89c5d0434eb1a0/klog.go#L1461-L1467

That could become:

func (v Verbose) InfoS(msg string, keysAndValues ...interface{}) {
	if false {
		slog.Info(msg, keysAndValues...)
	}
	if v.enabled {
		logging.infoS(v.logger, logging.filter, 0, msg, keysAndValues...)
	}
}

Then sloglint will warn about InfoS.

[pohly]

I'll take a look at go vet's implementation

It's here: https://cs.opensource.google/go/x/tools/+/refs/tags/v0.14.0:go/analysis/passes/printf/printf.go

We could also implement it as a list of custom functions in the linter config, like I did here. Would such API work for you?

It would work for Kubernetes, but the list would be long and everyone else using klog would have to copy it into their own golangci-lint config. Automatic detection would be nicer 😁

[pohly]

Another example where I would like to use it:

func HandleErrorWithContext(ctx context.Context, err error, msg string, keysAndValues ...any) {... }

[tmzane]

So, klog wraps slog with key-value style arguments, and you'd like to perform slog checks on it, did I get it right?

I'll work on the wrappers support and let you know 🤝

[pohly]

klog can be used as wrapper around slog - it's a bit more flexible than that, though. But the net effect is the same: the same rules as for slog key/value pairs also apply to it. So yes, your understanding is correct.

[tstraley]

I'd like to express interest in this use-case, as well.

We have our own log package that wraps slog, but would like to apply this linter against our code. We can do so today by cloning this code and modifying slogFuncs in sloglint.go to match our funcs, but that becomes a headache especially when wanting to use golangci-lint to run all of our linters.

I will note that loggercheck has a way to add custom rules as a list of additional function/method signatures eg.

rules:
      - k8s.io/klog/v2.InfoS
      - (github.com/go-logr/logr.Logger).Error
      - (*go.uber.org/zap.SugaredLogger).With

But loggercheck doesn't work with Attrs, just key-value pairs, which is why we like sloglint. :)

[tmzane]

@tstraley Could you provide an example of the wrappers you use? I'm currently researching the possibility to automate detection of slog wrappers, without the need to specify custom rules first. It'd help a lot, thanks.

[pedantic79]

@tmzane The wrapper my team uses looks like this:

type Logger struct {
	*slog.Logger
}

This is mostly to provide helper functions for custom levels.

// Critical logs at LevelCritical.
func (logger *Logger) Critical(msg string, args ...any) {
	logHelper(logger, context.Background(), LevelCritical, msg, args...)
}

// CriticalContext logs at LevelCritical with the given context.
func (logger *Logger) CriticalContext(ctx context.Context, msg string, args ...any) {
	logHelper(logger, ctx, LevelCritical, msg, args...)
}

Where logHelper is similar to the log/slog's internal func (l *Logger) log(ctx context.Context, level Level, msg string, args ...any)

We also have overrides of With() and WithGroup() methods that return a wrapped *Logger instead of a *slog.Logger and some helper methods to handle common logging practices in our code.

Specifically this, where we use to do log.Error(err)

func (logger *Logger) LogErr(level slog.Level, err error)

[pohly]

@tmzane: gentle ping regarding this... it would be great to get this working because in Kubernetes we start to have more APIs which accept log parameters and currently have no linter for those. Our own "logtools/logcheck" doesn't support it either yet.

[tmzane]

Hey, sorry for the late reply, I'm kinda busy these days.

I've actually got something close to working regarding this feature, just need a bit more work to handle edge cases. I'm going to get back to it as soon as I have some free time. Will let you know when there is something ready for testing.