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.

Sign up for GitHub

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

Jump to bottom

Add capability to set loglevel to trace during runtime #1171

Merged
merged 1 commit into from
Oct 8, 2024
Merged

Add capability to set loglevel to trace during runtime #1171

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,10 @@ func main() {
logrus.SetLevel(level)
log.EnableTracing(true)
log.FromContext(ctx).WithField("duration", time.Since(now)).Infof("completed phase 1: get config from environment")
logruslogger.SetupLevelChangeOnSignal(ctx, map[os.Signal]logrus.Level{
syscall.SIGUSR1: logrus.TraceLevel,
syscall.SIGUSR2: level,
})
Comment on lines +138 to +141
Copy link
Member

Choose a reason for hiding this comment

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

I tihnk we can ignore it if current lelvel is trace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

I think adding a new 'if branch' is lesser evil than having extra goroutine and subscription on the signals.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay, you convinced me.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:'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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


// ********************************************************************************
// Configure Open Telemetry
Expand Down
Loading