-
Notifications
You must be signed in to change notification settings - Fork 360
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
tetra: Added dynamic log level change option #2643
tetra: Added dynamic log level change option #2643
Conversation
✅ Deploy Preview for tetragon ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Like the idea started reviewing before I noticed it was draft. But would be a good addition thanks for working on it. |
fcf5244
to
ff3a4aa
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.
Hey, sorry I have not reviewed that until now.
As we discussed offline I have no strong feeling over that and thought that it was maybe a bit overkill to dedicate 3 RPCs for this, I would see how we could extend that in the future to do more debugging things:
- enabling gops
- enable pprof
Maybe we could factor all that into SetDebug
and have more options for evolution so that we don't need many calls. Not sure Reset or Get is particularly needed but maybe I'm missing something.
So to recap my opinion would be to reduce this to 1 RPC that could be extended. But honestly, I'm not sure that's better so that would be cool to have other peoples' opinion or just merge this if it's satisfying to someone. It would be cool to add a test though!
Overall the PR is great and thanks for adding docs that's amazing.
eb75794
to
a4f6457
Compare
@mtardy Thanks a lot for your valuable input! You're right. Adding three different gRPC endpoints solely for this is an overkill 😅 . Let's make it a bit more generic so that, e.g., gops and pprof could also be added in the future. How about something like this: // For now, we only want to support debug-related config flags to be configurable.
enum ConfigFlag {
CONFIG_FLAG_LOG_LEVEL = 0;
}
enum LogLevel {
LOG_LEVEL_PANIC = 0;
LOG_LEVEL_FATAL = 1;
LOG_LEVEL_ERROR = 2;
LOG_LEVEL_WARN = 3;
LOG_LEVEL_INFO = 4;
LOG_LEVEL_DEBUG = 5;
LOG_LEVEL_TRACE = 6;
}
message GetDebugRequest{
ConfigFlag flag = 1;
}
message GetDebugResponse{
ConfigFlag flag = 1;
LogLevel level = 2;
}
message SetDebugRequest{
ConfigFlag flag = 1;
LogLevel level = 2;
}
message SetDebugResponse{
ConfigFlag flag = 1;
LogLevel level = 2;
}
service FineGuidanceSensors {
...
rpc GetDebug(GetDebugRequest) returns (GetDebugResponse) {}
rpc SetDebug(SetDebugRequest) returns (SetDebugResponse) {} This could then be used within func (s *Server) GetDebug(_ context.Context, req *tetragon.GetDebugRequest) (*tetragon.GetDebugResponse, error) {
switch req.GetFlag() {
case tetragon.ConfigFlag_CONFIG_FLAG_LOG_LEVEL:
logger.GetLogger().Debugf("Client requested current log level: %s", logger.GetLogLevel().String())
return &tetragon.GetDebugResponse{
Flag: tetragon.ConfigFlag_CONFIG_FLAG_LOG_LEVEL,
Level: tetragon.LogLevel(logger.GetLogLevel()),
}, nil
default:
logger.GetLogger().WithField("request", req).Warnf("Client requested unknown config flag %d", req.GetFlag())
return nil, fmt.Errorf("client requested unknown config flag %d", req.GetFlag())
}
}
func (s *Server) SetDebug(_ context.Context, req *tetragon.SetDebugRequest) (*tetragon.SetDebugResponse, error) {
switch req.GetFlag() {
case tetragon.ConfigFlag_CONFIG_FLAG_LOG_LEVEL:
currentLogLevel := logger.GetLogLevel()
changedLogLevel := logrus.Level(req.GetLevel())
logger.SetLogLevel(changedLogLevel)
logger.GetLogger().WithField("request", req).Warnf("Log level changed from %s to %s", currentLogLevel, changedLogLevel.String())
return &tetragon.SetDebugResponse{
Flag: tetragon.ConfigFlag_CONFIG_FLAG_LOG_LEVEL,
Level: tetragon.LogLevel(changedLogLevel),
}, nil
default:
logger.GetLogger().WithField("request", req).Warnf("Client requested change of unknown config flag %d", req.GetFlag())
return nil, fmt.Errorf("client requested change of unknown config flag %d", req.GetFlag())
}
} I basically gave this a try via 077b523. IMO, this approach keeps our two new gRPC endpoints extendable for the future while not overengineering things for now. gops and pprof should be addressed in separate PRs, when required. |
a4f6457
to
c54d30f
Compare
Added a tetra loglevel subcommand which allows one to dynamically change Tetragon's log level without restarting it. Signed-off-by: Philip Schmid <[email protected]>
c54d30f
to
fa37e5b
Compare
(Fixed two failed CI tests with the latest two force pushes.) |
Signed-off-by: Philip Schmid <[email protected]>
Due to this new refactoring it's possible to only use two new gRPC endpoints which are then extendable in the future for things like gops, pprof, etc. configurations. Signed-off-by: Philip Schmid <[email protected]>
fa37e5b
to
077b523
Compare
I just added basic tests for |
Signed-off-by: Philip Schmid <[email protected]>
46be259
to
a0c4b49
Compare
CONFIG_FLAG_LOG_LEVEL = 0; | ||
} | ||
|
||
enum LogLevel { |
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.
Thanks, sorry for the delay I was on PTO 🏝️ |
Amazing, it seems we have a first candidate to extend this debug RPC with process cache dump, Anastasios was thinking adding a RPC just for that might be overkill and we just have this now here, #2246. Thanks @PhilipSchmid for making this more generic, it seems it will be useful. |
See also: #2876 |
A tetra loglevel subcommand was added, which allows one to change Tetragon's log level dynamically without restarting it.
Supported subcommands:
get Prints the current log level set Set the log level
Fixes: #2545
I manually tested it from a Linux and macOS client, issuing all sorts of
tetra loglevel *
commands. Worked flawlessly. Unfortunately, I don't have a Windows client, so I can't test it from there, but I'd expect the same results as it doesn't depend on local files or binaries.