-
Notifications
You must be signed in to change notification settings - Fork 463
feat: Add tracing to origin DownloadBlob operation #548
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: feat/origin-trace-headers-nginx
Are you sure you want to change the base?
feat: Add tracing to origin DownloadBlob operation #548
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces context-aware, traced DownloadBlob operations in the origin blob client and propagates the new signature throughout the call chain (cluster client, proxy prefetch/preheat, registry transfer, and tag resolution), along with corresponding test and mock updates.
Changes:
- Extend
blobclient.Clientandblobclient.ClusterClientDownloadBlobsignatures to acceptcontext.Context, and add OpenTelemetry spans, attributes, and logging for origin and cluster-level download operations. - Update all production call sites (proxy preheat/prefetch, Docker tag resolution, registry read/write transferer, origin server tests) to pass a context (typically
context.Background()), and wire the context through to HTTP requests viahttputil.SendTracingContext. - Regenerate/update GoMock mocks and unit tests to match the new method signatures, and ensure existing download/error-handling behaviors (e.g.,
ErrBlobNotFound, 404/202 handling, Poll backoff behavior) remain covered.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
proxy/proxyserver/server_test.go |
Updates origin client mock expectations for DownloadBlob to include a context.Context argument via gomock.Any(), keeping proxy server tests aligned with the new client interface. |
proxy/proxyserver/preheat.go |
Passes context.Background() into clusterClient.DownloadBlob when fetching manifests for preheat, enabling traced, context-aware origin downloads. |
proxy/proxyserver/prefetch.go |
Uses context.Background() in all clusterClient.DownloadBlob calls (manifest fetch and layer fetches) to participate in the new traced download path while preserving existing metrics and error handling. |
origin/blobserver/testutils_test.go |
Imports context and updates ensureHasBlob to call DownloadBlob with a context, keeping test utilities compatible with the new client signature. |
origin/blobserver/server_test.go |
Adds context import and updates the DownloadBlob call in TestDownloadBlobNotFound to pass context.Background(), preserving the 404 behavior test with the new API. |
origin/blobserver/cluster_client_test.go |
Adjusts cluster client tests to use context-aware DownloadBlob in both direct calls and Poll callbacks, and updates mock expectations to accept an extra context.Context parameter. |
origin/blobclient/cluster_client.go |
Extends the ClusterClient interface to include ctx in DownloadBlob, wraps the operation in an OpenTelemetry span with relevant attributes, and routes the context into underlying clients while preserving ErrBlobNotFound and error-handling semantics. |
origin/blobclient/client.go |
Extends Client.DownloadBlob to accept ctx, instruments the HTTP GET/download with OTel spans (including origin address and bytes transferred), and propagates the tracing context into httputil.Get via SendTracingContext. |
mocks/origin/blobclient/clusterclient.go |
Updates generated GoMock MockClusterClient.DownloadBlob and its recorder to include a context.Context argument, aligning mocks with the new ClusterClient interface. |
mocks/origin/blobclient/client.go |
Updates generated GoMock MockClient.DownloadBlob and its recorder to accept context.Context, matching the updated Client interface. |
lib/dockerregistry/transfer/rw_transferer_test.go |
Updates origin cluster mock expectations for DownloadBlob to include a context parameter for the cache-populating download path. |
lib/dockerregistry/transfer/rw_transferer.go |
Wraps registry-side Download and downloadFromOrigin in OTel spans and passes the tracing context into originCluster.DownloadBlob, adding detailed status and error annotations while keeping existing CAS and error behavior. |
build-index/tagtype/map_test.go |
Adjusts originClient.DownloadBlob expectations in TestMapResolveDocker to account for the new context.Context parameter. |
build-index/tagtype/docker_resolver_test.go |
Adds a context import and updates all origin client DownloadBlob expectations and DoAndReturn signatures to accept a context.Context, ensuring resolver retry semantics are still verified. |
build-index/tagtype/docker_resolver.go |
Imports context and calls originClient.DownloadBlob(context.Background(), ...) in downloadManifest, making Docker resolver downloads use the new context-aware, traced download API. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return client.DownloadBlob(ctx, namespace, d, dst) | ||
| }) | ||
| if httputil.IsNotFound(err) { | ||
| span.SetStatus(codes.Error, "blob not found") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is sometimes an expected scenario that happens quite a bit in production actually (although im not sure why). At the same time, we have situations where it is not an expected scenario (when the blob shd be in GCS but isn't due to writeback delay). So based on those circumstances, should we be treating this scenario as an error or as normal?
| interval = interval * 2 | ||
| } | ||
| if err := ph.clusterClient.DownloadBlob(repo, d, buf); err == nil { | ||
| if err := ph.clusterClient.DownloadBlob(context.Background(), repo, d, buf); err == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the context be generated in the handler instead of the helper funcs?
This PR introduces context-aware, traced DownloadBlob operations in the origin blob client and propagates the new signature throughout the call chain (cluster client, proxy prefetch/preheat, registry transfer, and tag resolution), along with corresponding test and mock updates.
Changes: