Optional enforcement of *Context methods

[mattdowdell]

I came across this on golangci/golangci-lint#4133 and it looks to be very close to some of what I wanted to do myself. Given that you have a good foundation here already, I wondered if you'd be open to an additional optional extra.

Right now, the slog API allows methods like slog.Info and slog.InfoContext. The stdlib doesn't appear to differentiate between these, but a custom handler might take values from the context and add them as fields, e.g. trace/span IDs. What I'd like to be able to enforce is making sure the context is always specified which means disallowing the methods not suffixed with Context.

I suspect this is behaviour that only makes sense with a custom backend, hence the optionality. Is this something you'd be open to?

Another couple of ideas I had that are about consistency and style than API usage:

• Be able to enforce a format for attribute keys, e.g. kebab-case, snake_case or camelCase, to ensure consistency in aggregated logs and make it easier to group values together the log viewer of choice.
• Detect and reject uses of fmt.Sprintf when building log messages in favour of putting interpolated values into attributes instead.

[tmzane]

Hi,

Thanks for sharing, I really like your ideas!

Let's start with implementing the context-only option and then we can discuss the rest in more detail.

Would you like to make a PR? 😉

[mattdowdell]

I would indeed :) #14

[mattdowdell]

@tmzane Would you be willing to take a PR for my last idea on my OP as well? If yes, any suggestion for the flag name?

[tmzane]

I'm not sure about the last one, looks like a really rare case 🤔

Maybe you have an example of such issue in some open source project?

I've seen something like this once:

msg := fmt.Sprintf(...)
slog.Info(msg, ...)

But msg could be used in other calls as well, which means a false-positive if we report it...

fmt.Sprintf can also be used to build a message with constants that aren't log arguments.

[mattdowdell]

I don't have any open source projects that would need it. Everything I work on day-to-day is unfortunately internal. But perhaps I can explain my rationale. If it's a no at the end, that's ok :)

As it stands, I work with lots of team using a variety of logging libraries, e.g. logrus, zap and zerlog. logrus and zap support things like logger.Infof("message: %s", arg). I'd like to start to adopting log/slog in new services, at which point teams are going to discover that interpolation is no longer a thing.

What we want to do is encourage teams to embrace structured logging and move dynamic values into attributes. That's mostly so we can improve our ability to group logs from a wide variety of services in our log aggregation system. It's relatively easy to search for "log.customer_id == 'whatever'" and know you're not going to get false positives. With everything mixed into the message, things get more complicated.

As an aside, if we're using fmt.Sprintf that means we're eagerly evaluating the interpolation. For log levels that aren't emitted by default according to your configuration (debug in my case), that's some amount of performance hit that doesn;t need to be paid. Obviously that's not a huge hit, but it's enough for langauges like Python to design their logging interpolation to be lazy. It may also be why zerolog and slog don't offer printf style logs, but I haven't spotted that in any design doc yet.

[tmzane]

I don't have any open source projects that would need it. Everything I work on day-to-day is unfortunately internal.

I meant maybe you've seen it somewhere on github or some other platform, so we can start from something :)

What we want to do is encourage teams to embrace structured logging and move dynamic values into attributes.

I totally get your rational! In the end, that's the whole point of structured logging.

For now, I'm just not sure if this check is common enough and worth a new option. I can see how other checks may be useful in an average project that uses log/slog, but this one looks rather exotic.

Is there anything else you'd like to report or is it just this case?

slog.Info(fmt.Sprintf(...))

[tmzane]

Now that I'm thinking about it, it might look like "message should be a constant value", because if it was dynamic, we wouldn't be able to search for it in a log aggregation system (at least without regexp) 🤔