-
Notifications
You must be signed in to change notification settings - Fork 110
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 and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
reset logger level when config isnt present #4232
Conversation
Brought up a discussion offline, but I'm curious if we still want to go down this path of "process all |
logging/logger_registry.go
Outdated
for _, name := range lr.getRegisteredLoggerNames() { | ||
err := lr.updateLoggerLevel(name, INFO) | ||
if err != nil { | ||
return err | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
config/reader.go
".
let's do the INFO reset after processing changes. i think the current approach has a small race condition where an existing logger configured with a non-INFO level is briefly reset to INFO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
after processing changes
Sorry which "processing changes" are you referring to? If we put this block at the end of the function, wouldn't everything get set to INFO
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, i meant just the loggers that are still in the registry but not in the config configured by the config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more specifically, we should keep a count of how many times each registered pattern is configured by a new config pattern, and then reset all registered loggers with 0 configurations back to INFO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah cool; thanks for the clarification.
3c1c1d8
to
78664a2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
discussed IRL, we should add a test that demonstrates this flow for a config that has resource log level configs.
logging/logger_registry.go
Outdated
logger, ok := globalLoggerRegistry.loggerNamed(name) | ||
globalLoggerRegistry.mu.Lock() | ||
defer globalLoggerRegistry.mu.Unlock() | ||
if !ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just noting that a logger can be created after we call loggerNamed
but before we grab the write lock to create one ourselves - not sure how big of an issue that is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logger, ok := globalLoggerRegistry.loggerNamed(name) | |
globalLoggerRegistry.mu.Lock() | |
defer globalLoggerRegistry.mu.Unlock() | |
if !ok { | |
globalLoggerRegistry.mu.Lock() | |
defer globalLoggerRegistry.mu.Unlock() | |
logger, ok := globalLoggerRegistry.loggers[name] | |
if !ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks.
Issue: Suppose we had a registered logger named
a.b.c
that we configured to have the levelWARN
. Then, we removed this configuration option and reconfigured. Thea.b.c
logger will still be set toWARN
despite not specifying this.Solution: Any loggers that no longer have a configuration are reset back to
INFO
. Also, logger changes are applied only once, so there is no undefined behavior of logs during reconfiguration.