Skip to content

Conversation

@arepala-uml
Copy link
Contributor

@arepala-uml arepala-uml commented Dec 7, 2025

  1. Modified logging part to use go log/slog JSON handler with RFC3339 timestamps, level parsing, and derived accesslogger. It replaces zap entirely.
  2. Removed gin-contrib/zap and converted all log statements to structured logger.Info/Error calls.
  3. Added slog logger everywhere and emit structured fields instead of Infof/Fatalf.
  4. Added new gin middleware where it will implement structured request logging and panic recovery using slog.

Closes #325


Summary by cubic

Switched tx-submit-api logging to Go’s log/slog with JSON output and RFC3339 timestamps, replacing zap and gin-contrib/zap. Added structured Gin middleware for access logging and panic recovery. Addresses Linear #325.

  • Refactors

    • Replaced zap with slog; converted Infof/Errorf/Fatalf to Info/Error with fields.
    • Added Gin slog middleware for request logging and recovery; supports skipPaths (e.g., /healthcheck).
    • JSON logs keep "timestamp" in RFC3339; added derived access logger (type=access).
    • Log level parsed from config (debug, info, warn, error).
  • Dependencies

    • Removed go.uber.org/zap and github.com/gin-contrib/zap from go.mod/go.sum.

Written for commit e356ef4. Summary will update automatically on new commits.

Summary by CodeRabbit

  • Refactor
    • Upgraded logging system to use structured, standardized log format for improved consistency and debuggability.
    • Removed external logging dependencies and replaced with built-in logging framework.
    • Enhanced HTTP request and error logging with consistent, machine-readable structured fields.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Dec 7, 2025

📝 Walkthrough

Walkthrough

This pull request implements a comprehensive migration from Uber's zap logging library to Go's standard library slog package. Changes include: replacing all formatted logging calls (Infof, Fatalf) with structured slog methods (Info, Error) across cmd/tx-submit-api/main.go and internal/api/api.go; removing the gin-contrib/zap and go.uber.org/zap dependencies from go.mod; refactoring the core logging backend in internal/logging/logging.go from zap-based SugaredLogger to slog.Logger with a JSON handler; adding two new Gin middleware functions (GinLogger and GinRecovery) in a new internal/logging/gin.go file for HTTP request and panic recovery logging; and adjusting error handling to use explicit os.Exit(1) instead of Fatalf in several places.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

  • Public API changes — GetLogger() signature changed (returning \*slog.Logger instead of \*zap.SugaredLogger); GetDesugaredLogger() replaced with GetAccessLogger(). Verify all call sites have been updated and no legacy zap code remains.
  • Logging backend replacement — Complete swap from zap to slog in internal/logging/logging.go. Review JSON handler configuration, timestamp key mapping, and level parsing logic to ensure output format meets operational requirements.
  • New middleware integration — Verify GinLogger and GinRecovery implementations handle edge cases (e.g., skipPaths filtering, panic recovery, stack trace inclusion) correctly.
  • Error handling behavior changes — Explicit os.Exit(1) calls replace Fatalf; confirm exit codes and signal handling remain appropriate.
  • Dependency cleanup — Ensure removal of zap dependencies does not leave orphaned transitive dependencies or cause issues with other packages that may reference zap types.

Pre-merge checks and finishing touches

✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: switching the logging framework from zap to Go's log/slog throughout the codebase.
Linked Issues check ✅ Passed The PR successfully implements the requirement to switch logging from zap to log/slog [#325], including structured logging, JSON handler configuration, and removal of zap dependencies.
Out of Scope Changes check ✅ Passed All changes are directly related to the logging migration: replacing zap with slog, removing zap/gin-contrib/zap dependencies, converting log statements, and adding slog middleware.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch switch_logging

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@arepala-uml arepala-uml marked this pull request as ready for review December 7, 2025 20:21
@arepala-uml arepala-uml requested a review from a team as a code owner December 7, 2025 20:21
Copy link

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

No issues found across 6 files

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
internal/api/api.go (1)

75-75: Remove redundant recovery middleware.

Line 75 uses gin.Recovery() but line 94 adds logging.GinRecovery(accessLogger, true). Having both recovery middlewares is unnecessary and the custom one should fully replace the default.

Apply this diff:

 	// Configure API router
 	router := gin.New()
-	// Catch panics and return a 500
-	router.Use(gin.Recovery())
 	// Configure CORS
🧹 Nitpick comments (1)
cmd/tx-submit-api/main.go (1)

86-90: Reconsider os.Exit(1) inside goroutine.

Calling os.Exit(1) from within a goroutine (line 89) terminates the entire process immediately, bypassing deferred cleanup and preventing graceful shutdown. Consider either:

  1. Returning the error to a channel and handling it in main, or
  2. Simply logging the error without exiting if the debug listener is optional

Since the debug listener appears to be optional (only started when configured), crashing the entire application on its failure may be too aggressive.

Example alternative approach:

 		go func() {
 			debugger := &http.Server{
 				Addr: fmt.Sprintf(
 					"%s:%d",
 					cfg.Debug.ListenAddress,
 					cfg.Debug.ListenPort,
 				),
 				ReadHeaderTimeout: 60 * time.Second,
 			}
 			err := debugger.ListenAndServe()
 			if err != nil {
 				logger.Error("failed to start debug listener", "err", err)
-				os.Exit(1)
 			}
 		}()
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 306d1e3 and e356ef4.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (5)
  • cmd/tx-submit-api/main.go (3 hunks)
  • go.mod (0 hunks)
  • internal/api/api.go (10 hunks)
  • internal/logging/gin.go (1 hunks)
  • internal/logging/logging.go (1 hunks)
💤 Files with no reviewable changes (1)
  • go.mod
🧰 Additional context used
🧬 Code graph analysis (3)
internal/api/api.go (1)
internal/logging/gin.go (2)
  • GinLogger (13-47)
  • GinRecovery (50-64)
internal/logging/logging.go (1)
internal/config/config.go (1)
  • LoggingConfig (36-39)
cmd/tx-submit-api/main.go (3)
internal/logging/logging.go (1)
  • GetLogger (55-57)
internal/version/version.go (1)
  • GetVersionString (27-33)
internal/api/api.go (1)
  • Start (52-179)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: cubic · AI code reviewer
🔇 Additional comments (9)
internal/api/api.go (2)

56-66: LGTM! Structured logging properly applied.

The startup messages now use structured slog with appropriate key-value pairs for address and port. The distinction between TLS and non-TLS modes is clear.


210-210: LGTM! Error logging properly converted to structured format.

All error logs now use structured slog with the "err" field for error objects, providing better log parsing and analysis capabilities.

Also applies to: 219-219, 233-233, 239-239, 245-245, 256-256, 294-294, 302-302, 338-338

cmd/tx-submit-api/main.go (2)

36-38: LGTM! Wrapper function appropriate for maxprocs.Logger interface.

The logPrintf function wraps slog to satisfy the maxprocs.Logger interface, which requires a Printf-style function. While this loses structure, it's necessary for library integration.


60-60: LGTM! Version logging properly structured.

The startup message now includes the version as a structured field, improving log parsing.

internal/logging/gin.go (2)

12-47: LGTM! GinLogger middleware correctly implemented.

The middleware properly:

  • Processes the request before logging (c.Next() first)
  • Skips configured paths efficiently
  • Captures comprehensive request metadata (status, method, path, latency, etc.)
  • Uses structured logging with key-value pairs
  • Includes Gin errors when present

49-64: LGTM! GinRecovery middleware correctly implemented.

The panic recovery middleware properly:

  • Uses defer-recover pattern for panic handling
  • Logs with structured fields including the error
  • Optionally includes stack trace for debugging
  • Aborts the request with HTTP 500 status
  • Allows request processing to continue (c.Next())
internal/logging/logging.go (3)

33-53: LGTM! slog configuration properly implemented.

The Setup function correctly:

  • Parses log level from config
  • Configures JSON handler with custom options
  • Replaces TimeKey with "timestamp" formatted as RFC3339
  • Creates both standard and access loggers
  • Uses log.Fatalf appropriately during bootstrap (before logger is available)

63-79: LGTM! Log level parsing correctly implemented.

The parseLevel function properly:

  • Defaults to Info for empty string
  • Performs case-insensitive matching
  • Supports both "warn" and "warning" aliases
  • Handles standard levels (debug, info, warn, error)
  • Returns error for invalid input

55-61: LGTM! Logger getter functions correctly implemented.

Both GetLogger and GetAccessLogger return the appropriate slog.Logger instances, maintaining the public API with updated types.

@arepala-uml
Copy link
Contributor Author

Tested the log/slog changes and please let me know if you want me to test anything here.

Screenshot 2025-12-07 at 2 26 29 PM

@arepala-uml arepala-uml merged commit c984a46 into main Dec 8, 2025
12 checks passed
@arepala-uml arepala-uml deleted the switch_logging branch December 8, 2025 05:56
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.

Switch logging to log/slog

3 participants