Add capability to set loglevel to trace during runtime #1171
Conversation
arp-est
force-pushed
the
arp_dyn_loglevel
branch
from
7abb780
to
d4f4d81
Compare
|
You mean something like adding a function that encapsulates all this signal thing to the sdk, and just calling a single function from the cmds? It sounds awesome, I feared that I would have to copy paste the current pr to every cmd, but this makes it a lot easier, I'll look around. |
main.go
Outdated
| func handleLogChangeSignal(ctx context.Context, ch chan os.Signal, logLevel logrus.Level) { | ||
| OUT: | ||
| for { | ||
| select { | ||
| case <-ctx.Done(): | ||
| break OUT | ||
| case sig := <-ch: | ||
| switch sig { | ||
| case syscall.SIGUSR1: | ||
| log.FromContext(ctx).Infof("Setting log level to %s", logrus.TraceLevel.String()) | ||
| logrus.SetLevel(logrus.TraceLevel) | ||
| case syscall.SIGUSR2: | ||
| log.FromContext(ctx).Infof("Setting log level to %s", logLevel.String()) | ||
| logrus.SetLevel(logLevel) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Also, I wonder, does it make sense to run the pprof server in case of enabling trace level via signals?
cc @szvincze
There was a problem hiding this comment.
@denis-tingaikin: Since the TRACE log level could have influence on the different profiles, like CPU and memory, I would not tie the log level and profiling together but keep them separately configurable. If someone needs both activated then configure them one by one.
arp-est
force-pushed
the
arp_dyn_loglevel
branch
from
d4f4d81
to
a71ced1
Compare
|
Put the implementation here, not sure whether the log directory, or the logrus subdirectory would be preferred, but this way, I could just use function from the log module directly which was convenient. |
arp-est
force-pushed
the
arp_dyn_loglevel
branch
2 times, most recently
from
1a5faf0
to
94bfc9b
Compare
main.go
Outdated
| logruslogger.SetupLevelChangeOnSignal(ctx, map[os.Signal]logrus.Level{ | ||
| syscall.SIGUSR1: logrus.TraceLevel, | ||
| syscall.SIGUSR2: level, | ||
| }) |
There was a problem hiding this comment.
I tihnk we can ignore it if current lelvel is trace.
There was a problem hiding this comment.
I'm not sure, I don't think its worth adding an extra 'if' statement to check against the config. It shouldn't matter that much the worst case would be having a few extra lines of log stating that the current level is trace.
There was a problem hiding this comment.
I think adding a new 'if branch' is lesser evil than having extra goroutine and subscription on the signals.
There was a problem hiding this comment.
okay, you convinced me.
There was a problem hiding this comment.
Also, I agree that it would be better to not add a new if branch; probably we could add a check on signal map size in the 'SetupLevelChangeOnSignal' and say if size is less than 2, then do nothing. Thoughts?
There was a problem hiding this comment.
:'D how about we add it to the function in the sdk instead? :'D
run golangci-lint Running [/home/runner/golangci-lint-1.53.3-linux-amd64/golangci-lint run --out-format=github-actions --timeout 3m --verbose] in [] ... Error: cyclomatic complexity 16 of func main is high (> 15) (gocyclo)
There was a problem hiding this comment.
Also, I agree that it would be better to not add a new if branch; probably we could add a check on signal map size in the 'SetupLevelChangeOnSignal' and say if size is less than 2, then do nothing. Thoughts?
Yeah sounds good
There was a problem hiding this comment.
I mean yeah in addition to checking if the original level is not trace, in addition to that it is a correct opinion to check the map's size too yeah.
Signed-off-by: Arpad Kiss <arpad.a.kiss@est.tech>
arp-est
force-pushed
the
arp_dyn_loglevel
branch
from
94bfc9b
to
bc6cb95
Compare
networkservicemesh/deployments-k8s#12296