search

go-simpler / sloglint

🪵 Ensure consistent code style when using log/slog
https://go-simpler.org/sloglint
Mozilla Public License 2.0
75 stars 5 forks source link

Check if log message is constant #17

Closed tmzane closed 8 months ago

tmzane commented 8 months ago

See #13 for the start of the discussion.

@mattdowdell, let's continue here :)

mattdowdell commented 8 months ago

Replying to https://github.com/go-simpler/sloglint/issues/13#issuecomment-1779899335 and https://github.com/go-simpler/sloglint/issues/13#issuecomment-1779927621

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(...))

fmt.Sprintf is the main one I had in mind. I can also appreciate it being fairly exotic and niche. Either way, I'm super happy that 2 out of the 3 suggestions I made were landed, on top of the useful options sloglint already provides.

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) 🤔

I think "message should be a constant value" is a much better description of what I should be looking for. And you're right to point out that doing msg := fmt.Sprintf("..."); slog.Info(msg) would trivially circumvent that. I'm less sure how one would ever lint for constant messages. String literals are easy enough to spot, without question. But I'm unsure whether const msg = "..." should be considered valid and whether you can reliably detect them.

tmzane commented 8 months ago

I'm less sure how one would ever lint for constant messages.

Well, we already do a similar thing, take a look at the no-raw-keys option. I think we could reuse this code with some modifications to check whether the message is a constant value.

But I'm unsure whether const msg = "..." should be considered valid

I think both slog.Info("...") and const msg = "..."; slog.Info(msg) are perfectly valid. If we just reported everything else, that'd be enough.

Also, this check looks like a good default to me (just like mixed-args). At least I can't imagine a situation where a dynamic message (var, fmt.Sprintf, etc) would be desirable. WDYT?

mattdowdell commented 8 months ago

At least I can't imagine a situation where a dynamic message (var, fmt.Sprintf, etc) would be desirable. WDYT?

That sounds good to me.

I did recently come across a case where a team I work with tried very hard to make the case that not all variables made sense as attributes if it hurt readability. Their example was having a message like "failed to connect to database, sleeping for %d seconds". I'm not sure the example is legitimate for a few reasons, but having the ability to disable it globally and/or suppress the linter in golangci-lint should suffice if there's a significant disagreement in the approach.

tmzane commented 8 months ago

Good example! There will be exceptions, totally. The problem is, if we made it a flag, like the other options, it'd be global on / global off. Which is not really convenient to deal with a few exceptions. golangci-lint has a //nolint mechanism to suppress specific reports, per line. Do you think it would be okay to rely on it and make this check a default?

tmzane commented 8 months ago

I'm going to search for log/slog in open source repositories to see if there are exotic message use cases. If you have more examples to add from your experience, I'd appreciate it 🙏

mattdowdell commented 8 months ago

golangci-lint has a //nolint mechanism to suppress specific reports, per line. Do you think it would be okay to rely on it and make this check a default?

I think it's a case of whether the suppressions outweigh the unsuppressed lint rules. Given how new slog is, we may not fully understand how it gets used in the while for some months, particularly as logging tends to be hard to replace in many established applications. If you're willing to be opinionated and your search is not overwhelmingly in the opposite direction, I'd say having this as default is fine.

If you have more examples to add from your experience, I'd appreciate it

Nothing yet, but I'll keep an eye out for them.

tmzane commented 8 months ago

Agree, we definitely need more data. Let's make it optional then, and if it's useful enough, we can make it a default in the future. Would you like to work on the implementation? :)

mattdowdell commented 8 months ago

Sure, I'll take a go at it over the weekend.