add additional debug logs to cli #1942
Conversation
WalkthroughThe changes systematically migrate the logging mechanism across the codebase from the standard Changes
Sequence Diagram(s)sequenceDiagram
participant Env as Environment
participant App as Application
participant Logger as slog.Logger
Env->>App: Set DIGGER_LOG_LEVEL (optional)
App->>Logger: initLogger() on startup
Logger-->>App: Configured logger (level: INFO or DEBUG)
App->>Logger: slog.Info/Warn/Error(key-value pairs)
Logger-->>App: Structured log output
Poem
✨ Finishing Touches
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. 🪧 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 (
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR migrates the Digger CLI from standard logging to structured logging using slog, enabling configurable log levels and improved observability.
- Introduces
DIGGER_LOG_LEVELenvironment variable indefault.goto control logging verbosity (DEBUG/INFO) - Inconsistent error level usage across files - some errors logged at ERROR when they should be INFO/WARN
- Potential duplicate error messages where errors are both logged and returned in
digger.go - Some functions still mix
slogand standardlogpackage usage, particularly inMergePullRequest - Missing error context fields in several
slogcalls that could benefit from additional structured data
9 file(s) reviewed, 9 comment(s)
Edit PR Review Bot Settings | Greptile
cli/cmd/digger/default.go
Outdated
| @@ -65,10 +81,10 @@ var defaultCmd = &cobra.Command{ | |||
|
|
|||
| defer func() { | |||
| if r := recover(); r != nil { | |||
| log.Println(fmt.Sprintf("stacktrace from panic: \n" + string(debug.Stack()))) | |||
| slog.Error(fmt.Sprintf("stacktrace from panic" + string(debug.Stack()))) | |||
There was a problem hiding this comment.
syntax: String concatenation missing space after 'panic' - should be 'stacktrace from panic: '
cli/pkg/digger/digger.go
Outdated
| } | ||
| } | ||
| return msg | ||
| } | ||
|
|
||
| func run(command string, job orchestrator.Job, policyChecker policy.Checker, orgService ci.OrgService, SCMOrganisation string, SCMrepository string, PRNumber *int, requestedBy string, reporter reporting.Reporter, lock locking2.Lock, prService ci.PullRequestService, projectNamespace string, workingDir string, planStorage storage.PlanStorage, appliesPerProject map[string]bool) (*execution.DiggerExecutorResult, string, error) { | ||
| log.Printf("Running '%s' for project '%s' (workflow: %s)\n", command, job.ProjectName, job.ProjectWorkflow) | ||
| slog.Error("Running command for project", "command", command, "project name", job.ProjectName, "project workflow", job.ProjectWorkflow) |
There was a problem hiding this comment.
logic: Using slog.Error for a routine status message. Should be slog.Info since this is not an error condition
| slog.Error("Running command for project", "command", command, "project name", job.ProjectName, "project workflow", job.ProjectWorkflow) | |
| slog.Info("Running command for project", "command", command, "project name", job.ProjectName, "project workflow", job.ProjectWorkflow) |
cli/pkg/digger/digger.go
Outdated
| if !policyEnabled { | ||
| msg := "skipping this drift application since it is not enabled for this project" | ||
| log.Printf(msg) | ||
| slog.Error(msg) | ||
| return msg, nil |
There was a problem hiding this comment.
logic: Using slog.Error for a normal policy check result. Should be slog.Info since this is expected behavior
| if !policyEnabled { | |
| msg := "skipping this drift application since it is not enabled for this project" | |
| log.Printf(msg) | |
| slog.Error(msg) | |
| return msg, nil | |
| if !policyEnabled { | |
| msg := "skipping this drift application since it is not enabled for this project" | |
| slog.Info(msg) | |
| return msg, nil |
| allAppliesSuccess, _, err := digger.RunJobs(jobs, prService, orgService, lock, reporter, planStorage, policyChecker, commentUpdater, noopBackendApi, spec.JobId, false, false, commentId, currentDir) | ||
| log.Printf("<========= DIGGER COMPLETED =========>") | ||
| slog.Info("<========= DIGGER COMPLETED =========>") | ||
| if err != nil || allAppliesSuccess == false { |
There was a problem hiding this comment.
style: redundant comparison with 'false' - use '!allAppliesSuccess' instead
| currentDir, err := os.Getwd() | ||
| if err != nil { | ||
| log.Printf("Failed to get current dir. %s", err) | ||
| slog.Error("Failed to get current dir", "error", err) | ||
| } |
There was a problem hiding this comment.
style: Error is logged but execution continues. This could lead to undefined behavior if currentDir is needed later
cli/pkg/github/github.go
Outdated
| slog.Error("error while marshalling yaml", "error", err) | ||
| } |
There was a problem hiding this comment.
logic: This error should be fatal since yaml marshaling failure indicates a serious configuration issue
| slog.Error("error while marshalling yaml", "error", err) | |
| } | |
| usage.ReportErrorAndExit(githubActor, fmt.Sprintf("Failed to marshal digger config: %v", err), 4) | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
cli/pkg/digger/digger.go (2)
603-604:⚠️ Potential issueMissed conversion from log.Fatalf to slog.Error
This is still using the standard log package instead of structured logging.
- log.Fatalf("failed to fetch AWS keys, %v", err) + slog.Error("failed to fetch AWS keys", "error", err) + os.Exit(1)
803-834:⚠️ Potential issueMultiple instances of standard logging not converted to slog
There are several unconverted
log.Fatalfcalls in the MergePullRequest function.These log.Fatalf calls should be converted to slog.Error followed by os.Exit(1) for consistency with the rest of the codebase. For example:
- log.Fatalf("error checking if PR is merged: %v", err) + slog.Error("error checking if PR is merged", "error", err) + os.Exit(1)The same pattern should be applied to all log.Fatalf calls in this function (lines 803, 810, 814, 820, 824, 829).
🧹 Nitpick comments (6)
cli/pkg/spec/spec.go (2)
19-19: Simple error logging in reportError functionThe log has been updated to use
slog.Errorwhich is appropriate for error messages. Consider adding structured fields to provide more context.-slog.Error(message) +slog.Error(message, "error", err)
55-55: Inconsistent field naming in structured logsWhile both logs correctly use structured formatting, there's an inconsistency in the field naming convention between "commitId" and "commitID".
Choose one consistent style for field names throughout the codebase. For example:
-slog.Info("fetching commit ID", "commitId", spec.Job.Commit) +slog.Info("fetching commit ID", "commitID", spec.Job.Commit)Also applies to: 66-66
cli/pkg/usage/usage.go (1)
112-112: Structured error logging in ReportErrorAndExit functionThe first log (line 112) could be enhanced with structured fields, but the second log (line 115) correctly uses structured format with error field.
Consider adding more context to the first error log:
-slog.Error(message) +slog.Error("Error reported", "message", message, "exitCode", exitCode)Also applies to: 115-115
cli/pkg/github/github.go (3)
28-41: Well-structured logger initializationThe
initLoggerfunction correctly sets up structured logging based on the environment variable DIGGER_LOG_LEVEL.Consider moving this identical logger initialization function to a common utility package since it's duplicated in default.go, to maintain DRY principles.
-func initLogger() { - logLevel := os.Getenv("DIGGER_LOG_LEVEL") - var level slog.Leveler - if logLevel == "DEBUG" { - level = slog.LevelDebug - } else { - level = slog.LevelInfo - } - logger := slog.New(slog.NewTextHandler(os.Stdout, &slog.HandlerOptions{ - Level: level, - })) - - slog.SetDefault(logger) -}
115-117: Simplify boolean expressionThe boolean comparison can be simplified for better readability.
- if diggerConfig.PrLocks == false { + if !diggerConfig.PrLocks { slog.Info("Using noop lock as configured in digger.yml") lock = core_locking.NoOpLock{} }🧰 Tools
🪛 golangci-lint (1.64.8)
115-115: S1002: should omit comparison to bool constant, can be simplified to
!diggerConfig.PrLocks(gosimple)
349-350: Minor: TODO comment should indicate what to improveThe TODO comment about improving the error message doesn't specify what aspects need improvement.
Consider adding more detail to the TODO comment to indicate what specific improvements are needed for the error message.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
cli/cmd/digger/default.go(3 hunks)cli/cmd/digger/main.go(0 hunks)cli/cmd/digger/root.go(2 hunks)cli/pkg/digger/digger.go(30 hunks)cli/pkg/drift/slack.go(2 hunks)cli/pkg/github/github.go(10 hunks)cli/pkg/spec/manual.go(4 hunks)cli/pkg/spec/spec.go(4 hunks)cli/pkg/usage/usage.go(5 hunks)
💤 Files with no reviewable changes (1)
- cli/cmd/digger/main.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
cli/pkg/github/github.go (6)
libs/policy/core.go (1)
PolicyCheckerProvider(21-23)libs/backendapi/backend.go (1)
Api(9-15)libs/ci/github/github.go (1)
GithubServiceProvider(21-23)libs/comment_utils/summary/provider.go (1)
CommentUpdaterProvider(8-10)cli/pkg/drift/Provider.go (1)
DriftNotificationProvider(10-12)libs/locking/locking.go (1)
NoOpLock(36-37)
🪛 golangci-lint (1.64.8)
cli/pkg/github/github.go
115-115: S1002: should omit comparison to bool constant, can be simplified to !diggerConfig.PrLocks
(gosimple)
cli/pkg/digger/digger.go
213-213: slog: slog.Error arg "err" should be a string or a slog.Attr (possible missing key or value)
(govet)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
🔇 Additional comments (23)
cli/pkg/spec/manual.go (6)
15-15: Package import updated to use structured loggingThe import change from standard
logtolog/slogaligns with the migration to structured logging throughout the codebase.
69-69: Improved error logging with structured formatGood conversion from
log.Printfto structured logging withslog.Error, properly separating the message from the error details.
81-81: Structured error logging for zip extractionWell-formatted structured log entry with clear message and error field.
89-89: Structured error logging for file removalGood implementation of structured logging for file removal errors.
96-96: Structured error logging for directory changeProperly implemented structured error logging.
138-138: Informational logs updated to structured formatThe start and completion markers have been correctly updated to use
slog.Info()which is the appropriate log level for these informational messages.Also applies to: 140-140
cli/pkg/spec/spec.go (2)
12-12: Package import updated to use structured loggingThe import change from standard
logtolog/slogaligns with the migration to structured logging throughout the codebase.
42-42: Properly structured error loggingGood implementation of structured logging with a clear message and error field.
cli/pkg/usage/usage.go (4)
9-9: Package import updated to use structured loggingThe import change from standard
logtolog/slogaligns with the migration to structured logging throughout the codebase.
58-58: Structured error logging for JSON marshallingGood implementation of structured logging for JSON marshalling errors.
67-70: Warning level used appropriately for telemetry issuesThe log level has been properly changed to
slog.Warnfor telemetry issues, which is appropriate since this is a non-critical warning. The structured format with the error field is well-implemented.
79-79: Structured error logging in init functionGood implementation of structured logging for directory retrieval errors.
cli/pkg/drift/slack.go (3)
8-8: Package import updated to use structured loggingThe import change from standard
logtolog/slogaligns with the migration to structured logging throughout the codebase.
79-79: Added structured logging for non-200 status codesGood addition of structured logging for HTTP status code errors, providing better context with status code and response body fields.
57-58:✅ Verification successful
Simplified error returns with structured logging
The error logging has been updated to use structured format, which is good. However, the error returns now just return the original error instead of wrapped errors with context. This may lose some context for error handling upstream.
Verify that the error handling at the call sites doesn't rely on the specific error messages that were previously returned. If the calling code checks for specific error messages, this change could break that logic.
Also applies to: 63-64, 70-71, 76-77
🏁 Script executed:
#!/bin/bash # Check for error handling patterns that might rely on specific error messages rg -A 3 -B 3 "SlackNotification.*Send" --type goLength of output: 486
No upstream error-handling relies on Slack error strings
I searched the repo for any calls to
SlackNotification.Sendand for usages oferr.Error()surrounding those calls and found none outside ofcli/pkg/drift/slack.go. There’s no code inspecting or matching on the returned error message, so returning the originalerrwithout additional wrapping will not break any existing logic.No changes required.
cli/cmd/digger/root.go (3)
111-112: Successful migration to structured logging using slogThe warning log has been properly converted from the standard
log.Printfto the structuredslog.Warn, which is more appropriate for warnings.
116-116: Good use of structured key-value loggingThis properly uses the structured
slog.Errorwith a descriptive message and "error" as the key for the error value, which makes logs easier to parse and analyze.
119-119: Simple and correct logging conversionThe informational log has been properly converted from
log.Printlntoslog.Info.cli/cmd/digger/default.go (4)
21-35: Well-structured logger initializationThe
initLoggerfunction is a good approach for centralized logging configuration. It correctly sets up the log level based on the environment variable.
40-40: Good placement of logger initializationCalling
initLogger()at the beginning of the command's Run function ensures proper logging configuration before any operation.
84-84: Proper conversion of panic loggingThe panic stacktrace logging has been correctly upgraded to use structured logging.
87-87: Good use of structured error loggingThe error log for failed log record sending properly uses structured key-value pairs with "error" as the key.
cli/pkg/digger/digger.go (1)
72-74: Good use of Debug level loggingAppropriate use of
slog.Debugfor variable information that should only be visible in debug mode.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
…q/digger into feat/cli-debug-logs-configurable
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
cli/pkg/github/github.go (2)
249-250: Fmt.Sprintf is redundant here since slog.Info already handles formatting.Use slog.Info with key-value pairs instead for better structured logging.
- slog.Info(fmt.Sprintf("- %s\n", p.Name)) + slog.Info("Impacted project", "name", p.Name)
122-124: 🛠️ Refactor suggestionUse ReportErrorAndExit for fatal errors.
For consistency with the rest of the codebase, fatal errors should use the ReportErrorAndExit function instead of manual logging and exit.
- slog.Error("error while marshalling yaml", "error", err) - os.Exit(1) + usage.ReportErrorAndExit(githubActor, fmt.Sprintf("Failed to marshal digger config: %v", err), 4)
🧹 Nitpick comments (3)
cli/pkg/github/github.go (3)
115-118: Simplify boolean comparison.The comparison with a boolean constant can be simplified.
- if diggerConfig.PrLocks == false { + if !diggerConfig.PrLocks { slog.Info("Using noop lock as configured in digger.yml") lock = core_locking.NoOpLock{} }🧰 Tools
🪛 golangci-lint (1.64.8)
115-115: S1002: should omit comparison to bool constant, can be simplified to
!diggerConfig.PrLocks(gosimple)
350-351: Clarify the TODO comment.The TODO comment lacks specific details about what needs improvement in the error message. Adding more context would be helpful for future developers.
- // TODO: improve the error message + // TODO: improve the log message with structured key-value pairs instead of string concatenation slog.Info(logMessage)
351-352: Refactor logCommands to use structured logging.The current implementation builds a single log message with concatenation. This could be improved by using structured logging with key-value pairs.
func logCommands(projectCommands []scheduler.Job) { - logMessage := fmt.Sprintf("Following commands are going to be executed:\n") - for _, pc := range projectCommands { - logMessage += fmt.Sprintf("project: %s: commands: ", pc.ProjectName) - for _, c := range pc.Commands { - logMessage += fmt.Sprintf("\"%s\", ", c) - } - logMessage += "\n" - } - // TODO: improve the error message - slog.Info(logMessage) + slog.Info("Commands to be executed") + for _, pc := range projectCommands { + slog.Info("Project commands", "project", pc.ProjectName, "commands", pc.Commands) + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cli/cmd/digger/default.go(3 hunks)cli/pkg/digger/digger.go(30 hunks)cli/pkg/github/github.go(10 hunks)libs/scheduler/aws.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- libs/scheduler/aws.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cli/cmd/digger/default.go
- cli/pkg/digger/digger.go
🧰 Additional context used
🪛 golangci-lint (1.64.8)
cli/pkg/github/github.go
115-115: S1002: should omit comparison to bool constant, can be simplified to !diggerConfig.PrLocks
(gosimple)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Build
- GitHub Check: binary (arm, linux)
- GitHub Check: Build
- GitHub Check: build-and-push-image
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
🔇 Additional comments (1)
cli/pkg/github/github.go (1)
28-41: Well-implemented logger initialization function.The initLogger function properly configures logging levels based on the DIGGER_LOG_LEVEL environment variable, allowing for debug-level logs when needed. This provides great flexibility for troubleshooting while maintaining clean logs in normal operation.
Completion of this issue for cli functionality #1923
Also added env variable
DIGGER_LOG_LEVELwhich if set to "DEBUG" will enable debug logsSummary by CodeRabbit