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

Jump to bottom

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

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

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

amarjeetanandsingh opened this issue Jul 22, 2020 · 4 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. pkgsite
Milestone
Unreleased

Comments

@amarjeetanandsingh
Copy link
Contributor

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
@gopherbot gopherbot added this to the Unreleased milestone Jul 22, 2020
@julieqiu julieqiu added pkgsite NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jul 22, 2020
@julieqiu
Copy link
Member

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

@amarjeetanandsingh
Copy link
Contributor Author

amarjeetanandsingh commented Jul 24, 2020 •
edited

@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 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

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

@golang golang locked and limited conversation to collaborators Aug 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. pkgsite
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@julieqiu @gopherbot @amarjeetanandsingh @jba