feat: change format logger based on logger from core#59
Conversation
Summary by CodeRabbit
WalkthroughRefactors logging from enum-based LoggerType to a Logger interface, adds Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Queue
participant Logger as Logger Interface
participant Redis
Client->>Queue: New(options)
activate Queue
alt options.Logger is nil
Queue->>Logger: create default logger
else
Queue->>Queue: assign provided Logger
end
note right of Queue: Respect Options.DisableLog
deactivate Queue
Client->>Queue: Enqueue / Process job
activate Queue
alt DisableLog == false
Queue->>Logger: Infof/Warnf/Errorf/Fatalf(msg, meta)
else
note over Queue: Logging skipped
end
alt job failure
Queue->>Redis: Set("<lowerqueue>:<jobID>", failureReason)
end
deactivate Queue
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
queue.go (3)
187-189: Don’t defer cancel inside loops (timer/goroutine leak).Create ctx per iteration and call cancel() after the select.
- ctx, cancel := context.WithTimeout(context.Background(), q.config.Timeout) - defer cancel() + ctx, cancel := context.WithTimeout(context.Background(), q.config.Timeout)Then add cancel() in both select branches (see below).
256-258: Same cancel leak in Retry loop.Avoid defer inside loop; call cancel after select.
- ctx, cancel := context.WithTimeout(context.Background(), q.config.Timeout) - defer cancel() + ctx, cancel := context.WithTimeout(context.Background(), q.config.Timeout)And add cancel() in both select branches (see below).
262-273: Data race on finishedJob; add synchronization and panic recovery.Appending to finishedJob from multiple goroutines races; also panics in Retry goroutines are unhandled and can crash the process.
- var finishedJob []string + var finishedJob []string + var fjMu sync.Mutex for i := range numJobs { job := numJobs[i] wg.Add(1) go func(job *Job) { defer wg.Done() - q.jobFnc(job) - if job.IsFinished() { - finishedJob = append(finishedJob, job.Id) - } + defer func() { + if r := recover(); r != nil { + failedReason := fmt.Sprintf("%v", r) + q.formatLog(LoggerError, "Error when retrying job: %v", failedReason) + job.HandlerError(failedReason) + } + }() + q.jobFnc(job) + if job.IsFinished() { + fjMu.Lock() + finishedJob = append(finishedJob, job.Id) + fjMu.Unlock() + } }(job) }Also add cancel() after the select:
- case <-done: - q.formatLog(LoggerInfo, "All jobs done when retry") + case <-done: + q.formatLog(LoggerInfo, "All jobs done when retry") + cancel() case <-ctx.Done(): q.MarkJobFailedTimeout(numJobs) + cancel()
🧹 Nitpick comments (6)
job.go (3)
73-73: Wording nit in progress log.Consider “Running job %s in progress” or simply “Running job %s”.
94-99: Use queue context for Redis write (and consider TTL).Avoid context.Background to honor queue cancellation/timeouts; set failure timestamp too if applicable.
Apply this diff:
- client := job.queue.client - _, err := client.Set(context.Background(), fmt.Sprintf("%s:%s", strings.ToLower(job.queue.Name), job.Id), job.FailedReason, 0).Result() + client := job.queue.client + _, err := client.Set(job.queue.ctx, fmt.Sprintf("%s:%s", strings.ToLower(job.queue.Name), job.Id), job.FailedReason, 0).Result()Optionally introduce an Options.ErrorTTL to expire failure keys rather than storing indefinitely.
90-106: Missing FinishedOn on failure; adjust severities.Set FinishedOn when a job fails; warn on retries; error on final failure.
func (job *Job) HandlerError(reasonError string) { job.FailedReason = reasonError job.Status = FailedStatus + job.FinishedOn = time.Now() @@ - if job.RetryFailures > 0 { + if job.RetryFailures > 0 { job.Status = DelayedStatus job.RetryFailures-- - job.queue.formatLog(LoggerInfo, "Add job %s for retry (%d remains) ", job.Id, job.RetryFailures) + job.queue.formatLog(LoggerWarn, "Add job %s for retry (%d remains)", job.Id, job.RetryFailures) } else { - job.queue.formatLog(LoggerInfo, "Failed job %s ", job.Id) + job.queue.formatLog(LoggerError, "Failed job %s", job.Id) } }constant.go (1)
6-12: Levels added are fine; consider deprecating LoggerDisabled.DisableLog now controls suppression; keeping LoggerDisabled may confuse users. Either document deprecation or remove in a follow-up.
queue.go (2)
67-71: Panic message clarity.“store missing config” is misleading; prefer “options is nil”.
- panic("store missing config") + panic("options is nil")
402-406: Don’t suppress fatal logs when DisableLog is true.If LoggerFatal is ever used, DisableLog should not block critical termination.
-func (q *Queue) formatLog(logType LoggerType, format string, v ...any) { - if q.config.DisableLog { - return - } - q.log(logType, format, v...) -} +func (q *Queue) formatLog(logType LoggerType, format string, v ...any) { + if q.config.DisableLog && logType != LoggerFatal { + return + } + q.log(logType, format, v...) +}If the library never intends to call Fatal, consider removing LoggerFatal entirely and returning errors instead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
logs/2025-10-20-info.logis excluded by!**/*.log
📒 Files selected for processing (5)
constant.go(1 hunks)job.go(4 hunks)logger.go(1 hunks)queue.go(15 hunks)queue_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
job.go (1)
constant.go (1)
LoggerInfo(7-7)
queue.go (2)
logger.go (1)
Logger(3-8)constant.go (5)
LoggerInfo(7-7)LoggerWarn(8-8)LoggerError(9-9)LoggerType(3-3)LoggerFatal(10-10)
🔇 Additional comments (4)
logger.go (1)
3-8: Interface shape looks good; please confirm external impl matches the method set.Ensure the logger returned by middleware/logger.Create implements Infof/Warnf/Errorf/Fatalf with identical signatures to avoid compile-time mismatch.
queue_test.go (1)
201-211: LGTM on switching to DisableLog.This aligns tests with the new logging control path.
queue.go (2)
84-86: Default logger init: verify compatibility and consider passing queue name.Ensure middleware/logger.Create returns a type satisfying queue.Logger; optionally include name in options for better tagging.
33-34: Remove unused Queue.Logger field.The Queue struct has a Logger field (line 33) that is never referenced. All logging consistently uses
q.config.Loggerinstead (lines 411, 413, 415, 417). This field is redundant with the Logger in the Options struct and should be removed to avoid confusion.
| _, err := client.Set(context.Background(), fmt.Sprintf("%s:%s", strings.ToLower(job.queue.Name), job.Id), job.FailedReason, 0).Result() | ||
| if err != nil { | ||
| job.queue.formatLog(LoggerInfo, "Failed to store error: %s\n\n", err.Error()) | ||
| job.queue.formatLog(LoggerInfo, "Failed to store error: %s", err.Error()) |
There was a problem hiding this comment.
Incorrect log level for storage failure.
Use error level, not info.
- job.queue.formatLog(LoggerInfo, "Failed to store error: %s", err.Error())
+ job.queue.formatLog(LoggerError, "Failed to store error: %s", err.Error())📝 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.
| job.queue.formatLog(LoggerInfo, "Failed to store error: %s", err.Error()) | |
| job.queue.formatLog(LoggerError, "Failed to store error: %s", err.Error()) |
🤖 Prompt for AI Agents
In job.go around line 97, the log call uses an info-level constant for a storage
failure; change the log level to the error-level constant (e.g., replace
LoggerInfo with LoggerError or the project's equivalent) so the failure is
logged as an error and retains the same message and error formatting.
| if r := recover(); r != nil { | ||
| failedReason := fmt.Sprintf("%v", r) | ||
| q.formatLog(LoggerInfo, "Error when processing job: %v\n", failedReason) | ||
| q.formatLog(LoggerInfo, "Error when processing job: %v", failedReason) | ||
| } | ||
| }() |
There was a problem hiding this comment.
Recover path does not mark job failed; severity should be error.
Jobs that panic before entering job.Process remain stuck as “ready”. Mark them failed and record the reason.
- defer func() {
- if r := recover(); r != nil {
- failedReason := fmt.Sprintf("%v", r)
- q.formatLog(LoggerInfo, "Error when processing job: %v", failedReason)
- }
- }()
+ defer func() {
+ if r := recover(); r != nil {
+ failedReason := fmt.Sprintf("%v", r)
+ q.formatLog(LoggerError, "Error when processing job: %v", failedReason)
+ job.HandlerError(failedReason)
+ }
+ }()| select { | ||
| case <-done: | ||
| q.formatLog(LoggerInfo, "All jobs done\n") | ||
| q.formatLog(LoggerInfo, "All jobs done") | ||
| case <-ctx.Done(): | ||
| q.MarkJobFailedTimeout(numJobs) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Call cancel() after select to release resources.
Add cancel() in both cases.
- case <-done:
- q.formatLog(LoggerInfo, "All jobs done")
+ case <-done:
+ q.formatLog(LoggerInfo, "All jobs done")
+ cancel()
case <-ctx.Done():
q.MarkJobFailedTimeout(numJobs)
+ cancel()📝 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.
| select { | |
| case <-done: | |
| q.formatLog(LoggerInfo, "All jobs done\n") | |
| q.formatLog(LoggerInfo, "All jobs done") | |
| case <-ctx.Done(): | |
| q.MarkJobFailedTimeout(numJobs) | |
| } | |
| select { | |
| case <-done: | |
| q.formatLog(LoggerInfo, "All jobs done") | |
| cancel() | |
| case <-ctx.Done(): | |
| q.MarkJobFailedTimeout(numJobs) | |
| cancel() | |
| } |
🤖 Prompt for AI Agents
In queue.go around lines 213 to 218, the select branches that handle <-done and
<-ctx.Done() currently don't call cancel(), leaking the cancel function's
resources; update both branches to call cancel() after q.formatLog(LoggerInfo,
"All jobs done") and after q.MarkJobFailedTimeout(numJobs) respectively so the
context cancel function is always invoked and resources are released.
ren0503
force-pushed
the
feat/ren/58-change-format-log
branch
from
9973852 to
ad5e91c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
queue.go (2)
199-203: Recover path does not mark job failed; severity should be error.This issue was flagged in a previous review. Jobs that panic before entering job.Process remain stuck as "ready" and the log level should be LoggerError, not LoggerInfo. Mark them failed and record the reason.
Apply this diff to fix:
defer func() { if r := recover(); r != nil { failedReason := fmt.Sprintf("%v", r) - q.formatLog(LoggerInfo, "Error when processing job: %v", failedReason) + q.formatLog(LoggerError, "Error when processing job: %v", failedReason) + job.HandlerError(failedReason) } }()
213-218: Call cancel() after select to release resources.This issue was flagged in a previous review. The cancel() function is not called in either select branch, causing a resource leak.
Apply this diff to fix:
select { case <-done: q.formatLog(LoggerInfo, "All jobs done") + cancel() case <-ctx.Done(): q.MarkJobFailedTimeout(numJobs) + cancel() }
🧹 Nitpick comments (1)
logger.go (1)
1-8: LGTM! Consider adding context support.The Logger interface is clean and follows common patterns. All methods use consistent signatures with formatted message strings.
For enhanced observability, consider adding context.Context as the first parameter to enable request tracing and cancellation propagation:
type Logger interface { - Infof(msg string, metadata ...any) - Warnf(msg string, metadata ...any) - Errorf(msg string, metadata ...any) - Fatalf(msg string, metadata ...any) + Infof(ctx context.Context, msg string, metadata ...any) + Warnf(ctx context.Context, msg string, metadata ...any) + Errorf(ctx context.Context, msg string, metadata ...any) + Fatalf(ctx context.Context, msg string, metadata ...any) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.gitignore(1 hunks)constant.go(1 hunks)job.go(2 hunks)logger.go(1 hunks)processor_test.go(3 hunks)queue.go(15 hunks)queue_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (2)
- job.go
- constant.go
🧰 Additional context used
🧬 Code graph analysis (2)
processor_test.go (1)
module.go (1)
Inject(88-95)
queue.go (2)
logger.go (1)
Logger(3-8)constant.go (5)
LoggerInfo(7-7)LoggerWarn(8-8)LoggerError(9-9)LoggerType(3-3)LoggerFatal(10-10)
🔇 Additional comments (8)
processor_test.go (2)
58-79: LGTM! Typo fix.The renaming from
appMouletoappModulecorrects the typo and both references are updated consistently.
93-99: LGTM! Appropriate nil-case test.The test correctly validates that
queue.Injectreturns nil when the requested queue is not registered in the module.queue_test.go (1)
210-210: LGTM! Clearer API with DisableLog boolean.The change from a Logger enum field to a
DisableLogboolean flag makes the API more intuitive and aligns with the broader logger refactoring.queue.go (5)
17-17: LGTM! Logger integration from core.The import and public Logger field appropriately integrate the external logger from the core package, aligning with the PR objectives.
Also applies to: 33-33
47-48: LGTM! Enhanced logging configuration.The addition of Logger and DisableLog fields provides both flexibility (custom logger) and simplicity (disable flag), improving the Options API.
67-69: LGTM! Good defensive check.The nil check provides a clear, early failure message instead of an obscure nil pointer dereference later.
84-86: LGTM! Proper default logger initialization.The initialization correctly provides a default logger when none is specified, using the external logger package.
401-418: LGTM! Proper logger interface integration.The formatLog and log methods correctly implement the new Logger interface, with appropriate DisableLog checking and proper routing of log levels to interface methods.
ren0503
force-pushed
the
feat/ren/58-change-format-log
branch
from
ad5e91c to
0fcb89a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
queue.go (2)
213-218: Explicit cancel() calls still missing (unresolved from previous review).While the
defer cancel()at line 188 will eventually release resources, the previous review comment recommended explicitly callingcancel()in both select branches to release resources sooner and make the intent clearer.Apply the previously suggested improvement:
select { case <-done: q.formatLog(LoggerInfo, "All jobs done") + cancel() case <-ctx.Done(): q.MarkJobFailedTimeout(numJobs) + cancel() }
199-203: Panic recovery still doesn't mark job as failed (unresolved from previous review).The previous review comment on these lines remains unaddressed:
- Log level is still
LoggerInfoinstead ofLoggerErrorjob.HandlerError(failedReason)is not called, so panicked jobs remain in an inconsistent state rather than being marked as failedApply the previously suggested diff:
defer func() { if r := recover(); r != nil { failedReason := fmt.Sprintf("%v", r) - q.formatLog(LoggerInfo, "Error when processing job: %v", failedReason) + q.formatLog(LoggerError, "Error when processing job: %v", failedReason) + job.HandlerError(failedReason) } }()
🧹 Nitpick comments (1)
queue.go (1)
280-285: Consider explicit cancel() calls for consistency.Similar to the select statement in
Run(), explicitly callingcancel()in both branches would release resources sooner and align with best practices.Apply this diff:
select { case <-done: q.formatLog(LoggerInfo, "All jobs done when retry") + cancel() case <-ctx.Done(): q.MarkJobFailedTimeout(numJobs) + cancel() }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.gitignore(1 hunks)constant.go(1 hunks)job.go(2 hunks)logger.go(1 hunks)processor_test.go(3 hunks)queue.go(15 hunks)queue_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- logger.go
- constant.go
🧰 Additional context used
🧬 Code graph analysis (4)
job.go (1)
constant.go (4)
LoggerInfo(7-7)LoggerFatal(10-10)LoggerWarn(8-8)LoggerError(9-9)
processor_test.go (1)
module.go (1)
Inject(88-95)
queue_test.go (1)
queue.go (1)
New(66-97)
queue.go (2)
logger.go (1)
Logger(3-8)constant.go (5)
LoggerInfo(7-7)LoggerError(9-9)LoggerWarn(8-8)LoggerType(3-3)LoggerFatal(10-10)
🔇 Additional comments (14)
.gitignore (1)
5-6: LGTM: Appropriate ignore rules for logging artifacts.Adding
logs/aligns with the PR's introduction of external logger integration.processor_test.go (2)
58-79: LGTM: Typo fix improves code quality.Correcting
appMouletoappModuleenhances readability.
93-99: LGTM: Good edge case coverage.The test verifies that
Injectcorrectly returnsnilwhen the queue is not found in the module.job.go (3)
6-6: LGTM: Import required for lowercase conversion.The
stringspackage is used at line 98 forstrings.ToLower.
70-91: LGTM: Control flow is correct.The early return after error handling is appropriate, and the removal of trailing newlines aligns with the new logger API.
98-98: The original review comment is based on incorrect assumptions and should be disregarded.The concerns raised assume that:
- There's prior HSet-based error storage to migrate from - not found (no HSet operations exist in the codebase)
- Error data is read back by the application - not found (FailedReason is only written to Redis, never read)
- This represents a breaking change for existing deployments - not applicable
The
client.Set()operation at line 98 stores error information with a consistent key format (<queue>:<id>), but this is write-only within the application. No code retrieves these error values from Redis. This suggests the Redis storage is either unused, reserved for external monitoring systems, or incomplete implementation—but it's not a breaking change affecting existing error data retrieval.Likely an incorrect or invalid review comment.
queue_test.go (3)
210-210: LGTM: Aligns with new DisableLog API.The change from
Loggerfield toDisableLogboolean is consistent with the PR's refactoring of the logging interface.
226-250: LGTM: Test uses default logger configuration.The test now uses the default logger (created via
logger.Create) instead of a custom logger value, which aligns with the new Logger interface approach.
471-476: LGTM: Good validation of nil config handling.The test appropriately verifies that passing
niloptions toqueue.Newresults in a panic, which aligns with the check added at lines 67-69 in queue.go.queue.go (5)
17-17: LGTM: Logger interface integration.The addition of the external
Loggerinterface andDisableLogflag provides flexible logging configuration.Also applies to: 33-33, 47-48
67-69: LGTM: Nil check prevents runtime errors.The panic on
niloptions prevents cryptic errors downstream and is validated byTestPanicin queue_test.go.
84-86: LGTM: Default logger initialization.Creating a default logger via
logger.Createwhen none is provided ensures logging is always available.
105-105: LGTM: Trailing newline removal aligns with new logger API.The logger interface methods (Infof, Warnf, etc.) handle formatting and newlines internally, so removing trailing newlines from the format strings is correct.
Also applies to: 108-108, 130-130, 133-133, 150-150, 162-162, 167-167, 215-215, 229-229, 282-282, 354-354
401-418: LGTM: Logger interface integration is correct.The
formatLogmethod correctly checksDisableLogand routes log calls through the appropriateLoggerinterface methods based on severity.
| _, err := client.Set(context.Background(), fmt.Sprintf("%s:%s", strings.ToLower(job.queue.Name), job.Id), job.FailedReason, 0).Result() | ||
| if err != nil { | ||
| job.queue.formatLog(LoggerInfo, "Failed to store error: %s\n\n", err.Error()) | ||
| job.queue.formatLog(LoggerFatal, "Failed to store error: %s", err.Error()) |
There was a problem hiding this comment.
Log level may be too severe for storage failure.
Using LoggerFatal for a Redis storage failure could be overly harsh. While the failure to persist error details is serious, it may not warrant a fatal-level log that typically implies program termination. Consider using LoggerError instead, which was suggested in a previous review.
Apply this diff to use error level:
- job.queue.formatLog(LoggerFatal, "Failed to store error: %s", err.Error())
+ job.queue.formatLog(LoggerError, "Failed to store error: %s", err.Error())📝 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.
| job.queue.formatLog(LoggerFatal, "Failed to store error: %s", err.Error()) | |
| job.queue.formatLog(LoggerError, "Failed to store error: %s", err.Error()) |
🤖 Prompt for AI Agents
In job.go around line 100, the log call using LoggerFatal for a Redis storage
failure should be changed to use LoggerError; replace LoggerFatal with
LoggerError in the job.queue.formatLog(...) call so the failure is logged at
error level (consistent with the prior review suggestion).
2 participants
No description provided.