count registry sockets and requests, and export them to otel - #971
Conversation
|
Warning Review limit reached
Next review available in: 16 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughRegistry transports now collect connection and request metrics. Tracing exports aggregate metrics. Executor and warmer commands log registry statistics on success and error paths. Telemetry documentation lists the new Build span attributes. ChangesRegistry connection telemetry
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant BuildCommand
participant RegistryTransport
participant connstats
participant Registry
participant tracing
BuildCommand->>RegistryTransport: Create instrumented registry transport
RegistryTransport->>connstats: Wrap dialing and HTTP requests
connstats->>Registry: Open sockets and send requests
Registry-->>connstats: Return connection and request events
BuildCommand->>tracing: Shut down tracing
tracing->>connstats: Snapshot registry statistics
tracing-->>BuildCommand: Export registry attributes and end root span
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
docs/telemetry.md (1)
51-66: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument that socket spans require a configured collector.
The section describes the spans but not the condition that produces them.
pkg/tracing/tracing.goemits them only duringShutdown, and only whenKANIKO_TELEMETRY_ENDPOINTis set and the build talked to a registry. Without the endpoint,connstatsstill keeps up to 256 records but nothing exports them.All documented attribute names match
socketSpansinpkg/tracing/tracing.golines 180-193.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/telemetry.md` around lines 51 - 66, Update the “Registry socket spans” documentation to state that socket spans are emitted during Shutdown only when KANIKO_TELEMETRY_ENDPOINT is configured and the build communicated with a registry; without the endpoint, connstats may retain up to 256 records but exports none.pkg/tracing/tracing.go (2)
170-195: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
socketSpansreadsrootSpanwithout a nil check.Line 173 guards
tracer, but line 178 dereferencesrootSpan. The only caller checksrootSpan != nilfirst, so this is correct today. The doc comment records themuprecondition but not therootSpanprecondition, so a future second caller can panic here.State both preconditions, or guard
rootSpandirectly.🛡️ Guard `rootSpan`
func socketSpans() { - if tracer == nil { + if tracer == nil || rootSpan == nil { return }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/tracing/tracing.go` around lines 170 - 195, Update socketSpans to guard rootSpan before calling SetAttributes, or explicitly document rootSpan as a required precondition alongside the existing mu precondition. Prefer a direct nil check in socketSpans so future callers cannot trigger a panic.
241-244: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSocket spans compete with the root span for the shutdown flush budget.
socketSpans()can add up to 256 spans to the batch processor.provider.Shutdownthen flushes everything undershutdownFlushTimeout. If the collector is slow, the added volume raises the chance that the flush times out and the root span never arrives. The root span carries the aggregate attributes that most consumers read, so losing it is worse than losing the per-socket detail.Consider one of:
- Raise
shutdownFlushTimeoutwhen socket spans are emitted.- Lower the 256 cap.
- Make the per-socket export opt-in with a separate environment variable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/tracing/tracing.go` around lines 241 - 244, Adjust the tracing shutdown/export behavior around socketSpans so emitting per-socket spans cannot starve the root span within shutdownFlushTimeout. Prefer making socket-span export opt-in through a separate environment variable, or otherwise reduce the socket span cap or increase the flush timeout; preserve root-span delivery and existing aggregate registryAttrs behavior.pkg/util/transport_util_test.go (1)
166-170: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test covers the new instrumentation.
The test only unwraps the transport to reach the existing TLS assertions. Nothing verifies that
MakeTransportwrappedDialContext, or thatconnstatscounts a socket and a request.A test against
httptest.NewServerwould cover the whole path: one request, thenconnstats.Snapshot()reports one socket opened and one request. That test needs a reset hook inconnstats, because the counters are package globals with no way to clear them.Do you want me to generate the test and the reset hook?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/util/transport_util_test.go` around lines 166 - 170, The transport tests around MakeTransport currently verify only TLS configuration; add an httptest.NewServer integration test that performs one request through the instrumented transport and asserts connstats.Snapshot reports one socket opened and one request. Add and use a connstats reset hook to clear the package-level counters before the test so results are isolated.pkg/util/transport_util.go (1)
123-129: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
LogRegistryConnectionsbelongs closer toconnstats.The function reads only
connstatsstate and writes a log line. It does not use anything fromutil. Placing it inpkg/connstatswould remove theutilimport fromcmd/warmer/cmd/root.gofor callers that need nothing else fromutil.This is a placement preference only. Keep it here if
utilis the established entry point for command-level helpers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/util/transport_util.go` around lines 123 - 129, Move LogRegistryConnections from pkg/util into the connstats package, since it only reads connstats state and logs the snapshot. Update its callers, including cmd/warmer/cmd/root.go, to use the new package location and remove the now-unneeded util import where applicable; preserve the existing logging behavior.pkg/connstats/connstats.go (2)
81-123: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueDial failures are not counted.
WrapDialreturns early on error. Failed dials add nothing todialTimeand are not counted anywhere. A build that spends most of its time on failing dials reportsdial.msnear zero, which is misleading for the pooling work this instrumentation supports.Consider counting failed dials and their duration in separate counters.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/connstats/connstats.go` around lines 81 - 123, Update WrapDial to record every dial attempt’s elapsed time, including failures, by adding the duration to a dedicated failed-dial counter before returning the error. Keep successful dial metrics and connection registration unchanged, and expose the separate failure metric through the existing connstats counters/reporting mechanism.
176-216: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
GotConnattribution silently drops HTTP/2 and proxied requests.
lookupmatches oninfo.Conn.LocalAddr().String(). Two cases returnniland lose the per-socket attribution without any signal:
- HTTP/2 requests, where
info.Conncan be the underlying TCP connection but many requests share it. Per-socket counts stay correct here, so this case is fine.- Any connection not created through the wrapped dialer, for example one produced by a proxy
DialContextreplacement.The aggregate counters still increment, so only the per-socket records diverge from the totals. Consider counting the unattributed requests so the socket-span sum can be reconciled against
kaniko.registry.requests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/connstats/connstats.go` around lines 176 - 216, Update the GotConn callback in tracedTransport.RoundTrip to record requests when lookup(info.Conn) returns nil, covering HTTP/2 and connections created outside the wrapped dialer. Add or reuse an unattributed-request counter associated with the aggregate connection statistics so per-socket totals can be reconciled with the overall requests count, while preserving the existing per-socket attribution path when lookup succeeds.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/warmer/cmd/root.go`:
- Around line 106-109: Ensure util.LogRegistryConnections() runs when cache
warming fails as well as on success by moving it into the exit() helper or
invoking it immediately before exit in the warmer error path. Update the warmer
command flow around warmer.WarmCache and exit, avoiding duplicate logging on
successful execution.
In `@docs/telemetry.md`:
- Line 35: Update the telemetry table entry so kaniko.registry.tls.ms has its
own table row, while keeping kaniko.registry.tls.handshakes in a separate row
with its existing description and documenting the duration metric in the new
row.
In `@pkg/connstats/connstats.go`:
- Around line 160-174: Update tracedTransport to implement CloseIdleConnections
by forwarding the call to its wrapped inner transport when it supports that
method, preserving registry clients’ ability to clean up idle connections
through the Trace wrapper.
---
Nitpick comments:
In `@docs/telemetry.md`:
- Around line 51-66: Update the “Registry socket spans” documentation to state
that socket spans are emitted during Shutdown only when
KANIKO_TELEMETRY_ENDPOINT is configured and the build communicated with a
registry; without the endpoint, connstats may retain up to 256 records but
exports none.
In `@pkg/connstats/connstats.go`:
- Around line 81-123: Update WrapDial to record every dial attempt’s elapsed
time, including failures, by adding the duration to a dedicated failed-dial
counter before returning the error. Keep successful dial metrics and connection
registration unchanged, and expose the separate failure metric through the
existing connstats counters/reporting mechanism.
- Around line 176-216: Update the GotConn callback in tracedTransport.RoundTrip
to record requests when lookup(info.Conn) returns nil, covering HTTP/2 and
connections created outside the wrapped dialer. Add or reuse an
unattributed-request counter associated with the aggregate connection statistics
so per-socket totals can be reconciled with the overall requests count, while
preserving the existing per-socket attribution path when lookup succeeds.
In `@pkg/tracing/tracing.go`:
- Around line 170-195: Update socketSpans to guard rootSpan before calling
SetAttributes, or explicitly document rootSpan as a required precondition
alongside the existing mu precondition. Prefer a direct nil check in socketSpans
so future callers cannot trigger a panic.
- Around line 241-244: Adjust the tracing shutdown/export behavior around
socketSpans so emitting per-socket spans cannot starve the root span within
shutdownFlushTimeout. Prefer making socket-span export opt-in through a separate
environment variable, or otherwise reduce the socket span cap or increase the
flush timeout; preserve root-span delivery and existing aggregate registryAttrs
behavior.
In `@pkg/util/transport_util_test.go`:
- Around line 166-170: The transport tests around MakeTransport currently verify
only TLS configuration; add an httptest.NewServer integration test that performs
one request through the instrumented transport and asserts connstats.Snapshot
reports one socket opened and one request. Add and use a connstats reset hook to
clear the package-level counters before the test so results are isolated.
In `@pkg/util/transport_util.go`:
- Around line 123-129: Move LogRegistryConnections from pkg/util into the
connstats package, since it only reads connstats state and logs the snapshot.
Update its callers, including cmd/warmer/cmd/root.go, to use the new package
location and remove the now-unneeded util import where applicable; preserve the
existing logging behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 23929f0a-147f-4c56-83ad-c3e03d1e5d43
📒 Files selected for processing (7)
cmd/executor/cmd/root.gocmd/warmer/cmd/root.godocs/telemetry.mdpkg/connstats/connstats.gopkg/tracing/tracing.gopkg/util/transport_util.gopkg/util/transport_util_test.go
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
5f46f5a to
d943b17
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/util/transport_util.go`:
- Line 118: Initialize tr.TLSClientConfig before the Certificates assignment in
the transport setup, ensuring the RegistriesClientCertificates-only path creates
a non-nil tls.Config while preserving existing configuration when already
initialized.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f9ba9cea-383c-4965-a620-1e337954ac6b
📒 Files selected for processing (7)
cmd/executor/cmd/root.gocmd/warmer/cmd/root.godocs/telemetry.mdpkg/connstats/connstats.gopkg/tracing/tracing.gopkg/util/transport_util.gopkg/util/transport_util_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/executor/cmd/root.go
- cmd/warmer/cmd/root.go
d943b17 to
77f4fad
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/util/transport_util.go`:
- Around line 82-91: The connstats.Trace wrapper must preserve the wrapped
transport’s lifecycle behavior. Update tracedTransport and its construction in
MakeTransport to forward lifecycle methods such as CloseIdleConnections to the
inner http.RoundTripper, while retaining the existing RoundTrip tracing
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 88131c1d-cb08-4044-aa34-a54ac72c10c3
📒 Files selected for processing (7)
cmd/executor/cmd/root.gocmd/warmer/cmd/root.godocs/telemetry.mdpkg/connstats/connstats.gopkg/tracing/tracing.gopkg/util/transport_util.gopkg/util/transport_util_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- cmd/executor/cmd/root.go
- docs/telemetry.md
- pkg/tracing/tracing.go
- cmd/warmer/cmd/root.go
- pkg/util/transport_util_test.go
- pkg/connstats/connstats.go
77f4fad to
93b811c
Compare
2 participants
Groundwork for #961, no behaviour change to how builds talk to registries.
Kaniko builds a fresh
http.Transportfor every registry operation, so almost nothing shares a connection pool. The evidence in #961 came from a counting TCP proxy in front of a plaintext local registry, which is a dead end for anything real: a proxy cannot see through TLS, so the numbers cannot be reproduced against the registry a user actually builds against. Before changing the pooling, kaniko should be able to count its own sockets.Two hooks, because neither sees what the other does. Opens, closes, still-open-at-exit and peak concurrency come from wrapping
Transport.DialContextand countingClose()on the conn it hands back, since net/http never reports a socket close to its caller and httptrace has no close hook. Reuse, idle time before reuse and handshake cost come fromhttptrace.ClientTrace, which the dialer cannot see, because a reuse is precisely a dial that never happened.A build with
KANIKO_TELEMETRY_ENDPOINTset gets ten numbers as attributes on the root span, where the per-build rollup reads them, and the same numbers go to one debug line for anyone without a collector. Five step build against a local registry: 26 sockets carrying 109 requests, 17 of them open at once, 15 still open when the process exits, which is the shape the proxy reported in the issue.Cost is a
ClientTraceand a few atomics per request, plus an atomic per socket. The conn wrapper only overridesClose, so reads and writes stay untouched.Summary by CodeRabbit
New Features
Documentation