Skip to content

Control Gorouter log level using debugserver reconfigurable sink #495

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

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

Conversation

navinms711
Copy link

@navinms711 navinms711 commented Jun 20, 2025

Summary

[1] Briefly explain why this PR is necessary

Gorouter uses slog with zap handler as backend for current logging levels. The Gorouter runs a debugserver (a http server) which listens to a few API endpoints. The /log-level -d <debug/info/error/fatal> was introduced to make changes to the log level on demand at runtime. However, a POST request to the log-level endpoint has no effect in the current code. This PR proposes changes needed to make log level change work through a POST request.

[2] Provide details of where this request is coming from including links, GitHub Issues, etc..

This is a request from Broadcom Engineering team to fix it and recorded in TNZ-23119.

[3] Provide details of prior work (if applicable) including links to commits, github issues, etc...

cloudfoundry/gorouter#452

[4] Backward Compatibility

Breaking Change? NO

[5] If this is a breaking change, or modifies currently expected behaviors of core functionality

@navinms711 navinms711 requested a review from a team as a code owner June 20, 2025 08:15
Copy link

linux-foundation-easycla bot commented Jun 20, 2025

CLA Not Signed

@@ -42,6 +43,27 @@ func DebugAddress(flags *flag.FlagSet) string {
return dbgFlag.Value.String()
}

func validateloglevelrequest(w http.ResponseWriter, r *http.Request, level []byte) error {
Copy link
Author

@navinms711 navinms711 Jun 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll remove this change in the vendor copy of debugserver/server.go. This change will be part of another PR for debugserver repo.

Copy link
Member

@ameowlia ameowlia Jun 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for calling this out. You can remove this change, and link the related PR in the main section. Regular contributors will know how to bump the submodules with multiple linked PRs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you've already made the change in the debugserver you can just add a commit on top of this PR to bump to dependency and re-vendor to make this PR self-contained.

@navinms711
Copy link
Author

Following log entries are seen in the /var/vcap/sys/log/gorouter/gorouter.stdout.log for various inputs on the Router VM.

$ curl http://127.0.0.1:17002/log-level -d info
✅ /log-level was invoked with Level: info

time=2025-06-20T07:46:21.091Z level=INFO msg="Gorouter logger -> zapcore log level updated." new-level=info

$ curl http://127.0.0.1:17002/log-level -d debug
✅ /log-level was invoked with Level: debug

time=2025-06-20T07:46:21.091Z level=INFO msg="Gorouter logger -> zapcore log level updated." new-level=debug

$ curl -X GET http://127.0.0.1:17002/log-level -d info
method not allowed, use POST

$ curl -X PUT http://127.0.0.1:17002/log-level -d info
method not allowed, use POST

$ curl -X POST https://127.0.0.1:17002/log-level -d info
curl: (35) error:0A00010B:SSL routines::wrong version number

$ curl -X POST http://127.0.0.1:17002/log-level -d {}
Invalid log level provided: {}

$ curl -X POST http://127.0.0.1:17002/log-level
log level cannot be empty

// lager's logging system. The Log method logs the message and source
// using the slog.Logger, and the LogLevel method returns the current
// logging level.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove these new-lines between the doc string and the function, otherwise they won't work as godoc comments, see https://go.dev/doc/comment.

Comment on lines +101 to +103
/*
GetLoggingLevel returns the current logging level.
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm not sure how these /* ... */ comments came in here, would you mind adjusting them to be // like we have everywhere else in the code-base? 🙈

@@ -42,6 +43,27 @@ func DebugAddress(flags *flag.FlagSet) string {
return dbgFlag.Value.String()
}

func validateloglevelrequest(w http.ResponseWriter, r *http.Request, level []byte) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you've already made the change in the debugserver you can just add a commit on top of this PR to bump to dependency and re-vendor to make this PR self-contained.

@hoffmaen
Copy link
Contributor

Hello @navinms711 ,
Please do not pollute main.go with code to setup the sink for debugserver.
With cloudfoundry/gorouter#435, we have put a lot of effort to the code base to have a clean and simple abstraction of the zap handler, and we use slog throughout the code as the logging frontend. We have functions to dynamically set the WriteSyncer, which allows to setup a custom sink and to dynamically set the log level (used e.g. in the tests).
Please use the provided library and, if missing, put the code required to run the debugserver into a separate go file.

@hoffmaen
Copy link
Contributor

One more thought: Instead of adding all that code to routing-release, couldn't you make the debugserver handle an slog logger instead of a lager logger?

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.

4 participants