Fix/logs and metrics#357
Conversation
Reviewer's Guide by SourceryThis pull request focuses on improving logging and metrics by replacing zerolog with slog, adding configurable logging options, and integrating new middleware. It also updates banner printing functions, adds build configurations for gateway-kube-proxy, and refactors type definitions for better type safety and readability. Additionally, it introduces a health check endpoint and a debug flag for logging configuration. File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @nxtcoder17 - I've reviewed your changes - here's some feedback:
Overall Comments:
- The improvements to the logging system, especially the switch to slog and the enhanced HTTP logging, are commendable. This should improve observability and debugging capabilities.
- Please provide an explanation for the removal of the pod-logs-proxy component. It's a significant change that should be documented in the PR description.
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| route = fmt.Sprintf("%s?%s", route, r.URL.RawQuery) | ||
| } | ||
|
|
||
| h.Logger.Info(fmt.Sprintf("❯❯ %s %s", r.Method, route)) |
There was a problem hiding this comment.
suggestion (performance): Consider using structured logging instead of fmt.Sprintf for better performance
Using structured logging with slog would be more efficient and provide better log output for parsing. For example: h.Logger.Info("incoming request", "method", r.Method, "route", route)
| h.Logger.Info(fmt.Sprintf("❯❯ %s %s", r.Method, route)) | |
| h.Logger.Info("incoming request", "method", r.Method, "route", route) |
| SilentPaths: []string{}, | ||
| }) | ||
| r.Use(httpLogger.Use) | ||
| _ = middleware.Logger |
There was a problem hiding this comment.
suggestion: Remove or explain the commented-out middleware.Logger line
If the middleware.Logger is no longer needed, consider removing this line entirely. If it's kept for future reference, add a comment explaining why it's commented out and when it might be used.
| _ = middleware.Logger | |
| // _ = middleware.Logger | |
| // The middleware.Logger line is commented out for future reference. | |
| // It might be used for logging HTTP requests in the future if needed. |
| save-always: true | ||
| restore-keys: go-${{ runner.os }}-gateway-kube-proxy- | ||
|
|
||
| - name: gateway logs proxy |
There was a problem hiding this comment.
suggestion: Use more descriptive names for GitHub Actions steps
Consider using more descriptive names for the steps, especially for the new gateway kube proxy steps. For example, 'Build and push gateway kube proxy image' would be clearer than 'gateway logs proxy'.
| - name: gateway logs proxy | |
| - name: Build and push gateway kube proxy image |
nxtcoder17
force-pushed
the
fix/logs-and-metrics
branch
from
a7be00b to
c157e0a
Compare
1 participant
closes kloudlite/kloudlite#260
Summary by Sourcery
This pull request introduces a new HTTP logger with enhanced customization options, adds a health check endpoint to the observability API, and refactors several components for improved logging and configuration. It also includes updates to the build and CI workflows to support the new 'gateway-kube-proxy' target, and removes the deprecated 'pod-logs-proxy' command.