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

x/pkgsite: support to set minimum log level #40339

Open
amarjeetanandsingh opened this issue Jul 22, 2020 · 4 comments
Open

x/pkgsite: support to set minimum log level #40339

amarjeetanandsingh opened this issue Jul 22, 2020 · 4 comments

Comments

@amarjeetanandsingh
Copy link

@amarjeetanandsingh amarjeetanandsingh commented Jul 22, 2020

Right now there isn't a proper way to disable a log level.

For example, if you want to disable the Debug logs, you need to comment out this line

I feel there should be better way to setup the minimum log level below which, no logs should be printed.

For example-
If we consider the log severity sequence as Debug < Info < Error < Fatal and the log level is set to Info, the Debug logs shouldn't be printed but Info Error & Fatal should be printed.

If allowed, I would like to work on the implementation.

/cc @julieqiu

@julieqiu
Copy link
Contributor

@julieqiu julieqiu commented Jul 22, 2020

Thanks, @amarjeetanandsingh! Feel free to send a CL for this issue.

@amarjeetanandsingh
Copy link
Author

@amarjeetanandsingh amarjeetanandsingh commented Jul 24, 2020

@julieqiu
How to take the log level as input? And what should be the default log level?

1)

Like other configs, I think we should use GO_DISCOVERY_LOG_LEVEL env variable to set the log level config. Then get the log level value in log package from config and put a check before logging.

We should go with default log level as error.

Is approach (1) acceptable?


2)

Other way i can think of is to take the log level as command line arg. But I am not sure this would be good approach in this context.

@jba
Copy link
Contributor

@jba jba commented Jul 29, 2020

I'm okay with this idea. Namely:

  • The internal/config package will read and store GO_DISCOVERY_LOG_LEVEL.
  • Th internal/log package will use that config value to drop log messages below the level.
  • The default will be the empty string, meaning "show all levels." We do not want to change default behavior. In particular, we use the Stackdriver log viewer to filter logs for us, so there is no upside to filtering the Stackdriver logs on the client and a big downside of losing valuable debugging information.
@gopherbot
Copy link

@gopherbot gopherbot commented Jul 30, 2020

Change https://golang.org/cl/245797 mentions this issue: internal/log: add log level support

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.