Skip to content

fix(zapgrpc): add WithVerbosity for correct grpclog V() semantics - #1597

Open
hardikkaurani wants to merge 1 commit into
uber-go:masterfrom
hardikkaurani:fix-zapgrpc-verbosity
Open

hardikkaurani wants to merge 1 commit into
uber-go:masterfrom
hardikkaurani:fix-zapgrpc-verbosity

Conversation

@hardikkaurani

Copy link
Copy Markdown

Fixes #1544

Problem

In grpclog.LoggerV2, V(l int) bool is documented to report whether verbosity level l is at or below the configured verbose logging threshold. However, zapgrpc.Logger.V historically confused verbosity levels with zap's severity levels by mapping the verbosity int to a severity via _grpcToZapLevel[level]. This meant that verbose traces gated on V(l) were toggled purely based on the core's severity level instead of the intended verbosity level logic.

Solution

This PR adds an opt-in WithVerbosity(int) option that preserves backwards compatibility but allows callers to correctly switch to grpclog's documented verbosity semantics.

When WithVerbosity(verbosity) is provided during Logger construction:

  • The internal useVerbosity flag is toggled on.
  • Calls to V(level) will evaluate level <= verbosity rather than looking up severity levels.

If WithVerbosity is not provided, the default behaviour is preserved and V(l) maps to zapcore severity, ensuring no disruption for current users.

Testing

  • Preserved existing test behaviour in TestLoggerV that verifies the fallback functionality.
  • Added a new table-driven test TestLoggerVWithVerbosity that verifies verbosity evaluations check level <= verbosity independent of the underlying core severity level.

Copilot AI lite review requested due to automatic review settings September 25, 2026 10:27

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@CLAassistant

CLAassistant commented Sep 25, 2026 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@hardikkaurani

Copy link
Copy Markdown
Author

@sywhang Just following up on #1544! This PR takes the safer, backwards-compatible approach (as discussed in #1546) by making the correct verbosity semantics strictly opt-in via a new \WithVerbosity(int)\ option. All tests are passing and it's ready for review when you have a moment. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

zapgrpc: verbosity level check is incorrect

3 participants