-
Notifications
You must be signed in to change notification settings - Fork 226
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
Fix Gorouter's debugserver endpoint to allow log level changes at runtime #452
base: main
Are you sure you want to change the base?
Conversation
"go.uber.org/zap" | ||
"go.uber.org/zap/exp/zapslog" | ||
"go.uber.org/zap/zapcore" | ||
) | ||
|
||
var ( | ||
conf dynamicLoggingConfig | ||
Conf DynamicLoggingConfig |
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.
why is this exported now? I assume for the change in main.go?
exposing the variable means anyone anywhere can change it. what about adding a getter function instead?
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.
We need the satisfy the debugserver's reconfigurable sink: https://github.com/cloudfoundry/debugserver/blob/main/server.go#L24-L26
The base gorouter logger is inited at the package level and privately puts us in a bit of an odd spot with further changes to the logger. I exported the config as we need the debugserver
to actually change the config via SetMinLevel()
Basically since the grlogger.baseLogger
is initialized on importation via init()
and private to the package, I pushed the config out as public as we need something to satisfy the ReconfigurableSinkInterface
. A getter doesn't quite fit the pattern. There is an argument for pushing it further down, but this was a bit simpler.
I went with the config as it's the area where adding the method to satisfy the ReconfigurableSinkInterface
was the least intrusive.
9a9826f
to
3ec0125
Compare
mutex.Lock() | ||
defer mutex.Unlock() |
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.
@ameowlia
I added the Mutex here since there are a few different ways the logging level can get changed.
We have a debugserver in gorouter + explicit code trying to support reconfiguring the log level on the fly. Being able to do this helps operators + support troubleshoot live issues without having to restart Gorouters. This has been broken since ~2017.
This change allows CF Administrators to change gorouter's logging on the fly to triage issue in much finer detail.
This is related to the change in the debug server cloudfoundry/debugserver#72