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

Allow eggos implementations to programatically set log levels #96

Merged
merged 5 commits into from
Feb 4, 2022
Merged

Allow eggos implementations to programatically set log levels #96

merged 5 commits into from
Feb 4, 2022

Conversation

jspc
Copy link
Contributor

@jspc jspc commented Feb 4, 2022

When running applications on platforms where there is no environment, calls to os.Setenv to set a loglevel have inconsistent results. Similarly, setting this environment variable before the first log event, and thus making that call moot, is racy.

Instead, then, export log levels, drop the potential stutter in log.LogLvl to log.Level, and allow implementations/ callers of this package to set a log level accordingly, while maintaining the default behaviour

jspc and others added 3 commits February 4, 2022 12:51
When running applications on environments where there is no environment, calls to os.Setenv to set a loglevel have inconsistent results. Similarly, setting this environment variable before the first log event, and thus making that call moot, is racy.

Instead, then, export log levels, drop the potential stutter in log.LogLvl to log.Level, and allow implementations/ callers of this package to set a log level accordingly, while maintaining the default behaviour
log/log.go Outdated
)

var (
loglvl int
loglvlonce sync.Once
Level int
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe a setter would be better, like SetLogLevel?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing, if that's the way you'd prefer it. It'd still be nice to have this public to be able to inspect the value- would you prefer a getter, or the naked var?

Copy link
Owner

Choose a reason for hiding this comment

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

Gopher prefer naked var 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My worry is that adding the setter, then, adds complexity and can be bypassed by setting directly on the naked value.

I'll make sure that the setter is well documented to explain that it does additional checking to make sure the specified level is a valid level. That's probably the best of all worlds!

Copy link
Owner

Choose a reason for hiding this comment

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

The current log package implementation is relatively simple, I think there is no problem if you implement it in a simple way.

@jspc
Copy link
Contributor Author

jspc commented Feb 4, 2022

@icexin ♻️

I've written a setter with bounds checking (with some tests) and made it accept an explicit log level type. I've left the init() function alone, so it still sets log.Level directly, since it does it's own bounds checking1

@jspc jspc requested a review from icexin February 4, 2022 15:24
Copy link
Owner

@icexin icexin left a comment

Choose a reason for hiding this comment

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

LGTM

@icexin icexin merged commit 86eec44 into icexin:main Feb 4, 2022
@jspc jspc deleted the programmatic_log_levels branch February 4, 2022 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants