feat(npm): emit the remote proxy's observability (S15 plan: 15/16)

What this does

Wires the three kind=remote npm read routes so the metrics from part 1 and the wide event from part 2 actually carry traffic. This is the top of the stack and the MR where the surface goes live.

Each route now begins an npm_remote_proxy record, stamps its status and code into it, and books the cache event, upstream response, and fill its answer earns.

The defect this MR exists to not ship

remote.SingleFlight hands a coalescing follower the leader's FetchResult whole — the leader's outcome, status, and length. Nothing downstream of the fetch can tell the two apart.

So a naive route books one upstream response and one cache fill per request instead of per upstream call. On a cold-cache herd for a popular tarball — the exact load this proxy exists to absorb — both counters inflate by the coalescing factor, and npm_remote_upstream_responses_total reports upstream traffic the registry never generated.

remote.FetchResult.Coalesced is the one field that can tell them apart, and its own doc comment says why it exists:

Format observers use it so cache_fill_bytes and upstream_responses_total count one fill and one upstream call, not one per coalesced request.

Both counters now consult it, matching observeMavenRemoteUpstream.

The composition test is the point. TestRemoteTarballRoute_CoalescedFollowersBookNoUpstreamCallOrFill drives five concurrent reads of one path over one long-lived remote.SingleFlight and asserts both counters advance by exactly one. Measured with the gate removed, it reports 5 on both — this is the shape AGENTS.md's "a shared-instance seam needs a composition test" rule asks for, and no per-request test can see it.

Three more corrections to what the numbers mean

A delivered body does not prove the fill committed

The fill histogram now requires delivered && err == nil, not delivered alone.

For a declared Content-Length, the cache tee reports a staging-flush or Commit failure as an ErrCacheFill on the read that ends the body — after io.Copy has already written the last chunk. So copied == size held while nothing was cached, and the histogram would report healthy fill volume straight through a storage or DB outage, which is exactly when an operator is asking "is the cache writing?"

A verdict the lookup earned can be withdrawn

On both the fresh-hit and the confirmed-304 paths, if the row's blob turns out to be gone the recorded verdict drops to miss — that read served nothing from the cache. Both withdrawals now have test rows; previously either line could have been deleted with the suite still green.

coalesced describes the read, not its last fetch

The wide event's coalesced field latches. ObserveFetch fires once per upstream call, and a stale row whose confirming 304 finds no blob behind it sends the read back upstream a second time (StandaloneFlow.serveRevalidatedfetchAndDispatch), so assigning the field left it reporting the recovery fetch alone and dropping what the revalidation said.

This is the one path where the wide event is the only surviving report: a coalesced call books no npm_remote_upstream_responses_total series, so the counters name the recovery fetch and nothing else records the flight the read joined. rmoWant now carries coalesced for every footprint row — the field had no route-level assertion before — and the new recovery row is the only one of the eleven that fails with the latch removed.

What stays declared and unemitted

The spec gains an ### Emitter coverage table, which is the whole list — a declared value not named there has an emitter.

Declared Emits today Why the rest waits
npm_operation ∈ {probe, packument, tarball} tarball Classifying an upstream answer needs a remote.FetchResult. The metadata routes resolve through remote.Standalone.Serve, which returns a ServeResult and nothing about the upstream behind it.
npm_cache_path, npm_cache_artifact tarball Same seam. A metadata fill commits inside remote.Standalone, which reports neither the commit nor its size to the route.
npm_remote_proxy field version never The tarball route puts the version on the per-request completion line; a metadata route addresses a document, not a version.
Why the metadata routes record no cache verdict at all, rather than a partial one

ServeResult.Source is four values wide and still not a cache verdict: ServeFromCache folds a fresh hit together with a stale row a 304 confirmed, and ServeFromUpstream folds a miss together with a stale row re-fetched whole.

ServeFromCacheDegraded alone would pin one value — the cache-fallback hit_stale. Counting only that one is deliberately not done: the four event values are documented as summing to a path's read count, so a counter that fires on the fallback serves alone reads as a hit ratio computed over every read. Emitting a verdict the seam cannot substantiate is worse than emitting none.

Both metadata routes do emit the wide event, so operation="packument" and operation="dist_tags" are live there even though the same two words are silent as metric label values. Tracked in #800.

How it is verified

remote_tarball_observability_test.go drives one real request per cache-and-upstream shape through the dispatcher and asserts the whole footprint each time — the wanted series advanced by one and every sibling left alone, so an implementation emitting two events for one read fails:

Shape Cache event Upstream Fill
Fresh cache hit hit_fresh
Cold miss, relayed miss ok
Stale hit, 304 confirms revalidated not_modified
Stale hit, served degraded hit_stale transport_error
Upstream 404 miss not_found
Upstream 502 miss server_error
Upstream unreachable miss transport_error
HEAD, drains upstream miss ok
HEAD from fresh cache hit_fresh
Fresh row, blob vanished miss (withdrawn) ok

The last three are new — the HEAD arm owns a cache-fill observation nothing previously exercised.

Plus the coalescing composition test above, and remote_metadata_observability_test.go for both metadata routes, which pins the deliberate absence of a cache field as well as the emission.

Reviewing this

Start with the coalescing test, then remoteTarballDelivery.ObserveFetch and serveUpstreamStream in remote_tarball.go — those are the call sites that carry the gates. The rest is table rows.

Diff size (1270 LOC, over the 500 ceiling)
Group LOC
Route wiring (remote_tarball.go, remote_packument.go, remote_disttags.go) 203
Route-level tests (three *_observability_test.go files) 976
Comment corrections (remote_metrics.go, now that the routes exist) 67
Docs (S15 ### Emitter coverage, observability.md, metrics.md) 24

77% is tests, which is the point of this MR — the wiring is small and the observability claims are what need pinning. This step is already split three ways.

Stack

MR Target
1 !1878 (merged) — metric families main
2 !1879 (merged) — the npm_remote_proxy wide event part 1
3 this one — the read routes part 2

Merge last. Parts 1 and 2 carry future-tense comments naming this part; merging out of order would leave them describing routes that do not exist. This MR resolves them to present tense.

Checklist

  • Tests included; golangci-lint run ./... clean with the counting caps disabled
  • Conformance tests unaffected: no response status, header, or body changes — every WriteHeader in the diff is an added observability stamp beside an existing write
  • No e2e scenario added or affected (docs/testing/) — the change is observability-only and no client-visible behaviour moves
  • No configuration, OpenAPI, or Bruno surface touched
  • Spec author sign-off needed — see below

Open questions for the spec author

Two readings this MR encodes, flagged rather than assumed:

  1. The ### Emitter coverage section is new spec text added from an implementation MR. Worth confirming it belongs in the spec rather than in the tracking issue.
  2. remote_repository_id on the wide event renders the parent repositories.id, matching maven_remote_proxy and the S13/S17 audit trail. docs/dev/observability.md records this as settled for both format proxies now, and calls out that oci_remote_proxy has a competing pull toward container_remote_repositories.id.

Related to #350 (closed)

Edited by Dzmitry (Dima) Meshcharakou

Merge request reports

Loading
Loading