flagd
flagd copied to clipboard
refactor: WIP: #1324 Slog refactor
This PR
The intent of this pr is to action #1324 by moving the existing concrete logger type, which is implementation-dependent upon uber-go/zaplog, into an interface type which may be implemented by either go-native slog, or uber-go/zap (or any other logging subysystem the project may become enamored with in the future)
Included is the Interface, along with implementations for both the pre-existing zap-based logger, as well as an interface implementation for slog. The default constructor is set to use the slog subsystem. We have included a full suite of table-based tests for the slog implementation, to prove correctness and demonstrate the logger's use.
Related Issues
#1324
Notes
This is a work-in-progress PR intended to begin the conversation about how the eventual logging refactor should take place. (Adam and I plan to be available at the community meeting to discuss)
Follow-up Tasks
TODOs: There are several places in the current flagd codebase where uber-go/zap is explicitly used by packages in a way that make the project dependent on zap, even outside the current concrete "logger" package, which was intended to abstract these away. These still need to be cleaned up, but that clean-up will depend on the direction the project decides to go for the final implementation of this interface. Examples:
-
Every method invocation to a logger that uses
WithFieldscurrently passes in an upstream type called azap.Field. We can clean this up in a number of ways. The current interface replacesWithFieldwith a method calledWithwhich takes variadicanyand can be extended to support zap.Field if we want to keep it. -
Various Tests in the telemetry builder and kubernetes packages create
zapcore.Observertypes which act like write buffers, enabling the tests to log into the buffer and then examine the buffer's content. The newly added slogger implementation simply takes an io.Writer instead, and can provide the same functionality, but we felt going through and fixing all of those tests was a tad beyond the scope of this WIP. When we're satisfied that this is the correct approach, we'll clean up those tests accordingly (90% of the test coverage for simple cases is already fixed in this PR).
How to test
We've included unit tests for core/pkg/logger. These compile and run. The project also compiles and runs with the new default Logger interface with an slog implementation backing it.
Deploy Preview for polite-licorice-3db33c canceled.
| Name | Link |
|---|---|
| Latest commit | ea7bc6437e1ecfa3ca49ec2c65ac1b7660ed4626 |
| Latest deploy log | https://app.netlify.com/projects/polite-licorice-3db33c/deploys/68cddb6c08f79500083e80b1 |
Summary of Changes
Hello @djosephsen, I'm Gemini Code Assist[^1]! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request initiates a significant refactoring of the project's logging infrastructure by introducing a logger.Logger interface. This change aims to decouple the application from specific logging implementations, allowing for greater flexibility and future extensibility. It includes new implementations for both Go's native log/slog (now the default) and a compatibility layer for the existing uber-go/zap logger. While many components have been updated to use the new interface, the PR is a work-in-progress, with identified follow-up tasks to fully address the handling of zap.Field types when using the slog backend and to refactor existing tests that rely on zapcore.Observer.
Highlights
- Logger Interface Introduced: A new
logger.Loggerinterface has been defined to abstract logging implementations, moving away from a concreteuber-go/zaptype. - slog Integration: A new
sloggerimplementation based on Go's nativelog/slogpackage has been added, and is now the default logger for new instantiations. - zap Compatibility Layer: The existing
uber-go/zaplogger has been adapted into azapLoggertype that conforms to the newlogger.Loggerinterface. - Unified Field Handling: The
WithFieldsmethod has been generalized toWith, now accepting variadicanyarguments. This change aims for a more generic approach, though explicitzap.Fieldtypes passed toslogimplementations are noted as a follow-up for proper handling. - Widespread Adoption: The new
logger.Loggerinterface and itsNewandWithmethods have been integrated across various core and service packages, updating field types and constructor calls. - New Test Suite: Comprehensive unit tests have been introduced for the
slogimplementation to validate its functionality and ensure correctness.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.
| Feature | Command | Description |
|---|
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with :thumbsup: and :thumbsdown: on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
[^1]: Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.