feat: Implement unified logging system with Zap and advanced features #507
Conversation
- Add new logger package with Zap-based structured logging - Implement global and custom logger configurations - Support console and file logging with rotation - Add development and production logging modes - Introduce context-based logging with fields - Create comprehensive testing infrastructure for logger - Support JSON and human-readable log formats - Add color-coded log levels with optional disabling - Provide global logging functions and named loggers - Enhance cross-platform logging capabilities This refactoring provides a more robust, performant, and flexible logging system for the application.
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (2)
You can disable this status message by setting the WalkthroughThis pull request introduces extensive logging improvements across multiple packages by replacing standard logging with a structured, Zap‐based logger. Several new helper methods and updated function signatures now accept logger parameters, enhancing context and error reporting. The changes also include dependency updates in the module file, rework of API and server initialization to use custom logging, and comprehensive updates to database, disk management, and image provider modules. Additionally, new functionality for log rotation and sensitive data redaction is implemented along with expanded testing in the logger package. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SSEHandler
participant Logger
Client->>SSEHandler: Establish SSE connection
SSEHandler->>Logger: Create connection-specific logger
Logger-->>SSEHandler: Log "Connection established"
SSEHandler->>SSEHandler: Add client with connectionID & clientIP
Client->>SSEHandler: Send heartbeat/data
SSEHandler->>Logger: Log activity and check for timeouts
SSEHandler->>Logger: Log client removal on disconnect
sequenceDiagram
participant APIController
participant Config
participant Logger
participant Echo
APIController->>Logger: Initialize structured logger
APIController->>Config: Retrieve API settings
APIController->>Echo: Setup routes with provided logger
Echo-->>APIController: Return initialized routes
APIController->>Logger: Log errors and shutdown events
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (14)
internal/httpcontroller/handlers/media.go (1)
9-9: Consider removing standard log package importThe standard
logpackage is still imported but most logging has been migrated to structured logging. However, it's still used inThumbnailAttribution(line 211). Consider migrating that remaining usage to the structured logger as well for consistency.internal/logger/README.md (2)
37-47: Recommend highlighting concurrency considerations for the global logger.
Although the documentation shows how to use the global logger, consider adding a note that the global logger is safe for concurrent usage due to Zap’s design. This clarifies for newcomers that locks or additional synchronization aren't required in typical usage.
186-187: Refine punctuation for clarity.
In the bullet list referencingCreateTestCoreand other test helpers, consider adjusting punctuation to align with standard style guides and improve readability (e.g., add a full stop or consistently format as a sentence).🧰 Tools
🪛 LanguageTool
[uncategorized] ~187-~187: Loose punctuation mark.
Context: ...nctions for testing: -CreateTestCore: Creates a logger core that writes to a ...(UNLIKELY_OPENING_PUNCTUATION)
internal/logger/config.go (2)
24-34: Clarify default log level inDefaultConfig.
WhenLevelis left empty, the actual log level will be determined only at runtime by the calling code (e.g., ingetZapLevel). Consider documenting in the comment that the default effectively becomes "debug" ifDevelopmentis true, to help future maintainers quickly understand the intended default behavior.
70-81: Log invalid log-level inputs.
Currently,getZapLevelquietly falls back to the default level if parsing fails. Consider logging a warning or an informational message when the user-supplied log level is invalid, to avoid confusion when typos appear in configuration.internal/httpcontroller/server_logger.go (2)
19-25: Support more granular Echo log levels.
Right now, all Echo log messages are mapped to.Info(). For more refined logging, consider parsing or mapping Echo’s log levels to the appropriate Zap levels (e.g.,.Debug(),.Warn(),.Error()) so critical logs stand out.
103-125: Vary logging level based on status codes in Echo request logs.
Currently, every request log is recorded at the Info level, even if the status code indicates a client or server error (e.g., 400 or 500). Consider elevating the logging level for 4xx/5xx to.Warn()or.Error()for better visibility into issues.internal/httpcontroller/server.go (2)
93-98: Default server settings look good.
Providing a default web server port of "8080" is a sensible fallback. Consider logging a message when defaults take effect, so users are aware they're running on port 8080.func configureDefaultSettings(settings *conf.Settings) { if settings.WebServer.Port == "" { + log.Printf("No port specified; defaulting to 8080") settings.WebServer.Port = "8080" } }
213-221: Logging server errors with structured fields is beneficial.
This method properly leveragess.Logger, falling back tolog.Printf. Ensure the channel is properly closed or consumed to prevent goroutine leaks.internal/logger/logger_test.go (1)
54-71: Consider naming returned results in captureOutput for clarity.The static analysis tools flag unnamed return results in this function. Naming them can improve readability and help others quickly see what’s being returned.
Possible improvement:
-func captureOutput(t *testing.T) (*bytes.Buffer, func()) { +func captureOutput(t *testing.T) (buf *bytes.Buffer, restore func()) { var localBuf bytes.Buffer ... return &localBuf, func() { ... } }🧰 Tools
🪛 GitHub Check: golangci / lint
[failure] 54-54:
unnamedResult: consider giving a name to these results (gocritic)🪛 GitHub Check: lint
[failure] 54-54:
unnamedResult: consider giving a name to these results (gocritic)internal/logger/logger.go (2)
102-107: Consider removing or limiting special characters/emojis in production logs.While these logs are helpful during development, emojis might not be desirable in some production environments. You could wrap them conditional on dev mode or remove them entirely.
122-124: Combine append calls for cleaner code.The linter flagged these chained append calls. Consider merging them for brevity.
if !config.DisableCaller { - opts = append(opts, zap.AddCaller()) - opts = append(opts, zap.AddCallerSkip(1)) + opts = append(opts, zap.AddCaller(), zap.AddCallerSkip(1)) }🧰 Tools
🪛 GitHub Check: golangci / lint
[failure] 123-123:
appendCombine: can combine chain of 2 appends into one (gocritic)🪛 GitHub Check: lint
[failure] 123-123:
appendCombine: can combine chain of 2 appends into one (gocritic)🪛 GitHub Actions: golangci-lint
[warning] 123-123: appendCombine: can combine chain of 2 appends into one (gocritic)
🪛 GitHub Actions: golangci-test
[warning] 123-123: appendCombine: can combine chain of 2 appends into one (gocritic)
🪛 GitHub Actions: Build docker image
[warning] 123-123: appendCombine: can combine chain of 2 appends into one (gocritic)
internal/httpcontroller/handlers/handlers.go (2)
82-90: Enforce consistent debug-level logging patterns.The fallback to
baseHandler.logInfo("DEBUG: ...")might be confusing in certain log aggregators, as it merges debug logs into the info stream. Consider a minimal approach or disabling debug logs entirely if the custom logger is unavailable.
109-128: Encourage distinct error handling paths.Merging both “error logging with fields” and “fallback standard logger” in one block is acceptable, but can be more readable when separated into smaller blocks or helper methods, especially if error details grow. This also helps avoid large if-else trees.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
go.mod(2 hunks)internal/httpcontroller/handlers/audio_level_sse.go(3 hunks)internal/httpcontroller/handlers/handlers.go(6 hunks)internal/httpcontroller/handlers/media.go(13 hunks)internal/httpcontroller/server.go(4 hunks)internal/httpcontroller/server_logger.go(1 hunks)internal/logger/README.md(1 hunks)internal/logger/config.go(1 hunks)internal/logger/exported.go(0 hunks)internal/logger/filehandler.go(0 hunks)internal/logger/global.go(1 hunks)internal/logger/logger.go(1 hunks)internal/logger/logger_test.go(1 hunks)internal/logger/rotation.go(1 hunks)internal/logger/testing.go(1 hunks)
💤 Files with no reviewable changes (2)
- internal/logger/exported.go
- internal/logger/filehandler.go
🧰 Additional context used
🪛 LanguageTool
internal/logger/README.md
[uncategorized] ~187-~187: Loose punctuation mark.
Context: ...nctions for testing: - CreateTestCore: Creates a logger core that writes to a ...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 GitHub Check: golangci / lint
internal/logger/logger.go
[failure] 123-123:
appendCombine: can combine chain of 2 appends into one (gocritic)
internal/logger/logger_test.go
[failure] 54-54:
unnamedResult: consider giving a name to these results (gocritic)
🪛 GitHub Check: lint
internal/logger/logger.go
[failure] 123-123:
appendCombine: can combine chain of 2 appends into one (gocritic)
internal/logger/logger_test.go
[failure] 54-54:
unnamedResult: consider giving a name to these results (gocritic)
🪛 GitHub Actions: golangci-lint
internal/logger/logger.go
[warning] 123-123: appendCombine: can combine chain of 2 appends into one (gocritic)
🪛 GitHub Actions: golangci-test
internal/logger/logger.go
[warning] 123-123: appendCombine: can combine chain of 2 appends into one (gocritic)
🪛 GitHub Actions: Build docker image
internal/logger/logger.go
[warning] 123-123: appendCombine: can combine chain of 2 appends into one (gocritic)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (28)
internal/httpcontroller/handlers/audio_level_sse.go (2)
151-151: Great use of structured logging!The transition from standard logging to structured logging with key-value pairs provides better context and readability. Including the
client_ipfield helps with debugging and tracking connection issues.
198-205: Smart implementation of rate-limited loggingLimiting debug logs for audio data to once every 5 seconds is an excellent optimization to prevent log spam while still providing visibility into the system's operation.
go.mod (3)
32-32: Appropriate logging library choiceAdding Zap as a dependency is a good choice for structured logging. It's high-performance and widely adopted in the Go ecosystem.
37-37: Good addition for log rotationIncluding lumberjack for log rotation complements the new logging system and aligns with the PR objectives for a comprehensive logging solution.
97-97: Dependency version updateUpdating to a newer version of
multierraligns with using the latest compatible versions of the Zap ecosystem.internal/httpcontroller/handlers/media.go (2)
37-39: Good practice for configurable debug verbosityAdding a toggle for media debug logging is a smart implementation that allows for fine-grained control over log verbosity, especially useful since media operations can generate verbose logs.
41-47: Well-implemented conditional logging methodThe
mediaDebugmethod properly checks both that debug logging is enabled and that a logger is available before attempting to log, which prevents potential nil pointer dereferences.internal/logger/testing.go (2)
11-45: Well-designed test utility for capturing log outputThe
CreateTestCorefunction is a valuable testing utility that allows for intercepting logger output during tests. The implementation correctly handles configuration options like log levels, JSON formatting, and color output.
47-69: Good implementation of custom logger creation for testsThe
NewLoggerWithCorefunction provides flexibility for testing by allowing injection of custom core behavior. The proper setup of options like caller information and development mode is handled well.internal/httpcontroller/server.go (4)
8-8: No issues with the new import usage.
The import of "os" is straightforward and aligns with environment-related functionality introduced below.
84-88: Conditional logger injection is appropriate.
This check ensures that the logger is only set after it is properly initialized. However, ensure that any downstream usage ofs.Handlers.Loggergracefully handles a nil logger if initialization fails.
129-129: Ensure errChan is closed on shutdown.
Spawning a goroutine to consume errors is fine, but verify thaterrChanis closed on graceful shutdown to avoid hangs. Otherwise,for err := range errChaninhandleServerErrorcould block indefinitely if the channel is never closed.
201-205: Environment-driven dev mode check is valid.
Your fallback toSettings.WebServer.Debugis a clear and concise approach. If you ever plan to allow explicit "false" environment variable values (e.g."0","false", etc.), you already handle them by returning the config setting.Also applies to: 208-209
internal/logger/rotation.go (4)
1-13: Imports and package definition look standard.
All required dependencies (zap,zapcore,lumberjack) are correctly included. There are no security or concurrency issues in these import statements.
15-38: CreateRotatingFileCore is clear and handles setup robustly.
The function correctly ensures the directory exists and configures rotation. Consider documenting the behavior whenMkdirAllfails to avoid confusion in calling code.
40-91: NewRotatingLogger: good logic for rotating logs.
Your defaulting to non-color output for file-based logs is wise. The calls tozap.AddCaller()andzap.AddCallerSkip(1)are typical for removing the logger frame from logs, but ensure logs reference the correct call site.
93-105: NewRotatingCoreLogger provides a convenient wrapper.
Wrapping the low-level Zap logger and sugar logger inside your ownLoggerstruct is a neat encapsulation. Overall, this approach is consistent with the rest of the logging framework.internal/logger/global.go (4)
1-15: Global logger variables are well-structured.
Usingsync.Onceto guard initialization helps avoid concurrency issues. TheerrDefaultLoggeris also well-handled.
17-24: InitGlobal ensures a one-time setup.
If multiple attempts to initialize occur, only the first call matters. This is a sensible approach for a global logger to prevent re-initialization races.
26-39: GetGlobal default fallback is practical.
If the logger is never explicitly initialized, creating a default that writes to stdout with a development-friendly config is a good fail-safe. The immediate exit on initialization failure is also a valid choice, as the app may not be viable without logging.
41-44: Global logging helpers are convenient.
The direct calls (Debug,Info, etc.) withGetGlobal()reduce boilerplate for clients. The approach is consistent with other global logging patterns, though keep in mind that some prefer explicit logger injection to avoid global state.Also applies to: 46-49, 51-54, 56-59, 61-64, 66-69, 71-74, 76-79
internal/logger/logger_test.go (2)
73-169: Validate edge cases for log level transitions.While the test coverage for different log levels is comprehensive, consider additional edge cases, such as:
- Setting an invalid log level in the Config to confirm it defaults to a known level.
- Testing corner cases like “fatal” level for potential abrupt program termination.
379-443: Great test coverage for context fields.The code effectively verifies that all context fields are preserved and logged as expected. This thorough approach prevents context leakage issues.
internal/logger/logger.go (2)
2-10: Excellent package-level documentation.The top-level doc comment clearly conveys the purpose, usage, and features of the logging system. This aids maintainability and onboarding.
264-267: Check for potential Sync errors during logger shutdown.Currently, the return value of
l.zap.Sync()is returned but not always acted upon. In some production scenarios, capturing and handling sync errors might be useful to ensure logs are fully flushed or to report I/O issues.internal/httpcontroller/handlers/handlers.go (3)
40-40: Praise for introducing a dedicated field for the custom logger.Adding
Logger *logger.Loggerensures consistent and structured logging across all handlers while preserving backward compatibility.
163-167: Convenient setter for custom logger.Exposing
SetLoggerallows dynamic logger injection, which is valuable for testing or reconfiguration. Ensure this method is called before critical logging statements.
205-215: High-quality contextual logging of HTTP errors.Including stacktrace, code, and message fields is ideal for quick troubleshooting. The approach ensures that essential debugging info is captured in the logs.
…lity - Simplify logger configuration by combining Zap caller options - Remove `captureOutput` utility function from logger test file - Improve code conciseness and reduce unnecessary complexity
…ntext - Add connection-specific logging for SSE and audio level streams - Implement detailed logging with unique connection IDs and client IPs - Improve log granularity for audio source activity and level changes - Introduce structured logging methods for SSE handler - Add connection metrics and performance tracking - Refactor logging to support more informative debug and error messages This update significantly improves observability and debugging capabilities for server-sent events and audio level monitoring.
…ture - Break down AudioLevelSSE method into smaller, focused methods - Introduce ConnectionState struct to manage connection-specific data - Extract connection handling, logging, and event processing into separate methods - Improve code readability and maintainability - Simplify error handling and logging across SSE connection lifecycle - Update error logging in detections handler to match new logging pattern This refactoring enhances the code organization and makes the SSE handler more modular and easier to understand.
…e approach - Add request-specific logging with unique request IDs and client IPs - Implement component-specific middleware loggers - Improve logging granularity for CSRF, cache control, and authentication middleware - Add conditional logging based on development mode - Integrate with existing logger infrastructure - Enhance error and debug message context This refactoring improves middleware observability and provides more detailed logging for request processing.
… path handling - Refactor media-related handlers to use structured, component-specific logging - Improve clip name sanitization with granular, modular path validation methods - Add detailed debug logging for spectrogram and audio clip serving - Implement OS-agnostic path handling with improved error context - Break down complex path sanitization logic into smaller, focused methods - Add comprehensive logging for file existence checks and path resolution This refactoring improves code readability, error handling, and observability in media-related operations.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (11)
internal/httpcontroller/handlers/detections.go (1)
192-192: Consider using structured fields instead of embedding the clip name in the message.
Logging frameworks like Zap excel at capturing contextual data. Rather than interpolatingnote.ClipNameinto the message string, you could pass it as a separate field for more structured logs and better filtering.internal/httpcontroller/handlers/media.go (1)
38-41: Remove or unify the unuseddebugLoggingvariable.
This global variable appears to be overshadowed byh.debugchecks in various functions. If the intent is to toggle debugging at runtime, ensure this is referenced consistently, or remove it if you prefer the handler’sh.debuglogic.internal/httpcontroller/middleware.go (1)
45-50: Avoid truncating the request ID to prevent potential collisions at scale.
Using only part of the UUID ([:8]) may cause collisions with large volumes of requests. Consider using the full or at least a longer substring of the UUID to ensure globally unique correlation IDs.internal/logger/logger.go (2)
80-97: Consistent logging methods.Providing separate functions (
Info,Warn,Error,Fatal) aligns well with structured logging patterns. Just be cautious with usingFatalin libraries, as it callsos.Exit(1)and might disrupt hosting applications if triggered unexpectedly.
99-243: Well-structured logger initialization.
NewLoggercovers multiple configuration pathways (console, file, tee logger) and includes development options and rotation logic. Good job ensuring directories exist before file logging. Consider clarifying what happens if a rotation config is provided for development mode—some teams might expect rotation in dev mode as well.internal/httpcontroller/handlers/sse.go (3)
67-183: Robust SSE event loop.Switch statements handle potential logging with different logger references or debug fallback, preventing null-pointer issues. The heartbeat approach ensures clients remain connected. Consider implementing logic to periodically remove stale or blocked channels if necessary for resource cleanup.
184-242: Broadcast notifications with flexible logger naming.
SendNotificationcategorizes logs by notification type, enabling more granular filtering. The logic for skipping blocked channels is commented as an option—this is a good place to add metrics or counters for repeated blocking.
296-306: Deprecated debug function still present.The plain
Debugfunction is labeled deprecated but still available. Schedule a plan to remove it or convert existing calls to the new structured logging methods for consistency.internal/httpcontroller/handlers/audio_level_sse.go (3)
57-63: Consider using the built-in math.Abs function.
Your customabsfunction replicatesmath.Abs; switching to the Go standard library might reduce code duplication.-import "strings" +import ( + "strings" + "math" + ... +) -func abs(x float64) float64 { - if x < 0 { - return -x - } - return x -} +// Use math.Abs in place of abs(...)
133-214: Refactor repeated name-setting logic to reduce duplication.
The code to assignaudioData.Nameis somewhat duplicated from the initialization logic. Extracting it into a helper could simplify maintenance.
255-283: Potential UUID collision nitpick.
Truncating the full UUID to 8 characters is unlikely to collide, but still possible. Consider using a longer substring or the entire UUID if collisions would be problematic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
internal/httpcontroller/handlers/audio_level_sse.go(4 hunks)internal/httpcontroller/handlers/detections.go(1 hunks)internal/httpcontroller/handlers/handlers.go(7 hunks)internal/httpcontroller/handlers/media.go(9 hunks)internal/httpcontroller/handlers/sse.go(4 hunks)internal/httpcontroller/middleware.go(7 hunks)internal/httpcontroller/server.go(5 hunks)internal/httpcontroller/server_logger.go(1 hunks)internal/httpcontroller/template_renderers.go(7 hunks)internal/logger/logger.go(1 hunks)internal/logger/logger_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/httpcontroller/server_logger.go
- internal/httpcontroller/server.go
- internal/httpcontroller/handlers/handlers.go
🧰 Additional context used
🪛 GitHub Check: golangci / lint
internal/httpcontroller/template_renderers.go
[failure] 143-143:
dynamicFmtString: use errors.New(errMsg) or fmt.Errorf("%s", errMsg) instead (gocritic)
[failure] 165-165:
dynamicFmtString: use errors.New(errMsg) or fmt.Errorf("%s", errMsg) instead (gocritic)
🪛 GitHub Check: lint
internal/httpcontroller/template_renderers.go
[failure] 143-143:
dynamicFmtString: use errors.New(errMsg) or fmt.Errorf("%s", errMsg) instead (gocritic)
[failure] 165-165:
dynamicFmtString: use errors.New(errMsg) or fmt.Errorf("%s", errMsg) instead (gocritic)
🪛 GitHub Actions: golangci-test
internal/httpcontroller/template_renderers.go
[error] 143-143: dynamicFmtString: use errors.New(errMsg) or fmt.Errorf("%s", errMsg) instead (gocritic)
🪛 GitHub Actions: golangci-lint
internal/httpcontroller/template_renderers.go
[error] 143-143: dynamicFmtString: use errors.New(errMsg) or fmt.Errorf("%s", errMsg) instead (gocritic)
🪛 GitHub Actions: Build docker image
internal/httpcontroller/template_renderers.go
[error] 143-143: dynamicFmtString: use errors.New(errMsg) or fmt.Errorf("%s", errMsg) instead (gocritic)
🪛 GitHub Check: CodeQL
internal/logger/logger.go
[failure] 76-76: Clear-text logging of sensitive information
Sensitive data returned by HTTP request headers flows to a logging call.
🔇 Additional comments (40)
internal/logger/logger_test.go (9)
19-47: Implementation of helper classes looks good.Your
testingWriterandtestLogCaptureprovide a straightforward way to capture logs in memory and within test logs. There's no concurrency concern here since the buffer is internal and only manipulated within the test context.
48-51: Simple and effective in-memory filesystem setup.The
setupTestFsfunction is concise and uses Afero for an in-memory filesystem, which is beneficial for fast and isolated tests.
53-149: Thorough test coverage for log levels.The table-driven approach in
TestLogLevelseffectively checks each level's behavior, ensuring logs are included or excluded as expected. The use ofNewTestLogCaptureand field checks is robust.
151-248: Comprehensive format testing.
TestLogFormattingverifies both console and JSON output formats, along with validating JSON structure. This ensures reliable log parsing and matches your intended output structure.
250-310: Dual testing approach for file output.The combination of an in-memory fs and a real filesystem test in
TestFileOutputis commendable. It adequately covers different scenarios without risking disk pollution in CI environments.
312-357: Pragmatic rotation test.
TestRotationchecks log file creation and basic rotation configuration. Skipping forced rotation in tests is a sensible compromise to avoid bloated test data.
359-423: Context logging test is well-designed.
TestLoggerWithContextthoroughly verifies that contextual fields are included in the final log output. The JSON parsing step is especially helpful in preventing regressions in structured logging.
425-466: Named logger coverage is sufficient.
TestLoggerNamingconfirms that custom logger names are accurately captured in the log output. This is crucial for distinguishing logs from different components.
468-503: Global logger test ensures shared state correctness.
TestGlobalLoggerverifies that global initialization and usage work together smoothly. Redirecting stdout is a neat trick for capturing output without polluting CI logs.internal/logger/logger.go (6)
2-72: Encoder configuration design is clear and flexible.The
createEncoderConfigandsimpleDateTimeEncoderfunctions keep your encoder consistent and easy to maintain. Using a human-readable timestamp without milliseconds is a sensible choice for many logging scenarios.
74-78: Potential risk of logging sensitive data.This code uses
l.sugar.Debugw(msg, fields...). Ensure no sensitive or PII data is passed in these fields, especially if debug logs might be enabled in production.🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 76-76: Clear-text logging of sensitive information
Sensitive data returned by HTTP request headers flows to a logging call.
245-252: Named logger usage.The
Namedmethod provides a newLoggerinstance while preserving the underlying configuration. This is especially useful for categorizing logs by subsystem or feature.
254-261: Custom fields for context.
Withmethod allows you to attach fields that persist across log calls, promoting consistent structured output.
263-266: Log flushing included.Exposing
Syncensures log entries aren't lost on abrupt application exit. Make sure to call it where it's critical (e.g., at shutdown).
268-272: Direct Zap logger access.
GetZapLoggerhelps integrate with third-party libraries needing direct zap APIs. It's good to maintain a single source of truth for logging.internal/httpcontroller/handlers/sse.go (7)
34-48: Well-defined SSEHandler initialization.
NewSSEHandlerneatly configures a named logger if provided. Logging subcomponents under"sse.handler"will help isolate SSE logs.
50-62: Connection-scoped logging approach.
getConnectionLoggerappends both a fixed “conn” name layer and connection-specific fields. This is a great way to track events for each client connection without clutter in global logs.
244-270: Synchronized client addition.
addClientis properly guarded by a mutex while preserving debug statements or structured logs. The channel buffer usage is disclosed in logs for easier debugging.
272-294: Graceful client removal and logging.
removeClientensures channels are closed safely, signals disconnection in logs, and updates in-memory structures under lock. This helps maintain SSE session integrity.
308-321: New structured LogDebug function is beneficial.
LogDebugcentralizes debug logic, bridging the structured logger and fallbacklog.Printfusage. This helps keep debug logs consistent across different environments.
323-334: Clear info-level logging.
LogInfoseamlessly integrates structured logs while providing a graceful fallback. This fosters consistent SSE info-level messages even without a configured logger instance.
336-351: Error logging with optional error object.
LogErrormerges the structured approach with standard logging. The appended"error", errfield ensures error details are never lost. Good fallback logic for the no-logger case.internal/httpcontroller/handlers/audio_level_sse.go (18)
7-7: No issues with importing "strings".
The addition of"strings"appears consistent with the RTSP-trimming logic.
11-11: UUID import is appropriate.
Using"github.com/google/uuid"is a standard choice for generating connection IDs.
13-13: Structured logging import looks good.
Importing"github.com/tphakala/birdnet-go/internal/logger"aligns with the new logging system requirements.
23-35: Connection-specific logger creation is well-designed.
Providing contextual fields such as connection ID and client IP is a good practice for traceability. Returningnilifh.Loggeris absent ensures safe handling.
36-55: Source-specific logger logic is clear.
Naming sub-loggers by source type (malgo, rtsp, etc.) helps maintain a meaningful logging hierarchy.
218-249: Activity check implementation is straightforward.
Looping through sources, marking them as inactive when needed, and logging changes is clear and well-structured.
285-295: ConnectionState struct is logically organized.
Storing times and counters here helps simplify SSE state management.
297-308: One-connection-per-IP policy might hamper legitimate multi-device usage.
Consider whether you want strictly one SSE connection per IP. If multiple devices share a NAT, they may be rejected.
310-317: Connection establishment logging is clear.
Having a dedicated method for logging new connections ensures consistency and readability.
319-327: Cleanup logic is succinct.
Removing the active connection and logging closure is straightforward.
329-342: Initialization of connection state looks good.
Properly sets up the needed maps and timestamps, avoiding uninitialized usage in later steps.
344-352: Error logging method is consistent.
Centralizing err logs to either the connection logger or the global logger is a clean approach.
353-392: Main SSE event loop is well-structured.
Handling timeouts, heartbeats, and incoming audio data in a select block suits SSE concurrency patterns.
394-404: Timeout handling is appropriate.
Stopping on a timed-out connection helps prevent ghost connections from lingering.
406-416: Client disconnect logic is straightforward.
Logs relevant metrics before exiting.
418-453: Rate-limited updates and selective logging are well done.
Sending SSE updates every 50ms avoids spamming the client, and the 5-second log interval avoids excessive log traffic.
455-468: Periodic activity checks help prevent stale data.
Marking inactive sources and pushing updates only when needed is efficient.
470-489: Heartbeat and connection stats.
Sending comment-based heartbeats keeps the SSE connection alive, and occasional stats logging is a useful touch.
…roved error handling - Add component-specific logging for detection-related handlers - Implement getDetectionLogger method for granular, context-aware logging - Refactor Debug method to support structured logging - Improve error logging in DetectionDetails, RecentDetections, and DeleteDetection methods - Add conditional logging based on debug mode and logger availability - Remove redundant Debug method from utils.go This refactoring improves logging consistency, error traceability, and observability in detection-related operations.
…roved error handling - Add component-specific logging for database operations in interfaces.go - Implement SetLogger and getDbLogger methods for granular, context-aware logging - Refactor database connection and migration methods to support custom logging - Improve error logging and retry mechanisms in SQLite and MySQL stores - Add detailed debug logging for database transactions, locks, and migrations - Enhance logging consistency across datastore implementations This refactoring improves database operation observability, error traceability, and provides more informative logging for database interactions.
…ime analysis - Replace standard log with structured logging in diskmanager package - Add logger initialization for disk-related operations - Improve error and debug logging in age and usage-based cleanup policies - Refactor file sorting and cleanup methods to use structured logging - Add more detailed context to log messages for better observability - Ensure consistent logging approach across diskmanager components This refactoring improves logging granularity and error traceability in disk management and cleanup processes.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (5)
internal/analysis/realtime.go (5)
112-114: 🛠️ Refactor suggestionInconsistent logging approach.
These lines still use the standard
log.Printfwhile the rest of the file uses the new structured logging system.- log.Printf("⚠️ Error initializing buffers: %v", err) - log.Println("⚠️ Some audio sources might not be available.") + logger.GetGlobal().Error("Error initializing buffers", "error", err) + logger.GetGlobal().Warn("Some audio sources might not be available")
153-153: 🛠️ Refactor suggestionInconsistent logging approach.
Similar to the previous issue, this line still uses the standard logging.
- log.Println("⚠️ Starting without active audio sources. You can configure audio devices or RTSP streams through the web interface.") + logger.GetGlobal().Warn("Starting without active audio sources. You can configure audio devices or RTSP streams through the web interface")
258-260: 🛠️ Refactor suggestionInconsistent logging approach.
This section still uses standard logging which should be updated to the new structured logging.
- log.Println("Received Ctrl+C, shutting down") - close(quitChan) // Close the quit channel to signal other goroutines to stop + logger.GetGlobal().Info("Received Ctrl+C, shutting down") + close(quitChan) // Close the quit channel to signal other goroutines to stop
265-269: 🛠️ Refactor suggestionInconsistent logging approach.
The database closing logs should also use the structured logging approach.
- log.Printf("Failed to close database: %v", err) + dbLogger.Error("Failed to close database", "error", err)- log.Println("Successfully closed database") + dbLogger.Info("Successfully closed database")
312-313: 🛠️ Refactor suggestionInconsistent logging approach.
These log statements should also be converted to use the structured logging system for consistency.
Consider creating and using a named logger for the image cache component to maintain consistency with the rest of the refactoring.
Also applies to: 319-320, 359-365
🧹 Nitpick comments (10)
cmd/root.go (1)
76-93: Consider validating logger configuration values.While the implementation is solid, consider adding validation for the log level and file path before initializing the logger. This could prevent runtime errors if invalid values are provided.
func initialize() error { // Initialize the global logger config := logger.Config{ Level: viper.GetString("log.level"), Development: viper.GetBool("debug"), FilePath: viper.GetString("log.path"), JSON: viper.GetBool("log.json"), DisableColor: viper.GetBool("log.disable_color"), } + // Validate log level + validLevels := map[string]bool{"debug": true, "info": true, "warn": true, "error": true} + if _, valid := validLevels[config.Level]; !valid && config.Level != "" { + return fmt.Errorf("invalid log level: %s", config.Level) + } + + // Validate log path if provided + if config.FilePath != "" { + // Check if directory exists + dir := filepath.Dir(config.FilePath) + if _, err := os.Stat(dir); os.IsNotExist(err) { + if err := os.MkdirAll(dir, 0755); err != nil { + return fmt.Errorf("failed to create log directory: %w", err) + } + } + } if err := logger.InitGlobal(config); err != nil { return fmt.Errorf("error initializing logger: %w", err) } // Log initialization success logger.Info("BirdNET-Go initialized", "version", viper.GetString("version"), "build_date", viper.GetString("build_date")) return nil }internal/diskmanager/policy_usage.go (6)
21-25: Consider centralizing logger initialization.
Repeating the “if diskLogger == nil { InitLogger() }” pattern within each function can be difficult to maintain and prone to forgetting in certain paths. Consider centralizing logger initialization in one place (e.g., during package init or a dedicated initializer) to keep the code DRY.
52-55: Ensure thorough disk usage context.
This debug statement logs usage and threshold. It might also be useful to log the difference or percentage above threshold when debugging.
85-89: Duplicate initialization logic.
Just like lines 21-25, consider extracting the logger initialization to a shared helper or ensuring it’s invoked once in a top-level function. Doing so will reduce repeated checks.
148-148: Error log on file removal failure.
Logging path and error is appropriate. Consider capturing file permissions or ownership if repeated failures are common.
172-174: Summarizes usage retention outcome.
Logging total files deleted and space freed is informative. Consider adding final disk usage for completeness.
181-185: Logger initialization repeated again.
Similar to lines 21-25 and 85-89, consider a single initialization approach to avoid code duplication.internal/datastore/sqlite.go (1)
67-67: Consistent backup filename pattern.
Switching from underscore to a dot in the extension is purely cosmetic but can help visually distinguish backups.internal/logger/logger.go (2)
69-72: Consider including milliseconds for precise timingWhile the simplified time format is readable, consider including milliseconds for debugging time-sensitive operations.
-func simpleDateTimeEncoder(t time.Time, enc zapcore.PrimitiveArrayEncoder) { - enc.AppendString(t.Format("2006-01-02T15:04:05")) +func simpleDateTimeEncoder(t time.Time, enc zapcore.PrimitiveArrayEncoder) { + enc.AppendString(t.Format("2006-01-02T15:04:05.000")) }
99-241: Consider decomposing the NewLogger functionWhile the implementation is thorough, this function is quite complex. Consider breaking it down into helper methods for each logger type to improve maintainability.
For example:
func createDevelopmentFileLogger(config Config, rc RotationConfig, level zapcore.Level, opts []zap.Option) (*zap.Logger, error) { // Implement case 1 logic here } func createProductionFileLogger(config Config, rc RotationConfig, level zapcore.Level, opts []zap.Option) (*zap.Logger, error) { // Implement case 2 logic here } func createConsoleLogger(config Config, level zapcore.Level, opts []zap.Option) (*zap.Logger, error) { // Implement case 3 logic here }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
cmd/root.go(2 hunks)internal/analysis/realtime.go(5 hunks)internal/datastore/interfaces.go(15 hunks)internal/datastore/manage.go(1 hunks)internal/datastore/mysql.go(1 hunks)internal/datastore/sqlite.go(1 hunks)internal/diskmanager/file_utils.go(6 hunks)internal/diskmanager/policy_age.go(6 hunks)internal/diskmanager/policy_usage.go(9 hunks)internal/httpcontroller/handlers/dashboard.go(7 hunks)internal/httpcontroller/handlers/detections.go(27 hunks)internal/httpcontroller/handlers/utils.go(0 hunks)internal/logger/logger.go(1 hunks)
💤 Files with no reviewable changes (1)
- internal/httpcontroller/handlers/utils.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (86)
cmd/root.go (4)
19-19: Confirm this new import is correctly used.The new logger package import is appropriate for the implementation of the structured logging system as described in the PR objectives.
76-83: Logger configuration looks well structured.The logger configuration is properly populated from viper settings, with appropriate fields for level, development mode, file path, JSON formatting, and color settings. This aligns with the PR objective of implementing a unified logging system with configuration options.
85-87: Good error handling for logger initialization.The error from logger initialization is properly wrapped with a descriptive message, making troubleshooting easier.
90-93: Excellent use of structured logging.This is a good example of structured logging with field-based contextual information (version and build date). This demonstrates the context-based logging capability mentioned in the PR objectives.
internal/diskmanager/policy_age.go (2)
15-18: Good defensive programming.Adding logger initialization check ensures logging works even if initialization was missed elsewhere.
29-29: Well-structured logging with context.The transition from standard logging to structured logging with key-value pairs greatly improves log quality and searchability. The consistent use of appropriate log levels (Error, Debug, Info) based on the context is excellent.
Also applies to: 34-36, 63-63, 69-69, 79-83, 89-89, 102-102, 111-111
internal/analysis/realtime.go (2)
72-74: Good approach to component-specific logging.Setting a named logger for the datastore component using the global logger is a good practice for organizing logs.
274-275: Good practice for component-specific logging.Creating a named logger for each functional component helps with log organization and filtering.
internal/diskmanager/file_utils.go (3)
36-42: Good design for module-specific logger.Creating a package-level logger variable and an initialization function provides a clean way to handle logging throughout the package.
46-49: Consistent defensive initialization.The consistent logger initialization checks throughout the package ensure logging works correctly regardless of initialization order.
Also applies to: 84-87, 183-186
98-98: Well-structured logging with context.The structured logs with key-value pairs provide more context than the previous simple log messages.
Also applies to: 207-207
internal/httpcontroller/handlers/dashboard.go (5)
26-27: Good component-specific logger design.Creating a dedicated method for obtaining the dashboard logger improves code organization and reusability.
Also applies to: 256-262
33-36: Comprehensive debug logging with performance metrics.The debug logging includes valuable performance metrics (duration, counts) and is properly guarded with debug flag checks. The yield point with logging during batch processing is an excellent practice for long-running operations.
Also applies to: 54-58, 75-81, 128-134, 164-168, 212-218, 222-227, 248-252
42-46: Well-structured error logging.Error logging consistently includes contextual information (date, species, etc.) which will be valuable for troubleshooting. The nil check for the logger is a good defensive practice.
Also applies to: 66-71, 86-91, 106-110, 117-121, 156-160, 193-198, 241-244
154-162: Improved error handling for template rendering.The addition of error handling and logging for template rendering is a good improvement that will help identify UI issues.
210-218: Excellent concurrency consideration.Adding a yield point during intensive processing prevents CPU blocking and improves responsiveness. The debug logging for batch progress is helpful for monitoring long-running operations.
internal/diskmanager/policy_usage.go (10)
39-41: Looks good for debug-level logging.
Logging base directory and threshold is helpful for troubleshooting.
76-78: Efficient zero-work path recognized.
This debug-level message helps confirm no cleanup is needed. This is good practice for a quick exit log.
98-98: Informational log on quit signal.
This effectively notifies operators that the cleanup is being halted. No issues found.
104-104: Good clarity on locked file skip.
This debug message succinctly explains why a file wasn’t removed.
125-128: Debug-level species count logging.
Logging species clip counts is helpful for diagnosing edge cases. Ensure it won’t cause excessive log volume in production if debug is accidentally enabled.
133-137: Meaningful skip message.
Indicates the reason a file is not being deleted. This is helpful for explaining incomplete cleanup runs.
142-142: Clear “Deleting file” debug message.
Short and straightforward. Ensures clarity when reviewing logs.
160-163: Good final log of file deletion.
This helps confirm that the operation completed, including updated clip counts.
187-187: Useful debug describing sort priority.
Confirms the sorting approach, which helps in diagnosing unexpected cleanup order.
228-228: “Files sorted” debug message.
Helpful for confirming sort completion. No issues noted.internal/datastore/mysql.go (10)
31-32: Logger retrieval in Open method.
Having a dedicated logger for MySQL operations is beneficial for scoping log output and diagnosing DB-specific issues.
40-40: GORM logger creation.
Creating a GORM-compatible logger ensures consistent, structured log output. No issues seen.
45-53: Detailed connection error logs.
Providing host, port, database, and the error is helpful for troubleshooting. This approach improves observability.
58-58: Return statement includes auto migration.
Performing auto migration here is consistent with the rest of the code. Consider verifying performance impact in production if migrations are large.
63-65: Acquiring logger in Close.
Same pattern as in Open. This ensures uniform log messages for MySQL connections.
68-72: Connection uninitialized check.
Appropriate error handling with descriptive message. Good approach to fail fast.
77-81: Logging DB object retrieval failure.
Capturing the error in both custom logger and fallback standard log is a good fallback pattern.
87-91: Graceful closure failure logs.
Same pattern for error logging. Consistent approach across the file.
95-97: Debug logging on successful close.
Good practice to confirm resource cleanup. No concerns here.
102-106: UpdateNote logic switched to uint.
Changing note ID to uint can reduce invalid ID conversions at runtime. Proper error wrapping helps debugging.internal/datastore/interfaces.go (14)
13-15: Introducing custom logger import.
This replaces the standard log package with a structured logger. The GORM logger alias also clarifies usage.
65-65: New SetLogger method in interface.
This addition allows for dynamic logger injection, increasing flexibility for testing and production.
70-71: DataStore now holds a logger.
Embedding a structured logger in the store fosters consistent logging across DB operations.
74-77: Implementation of SetLogger.
Straightforward setter method. No issues found.
79-86: getDbLogger for granular DB operations.
Naming sub-loggers after operations (e.g., “db.save”, “db.lock”) fosters clarity. Good approach.
110-112: Transaction-specific logger retrieval.
Defining a distinct logger name for each DB transaction step helps trace issues more precisely.
168-181: Warn logs for “database locked” with retry details.
Providing transaction ID, attempt count, and delay is excellent for diagnosing concurrency issues.
191-199: Info logs for successful retry.
This indicates how many attempts were needed. Great for measuring lock contention frequency.
207-212: Error logging after all attempts fail.
Consolidated context (tx_id, error, max_attempts) is beneficial for triaging fatal DB lock scenarios.
439-444: Structured log for retrieval performance.
Including the retrieval time in milliseconds can help measure DB performance. Good addition.
646-647: Silencing GORM logger for sensitive query.
Switching the gormlogger to Silent is sometimes necessary for privacy or noise reduction. This approach is valid.
982-982: Disabling GORM logs for GetImageCache.
Reduces noise for repeated image cache lookups. Keep an eye on debug needs if diagnosing image cache issues.
999-999: Disabling GORM logs for SaveImageCache.
Similar reasoning as above. Minimizing log spam can benefit performance.
1013-1013: Disabling GORM logs for GetAllImageCaches.
Consistent with other queries. Fine approach for read-heavy or less critical queries.internal/datastore/sqlite.go (21)
59-60: Backup function extracted from method scope.
Making createBackup a standalone function is useful if it’s reused or tested independently.
65-65: Comment clarifies backup filename creation.
The timestamp-based naming is good for versioning.
72-72: Effective error context.
This message clarifies that opening the source file failed, simplifying analysis.
77-77: Destination file creation message.
No issues, short and descriptive.
83-86: Copying file content with error checking.
Properly returning the underlying error ensures any backup failures are surfaced.
93-97: Open method logic restructured.
Performing SQLite config validation early is a clear improvement. Good for ensuring the DB is set up properly.
98-99: Component-specific logger.
This mimics patterns in mysql.go. Maintaining consistent approach is beneficial for debugging differences.
101-103: Retrieving DB path from config.
Falling back to defaults if empty is user-friendly. Good practice.
116-116: Use of default config path.
Selecting the first config directory if the path is empty is sensible. However, handle edge cases if multiple directories exist or if the user wants a different one.
120-128: Ensure parent directory creation.
Creating directories if they don’t exist is helpful for first-run initialization, with thorough error logging.
131-138: Write permission checks.
Proactively discovering permission errors prevents confusing runtime issues downstream. Good approach.
141-160: Disk space validation.
Warnings on partial failure (missing space measurement) is a good fallback strategy. The user is at least informed.
163-175: Conditional backup creation.
Creating a backup only when the DB is over 1MB helps reduce overhead. Good trade-off.
179-179: New GORM logger creation.
Using a custom logger aligns with the overall unified logging system in this PR.
182-183: Attaching logger to gorm.Config.
Ensures consistency for every query.
187-197: SQLite database opening with WAL & timeout.
Enables improved concurrency. Logging errors with “path” and “error” is good thoroughness.
199-214: Setting connection pool parameters.
Including error logs if DB() retrieval fails is consistent. No issues found.
215-216: Perform auto-migration post-initialization.
Keeps the pattern consistent with the MySQL store.
220-230: Close method logger pattern.
Matching other DB store closes. The early return if DB is nil is a good safeguard.
232-240: Retrieving generic DB object and logging failures.
Consistent approach to error reporting. Clear context is provided in log messages.
243-255: Final close with debug on success.
Mirrors the MySQL approach. Ensures the user is informed about successful or failed closure.internal/datastore/manage.go (4)
9-11: Nice imports organizationGood job adding the custom logger import and using a named import for the GORM logger to avoid naming conflicts.
14-28: Well-documented logger integrationGood approach to maintain backward compatibility while allowing for future custom logger integration. The comments clearly explain the current limitations and rationale.
Consider implementing the GORM logger adapter in the future to fully leverage structured logging for database operations.
30-53: Clean implementation of logger fallbackThe function properly uses the custom logger when available and gracefully falls back to standard logging. This ensures backward compatibility while enabling structured logging.
55-62: Well-implemented component-specific loggerGood pattern for creating named loggers that follow a hierarchical structure. This will help with log filtering and organization.
internal/logger/logger.go (5)
2-30: Excellent package documentationThe documentation clearly explains the key features, usage patterns, and configuration options. The examples are particularly helpful for new developers.
44-49: Good logger structure designWrapping both the standard Zap logger and sugared logger gives flexibility while maintaining a clean API.
51-67: Well-configured encoder settingsThe encoder configuration provides a good balance between readability and information density.
75-77: Review potential security concern with header loggingThere was a previous code scanning alert about sensitive data from HTTP headers being logged. Ensure the Debug method doesn't inadvertently log sensitive information from headers.
243-270: Good utility methods for logger contextualizationThe Named, With, Sync, and GetZapLogger methods provide a clean API for logger customization and management.
internal/httpcontroller/handlers/detections.go (6)
166-170: Good structured logging implementationThe error logging now includes context about what operation failed along with the error details, making debugging easier.
178-186: Well-implemented component-specific loggerUsing a dedicated logger for each component helps with log organization and filtering.
255-271: Properly deprecated legacy logging methodGood approach to maintain backward compatibility while encouraging migration to structured logging.
675-712: Excellent retry logic with structured loggingThe retry mechanism is well implemented with proper logging of attempts, delays, and outcomes.
714-863: Good separation of concerns for database operationsBreaking down the review and lock operations into separate methods improves readability and maintainability.
1021-1027: Useful debug logging for lock operationsThe structured logging includes all relevant context for debugging lock-related issues.
- Remove hardcoded database backup mechanism in Open() method - Simplify database initialization process - Align with ongoing logging and error handling refactoring efforts This change removes redundant backup logic, potentially to be replaced with a more robust backup strategy in future implementations.
- Extract encoder, level, and option configuration into separate helper functions - Improve code modularity in logger initialization and configuration - Add more flexible and reusable methods for creating loggers - Standardize log level, encoder, and zap option generation - Simplify logger creation process across different use cases - Ensure consistent handling of caller information and development mode This refactoring improves the logger's flexibility, readability, and makes configuration more modular and maintainable.
…ge fetching - Replace standard logging with structured, component-specific logging in image provider - Use global logger with named components in Wikipedia image fetching - Improve debug logging with more context and request tracing - Enhance error handling and logging in thumbnail and author info retrieval - Remove redundant log statements and simplify logging configuration - Add more informative and consistent log messages for image cache and fetching operations This refactoring improves logging consistency, error traceability, and observability in image-related operations.
- Remove redundant startup log message for audio sources - Rename clip cleanup monitor logger to a more concise "cleanup" - Maintain consistent logging approach across realtime analysis components
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
internal/httpcontroller/server_logger.go (1)
28-47: Consider simplifying conditional message formatting.The Debug method correctly handles development mode logging with fallback to standard logging, but the message formatting logic could be more concise.
func (s *Server) Debug(format string, v ...interface{}) { if s.isDevMode() { // In debug mode, always use the logger if available if s.Logger != nil { - message := format - if len(v) > 0 { - message = fmt.Sprintf(format, v...) - } - s.Logger.Debug(message) + if len(v) > 0 { + s.Logger.Debug(fmt.Sprintf(format, v...)) + } else { + s.Logger.Debug(format) + } } else { // Fall back to standard log if logger isn't initialized yet if len(v) == 0 { log.Print(format) } else { log.Printf(format, v...) } } } }internal/logger/logger.go (2)
75-174: Comprehensive logger initialization with good error handlingThe NewLogger function handles multiple configuration scenarios well and provides appropriate error handling. The implementation intelligently branches based on configuration to create the right logger for each use case.
Consider adding a helper method to allow for dynamic log level changes at runtime, which would be useful for debugging production issues without restarting the application.
176-192: Effective context enrichment methodsThe Named and With methods provide good support for creating context-rich logs, which is essential for traceability in complex applications.
Consider adding a WithContext method that extracts logging fields from a context.Context object, which would better integrate with Go's context pattern for request-scoped logging.
internal/imageprovider/imageprovider.go (1)
41-41: Consider using an interface for the logger field.
Exposing the logger as*logger.Loggeris fine, but using an interface can make testing easier.internal/logger/config.go (2)
27-37: Consider providing a more specific default log level.In the DefaultConfig, the Level field is set to an empty string. While this gets interpreted properly in GetLogLevel, it would be more explicit to set it to "debug" directly for development environments.
return Config{ - Level: "", + Level: "debug", JSON: false, Development: true, FilePath: "", DisableColor: false, DisableCaller: true, }
91-94: Consider adding timezone information in logs.The simpleDateTimeEncoder formats time without timezone information, which could make troubleshooting across different environments challenging. Consider adding timezone offset or using UTC explicitly.
func simpleDateTimeEncoder(t time.Time, enc zapcore.PrimitiveArrayEncoder) { - enc.AppendString(t.Format("2006-01-02T15:04:05")) + enc.AppendString(t.Format("2006-01-02T15:04:05Z07:00")) }internal/imageprovider/wikipedia.go (1)
287-298: Redundant logging statements could be consolidated.There are multiple consecutive debug statements logging similar information about the thumbnail. Consider consolidating these into a single, more comprehensive log entry to reduce log verbosity.
if l.debug && l.logger != nil { l.logger.Debug("Successfully retrieved thumbnail", "request_id", reqID, - "url", url, - "file", fileName) - l.logger.Debug("Successfully retrieved thumbnail URL", - "request_id", reqID, "url", thumbnailURL, "file", fileName) - l.logger.Debug("Thumbnail source file", - "request_id", reqID, - "file", fileName) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
cmd/root.go(2 hunks)internal/analysis/realtime.go(5 hunks)internal/datastore/sqlite.go(1 hunks)internal/httpcontroller/server.go(5 hunks)internal/httpcontroller/server_logger.go(1 hunks)internal/httpcontroller/template_renderers.go(8 hunks)internal/imageprovider/imageprovider.go(18 hunks)internal/imageprovider/wikipedia.go(6 hunks)internal/logger/config.go(1 hunks)internal/logger/global.go(1 hunks)internal/logger/logger.go(1 hunks)internal/logger/testing.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- cmd/root.go
- internal/logger/testing.go
- internal/httpcontroller/server.go
- internal/analysis/realtime.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (74)
internal/httpcontroller/server_logger.go (5)
14-26: Well-designed adapter pattern for Echo logging integration.The
echoLogAdapterprovides a clean solution to bridge the custom logger with Echo's logging system by implementing theio.Writerinterface. The implementation correctly handles whitespace trimming and empty message filtering.
49-78: Robust logger initialization with proper error handling.The
initLoggermethod correctly handles configuration, development mode detection, and integration with Echo's logging system. Using a named logger from the global instance promotes consistency across the application.
80-109: Well-structured request logger setup with context enrichment.The first part of
setupRequestLoggerproperly configures the middleware and captures important request context like request IDs and path parameters for correlation.
110-125: Smart log level selection based on HTTP status codes.The status-based logging level selection is a well-thought-out approach that ensures appropriate visibility for different types of responses. Using error logs for 5xx, warn for 4xx, and debug for 3xx (in dev mode) is a good practice.
126-173: Comprehensive structured logging with conditional field inclusion.The log entry construction is thorough and efficient, including:
- Essential fields for all requests
- Conditional inclusion of optional fields
- Special handling for error cases
- Development-mode-specific detailed logging
This approach balances verbosity with performance by only including necessary context.
internal/logger/logger.go (5)
1-41: Well-structured package documentation with clear examplesThe package documentation provides a comprehensive overview of the logger's capabilities and includes practical usage examples. This is excellent for developer onboarding and understanding the package's features.
43-48: Good encapsulation of zap loggersThe Logger struct effectively wraps both the standard zap.Logger and the SugaredLogger, providing a clean interface while maintaining access to zap's powerful features.
50-53: Potential security concern with Debug loggingBe cautious with debug logging as it might inadvertently log sensitive information. Consider implementing sanitization for sensitive fields when in debug mode.
This comment addresses the previously identified security issue where "Sensitive data returned by HTTP request headers flows to a logging call."
50-73: Consistent logging methods with structured logging supportThe implementation of logging methods (Debug, Info, Warn, Error, Fatal) follows a consistent pattern and properly utilizes zap's structured logging capabilities.
194-203: Important utility methods for advanced usageThe Sync method is crucial for preventing log loss at application shutdown, and providing access to the underlying zap.Logger offers flexibility for advanced use cases.
internal/imageprovider/imageprovider.go (22)
6-6: No issues with keepingfmtimport.
It’s okay to retainfmtfor potential string formatting or error wrapping.
13-13: Importing the custom logger.
This import is essential for structured logging.
72-74: Structured debug logging for refresh start.
Keeps logs cleaner and only logs in debug mode.
87-88: Same debug-check pattern.
107-109: Same debug-check pattern.
114-116: Same debug-check pattern.
124-127: Same debug-check pattern.
134-135: Same debug-check pattern.
140-142: Same debug-check pattern.
166-168: Same debug-check pattern.
173-176: Same debug-check pattern.
183-187: Same debug-check pattern.
214-216: Naming the logger for clarity.
Good approach to differentiate loggers by component name.
223-223: Assigning the component logger.
Ensures logs include “imageprovider.cache”.
244-246: Same debug-check pattern.
252-256: Same debug-check pattern.
279-281: Same debug-check pattern.
297-300: Same debug-check pattern.
309-310: Same debug-check pattern.
416-418: Same debug-check pattern.
425-428: Same debug-check pattern.
473-475: Same debug-check pattern.internal/datastore/sqlite.go (3)
59-90: Backup logic simplified successfully.
Creating a backup only if the file exists is a sensible check. Using a timestamp with a dot separator is fine. Error messages are clearer now.
93-201: Open method refactor.
- Good structured logging with
sqliteLogger.- Proper checks for path, directory creation, permissions, and disk space.
- The fallback to standard log when
sqliteLoggeris nil is consistent but consider guaranteeing the logger is never nil.
204-241: Close method improvements.
- Checks if
store.DBis nil to prevent panic.- Logs errors if closing fails.
- Debug log for successful close is a nice detail.
internal/httpcontroller/template_renderers.go (15)
13-13: Logger import.
Facilitates new structured logging.
36-37: New logger and debug fields in struct.
Enables structured debug logs. Great addition.
57-64: Error logging on template execution failure.
Good structured context with template name and error.
72-79: Logging write errors.
Again, structured approach is helpful.
87-90: Setting up a named logger.
Enhances traceability for template-related logs.
125-128: Component-specific logger for rendering.
Keeps logs organized by functionality.
134-139: Invalid data type error.
Returning typed error is good. Lint warnings about directfmt.Errorf(errMsg)can appear; see prior notes if that rule is still enforced.
156-161: No route found scenario.
Using structured logging clarifies the missing route.
179-183: Successful render in debug mode.
Nice detail to confirm in development.
192-195: Named logger for settings.
Continues the theme of specialized loggers.
202-220: CSRF token checks with structured logging.
Warnings for missing token are helpful. Good environment-based decision for debug logs.
233-239: Species config logging in debug mode.
Useful for diagnosing data passed to the template.
247-256: Detailed logging on rendering errors.
Printing data keys on failure can be quite helpful for debugging.
260-265: Debug message for successful render.
Keeps track of successful template usage.
270-278: Helper function for data keys.
Aids logging clarity by listing relevant fields.internal/logger/global.go (13)
1-2: File introduction and package declaration.
Organizes global logger in a dedicated package.
4-8: Imports and sync usage.
synchronizeglobal logger initialization is well handled.
10-15: Global variables for default logger.
Thesync.Onceapproach prevents race conditions.
17-34: InitGlobal method.
- Enforces
DisableCaller.- Prints a debug message about original config.
- Retains first successful init only.
36-56: GetGlobal method.
- Provides lazy initialization.
- Logs config on creation.
- Exits if initialization absolutely fails.
58-61: Global Debug function.
Centralizes debug logging. No concerns.
63-66: Global Info function.
No issues. Using the global logger is convenient.
68-71: Global Warn function.
Properly defers to the underlyingWarn.
73-76: Global Error function.
Centralized approach, consistent with other levels.
78-81: Global Fatal function.
Exits after logging. Ensure usage is deliberate.
83-86: Named logger derived from global.
Helps maintain separate contexts for different subsystems.
88-91: GlobalWithfor field context.
Chains fields effectively.
93-96: Sync method.
Clean flush approach for the global logger.internal/logger/config.go (5)
11-25: Good structure for the Config type with clear documentation.The Config struct is well-designed with comprehensive field documentation and appropriate JSON tags. The fields cover essential logging configuration needs including level, format options, and output destinations.
51-61: Well-designed RotationConfig with appropriate defaults.The RotationConfig struct properly encapsulates all necessary parameters for log rotation with clear documentation and JSON tags.
112-122: Good implementation of log level determination.The GetLogLevel function intelligently handles default levels based on environment and allows for explicit overrides through configuration, providing good flexibility.
124-142: Good handling of console vs file output formatting.The GetEncoderConfig function properly distinguishes between console and file outputs, applying color coding only when appropriate for the output medium and user preferences.
152-163: Robust level parsing with graceful fallback.The getZapLevel function handles empty values and parsing errors gracefully by providing sensible defaults, which prevents configuration issues from causing application failures.
internal/imageprovider/wikipedia.go (6)
16-16: Good integration of the new logger package.Adding the logger package import is appropriate for the structured logging implementation.
27-27: Well-implemented logger integration in provider struct.The addition of the logger field to the struct and initialization using a named component logger follows good logging practices for component isolation.
Also applies to: 47-49, 56-57
168-169: Well-implemented structured logging with request ID tracing.The implementation of structured logging with request IDs provides excellent traceability across operation steps. The conditional checks for both debug mode and logger existence prevent unnecessary processing.
Also applies to: 65-69, 83-88, 100-104, 108-112, 116-120, 124-133, 140-146, 150-159
162-163: Improved error message clarity.Changing from "no pages found for request" to "no pages found in response" provides a more precise description of the actual error condition.
333-340: Improved error handling for empty imageInfo array.The code now properly checks if the imageInfo array is empty before attempting to access its elements, making the error handling more robust.
365-376: Good implementation of HTML parsing with fallback.The code properly handles extracting artist information from HTML content with a fallback to plain text when HTML parsing fails, making the system more resilient.
…cation components - Add named root logger in application initialization - Implement hierarchical logger propagation in HTTP server and API components - Update logger initialization to support explicit parent logger injection - Improve logger naming consistency across server, API, and other components - Remove redundant global logger debug print statements - Enhance logger configuration with more flexible initialization methods This refactoring standardizes logging approach, improves component-specific logging, and provides more consistent and informative log messages throughout the application.
- Remove global logger dependency in root command and realtime analysis - Implement hierarchical logger propagation in BirdNET core initialization - Add logger injection support for BirdNET interpreter and related components - Update model reload and debug methods to use structured logging - Improve logging configuration with more flexible initialization methods - Remove redundant debug print statements and replace with structured logging This refactoring standardizes logging approach, improves component-specific logging, and provides more consistent and informative log messages in BirdNET core functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (16)
internal/logger/global.go (4)
10-15: Consider the implications of introducing a global logger.Relying on global state can make tests more complex to isolate and can introduce hidden dependencies between modules. You may want to allow an option for injecting a custom logger instance to keep the code more modular and maintainable.
36-47: Avoid abrupt termination withos.Exit(1).Libraries or shared modules typically avoid calling
os.Exit(1), as it terminates the entire process prematurely. Consider returning an error or using fallbacks (e.g., a minimal logger) to allow applications using this library to decide how to handle failures.
52-75: Promote testability by minimizing global logger calls.Accessing the global logger directly in multiple helper methods can complicate testing, especially when stubbing or capturing logs. Consider injecting dependencies (e.g., a logger interface) instead of relying on package-level logging methods for improved reusability and easier test isolation.
87-90: Clarify how callers should handle the returned error.The
Sync()function returns an error code, but it is unclear how or where this error should be processed. If you expect callers to handle or log the error, consider adding usage instructions or an example in the package documentation.cmd/benchmark/benchmark.go (1)
88-92: Consider adding log statements for benchmark progressThe logger has been properly integrated, but there's an opportunity to enhance the benchmarking process by adding structured log entries for key events such as start, completion, and performance metrics. This would complement the existing console output with structured logs for automated analysis.
func runInferenceBenchmark(settings *conf.Settings, results *benchmarkResults) error { // Initialize a logger for the benchmark benchmarkLogger := logger.Named("benchmark") + benchmarkLogger.Info("Starting benchmark run", + "useXNNPACK", settings.BirdNET.UseXNNPACK) // Initialize BirdNET bn, err := birdnet.NewBirdNET(settings, benchmarkLogger)And near line 132, consider adding:
results.totalInferences = totalInferences results.avgTime = totalDuration / time.Duration(totalInferences) results.inferencesPerSecond = float64(totalInferences) / duration.Seconds() + + benchmarkLogger.Info("Benchmark completed", + "totalInferences", totalInferences, + "avgTimeMs", results.avgTime.Milliseconds(), + "inferencesPerSecond", results.inferencesPerSecond, + "useXNNPACK", settings.BirdNET.UseXNNPACK)internal/api/v2/streams.go (1)
168-168: Improved error logging in writePump using the new logf methodAll error logging in the writePump method now uses the new centralized logf method, which provides consistent error reporting across the client methods.
Consider adding more structured context to these error logs by using key-value pairs to provide additional debugging information when errors occur:
-client.logf("Failed to set write deadline: %v", err) +client.logf("Failed to set write deadline: %v", err)In a future update, you might consider converting this to fully structured logging with a logger that supports key-value pairs:
client.logger.Error("Failed to set write deadline", "error", err)Also applies to: 175-175, 182-182, 187-187, 195-195, 201-201, 207-207, 212-212, 217-217
internal/logger/README.md (2)
669-671: Missing language specification in fenced code blockThe fenced code block showing the nil pointer panic doesn't specify a language.
-``` -panic: runtime error: invalid memory address or nil pointer dereference -``` +```text +panic: runtime error: invalid memory address or nil pointer dereference +```🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
669-669: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
187-187: Loose punctuation mark in list itemThere's an extra space after the colon in the list of test helpers.
-- `CreateTestCore`: Creates a logger core that writes to a test buffer +- `CreateTestCore`: Creates a logger core that writes to a test buffer🧰 Tools
🪛 LanguageTool
[uncategorized] ~187-~187: Loose punctuation mark.
Context: ...nctions for testing: -CreateTestCore: Creates a logger core that writes to a ...(UNLIKELY_OPENING_PUNCTUATION)
internal/api/v2/api.go (1)
229-251: Consistent logging helpers with appropriate levelsThese helper methods provide a clean interface for formatted logging at different levels, maintaining consistency across the codebase.
However, there's some code duplication between Debug and LogfDebug methods where both check the debug setting. Consider refactoring to avoid this duplication.
// LogfDebug is a helper for logging debug messages with Printf-style formatting using the structured logger func (c *Controller) LogfDebug(format string, v ...interface{}) { - if !c.Settings.WebServer.Debug { - return - } - - c.logger.Debug(fmt.Sprintf(format, v...)) + if c.Settings.WebServer.Debug { + c.logger.Debug(fmt.Sprintf(format, v...)) + } }internal/analysis/realtime.go (7)
32-34: Document the function more thoroughly.
While these lines declareRealtimeAnalysisand anerrvariable, consider documenting the return behavior and any potential side effects in more detail, particularly given the complexity of the routine.
166-169: Processor logger.
Currently, you’ve commented outproc.SetLogger(...). If the processor eventually supports logger injection, consider finalizing it to further unify logging.
180-185: Buffer manager logger injection.
Although it’s commented out, consistent logger injection across all major components (e.g., the buffer manager) will help unify logs. Consider implementing it once the buffer manager supports it.
257-262: Clip cleanup monitor with injected logger.
Injecting the logger fosters more granular logs for disk cleanup. Also note thatdiskmanager.InitLogger()is still a separate step; performing direct injection is usually preferable over static initialization.
270-302: New clipCleanupMonitor function looks coherent.
The function polls at 5-minute intervals, handles “age” and “usage” retention policies, logs errors, and exits gracefully upon a quit signal.
- Consider verifying concurrency correctness, ensuring no race conditions with the dataStore or disk usage checks.
- The repeated checks for
kill signalwithin the cleanup calls are appropriate to handle abrupt shutdown.
326-346: Telemetry endpoint logger injection.
In line with the rest of your design, you’ve included the telemetry endpoint in the hierarchical logging structure. The disabled block forSetLogger(logger)is fine, but consider implementing once that component is ready.
372-435: initBirdImageCache is well-structured.
Your approach:
- Create the cache.
- Retrieve species from DB.
- Launch goroutines to fetch images concurrently.
Suggestions:
- Make sure concurrency is thoroughly tested, especially for partial failures or repeated calls.
- Consider a wait before repeated attempts, or caching results more aggressively, if startup times become an issue.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
cmd/benchmark/benchmark.go(2 hunks)cmd/rangefilter/print.go(2 hunks)cmd/realtime/realtime.go(1 hunks)cmd/root.go(1 hunks)go.mod(3 hunks)internal/analysis/birdnet_init.go(2 hunks)internal/analysis/realtime.go(8 hunks)internal/api/v2/api.go(7 hunks)internal/api/v2/integration.go(0 hunks)internal/api/v2/integrations.go(3 hunks)internal/api/v2/streams.go(9 hunks)internal/api/v2/weather.go(4 hunks)internal/birdnet/birdnet.go(12 hunks)internal/httpcontroller/server.go(6 hunks)internal/httpcontroller/server_logger.go(1 hunks)internal/logger/README.md(1 hunks)internal/logger/global.go(1 hunks)
💤 Files with no reviewable changes (1)
- internal/api/v2/integration.go
✅ Files skipped from review due to trivial changes (1)
- cmd/root.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/httpcontroller/server_logger.go
🧰 Additional context used
🪛 LanguageTool
internal/logger/README.md
[uncategorized] ~187-~187: Loose punctuation mark.
Context: ...nctions for testing: - CreateTestCore: Creates a logger core that writes to a ...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 markdownlint-cli2 (0.17.2)
internal/logger/README.md
669-669: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (63)
internal/logger/global.go (1)
20-31: Good use ofsync.Oncefor safe concurrency.Your initialization logic efficiently prevents race conditions and unnecessary re-initialization, ensuring a thread-safe setup.
internal/analysis/birdnet_init.go (1)
18-20: Well-structured logger implementationThe integration of the structured logger with a clear hierarchical naming pattern (
birdnet-go.analyzer) follows good practices for organized logging. Passing the logger toNewBirdNETallows for component-specific logging.go.mod (3)
32-32: Good choice of logging libraryAdding Zap is appropriate for implementing structured logging as specified in the PR objectives. Zap is a high-performance, structured logging library that will support the desired JSON and human-readable formats.
37-37: Appropriate log rotation implementationThe addition of lumberjack for log rotation is a good choice that complements the Zap logging system, enabling proper log file management as mentioned in the PR objectives.
97-97: Library version update looks goodUpdating multierr to v1.11.0 aligns well with the Zap dependency, as they are from the same ecosystem.
cmd/rangefilter/print.go (1)
19-22: Consistent logger naming patternThe logger naming follows the same pattern as seen in other files, maintaining consistency across the codebase. The placement right before using it in the
NewBirdNETfunction creates a clear flow.internal/api/v2/streams.go (5)
56-63: Good implementation of thelogfmethod with proper fallbackThe helper method is well-implemented with a proper fallback to the standard logger when the client logger is nil, providing consistent logging behavior.
80-80: Using the new controller logging method correctlyThis line correctly uses the new controller's structured logging method for error reporting.
91-91: Consistent logger initialization across client creation methodsBoth client creation blocks now consistently initialize the logger field with the default logger.
Also applies to: 121-121
100-100: Passing logger to readPump for consistencyThe default logger is now consistently passed to the readPump method, ensuring that the client always has a logger available.
Also applies to: 129-129
238-238: Standardized error logging in readPumpThe readPump method now uses the client's logf method for logging errors related to setting read deadlines, providing consistent error reporting.
Also applies to: 247-247
internal/api/v2/weather.go (4)
5-5: Import of fmt package for string formattingThe fmt package is now imported to properly support the string formatting needed in the new structured logging calls.
137-140: Improved structured logging for future date warningThe logging calls have been updated to use structured logging with key-value pairs, which is more consistent with modern logging practices and makes logs easier to parse and analyze.
155-158: Consistent structured logging for missing data warningThis logging statement uses the same structured format as other logs, improving consistency throughout the application.
446-449: Enhanced error logging with structured key-value pairsThis logging statement now provides clear context with key-value pairs for the date, error, and endpoint, making it easier to track and debug issues.
cmd/realtime/realtime.go (2)
14-14: Simplified function documentationThe comment has been updated to be more concise while still accurately describing the function's purpose.
21-21: Added helpful comments about notification channel and logger initializationThese comments clearly explain that a notification channel is being created and that the RealtimeAnalysis function now handles its own logger initialization, which helps future developers understand the code flow.
Also applies to: 23-23
internal/logger/README.md (1)
1-938: Comprehensive logger package documentationThe README is extremely well-written and thorough, covering all aspects of the logger package including features, usage examples, configuration options, and best practices.
The documentation includes:
- Clear explanations of the logger's features and benefits
- Detailed examples for different usage scenarios
- Configuration options with explanations
- Testing utilities and approaches
- Comprehensive best practices and application-wide logging patterns
- Cross-platform considerations
- Future enhancement plans
This will be very valuable for developers working with the new logging system.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~187-~187: Loose punctuation mark.
Context: ...nctions for testing: -CreateTestCore: Creates a logger core that writes to a ...(UNLIKELY_OPENING_PUNCTUATION)
🪛 markdownlint-cli2 (0.17.2)
669-669: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
internal/api/v2/integrations.go (4)
87-87: Improved logging level categorizationThe change from a generic Printf to a specific LogfWarn method improves log categorization and makes the log message more actionable.
92-92: Appropriate warning level for control channel issueGood choice of log level for this operational issue, making it distinguishable from errors but still highlighting a potential problem.
227-227: Better error classification with structured loggingUsing LogfError instead of Printf properly categorizes this as an error condition, which will improve observability and alerting capabilities.
239-239: Consistent error logging approachThis change maintains consistency with the new logging approach throughout the codebase, aiding in log aggregation and analysis.
internal/birdnet/birdnet.go (6)
48-48: Good addition of structured logger fieldAdding the logger as a field in the BirdNET struct ensures consistent logging throughout the lifecycle of the object.
53-60: Well-implemented logger initialization with fallbackThe constructor properly accepts a parent logger, establishing a proper hierarchy while providing a fallback to the global logger when needed.
112-113: Descriptive warning with contextual informationThe warning now includes helpful context about the XNNPACK delegate failure with a recommendation, making troubleshooting easier.
362-365: Enhanced error logging with structured fieldsThe error message now includes additional context using key-value pairs, which improves the diagnostic value of the logs.
453-458: Simplified debug method with conditional executionThe Debug method cleanly implements conditional logging based on settings, making debug statements more performant.
460-467: Well-designed SetLogger method with proper fallbackThis method allows for changing the logger after initialization, maintaining the module prefix and providing a fallback to the global logger.
internal/httpcontroller/server.go (5)
92-146: Excellent constructor pattern with explicit logger supportThe new constructor ensures proper logger hierarchy and explicitly documents the preferred way to initialize the server. This promotes better logging practices throughout the codebase.
230-233: Good initialization sequence for logger dependencyThe code ensures that handlers receive a logger only after it's properly initialized, preventing potential null pointer issues.
261-271: Well-implemented environment-aware development mode detectionThe isDevMode method properly checks both environment variables and configuration settings, providing flexibility in how development mode is enabled.
274-281: Improved error handling with nullability checkThe method properly checks if logger is initialized before using it, preventing potential null pointer exceptions.
283-351: Comprehensive logging utilities for different contextsThese helper methods create a well-structured logging hierarchy that will help with log organization and filtering. The request context logger is particularly useful for tracing requests through the system.
internal/api/v2/api.go (3)
116-146: Well-designed API initialization with logger hierarchyThe InitializeAPI function properly handles the logger dependency and creates a hierarchical logger structure, improving log organization.
203-207: Enhanced error reporting with correlation IDThe structured error logging now includes a correlation ID, which greatly improves traceability of errors across the system.
212-227: Conditional debug logging with proper formattingThe Debug method now properly handles the case where no format arguments are provided and respects the debug setting from configuration.
internal/analysis/realtime.go (27)
15-15: Use imports consistently.
The addition of the viper import is aligned with the new logging configuration. This looks correct, and it follows typical patterns for environment-based configurations.
23-23: New logger import.
Importing"github.com/tphakala/birdnet-go/internal/logger"introduces your custom logger. No immediate issues noted here.
36-44: Centralized logging configuration is a good approach.
You’ve consolidated logging settings (level, file path, JSON format, etc.) into a singleconfigobject. This will simplify changes for future logging modifications. Consider validating the config fields if they come from external sources (e.g., environment variables).
46-48: Global logger initialization.
Initializing a global logger is acceptable, but be mindful of the complexities when running concurrent tests or operations that might also need logger changes. Ensure final log flush is invoked (if needed) on application exit to avoid losing logs in buffers.
50-55: Creating hierarchical loggers.
UsingappLogger := logger.Named("birdnet-go")and then child loggers likeanalyzerLogger := appLogger.Named("analyzer")fosters good structure. This approach helps trace logs back to their origin context.
59-59: Error logs are consistent.
You are logging the error context with"error", err. This is excellent for debugging. Ensure upstream callers handle the returned errors properly and fail fast where necessary.Also applies to: 70-70
83-95: Contextual logs for system information.
Logging environment details (OS, hardware, threshold, overlap) at startup is very helpful for troubleshooting. This clarity is beneficial for debugging.
100-103: Hierarchical logger for datastore.
Assigning a distinctdbsub-logger is a neat practice to keep logs from various subsystems well-structured.
106-112: Error handling on DB open.
The code logs the failure and returns the error promptly, ensuring the application halts if the DB connection is not ready. Usingdefer closeDataStore(...)is also a good pattern.
140-141: Graceful degradation logging.
Warning about buffer initialization issues or missing audio sources is valuable to help operators diagnose missing RTSP configurations.Also applies to: 144-144
153-154: Metrics initialization error handling.
Immediately logging the error and returning ensures the system doesn’t proceed in an invalid state. This is clean and consistent with the overall style.
160-160: Initializing bird image cache with a child logger.
Passing a logger toinitBirdImageCacheensures more context in logs. Make sure each submodule can also be tested individually without a global logger.
174-174: HTTP server creation with logging.
Creating thehttpServerwith an inherited logger is a good step for consistent logs from your web layer. No immediate issues here.
193-195: Audio capture logger usage.
Starting audio capture with a dedicatedaudioLoggermaintains clear, structured logs for audio operations. This is well-aligned with your modular logging approach.
197-200: Clip cleanup logger usage.
Great job introducing acleanupLoggerfor the cleanup routine. This helps quickly identify any clip cleanup issues in logs.
202-206: Weather polling logger usage.
Assigning a distinctweatherLoggerensures clarity if weather polling fails or needs troubleshooting.
208-211: Telemetry endpoint logger.
Consistent approach with naming loggers for discrete functionalities. This pattern fosters maintainability.
212-215: Control monitor logger usage.
A separate logger for hot reload or control signals is a nice addition, clarifying logs around dynamic configuration changes.
216-217: Quit signal monitoring.
Passing the analyzer logger tomonitorCtrlCensures a single consistent place for shutdown logs. This is beneficial for tracing final application events.
223-223: Shutdown logs.
Providing explicit logs during shutdown helps operators confirm that graceful termination took place and logs the final sequence of events.Also applies to: 232-232
238-240: Restart logic.
Restarting audio capture is well-logged, giving operators immediate feedback that the system has begun reinitializing the audio pipeline.
245-250: Extended signature with logger parameter.
Introducinglogger *logger.LoggerintostartAudioCaptureis consistent with your new logging architecture. Consider verifying thatmyaudio.CaptureAudioitself can handle or at least ignore the logger if not used.
266-266: Direct call to clipCleanupMonitor.
This direct handoff is consistent. Confirm that any configuration changes instartClipCleanupMonitordo not disrupt concurrency or test scenarios.
305-318: startWeatherPolling with logger.
You log errors on weather service creation and have a plan to set the logger if the service supports injection. Looks consistent with your approach so far.
349-360: monitorCtrlC function with logger.
Encapsulating signal listening in a small function is a good separation of concerns. Logging the signal provides clarity in real-time usage and in logs.
363-369: closeDataStore with logger.
Simple but important. Logging successful closure or errors ensures consistent tracking of DB connection states.
440-450: startControlMonitor with logger.
Yet another place consistently leveraging a discrete logger. This final piece appears to align with your new logging architecture.
- Replace 'birdnet-go' and 'analyzer' named loggers with a single 'core' logger - Update BirdNET initialization and realtime analysis to use consistent logger naming - Simplify logger hierarchy and reduce redundant logger creation - Hide server port in HTTP controller initialization This change streamlines logging configuration and reduces complexity in logger management across the application.
- Add comprehensive sensitive data redaction for all logger methods - Implement regex-based pattern matching for tokens, passwords, and other sensitive information - Create dedicated functions for redacting log messages and field values - Add unit tests to validate sensitive data handling - Improve logging security by preventing accidental exposure of sensitive data - Maintain existing logging structure while adding an extra layer of protection This change introduces automatic sensitive data protection in logging, ensuring that potentially confidential information is not accidentally logged or exposed.
…ization - Remove global.go file and associated global logger functions - Update README.md to reflect new logger initialization approach - Modify components to use explicit logger injection and creation - Remove fallback mechanisms for global logger in various components - Simplify logger initialization by requiring explicit logger creation - Improve logger dependency management across application components This change eliminates the global logger, enforcing more explicit and controlled logger initialization and propagation throughout the application.
…tput - Add `ForceJSONFile` configuration option to control file log format - Modify `CreateEncoder` to support dual-format logging - Introduce `DualLogConfig` helper function for easy dual-format logger setup - Update logger initialization to support human-readable console and JSON file logs - Enhance README with comprehensive documentation on new logging features - Refactor realtime analysis logging to use new dual-format configuration This change provides more flexible logging capabilities, allowing developers to have human-readable console logs while maintaining structured JSON logs for analysis tools.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (20)
internal/logger/README.md (2)
5-16: Comprehensive Feature List.
The bullet‐list clearly outlines the package’s capabilities. For consistency and clarity, consider formatting key terms (e.g. using backticks forlumberjack) so that they stand out as code or library names.
183-191: Configuration Options Overview.
The configuration section lists and describes all parameters (e.g.Level,JSON,ForceJSONFile, etc.). Consider using inline code formatting (e.g.Level) for parameter names to enhance readability.internal/logger/logger.go (8)
34-36: Consolidate imports or provide rationale for each import separately.While functional, consider grouping standard library imports (like
log,os,filepath), third-party imports (go.uber.org/zap, etc.), and local imports to improve readability.Also applies to: 38-40
43-43: Consider adding more details to the Logger struct comment.Currently, the comment is minimal. You might expand it to clarify how
zapandsugardiffer in usage so newcomers understand when to switch between structured vs. sugared logging methods.
45-48: Expose struct fields cautiously or consider making them private.The
zap,sugar, andconfigfields are exported, which means they can be modified outside this package. If you want to ensure immutability, consider making them private and exposing them via getters.
58-64: Segregate debug vs. info logs carefully.Calling
Infomight sometimes lead to large logs in production if used for frequent events. Encourage developers to useDebugfor high-volume logs.
66-72: Warn-level logs may need alerts or metrics.Consider integrating a monitoring tool that increments a counter or triggers an alert for repeated warn logs, ensuring issues get noticed quickly.
74-80: Consider reusing code for repeated redaction.The
Errorand other log methods each perform redactions. Factor out the redaction logic into a small helper to limit repetition and reduce potential for future errors.
82-88: Fatal logs should be used sparingly.Crashing the application can be disruptive. In critical production systems, consider using graceful shutdown approaches.
195-202: Chaining "With" calls.Chaining calls, for example
logger.With(fields...).With(moreFields...), might overload logs with repeated context. Ensure usage remains consistent so you don’t inadvertently balloon logs.internal/logger/config.go (5)
11-27: Consider default values carefully.Providing empty or default values for log-level, color, or caller settings may mislead integrators who are unaware of the chosen defaults. Document these defaults clearly in your module’s main README.
42-52: Consider differentiating ProductionConfig from DefaultConfig more.Right now, the only changes are the log level, JSON format, and color. If you foresee a reason to diverge further, define those in synergy with your environment detection logic.
95-98: Time format: No timezone or milliseconds.Some operators need finer granularity. Consider providing a config option to switch to a more detailed time format if required.
116-126: Default log level in dev.Ensuring
DebugLevelis used in dev mode is consistent with your doc. Double-check that no unrelated system logs get overloaded in dev.
128-146: Using color in console logs is beneficial.Make sure there are fallback indicators for visually impaired users if color is disabled. Possibly consider extended text or a tag to identify severity in the console.
internal/analysis/realtime.go (4)
140-145: Warning for partial buffer initialization
Informing the user that some audio sources might be unavailable is helpful. Consider adding more context if certain sources fail frequently.
165-172: Commented-out logger injection
Consider fully implementing aSetLoggermethod onprocessoror removing these lines if injection is not needed.
180-186: Buffer manager logging
Similar to the processor, you might optionally introduce aSetLoggermethod to the buffer manager instead of leaving these references commented.
271-303: Implementation ofclipCleanupMonitor
Uses a 5-minute ticker and handles both age-based and usage-based cleanup. This is clear and maintainable. If you anticipate making this interval configurable, consider sourcing that fromsettings.internal/imageprovider/wikipedia.go (1)
103-107:queryAndGetFirstPagemethod
Well-structured checks for empty or invalid responses. Logging intermediate steps is valuable for debugging. Ensure you’re not inadvertently printing overly large data sets in logs.Also applies to: 112-116, 119-123, 127-139, 141-166
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
internal/analysis/birdnet_init.go(1 hunks)internal/analysis/realtime.go(8 hunks)internal/api/v2/api.go(7 hunks)internal/diskmanager/README.md(1 hunks)internal/diskmanager/file_utils.go(4 hunks)internal/diskmanager/policy_age.go(6 hunks)internal/diskmanager/policy_usage.go(8 hunks)internal/httpcontroller/middleware.go(7 hunks)internal/httpcontroller/server.go(6 hunks)internal/httpcontroller/server_logger.go(1 hunks)internal/httpcontroller/template_renderers.go(9 hunks)internal/imageprovider/imageprovider.go(17 hunks)internal/imageprovider/wikipedia.go(7 hunks)internal/logger/README.md(1 hunks)internal/logger/config.go(1 hunks)internal/logger/logger.go(1 hunks)internal/logger/rotation.go(1 hunks)internal/logger/sensitive.go(1 hunks)internal/logger/sensitive_test.go(1 hunks)internal/logger/testing.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- internal/analysis/birdnet_init.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/logger/testing.go
- internal/httpcontroller/template_renderers.go
- internal/logger/rotation.go
- internal/imageprovider/imageprovider.go
- internal/api/v2/api.go
🧰 Additional context used
🪛 LanguageTool
internal/logger/README.md
[uncategorized] ~200-~200: Loose punctuation mark.
Context: ...nctions for testing: - CreateTestCore: Creates a logger core that writes to a ...
(UNLIKELY_OPENING_PUNCTUATION)
internal/diskmanager/README.md
[duplication] ~26-~26: Possible typo: you repeated a word.
Context: ...icy_age_test.go** - Tests for age-based policy - policy_usage_test.go - Tests for usage-based...
(ENGLISH_WORD_REPEAT_RULE)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (110)
internal/logger/README.md (32)
1-3: Clear Introduction Header.
The header and introductory statement immediately communicate the purpose of the package and that it wraps Uber’s Zap for structured logging.
17-25: Well-Organized Package Structure.
The “Package Organization” section provides a concise breakdown of the files (such asconfig.go,logger.go,rotation.go, andtesting.go). This helps users quickly understand the repository layout.
26-33: Basic Usage – Import Statement.
The code snippet showing how to import the package is straightforward and easy to follow.
34-52: Example: Logger Initialization and Usage.
This section clearly demonstrates creating a logger using the default configuration, error handling, and invoking various log methods (including the use oflog.Fatal, with a helpful comment noting that it callsos.Exit(1)).
54-88: Custom Logger Configuration Example.
The custom logger example illustrates both console-only and file-with-rotation setups. It might be useful to add a brief note describing the difference in behavior between development and production configurations.
90-116: Sharing Loggers Across Components.
The example of creating a root logger and then generating named child loggers for different components (usingNamed) is clear and emphasizes modularity in logging.
118-129: Structured Logging Usage.
The snippet demonstrating the use of structured key-value pairs (e.g."user_id", 123) is succinct and illustrates the intended usage perfectly.
131-144: Named Loggers with Nested Notation.
The documentation shows two approaches for naming loggers (chaining with.Named()and dot notation). This dual example is very useful for developers to understand the flexibility in naming.
146-157: Persistent Context in Loggers.
The “With Context” example clearly demonstrates how additional fields (likeuser_idandsession_id) can be injected into all subsequent log entries. This is an effective pattern for maintaining context.
159-181: Log Rotation Implementation.
The log rotation snippet—showing the creation of a rotation configuration and its integration into the logger—is well-documented. Error handling is appropriately included.
192-232: Testing Guidelines and Helpers.
The testing section is comprehensive; it details how to create test cores, configure a logger for tests, and capture log output. This will greatly assist in ensuring the logging functionality is correctly integrated and behaves as expected.🧰 Tools
🪛 LanguageTool
[uncategorized] ~200-~200: Loose punctuation mark.
Context: ...nctions for testing: -CreateTestCore: Creates a logger core that writes to a ...(UNLIKELY_OPENING_PUNCTUATION)
233-244: Mocked Filesystems for Testing.
The example of usingaferoto create an in-memory filesystem for testing file operations is a nice touch. It shows attention to cross-platform and environment-independent testing.
245-252: Running the Logger Tests.
The instructions for executing the tests (usinggo test) are clear and provide a direct command for developers to verify functionality.
253-267: Logging Best Practices.
The “Good Practices” section offers solid recommendations such as deferringlog.Sync()and using structured logging consistently. These guidelines will help maintain high-quality log output during both development and production.
269-280: Component-Specific Logger Examples.
The guidelines for creating separate loggers (likedbLoggerandauthLogger) exemplify how to organize logs by application component. This supports easier debugging and log filtering.
282-290: Helper Functions for Configuration.
The examples usinglogger.DefaultConfig()andlogger.ProductionConfig()succinctly show how to quickly configure the logger for different environments.
291-299: Sensitive Data Handling Overview.
The section clearly explains how sensitive information is automatically redacted, which is crucial for maintaining security in log files.
300-314: Sensitive Data Redaction Example.
The before-and-after code example effectively demonstrates how sensitive fields (like tokens) are transformed into a redacted format, making the behavior very clear.
315-327: List of Recognized Sensitive Data.
This bulleted list is comprehensive and well-organized. It gives developers confidence that common sensitive items (e.g. passwords, API keys) will be appropriately handled.
328-338: Custom Sensitive Data Handling.
The example highlighting the use oflogger.RedactSensitiveDataandlogger.RedactSensitiveFieldsis straightforward. A brief mention of the expected input types or use cases might further enhance clarity.
339-347: Security Considerations.
This section reinforces the importance of cautious logging practices and provides actionable recommendations to prevent inadvertent exposure of sensitive data.
349-376: Application-Wide Logging Best Practices.
The detailed guidelines for managing logger initialization, dependency injection, and hierarchical logger naming across the application are thorough and very useful.
377-384: Logger Initialization in Components.
The code example demonstrates proper error handling while initializing a logger when one is not provided. This defensive approach improves robustness.
388-404: Real-World Example – Dual-Format Logging.
The complete example from the real-time analysis module shows how to set up dual-format logging (human-readable console output and JSON file logs). This practical demonstration makes the advanced features very accessible.
611-618: Cross-Platform File Paths.
The snippet advises against using platform-specific path separators and recommendsfilepath.Joinfor portability. This is a valuable tip for developers targeting multiple operating systems.
620-631: File Permissions and Cross-Platform Compatibility.
Using a constant (likeconst filePerms = 0644) to set file permissions improves both readability and maintainability across different platforms.
633-642: Terminal Colors Support.
The example illustrates how to conditionally disable colored output on Windows when terminal support is lacking. This ensures consistent behavior across environments.
649-651: File Locking Considerations.
A brief note on the need for platform-specific file locking is helpful. Although the snippet is short, it nudges developers to consider concurrency issues when handling log files.
653-660: Normalization of Line Endings.
The guidelines for handling different line endings (e.g. Windows vs. Unix) are a thoughtful addition, ensuring log consistency and easier parsing across platforms.
667-669: Environment Variable Conventions.
The suggestion to use uppercase with underscores for environment variables (likeLOG_LEVEL) follows common best practices and improves clarity in configuration management.
676-704: Process Signals Handling.
The signal handling snippet demonstrates cross-platform considerations, distinguishing between Windows and Unix-like systems. It correctly outlines the difference in behavior for SIGHUP versus other signals. For complex applications, it might be beneficial to document further customization options.
708-761: Implementation Notes and Future Enhancements.
The final sections provide valuable context regarding the internal decisions (such as using Zap and lumberjack) and outline areas for future improvements (like advanced rotation strategies and multiple output destinations). This roadmap is beneficial for ongoing maintenance and evolution of the logging system.internal/logger/sensitive_test.go (2)
7-63: Well-structured comprehensive test for RedactSensitiveDataThis test function comprehensively covers various sensitive data patterns including empty strings, regular messages, and different types of sensitive data (bearer tokens, JWT tokens, API keys, passwords, session cookies, and CSRF tokens).
The table-driven test approach makes it easy to add additional test cases in the future. Each test case has a clear name and expected output.
65-125: Thorough test coverage for RedactSensitiveFieldsThe test for RedactSensitiveFields covers a good range of scenarios including empty slices, non-sensitive fields, various sensitive field types, mixed case handling, and non-string values.
The explicit length check before element comparison provides better error reporting, making test failures easier to diagnose.
internal/httpcontroller/middleware.go (4)
31-60: Good implementation of structured logging with proper contextThe new logger methods provide a clean abstraction for component-specific and request-specific logging. The request logger includes useful context like request ID, client IP, method, and path.
The UUID generation for request IDs ensures unique tracking across requests, and setting the header allows for consistent tracing throughout the request lifecycle.
88-109: Enhanced CSRF error logging with redaction safeguardsThe CSRF error logging now captures detailed context while ensuring sensitive data is properly redacted. The fallback for development mode includes additional safety by only logging non-sensitive parts of the request.
This approach provides valuable debugging information while maintaining security best practices.
140-154: Improved cache control middleware loggingThe middleware now uses structured logging with request-specific context, making it easier to trace and debug cache-related issues. The conditional logging based on development mode helps reduce noise in production while providing detailed information when needed.
222-255: Comprehensive authentication loggingThe auth middleware now provides detailed, contextual logging for authentication decisions. The inclusion of redirect paths and HTMX status in logs will make troubleshooting authentication issues much easier, especially in complex browser interactions.
internal/diskmanager/file_utils.go (1)
38-55: Good DiskManager struct design with proper logger handlingThe new
DiskManagerstruct and constructor are well designed. The constructor takes an optional parent logger and creates a properly named component logger, allowing for hierarchical logging.This pattern enables better organization of logs and proper context for all disk management operations.
internal/logger/sensitive.go (4)
9-23: Comprehensive regex patterns for sensitive data detectionThe
SensitiveDataPatternsarray contains well-crafted regular expressions for identifying common types of sensitive data including authentication tokens, API keys, and session information.The patterns are specific enough to match common formats while being flexible enough to catch variations, and the use of case-insensitive matching (
(?i)) is appropriate.
25-29: Complete list of sensitive keywords for field detectionThe
SensitiveKeywordslist covers all common names for sensitive fields. This comprehensive list ensures that fields containing credentials, tokens, and other sensitive information will be properly identified for redaction.
31-43: Efficient and safe sensitive data redaction functionThe
RedactSensitiveDatafunction handles empty inputs correctly and applies all regex patterns efficiently. The approach of preserving the prefix (like "Bearer " or "password=") while redacting only the sensitive part is user-friendly and maintains log readability.
45-71: Well-designed field redaction with defensive programmingThe
RedactSensitiveFieldsfunction shows good attention to detail:
- Creates a copy to avoid modifying the original data
- Handles key-value pairs properly
- Performs case-insensitive matching
- Only redacts string values
- Preserves non-string values unchanged
The implementation is robust and handles edge cases appropriately.
internal/diskmanager/policy_usage.go (3)
19-34: Good implementation of deprecation notice and early returnThe deprecation notice clearly guides users to the newer
DiskManager.UsageBasedCleanupmethod. The early check for usage policy is an effective way to avoid unnecessary processing.
43-46: Well-structured logging with contextual fieldsThe conversion to structured logging with named fields (like
base_dir,threshold,usage) provides much richer context than simple string messages, making logs more searchable and analyzable.Also applies to: 56-59, 80-83
228-294: Good encapsulation in the DiskManager structMoving the cleanup functionality into a method of the
DiskManagerstruct is a good design choice that:
- Maintains consistent access to the logger through
dm.Logger- Provides cleaner access to the database through
dm.DB- Improves code organization by grouping related functionality
The implementation maintains the same core logic while transitioning to the new logging system.
internal/diskmanager/policy_age.go (4)
122-167: Improved function organization with clear responsibilitiesThe new
DiskManager.AgeBasedCleanupmethod is well-structured with:
- Clear separation of configuration and execution logic
- Improved error handling with contextual logging
- Smart creation of a lookup map for locked clips
- Delegation to specialized methods for the actual processing
This is a good example of the single responsibility principle in action.
169-217: Well-factored batch processing logicBreaking out the file processing logic into a separate method with its own responsibility improves maintainability and readability. The method handles interruption signals cleanly and maintains proper state tracking.
219-277: Good separation of single-file processing logicThe
processFileForDeletionmethod:
- Has clear, focused responsibility for a single file
- Uses early returns effectively to handle skip conditions
- Provides detailed debug logging with context
- Returns a boolean indicating success and an error when needed
This is a good example of modular, maintainable code.
279-290: Effective extraction of utility functionMoving the species count map creation to a separate function improves code reuse and readability. This is a good application of the DRY principle.
internal/httpcontroller/server_logger.go (4)
14-26: Good adapter implementation for Echo's logging systemThe
echoLogAdaptereffectively bridges the gap between Echo's expectedio.Writerinterface and the custom logger. The implementation properly handles whitespace trimming and empty message filtering.
28-47: Well-designed debug logging with fallback mechanismThe
Debugmethod:
- Only logs when in development mode for efficiency
- Handles both formatted and unformatted messages correctly
- Falls back to standard logging when the custom logger is unavailable
- Properly handles variable arguments
This ensures logging works consistently throughout the application lifecycle.
49-70: Clean logger initialization with proper configurationThe
initLoggermethod checks if logging is enabled, detects development mode, and correctly configures the Echo logger with the custom adapter. The early return when logging is disabled is a good optimization.
72-165: Comprehensive HTTP request logging configurationThe request logging implementation is excellent with:
- Intelligent log level selection based on status codes
- Inclusion of essential request metadata (method, URI, latency)
- Conditional inclusion of detailed fields based on response status
- Proper extraction and inclusion of request IDs for correlation
- Smart handling of path parameters
This provides valuable observability without excessive logging.
internal/httpcontroller/server.go (5)
92-143: Excellent logger-aware constructor implementationThe
NewWithLoggerconstructor:
- Creates a proper hierarchical logger structure
- Sets default settings early
- Initializes handlers with a properly named child logger
- Logs server initialization with relevant context
This ensures consistent logging throughout the server's lifecycle.
227-232: Good logger initialization sequenceSetting the logger in handlers after server initialization ensures all components have access to the properly configured logger. This avoids potential null reference issues and maintains logging consistency.
259-269: Smart development mode detection with environment overrideThe
isDevModemethod properly checks environment variables first before falling back to configuration settings. This provides flexibility for runtime environment changes without configuration updates.
271-279: Improved error handling with structured loggingThe updated
handleServerErrormethod now uses the structured logger when available, ensuring consistent error reporting format throughout the application.
281-349: Well-organized logger hierarchy creationThe helper methods for creating component-specific loggers:
- Maintain a consistent hierarchical naming structure (e.g., "http.api.v1")
- Properly handle the case when no logger is available
- Add relevant context fields to request loggers
- Intelligently determine logger components based on request paths
This ensures logs are properly categorized and contain relevant context.
internal/logger/logger.go (6)
2-30: Great documentation block to introduce the logger package.This comprehensive overview helps developers quickly understand the package usage and features.
50-56: Redaction approach looks good.Using
RedactSensitiveDataandRedactSensitiveFieldsensures sensitive information is protected. Encourage thorough tests to verify that typical personally identifiable information and other secrets are appropriately redacted.
90-184: Ensure concurrent safety for file writes in multi-goroutine contexts.While
zapcores are generally safe for concurrent use, verify your usage patterns—particularly with file rotation across threads—to prevent potential race conditions.
186-193: Named loggers are helpful for grouping logs by component.This approach is beneficial for debugging microservices and complex systems. No issues spotted here.
204-207: Remember to call Sync() upon shutdown.
l.zap.Sync()ensures all buffers are flushed. Consider placing this in a deferred call in yourmain()function or top-level server code.
209-213: Returning the raw zap logger.Exposing the underlying zap logger is flexible but can lead to bypassing your redaction logic. Remind developers to remain consistent with your sanitization approach.
internal/diskmanager/README.md (1)
1-323: Well-structured documentation.This README thoroughly explains the disk manager's usage, configuration, and integration with other parts of the system. No major issues. One minor note: double-check for any repeated words or small typos flagged by tooling.
🧰 Tools
🪛 LanguageTool
[duplication] ~26-~26: Possible typo: you repeated a word.
Context: ...icy_age_test.go** - Tests for age-based policy - policy_usage_test.go - Tests for usage-based...(ENGLISH_WORD_REPEAT_RULE)
internal/logger/config.go (8)
1-2: File layout is clear.This file is introduced to hold all logger configuration logic. Keeping these configs distinct is a good design choice.
29-40: “DefaultConfig” might specify a log level.Leaving
Levelas an empty string can lead to a fallback. If that fallback isDebugLevelfor dev, clarify in the docs to avoid confusion.
55-74: RotationConfig is straightforward.No concerns. The structure covers all major rotation parameters. Ensure that validation for max values or negative values is handled upstream, if needed.
77-93: “createEncoderConfig” is well-structured.This function sets sensible defaults. The short caller encoding is helpful for local debugging. No concerns noted.
100-114: Zap options handle Dev mode well.Use
zap.Development()with caution, as it changes DPanic behavior. Document this to ensure devs understand the difference.
148-160: CreateEncoder usage.This function logically splits out JSON vs console logic. Looks good. Just note that if someone passes an empty file path but sets ForceJSONFile to true by accident, the code might create console logs in JSON format.
162-173: Graceful fallback for invalid log levels.The fallback to
defaultLevelis a nice approach. No issues.
175-197: DualLogConfig helps unify logging.Combining human-readable console output with structured JSON for files is a powerful approach. Nicely done.
internal/analysis/realtime.go (30)
15-23: Use of viper and logger imports
These new imports are consistent with the PR objective of integrating a structured logging system and reading config values from Viper.
32-35: Initialization oferrvariable
Defining and reusing theerrvariable within this scope is standard practice. No issues found here.
36-46: Logger configuration block
Usingviper.GetString("log.level")and other retrieved values to populate thelogger.Configstruct seems correct. Make sure these keys (log.level,log.disable_color, etc.) are documented and validated for correctness.
47-51: Initializing a new logger instance
The error handling and fallback mechanism are appropriate. Good job returning early on error.
53-55: Named logger usage
Usinglogger.Named("core")for the realtime analyzer is a good approach to differentiate logs from various components.
59-59: Error logging with context
Including the"error", errfield is consistent with structured logging best practices.
70-70: Consistent use of structured logging for errors
Good usage ofcoreLogger.Errorto log errors with context.
84-88: Logging system details
Populating fields likeos,platform,version, andhardwareis helpful for troubleshooting.
90-95: Logging analyzer configuration
Logging BirdNET threshold, overlap, and other configs is useful for diagnosing runtime configuration issues.
100-103: Hierarchical naming for datastore logger
This approach helps group datastore-related log messages. Make sure calls todataStoremethods produce meaningful logs.
111-111: Ensuring database closure
DeferringcloseDataStoreis a good pattern to avoid leaving connections open.
153-154: Metrics initialization error logging
Consistent with the rest of your error logging approach.
160-160: Passing logger toinitBirdImageCache
Providing a scoped logger for image caching helps keep logs organized.
173-176: HTTP server logging
Initializes the HTTP server with a structured logger. You might want to confirm if the server logs relevant startup errors or warnings.
193-195: Audio capture logger
Creating a separateaudioLoggeris a good way to isolate audio device logs. Verify whetherCaptureAudioitself will eventually accept this logger.
197-200: Clip cleanup monitor start
Providing a specialized logger for the cleanup routine helps with clarity.
202-205: Weather polling logger
A separate logger for weather polling is consistent with the modular logging strategy.
209-210: Telemetry endpoint logger
Great use of a named logger for telemetry. Properly logs the endpoint’s lifecycle events.
214-214: Control monitor logging
startControlMonitorwith a dedicated logger is consistent.
217-217: SIGINT monitor with logger
Nice improvement over silent termination logs. Helps with debugging shutdown events.
223-223: Shutdown log statements
Logging shutdown progress at key points is excellent for diagnosing graceful termination issues.Also applies to: 232-232
236-240: Restarting audio capture
Good use of separate log statements to indicate restarts and re-initialize capture.
245-254: Signature update forstartAudioCapture
Including a logger parameter fosters consistency and helps unify logs for audio capture.
257-269: EnhancedstartClipCleanupMonitor
Spawning adiskmanagerwith a named logger ensures disk operations are logged distinctly.
306-325: Weather polling routine signature
Passing the logger in ensures all weather service logs have consistent context fields.
327-348: Telemetry endpoint routine
Clean approach for toggling telemetry. Use of structured logging for both enabled and disabled states is clear.
350-354: Monitor Ctrl+C signature and log message
Properly logs Ctrl+C interrupts and signals downstream goroutines to exit.Also applies to: 358-358
364-369: Safe database closure with structured logs
This change helps identify any error during DB closure—good practice.
373-399:initBirdImageCachesignature and structured logging
Integrating the logger ensures full visibility into image cache initialization, including species fetch progress.
441-449: Control monitor injection
Passing a logger to the control monitor can provide better traceability if dynamic changes occur at runtime.internal/imageprovider/wikipedia.go (6)
16-17: Introducing logger field
Replacing the standard log with a*logger.Loggerfield aligns with the new unified logging system and ensures consistent, structured log output.Also applies to: 27-28
38-61:NewWikiMediaProviderwith parent logger
The hierarchical logging approach is effective, naming the loggerimageprovider.wikipedia. Rate limiting and client initialization also look correct.
68-72:queryWithRetrylogging
Emphasizing debug logs under adebugboolean is a good pattern—this avoids spamming logs in production.Also applies to: 86-91
168-234:Fetchmethod
Comprehensive logging at each step—thumbnail retrieval, error handling, author info. Fallback to an image without attribution if author data fails is a user-friendly design.
236-304:queryThumbnailmethod
Clearly logs each step in retrieving a thumbnail. The fallback logic for missing or invalid URLs is well-defined.
306-412:queryAuthorInfomethod
Structured logging of both success and failures. Returning partial results if the license or author fields are missing can improve user experience.
| diskLogger.Info("Sorted files written", "file_path", filePath) | ||
| return nil |
There was a problem hiding this comment.
Potential nil pointer dereference in info log
The diskLogger.Info() call doesn't check if diskLogger is nil before using it, which could cause a panic if the logger isn't initialized.
- diskLogger.Info("Sorted files written", "file_path", filePath)
+ if diskLogger != nil {
+ diskLogger.Info("Sorted files written", "file_path", filePath)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| diskLogger.Info("Sorted files written", "file_path", filePath) | |
| return nil | |
| if diskLogger != nil { | |
| diskLogger.Info("Sorted files written", "file_path", filePath) | |
| } | |
| return nil |
| diskLogger.Debug("Found protected clips", "count", len(lockedClips)) | ||
| } |
There was a problem hiding this comment.
Potential nil pointer dereference in debug log
The diskLogger.Debug() call doesn't check if diskLogger is nil before using it, which could cause a panic if the logger isn't initialized.
if debug {
- diskLogger.Debug("Found protected clips", "count", len(lockedClips))
+ if diskLogger != nil {
+ diskLogger.Debug("Found protected clips", "count", len(lockedClips))
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| diskLogger.Debug("Found protected clips", "count", len(lockedClips)) | |
| } | |
| if debug { | |
| if diskLogger != nil { | |
| diskLogger.Debug("Found protected clips", "count", len(lockedClips)) | |
| } | |
| } |
| // Initialize logger if it hasn't been initialized | ||
|
|
||
| var files []FileInfo |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Incomplete logger initialization comment
The comment "Initialize logger if it hasn't been initialized" suggests code should follow to initialize the logger, but no such initialization exists. This misleading comment should either be removed or the missing initialization code should be added.
- // Initialize logger if it hasn't been initialized
+ // Ensure logger is available
+ if diskLogger == nil {
+ diskLogger = logger.NewLogger()
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Initialize logger if it hasn't been initialized | |
| var files []FileInfo | |
| // Ensure logger is available | |
| if diskLogger == nil { | |
| diskLogger = logger.NewLogger() | |
| } | |
| var files []FileInfo |
| // diskLogger is a named logger for disk operations | ||
| var diskLogger *logger.Logger | ||
|
|
There was a problem hiding this comment.
Missing initialization for diskLogger global variable
The global diskLogger variable is declared but never initialized before being used. This could lead to nil pointer dereferences when methods are called on it.
Consider initializing the logger in an init() function or modifying functions like GetAudioFiles and WriteSortedFilesToFile to use the logger from the new DiskManager struct instead of the global variable.
+func init() {
+ diskLogger = logger.NewLogger()
+}Or, better yet, refactor the functions to be methods on the DiskManager struct to use its logger:
-func GetAudioFiles(baseDir string, allowedExts []string, db Interface, debug bool) ([]FileInfo, error) {
+func (dm *DiskManager) GetAudioFiles(baseDir string, allowedExts []string, debug bool) ([]FileInfo, error) {
// Use dm.Logger instead of diskLogger
// Use dm.DB instead of the db parameter📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // diskLogger is a named logger for disk operations | |
| var diskLogger *logger.Logger | |
| // diskLogger is a named logger for disk operations | |
| var diskLogger *logger.Logger | |
| func init() { | |
| diskLogger = logger.NewLogger() | |
| } |
This refactoring provides a more robust, performant, and flexible logging system for the application.