refactor(promslog): make NewNopLogger() wrapper around New() by tjhop · Pull Request #783 · prometheus/common (original) (raw)
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation8 Commits2 Checks4 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
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 withpromslog.New(). By refactoring NewNopLogger to wrap New, it inherits
that same setup logic.
Signed-off-by: TJ Hoplock t.hoplock@gmail.com
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 t.hoplock@gmail.com
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
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.
refactor(promslog): make refactor(promslog): make NewNopLogger() wrapper around New()NewNopLogger() wrapper around New(); turn on debug log flag
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()
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.
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
Signed-off-by: TJ Hoplock t.hoplock@gmail.com
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
refactor(promslog): make refactor(promslog): make NewNopLogger() wrapper around New(); turn on debug log flagNewNopLogger() wrapper around New()
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.
