Skip to content
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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

MarcPaquette
Copy link
Contributor

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

@MarcPaquette MarcPaquette requested a review from a team as a code owner October 31, 2024 14:34
"go.uber.org/zap"
"go.uber.org/zap/exp/zapslog"
"go.uber.org/zap/zapcore"
)

var (
conf dynamicLoggingConfig
Conf DynamicLoggingConfig
Copy link
Member

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?

Copy link
Contributor Author

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.

Comment on lines +95 to +96
mutex.Lock()
defer mutex.Unlock()
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

3 participants