feat: log registry store operations - #2901
Open
charleswool wants to merge 1 commit into
Open
Conversation
internal/store/registrystore only builds a ratify-go RegistryStore, and that library is deliberately dependency-free and does not log, so every registry round trip the executor makes is invisible: a slow ListReferrers, a manifest fetch that 404s or a blob pull that times out leaves no trace on the server. ratify.Store is an interface and the factory already returns it, so wrap the constructed store in a decorator rather than changing the dependency. Each operation now logs at debug on success with the digest, size and duration, and at error on failure. The referrer count is accumulated across pagination callbacks so it reflects the whole listing. Logging goes through internal/logger, so entries carry the request trace ID and component-type=referrerStore. That correlation is the reason this belongs here rather than upstream. Signed-off-by: Charles Wu <yuewu2@microsoft.com>
charleswool
requested review from
akashsinghal,
binbin-li,
fseldow,
jimmyraywv,
luisdlp,
susanshi and
toddysm
as code owners
August 11, 2026 00:51
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2901 +/- ##
==========================================
+ Coverage 77.10% 77.31% +0.21%
==========================================
Files 90 91 +1
Lines 4315 4355 +40
==========================================
+ Hits 3327 3367 +40
Misses 831 831
Partials 157 157 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a ratify.Store decorator in internal/store/registrystore so every registry round-trip (resolve, list referrers, fetch blob, fetch manifest) is logged through internal/logger, improving observability on the verification path without changing the upstream ratify-go dependency.
Changes:
- Wrap the
ratify-goRegistryStorewith aloggingStoredecorator returned from the registrystore factory. - Log success (debug) and failure (error) for
Resolve,ListReferrers,FetchBlob, andFetchManifest, including durations and basic identifiers. - Add unit tests covering success/failure paths and pagination referrer counting.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| internal/store/registrystore/register.go | Wraps the created RegistryStore with a logging decorator. |
| internal/store/registrystore/logging.go | Introduces the loggingStore decorator that logs each store operation. |
| internal/store/registrystore/logging_test.go | Adds tests validating emitted log messages and pagination referrer counting. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+50
to
+60
| start := time.Now() | ||
| // ListReferrers paginates, so the total is only known once fn stops being called. | ||
| count := 0 | ||
| err := s.inner.ListReferrers(ctx, ref, artifactTypes, func(referrers []ocispec.Descriptor) error { | ||
| count += len(referrers) | ||
| return fn(referrers) | ||
| }) | ||
| if err != nil { | ||
| log.Errorf("failed to list referrers for %s after %dms: %v", ref, time.Since(start).Milliseconds(), err) | ||
| return err | ||
| } |
Comment on lines
+69
to
+72
| if err != nil { | ||
| log.Errorf("failed to fetch blob %s from %s after %dms: %v", desc.Digest, repo, time.Since(start).Milliseconds(), err) | ||
| return nil, err | ||
| } |
Comment on lines
+81
to
+84
| if err != nil { | ||
| log.Errorf("failed to fetch manifest %s from %s after %dms: %v", desc.Digest, repo, time.Since(start).Milliseconds(), err) | ||
| return nil, err | ||
| } |
Comment on lines
+72
to
+83
| func newHook(t *testing.T) *test.Hook { | ||
| t.Helper() | ||
| base := logrus.StandardLogger() | ||
| previous := base.GetLevel() | ||
| base.SetLevel(logrus.DebugLevel) | ||
| hook := test.NewLocal(base) | ||
| t.Cleanup(func() { | ||
| base.SetLevel(previous) | ||
| hook.Reset() | ||
| }) | ||
| return hook | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Closes the last in-repo logging blind spot on the verification path: every registry round trip is currently invisible.
internal/store/registrystoreonly builds aratify-goRegistryStore, and that library is deliberately dependency-free — [COMPUTED] it has zero log calls in the entire module, which is reasonable for a library. The consequence is that a slowListReferrers, a manifest fetch that 404s, or a blob pull that times out leaves no trace on the server at all. The operator sees only the final "verification failed" with no idea which registry call was responsible.Note
In #2877 I claimed this "cannot be added from this repo" and would need an upstream issue. That was wrong, and I've posted a correction there.
ratify.Storeis an interface, and our factory already returns it, so the seam is local. No upstream change is needed.Change
ratify.Storehas four methods, andinternal/store/registrystorealready returns that interface from its factory:So the store is wrapped in a decorator rather than changing the dependency:
Each operation now logs:
debugon success — the resolved digest, the referrer count, the blob/manifest digest with media type and size, and the duration in ms.erroron failure — the reference or digest, the duration, and the error.ListReferrerspaginates, so the referrer count is accumulated across every callback rather than taken from the first page.Logging goes through
internal/logger, so entries carry the request trace ID andcomponent-type=referrerStore. That correlation is the main reason this belongs here rather than upstream:ratify-gohas no concept of the request context Ratify attaches.Example output that previously did not exist at all:
No credentials, tokens or blob contents are logged — only references, digests, media types, sizes and durations.
Testing
logging_test.godrives every method through a stubratify.Storeon both the success and failure paths.internal/store/registrystoreis at 100% statement coverage.TestLoggingStore_ListReferrersCountsAllPagesspecifically covers the pagination accumulation, and asserts the wrapped callback still forwards every page to the caller.findEntryhelper rather than on log level alone, so a shared hook cannot make them pass spuriously.go build ./...,go vet, package tests andgolangci-lintpass. Nogo.modchange.Additive logging only; the decorator returns the inner store's values and errors unchanged.
Enable with
--set logger.level=debug(#2846).