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

Fix zap grpclogger verbosity check #366

Merged
merged 2 commits into from
Nov 28, 2020

Conversation

bouk
Copy link
Contributor

@bouk bouk commented Nov 27, 2020

Currently it only logs entries that are lower or equal to the
configured level, when you want the opposite. This means that by default
only INFO messages are logged, and anything more urgent is dropped.

Currently it only logs entries that are *lower* or equal to the
configured level, when you want the opposite. This means that by default
only INFO messages are logged, and anything more urgent is dropped.
@johanbrandhorst
Copy link
Collaborator

Hi, thanks for this PR! Could you add a test to check that this is indeed the correct behavior? Thanks!

@bouk
Copy link
Contributor Author

bouk commented Nov 28, 2020

Added!

@codecov-io
Copy link

codecov-io commented Nov 28, 2020

Codecov Report

Merging #366 (78812ab) into master (c05cb40) will increase coverage by 0.60%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #366      +/-   ##
==========================================
+ Coverage   72.81%   73.42%   +0.60%     
==========================================
  Files          41       41              
  Lines        1317     1317              
==========================================
+ Hits          959      967       +8     
+ Misses        304      296       -8     
  Partials       54       54              
Impacted Files Coverage Δ
logging/zap/grpclogger.go 30.76% <100.00%> (+30.76%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c05cb40...78812ab. Read the comment docs.

@johanbrandhorst johanbrandhorst merged commit 61b061e into grpc-ecosystem:master Nov 28, 2020
@johanbrandhorst
Copy link
Collaborator

Thanks for your contribution! Could you please cherry pick this against the v2 branch?

@bouk
Copy link
Contributor Author

bouk commented Nov 29, 2020

The v2 branch doesn't seem to do anything with the global grpc logger

@bouk bouk deleted the zap-grpc-verbosity branch November 29, 2020 15:33
@fxrlv
Copy link

fxrlv commented Aug 17, 2021

verbosity IS NOT about logging level
grpclog package logs error by calling method (*zapGrpcLoggerV2).Error*
verbosity controls how much details we log, NOT level
could you please revert this pull request?

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.

4 participants