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

log/zap: Allow configuring formatter independently of debug option #442

Closed
alvaroaleman opened this issue May 24, 2019 · 8 comments · Fixed by #560
Closed

log/zap: Allow configuring formatter independently of debug option #442

alvaroaleman opened this issue May 24, 2019 · 8 comments · Fixed by #560
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/feature Categorizes issue or PR as related to a new feature. priority/backlog Higher priority than priority/awaiting-more-evidence.

Comments

@alvaroaleman
Copy link
Member

Right now, the formatter/encoder of the log/zap package is inferred from the debug option, if its enabled a console encoder is used, otherwise a json encoder. This does not always make sense, e.G. sometimes debug log may help in production, but that should not result in unstructured logging.

It would be awesome, if another option was added to RawLoggerTo to make this configurable. Alternatively, we could use a different type for opts that contains an option for the encoder.

@DirectXMan12
Copy link
Contributor

/kind feature
/priority backlog
/help-wanted

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. priority/backlog Higher priority than priority/awaiting-more-evidence. labels Jun 3, 2019
@DirectXMan12
Copy link
Contributor

/help

@k8s-ci-robot
Copy link
Contributor

@DirectXMan12:
This request has been marked as needing help from a contributor.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

/help

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label Jun 3, 2019
@hpandeycodeit
Copy link

@alvaroaleman @DirectXMan12 made some changes to make this configurable. #467 Still WIP as I need to modify the LoggerTo from other places as well if we are looking to change it dynamically.

One question here, how are we deciding when to use console encoder and when not?

@DirectXMan12
Copy link
Contributor

By default, it was debug vs non-debug, but I think we need to start considering how we want to expose these options.

@hpandeycodeit
Copy link

@DirectXMan12 can't we use existing development flag to use this option in production as well?

// If development is true, a Zap development config will be used
// (stacktraces on warnings, no sampling), otherwise a Zap production
// config will be used (stacktraces on errors, sampling).

@DirectXMan12
Copy link
Contributor

The development flag control also controls output format (human-readable vs json), so we need more granular options.

@DirectXMan12
Copy link
Contributor

Perhaps we should adopt the "functional options" style here too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/feature Categorizes issue or PR as related to a new feature. priority/backlog Higher priority than priority/awaiting-more-evidence.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants