Migrate to Structured Logging with zerolog and Improve Logging for Manifest Purge Logic#473
Migrate to Structured Logging with zerolog and Improve Logging for Manifest Purge Logic#473
Conversation
Co-authored-by: estebanreyl <25871335+estebanreyl@users.noreply.github.com>
…purge logic Co-authored-by: estebanreyl <25871335+estebanreyl@users.noreply.github.com>
…t commands Co-authored-by: estebanreyl <25871335+estebanreyl@users.noreply.github.com>
…tation Co-authored-by: estebanreyl <25871335+estebanreyl@users.noreply.github.com>
estebanreyl
left a comment
There was a problem hiding this comment.
Some additional cleanup is necessary
cmd/acr/annotate.go
Outdated
| Str("repository", repoName). | ||
| Int("status_code", resultTags.StatusCode). | ||
| Msg("Repository not found during annotation operation") | ||
| fmt.Printf("%s repository not found\n", repoName) |
There was a problem hiding this comment.
all fmt.Printf messages should be removed as they are being replaced by the new structured logger
There was a problem hiding this comment.
Removed fmt.Printf statements from annotate.go as requested. Replaced with structured logging where appropriate. Changes in commit 42e878c.
internal/worker/annotator.go
Outdated
| ref := fmt.Sprintf("%s/%s@%s", a.loginURL, a.repoName, digest) | ||
| if err := a.orasClient.Annotate(ctx, ref, a.artifactType, a.annotations); err != nil { | ||
| // Keep fmt.Printf for user output consistency | ||
| fmt.Printf("Failed to annotate %s/%s@%s, error: %v\n", a.loginURL, a.repoName, digest, err) |
There was a problem hiding this comment.
Remove fmt.Print statements
There was a problem hiding this comment.
Removed fmt.Print statements from annotator.go as requested. Commit 42e878c.
cmd/acr/annotate.go
Outdated
| dryRun bool) (int, error) { | ||
|
|
||
| if !dryRun { | ||
| log := logger.Get() |
There was a problem hiding this comment.
I don't like this constant logger.Get() . The logger should be acquired once at the top of the function and then used as required.
There was a problem hiding this comment.
Fixed logger.Get() usage pattern - now acquiring logger once at function top instead of repeated calls. Changes in commit 42e878c.
| Str("repository", repoName). | ||
| Str("tag", *tag.Name). | ||
| Str("manifest", *tag.Digest). | ||
| Msg("Tag marked for annotation in dry run") |
There was a problem hiding this comment.
Should use Bool("dry_run", true)
There was a problem hiding this comment.
Fixed to use Bool("dry_run", true) instead of string format. Changes in commit 42e878c.
cmd/acr/manifest.go
Outdated
| log := logger.Get() | ||
|
|
||
| log.Debug(). | ||
| Str("repository", repoName). |
There was a problem hiding this comment.
A lot of these log fields like "repository" or "dry-run" are common throughout. Make those into constants in the internal logger package and reference them instead. Additionally if a particular field like repository is relevant to all logs in one function you can use .With on logger initialization to add the properties there and avoid adding them everytime through Str.
There was a problem hiding this comment.
Created constants for common log fields in internal/logger package and implemented .With() pattern for repository context to avoid repetition. Changes in commit 42e878c.
cmd/acr/manifest.go
Outdated
| lastManifestDigest = *manifests[len(manifests)-1].Digest | ||
| resultManifests, err = acrClient.GetAcrManifests(ctx, repoName, "", lastManifestDigest) | ||
| if err != nil { | ||
| log.Error(). |
There was a problem hiding this comment.
Errors are propagated through the errors.Wrap. There is no no need to log the error at every level. Additionally, where seen errors.wrap should be replaced with fmt.Errorf(%w).
There was a problem hiding this comment.
Replaced errors.Wrap with fmt.Errorf(%w) and removed redundant error logging at every level. Changes in commit 42e878c.
cmd/common/image_functions.go
Outdated
| Str("repository", repoName). | ||
| Str("manifest", *manifest.Digest). | ||
| Msg("Error checking manifest deletability") | ||
| return err |
There was a problem hiding this comment.
Where extra information for the error is helpful we can add a wrap through fmt.Errorsf instead of logging.
There was a problem hiding this comment.
Replaced error logging with fmt.Errorf wrapping to provide context without duplicate logging. Changes in commit 42e878c.
cmd/common/image_functions.go
Outdated
| Str("repository", repoName). | ||
| Str("manifest", *manifest.Digest). | ||
| Str("reason", "protected_by_dependencies"). | ||
| Msg("Manifest excluded from final deletion list - protected by dependencies") |
There was a problem hiding this comment.
This indicates the type is a dependency rather than it being protected by its dependencies. Either it's a referrer or a child of an index.
There was a problem hiding this comment.
Fixed log message to correctly indicate manifest "is a dependency" rather than "protected by dependencies". Changes in commit 42e878c.
|
|
||
| ## New Command Line Flags | ||
|
|
||
| ### `--log-level` |
There was a problem hiding this comment.
Flag information should be exposed on the usage portion of the main project readme. Its okay to link additional documentation like this but it should be referenced there.
There was a problem hiding this comment.
Added logging flags documentation to main README.md with usage examples and reference to detailed docs. Changes in commit 42e878c.
internal/worker/purger.go
Outdated
| var deletedManifests atomic.Int64 // Count of successfully deleted tags | ||
| log := logger.Get() | ||
|
|
||
| log.Debug(). |
There was a problem hiding this comment.
Changed log level from debug to info in purger.go. Changes in commit 42e878c.
…mize logger usage Co-authored-by: estebanreyl <25871335+estebanreyl@users.noreply.github.com>
Completed cleanup addressing all specific review feedback: removed fmt.Printf statements, optimized logger usage patterns, added constants for common fields, replaced errors.Wrap with fmt.Errorf, and updated documentation. Changes in commit 42e878c. Warning Firewall rules blocked me from connecting to one or more addressesI tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
This PR migrates the ACR CLI from
fmt.Printfto structured logging using zerolog, addressing performance issues with concurrent operations and providing enhanced observability for manifest purge operations.Changes Made
🚀 Core Infrastructure
internal/logger--log-level(debug, info, warn, error) - defaults toinfo--log-format(console, json) - defaults toconsolefor CLI usability🔍 Enhanced Manifest Purge Logging
The manifest purge logic now includes detailed structured logging explaining decision points:
Debug Level - Shows why each manifest is excluded:
{"level":"debug","repository":"myrepo","manifest":"sha256:abc...","reason":"has_tags","tag_count":3,"message":"Manifest excluded from purge - has remaining tags"} {"level":"debug","repository":"myrepo","manifest":"sha256:def...","reason":"delete_disabled","message":"Manifest excluded from purge - deletion disabled by attributes"}Info Level - Operation summaries:
{"level":"info","repository":"myrepo","candidate_count":15,"message":"Found candidate manifests for deletion"} {"level":"info","repository":"myrepo","deleted_count":5,"attempted_count":7,"message":"Successfully completed manifest purge operation"}Warn Level - 404 errors and skipped operations:
{"level":"warn","repository":"myrepo","manifest":"sha256:ghi...","status_code":404,"message":"Manifest not found during deletion, assuming already deleted"}📊 Structured Context
All log entries include relevant structured fields:
repository: Repository being processedmanifest/tag: Artifact identificationreason: Decision justification (e.g., "has_tags", "protected_by_dependencies")*_count: Operational metrics (deleted_count, attempted_count, etc.)status_code: HTTP response trackingdry_run: Operation mode indication⚡ Performance & Compatibility
fmt.Printfkept for user-facing messages)Usage Examples
Files Modified
cmd/acr/root.go- Added logging flags and setupcmd/acr/purge.go- Enhanced manifest purge loggingcmd/common/image_functions.go- Detailed manifest evaluation logginginternal/worker/purger.go- Structured deletion operationsinternal/worker/annotator.go- Annotation operationscmd/acr/annotate.go- Annotation workflowcmd/acr/manifest.go- Manifest operationsinternal/cssc/cssc.go- Filter resultsdocs/structured-logging.md- Comprehensive documentationBenefits
Fixes #470.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.