refactor(promslog): make NewNopLogger() wrapper around New()

[tjhop]

While discussing the fix for prometheus/prometheus#16466, it was pointed
out that our promslog.NewNopLogger() convenience function would
benefit from using the same logic used when initializing loggers with
promslog.New(). By refactoring NewNopLogger to wrap New, it inherits
that same setup logic.

Signed-off-by: TJ Hoplock [email protected]

[tjhop]

cc: @machine424 @SuperQ

[machine424]

Thanks for this.

Thinking about it more for the Prometheus use case, others may argue that the logger is no longer truly a “no-op”, it does more things now, and the added overhead may not be acceptable in some scenarios.

If that’s a concern, perhaps we can have that under a new NewDiscardLogger, NewSilentLogger or something to avoid that overhead. In Prometheus, we’d use that everywhere (we could even employ forbidigo or something similar to block NewNopLogger usage, as for Prometheus itself we always initialize a new real logger) and clearly communicate this to “Prometheus as a library” users. They could still override it on their side with NewNopLogger (if they use common) or any other no-op logger if they need.

cc @bboreham @dimitarvdimitrov as they worked on related stuff according to prometheus/prometheus#15993

[bboreham]

I see the genesis of this was for testing. I don't think we want to turn on debug logging for nop-logger in non-test situations.

Also I'm going to change the title to reflect this change.

[tjhop]

Agreed, I don't find the debug logging to be a requirement here, personally. It was a suggestion from the linked issue, the original patch I suggested just updated the function to wrap New()

[machine424]

Yes it was my idea, because the issue/panic that made me suggest this change was at a Debug log https://github.com/prometheus/prometheus/blob/2e9ab9cc62c563cfeed21eb4100d3960812ca9a6/discovery/azure/azure.go#L725

It's not ideal that when you need the Debug logs the most you find them panicing, it would just add to the frustration.

Thant's why I suggested the following for non-test/prod situations where Prom is used as a library:

If that’s a concern, perhaps we can have that under a new NewDiscardLogger, NewSilentLogger or something to avoid that overhead. In Prometheus, we’d use that everywhere (we could even employ forbidigo or something similar to block NewNopLogger usage, as for Prometheus itself we always initialize a new real logger) and clearly communicate this to “Prometheus as a library” users. They could still override it on their side with NewNopLogger (if they use common) or any other no-op logger if they need.

But if you still think that's not worth it and if you're ok with the overhead, even without debug level, that the change adds to NewNopLogger, let's go for the initial change and just change NewNopLogger.

[tjhop]

I think it's reasonable to do within the context of the existing noop logger. This is the prometheus project's opinionated library, and I think aligning the initialization of the constructors of New() and NewNopLogger() is reasonable. It still accomplishes the same goal that a library user has, which is a logger that doesn't do anything.

I don't disagree with you on the semantics though -- the promslog's NewNopLogger() was arguably never a true noop logger, as it always just writes to io.Discard, whereas a proper noop logger should likely be a custom handler that just returns nil in the Handle() func. It was created as a convenience function and named like go-kit/log's NewNopLogger(), which was heavily used through the prometheus ecosystem before we adopted log/slog.

It's not ideal that when you need the Debug logs the most you find them panicing, it would just add to the frustration.

Fully agreed, but just to be clear, the level isn't relevant to this particular bug; #745 and #746 have more details, but the original formatting code I wrote for log formatting was naive and didn't handle duplicate keys well. This bug could've triggered at any log level. I'll still vote to remove the debug level and update the PR; it can always be added in the future if needed

[machine424]

thanks for the context.
with if !l.Enabled(ctx, level) { early returns, we'd still have code that only run at debug level, but let's start with this, if others are ok with it. We can always reconsider later.

[machine424]

by the way, I had given this version a try on Prometheus, and only a test TestNewGroup which compares loggers would need to be adapted.