Skip to content

Replace internal logger with uber-go/zap #1362

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rvarun11
Copy link
Contributor

@rvarun11 rvarun11 commented Aug 12, 2025

Closes #383

Eliminates singleton logger pattern in favour of per-component zap instances. Replaces slog-based logging interface with direct zap calls throughout the codebase.

  • Remove global logger singleton and wrapper functions
  • Replace slog with zap SugaredLogger
  • Pass logger instances to all components and functions
  • Maintain structured/unstructured log switching via UNSTRUCTURED_LOGS
  • Preserve debug flag integration for log level control
  • Use go-logr/zapr for NewLogr() Kubernetes logr.Logger compatibility

Design Approach

This PR follows the design proposal outlined in Issue #383.

Seeking feedback on design decisions: Please review the approach of passing logger instances through function parameters and constructor injection patterns used throughout the codebase.

The following files have been temporarily initialized with a package-level logger to simplify the initial design. Based on the feedback, these will updated in the final draft as well:

  1. cmd/thv/
    • main.go
    • app/common.go
  2. cmd/thv-proxy-runner/app/commands.go
  3. cmd/regup/
    • main.go
    • app/root.go

Additionally, pkg/audit is still using slog internally but can be updated if required, once we confirm the rest of the design is good to go.

Next Steps:

  • Gather feedback on the per-component logger injection approach and discuss any additional edge cases that may have been missed.
  • Finalize zap logger configuration and tests. Refer to TODOs in logger.go and logger_test.go
  • Remove remaining package-level loggers in cmd/ dirs, if required
  • Migrate pkg/audit from slog to zap, if required

@rvarun11 rvarun11 marked this pull request as ready for review August 12, 2025 00:49
@rvarun11 rvarun11 force-pushed the main branch 3 times, most recently from a7681d9 to 61c7518 Compare August 12, 2025 18:23
Eliminates singleton logger pattern in favor of per-component zap
instances. Replaces slog-based logging interface with direct zap
calls throughout the codebase.

* Remove global logger singleton and wrapper functions
* Replace slog with uber-go/zap SugaredLogger
* Pass logger instances to all components and functions
* Maintain structured/unstructured log switching via UNSTRUCTURED_LOGS
* Preserve debug flag integration for log level control
* Use go-logr/zapr for NewLogr() Kubernetes logr.Logger compatibility

Signed-off-by: Varun Rajput <[email protected]>
Copy link
Member

@dmjb dmjb left a comment

Choose a reason for hiding this comment

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

While we wanted to get rid of the singleton logger instance which we had, I do not like having to pass the logger everywhere - this pollutes all our interfaces with logging. I would rather take an approach where we rely on zap's global logger (which is more resilient than our singleton instance).

See for example: https://betterstack.com/community/guides/logging/go/zap/#getting-started-with-zap (the "Setting up a global logger" section)

Given that this PR touches almost all of the codebase, I also wonder if there's a way that this change could be broken to minimize the impact of merge conflicts. Perhaps, for example, the existing interface could be switched over to use zap, and then replace use of the internal interface piece by piece.

@rvarun11
Copy link
Contributor Author

rvarun11 commented Aug 14, 2025

Thanks @dmjb for the review! I agree that zap's global logger is solid - we use it in our project over component based loggers too. I went with this approach because the second requirement in #383 asks to "instantiate a logger per component or per function" and that's why I had this in my draft proposal as well.

That being said, I'm flexible on this. I'm happy to redraft the proposal for this. We can do this in two parts:

  1. Replace the singleton slog logger instance with the zap's global logger: This is what I had initially when I was testing the repository. For this, I don't think we even need to use the existing interface. As we'll be using SugaredLogger, direct calls to zap's existing interface should work. Will inspect it again to make sure, I haven't missed anything.
  2. Once we have zap's global logger, we can decide if you want still want the logger per component or per function based approach or not. Then we can pick up this.

Let me know what you think.

@rvarun11 rvarun11 marked this pull request as draft August 14, 2025 12:19
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.

Replace internal logger with zap library
2 participants