Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds OpenTelemetry distributed tracing support to the proxy prefetch API endpoints. The implementation replaces the previous custom trace_id field with proper OpenTelemetry instrumentation, enabling better observability of prefetch operations across the system.
Changes:
- Added OpenTelemetry tracing middleware to prefetch endpoints (v1 and v2)
- Removed
trace_idfield from request/response structures, replacing custom tracing with OpenTelemetry spans - Integrated tracing throughout prefetch workflow with span creation, attributes, and error recording
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| proxy/proxyserver/server.go | Added otelhttp middleware to prefetch endpoints for automatic trace context propagation |
| proxy/proxyserver/prefetch.go | Implemented comprehensive tracing with spans for prepare, download, and trigger operations; replaced custom trace_id with OpenTelemetry context propagation |
| proxy/proxyserver/server_test.go | Updated test fixtures to remove deprecated trace_id field from request/response structures |
Comments suppressed due to low confidence (1)
proxy/proxyserver/prefetch.go:195
- The context stored in
input.ctxcontains a span frompreparePrefetchthat will end immediately after this function returns (line 216 of preparePrefetch hasdefer span.End()). When the prefetch is asynchronous (line 194), the goroutine will use a context with an already-ended span, which can cause incorrect trace hierarchies and lost trace data.
Consider creating a detached context for async operations or documenting that v1Synchronous should be true for proper tracing.
if ph.v1Synchronous {
ph.downloadBlobs(input)
} else {
// Download blobs asynchronously.
go ph.downloadBlobs(input)
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
Comments suppressed due to low confidence (1)
proxy/proxyserver/prefetch.go:195
- In the async path (
go ph.downloadBlobs(input)),input.ctxis derived fromr.Context(). Innet/http, the request context is canceled whenServeHTTPreturns, so the background goroutine will typically see a canceled context and may abort downloads immediately. To keep async prefetch working, detach from request cancellation (e.g., use a background context while preserving trace linkage) for the goroutine's work.
if ph.v1Synchronous {
ph.downloadBlobs(input)
} else {
// Download blobs asynchronously.
go ph.downloadBlobs(input)
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
proxy/proxyserver/prefetch.go:195
- In the async V1 path (
go ph.downloadBlobs(input)),input.ctxis derived fromr.Context(). Once the handler returns (after writing the 200), the request context will be canceled, which can cause the backgroundDownloadBlobcalls to fail immediately withcontext canceledand prevent prefetching. Detach the background work from request cancellation (e.g., usecontext.WithoutCancel(r.Context())or acontext.Background()that carries the current span context) before launching the goroutine, while still preserving trace correlation.
if ph.v1Synchronous {
ph.downloadBlobs(input)
} else {
// Download blobs asynchronously.
go ph.downloadBlobs(input)
}
proxy/proxyserver/prefetch.go:195
- The test server harness (
startServer) always constructs the proxy withsynchronous=true, so the V1 async path (go ph.downloadBlobs) is currently untested. Since tracing changes modified the contexts passed into background work, add a test that runs the server withsynchronous=falseand asserts the backgroundDownloadBlobcalls still occur (i.e., they aren’t canceled with the request).
if ph.v1Synchronous {
ph.downloadBlobs(input)
} else {
// Download blobs asynchronously.
go ph.downloadBlobs(input)
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| attribute.String("operation", "prepare_prefetch"), | ||
| ), | ||
| ) | ||
| defer span.End() |
There was a problem hiding this comment.
preparePrefetch returns ctx that was created from ph.tracer.Start(...), but the span is ended via defer span.End() before the caller starts prefetch.download_blobs / prefetch.trigger_prefetch. This makes those later spans children of an already-ended parent span, producing confusing timelines in traces. Consider returning a context whose active span is still appropriate for downstream work (e.g., return r.Context() / the otelhttp span context, or create a higher-level span in HandleV1/HandleV2 that stays open for the synchronous portion and use child spans for prepare/trigger/download).
| defer span.End() | |
| defer span.End() | |
| // Use the original request context for downstream work to avoid returning | |
| // a context tied to a span that will be ended before it is used. | |
| ctx = r.Context() |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if ph.v1Synchronous { | ||
| ph.downloadBlobs(input) | ||
| ph.downloadBlobs(r.Context(), input) | ||
| } else { | ||
| // Download blobs asynchronously. | ||
| go ph.downloadBlobs(input) | ||
| go ph.downloadBlobs(r.Context(), input) | ||
| } |
There was a problem hiding this comment.
In the async V1 path, the goroutine is started with r.Context(). http.Request.Context() is canceled when the handler returns, so the background downloadBlobs work (and its DownloadBlob calls) may be canceled immediately after the response is written. Use a detached context for background work (e.g., context.WithoutCancel(r.Context()) or context.Background() + propagate the span context) so async prefetch isn’t inadvertently aborted.
| // Add tracing middleware for prefetch/preheat endpoints | ||
| tracingMiddleware := otelhttp.NewMiddleware("kraken-proxy", | ||
| otelhttp.WithTracerProvider(otel.GetTracerProvider())) | ||
|
|
||
| r.Post("/registry/notifications", handler.Wrap(s.preheatHandler.Handle)) | ||
| r.Post("/proxy/v1/registry/prefetch", s.prefetchHandler.HandleV1) | ||
| r.Post("/proxy/v2/registry/prefetch", s.prefetchHandler.HandleV2) | ||
| r.With(tracingMiddleware).Post("/proxy/v1/registry/prefetch", s.prefetchHandler.HandleV1) | ||
| r.With(tracingMiddleware).Post("/proxy/v2/registry/prefetch", s.prefetchHandler.HandleV2) |
There was a problem hiding this comment.
The comment says tracing middleware is for "prefetch/preheat endpoints", but the middleware is only applied to the two /proxy/.../prefetch routes; /registry/notifications (preheat) is not wrapped. Either apply the middleware to the preheat route as well or update the comment to match the actual behavior.
This PR adds OpenTelemetry distributed tracing support to the proxy prefetch API endpoints. The implementation replaces the previous custom trace_id field with proper OpenTelemetry instrumentation, enabling better observability of prefetch operations across the system.
Changes: