feat(chproxy): Update logging and correctly handle SIGs#2945
Conversation
- Implemented log/slog - Add setupLogger() function to output to STDOUT in JSON format - Replace most log functios with appropriate logger calls - Wrap http server in a goroutine so that it will catch and handle the signals appropriately Signed-off-by: Ian Meyer (imeyer) <k@imeyer.io>
imeyer
requested review from
MichaelUnkey,
chronark,
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
📝 WalkthroughWalkthroughThe changes involve the removal of a redundant call to Changes
Sequence Diagram(s)sequenceDiagram
participant Init
participant Logger
participant Main
Note over Init: Application Startup
Init->>Logger: setupLogger()
Note right of Logger: Logger is configured before other tasks
Main->>Env: Retrieve environment variables
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (16)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
|
@chronark this came about because i was fixing a typo in the logging ( |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/chproxy/main.go (1)
216-223: Good implementation of HTTP server with signal handling.Running the HTTP server in a goroutine allows the main thread to properly handle signals (SIGTERM and SIGINT). However, consider implementing graceful shutdown for the server.
go func() { logger.Info("listening", slog.String("port", PORT)) if err := srv.ListenAndServe(); err != nil && err != http.ErrServerClosed { logger.Error("failed to start server", slog.String("error", err.Error())) os.Exit(1) } }() <-ctx.Done() +// Implement graceful shutdown with timeout +shutdownCtx, shutdownCancel := context.WithTimeout(context.Background(), 10*time.Second) +defer shutdownCancel() +if err := srv.Shutdown(shutdownCtx); err != nil { + logger.Error("server shutdown error", slog.String("error", err.Error())) +} close(buffer) <-done
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/chproxy/main.go(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: Test Packages / Test ./packages/rbac
- GitHub Check: Test Packages / Test ./packages/nextjs
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Test Packages / Test ./packages/cache
- GitHub Check: Test Packages / Test ./packages/api
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Packages / Test ./internal/resend
- GitHub Check: Test Packages / Test ./internal/keys
- GitHub Check: Test Packages / Test ./internal/hash
- GitHub Check: Test Packages / Test ./internal/encryption
- GitHub Check: Test Packages / Test ./internal/billing
- GitHub Check: Test Go API Local / test_agent_local
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Test API / API Test Local
- GitHub Check: autofix
- GitHub Check: Build / Build
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (10)
apps/chproxy/main.go (10)
8-8: Good import choice for structured logging.Using
log/slogfor structured logging is appropriate as it's the standard package for structured logging in Go since Go 1.21, providing better observability.
28-28: Good practice using a global logger instance.Using a global
loggervariable makes it accessible throughout the application, ensuring consistent logging format.
31-44: Well-structured logger setup function.The
setupLoggerfunction is well-designed, setting up JSON logging appropriately with:
- JSON output to stdout
- Disabling source code information
- Setting up the global instance
- Setting it as the default logger
- Logging startup information with structured fields
This ensures consistent structured logging throughout the application.
98-100: Good structured logging for successful persistence.The structured logging approach with relevant fields (count and query) provides clear context for successful operations.
106-109: Comprehensive error logging for persistence failures.Good use of structured logging to capture all relevant information for debugging persistence issues:
- HTTP response body
- Status code
- The query being executed
This will make troubleshooting much easier.
134-136: Well-structured batch flush error logging.Good implementation of structured logging for batch flush errors with appropriate context.
165-165: Clear log message for flush events.The log message clearly explains why flushing is occurring (max size reached).
169-169: Clear log message for ticker-based flushing.The log message clearly identifies the source of the flush event (ticker).
182-184: Security-focused logging for authorization failures.Good practice including both expected and received authorization headers in logs, helping to diagnose authentication issues.
229-229: Clear shutdown completion message.Good practice to log when shutdown is complete, providing confirmation that the application terminated gracefully.
- Missing a return after http.Error when the request body can’t be read Signed-off-by: Ian Meyer (imeyer) <k@imeyer.io>
Signed-off-by: Ian Meyer (imeyer) <k@imeyer.io>
imeyer
force-pushed
the
chproxy-update-logging-and-handle-signals
branch
from
1043c78 to
8475365
Compare
|
the autofix.ci thing is a bit.. odd.. makes me have to force push and that's generally a bad thing in a repo others are collaborating on 🤷 |
chronark
changed the title
2 participants
Type of change
How should this be tested?
Build
chproxywithgo build -o chproxythen start the process.. sendSIGTERMorSIGINT... you should seeSummary by CodeRabbit