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 to change log level for server. #346

Merged
merged 6 commits into from
Dec 23, 2021
Merged

Allow to change log level for server. #346

merged 6 commits into from
Dec 23, 2021

Conversation

miry
Copy link
Contributor

@miry miry commented Dec 22, 2021

Add possobility to change output verbosity via environment varaible
LOG_LEVEL. There are several values: panic, fatal, error, warn or
warning, info, debug and trace.
By default info is used.

close: #343

Add possobility to change output verbosity via environment varaible
`LOG_LEVEL`. There are several values: panic, fatal, error, warn or
warning, info, debug and trace.
By default `info` is used.
@miry miry self-assigned this Dec 22, 2021
@miry miry added the Toxiproxy label Dec 22, 2021
Copy link
Contributor

@Strech Strech left a comment

Choose a reason for hiding this comment

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

Should we try to use logrus level as default instead of string?

@@ -42,3 +46,18 @@ func main() {

server.Listen(host, port)
}

const LOG_LEVEL_DEFAULT = "info"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we call it DEFAULT_LOG_LEVEL?

Suggested change
const LOG_LEVEL_DEFAULT = "info"
const DEFAULT_LOG_LEVEL = logrus.InfoLevel

cmd/server/server.go Outdated Show resolved Hide resolved
Copy link
Contributor

@thepwagner thepwagner left a comment

Choose a reason for hiding this comment

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

LGTM - one typo but the rest is optional.

Seeing "config file" in the diff, I wondered about why this was an environment variable instead of part of the config. I grok that the configuration is rooted as an array, so there's no nice place to add a "global" configuration option. Environment variables make sense!

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
if ok {
lvl, err := logrus.ParseLevel(val)
if err != nil {
return
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: is err is worth logging here, perhaps at .Error()?

Since we used LookupEnv, we know the user provided a LOG_LEVEL value. They may want feedback if that value was incorrect:

  • err will give them not a valid logrus Level: FOO_BAR
  • Overkill, but you could build a message from logrus.AllLevels to include what levels are accepted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thepwagner Updated the code to print error message in case the LOG_LEVEL is not valid.

miry and others added 4 commits December 22, 2021 15:16
Co-authored-by: Pete Wagner <1559510+thepwagner@users.noreply.github.com>
Co-authored-by: Pete Wagner <1559510+thepwagner@users.noreply.github.com>
@miry miry merged commit 0d52fb6 into master Dec 23, 2021
@miry miry deleted the log-level branch December 23, 2021 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Docker container log level
4 participants