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_bytesandupstream_responses_totalcount 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.serveRevalidated → fetchAndDispatch), 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
WriteHeaderin 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:
- The
### Emitter coveragesection is new spec text added from an implementation MR. Worth confirming it belongs in the spec rather than in the tracking issue. remote_repository_idon the wide event renders the parentrepositories.id, matchingmaven_remote_proxyand the S13/S17 audit trail. docs/dev/observability.md records this as settled for both format proxies now, and calls out thatoci_remote_proxyhas a competing pull towardcontainer_remote_repositories.id.
Related to #350 (closed)