feat(npm): cover the remote tarball read path and instrument its route
🎯 What this MR does
Completes the tarball handler's unit coverage with the read-path suite, and makes the three kind=2 npm read routes report a request metric and a completion log line when a handler is wired into their slot. Step 12c of a 4-MR stack.
Mounts nothing. The slots still serve the interim 501, so nothing is emitted yet; this MR is what makes the emission exist once 12d wires one.
| Step | MR | What it delivers |
|---|---|---|
| 12a | !1685 (merged) | npmremote.FlightRegistry + npm.RemoteOperationsProvider |
| 12b | !1686 (merged) | npm.RemoteTarballHandler + the response-contract suite |
| 12c · this MR | The read-path suite + instrumenting the remote routes | |
| 12d | !1688 (closed) | Composition root: mount, integration suite, spec, plan, recipe |
🧩 The two pieces
1️⃣ The read-path suite
remote_tarball_read_test.go covers what the handler resolves a request through, which 12b's contract suite deliberately left out: which requests are rejected before any cache lookup or upstream request, how a fresh, stale, or missing row resolves, what the fetch asks for (the encoded path as the key, no transform, the blob size-cap class), how each upstream status maps, and how concurrent cold reads for one path coalesce into one upstream request and one fill.
The split is by subject rather than by size — 12b owns the fixtures and the client-visible contract, this owns the path behind it — so neither file carries two subjects. The helpers only this suite uses moved here with it; the shared ones stay in remote_tarball_test.go and remote_testsupport_test.go.
Merge order matters: 12b's read-path arms are uncovered until this lands, so these two should merge together or not at all.
2️⃣ Instrumenting the remote routes
instrument is unexported, so a composition root in another package cannot apply it. An option that only stored the handler therefore left a live artifact route with no npm_request_total, no npm_request_duration_seconds, and no completion log line — while every hosted route beside it had all three, and nothing in the type system said so. The three WithRemote*Handler options now wrap what they are given, so a wired proxy route cannot be left uninstrumented by a caller who did not know to. All three land together because all three routes already exist; a route still on the interim 501 reports nothing.
Each remote route carries its own handler label — remote_packument, remote_disttags, remote_download — rather than sharing its hosted twin's. The two serve the same URL and do different work, so merging them would hide proxy latency and proxy error rate inside the hosted series, which is exactly the per-pattern separation ADR-005 asks for under Monitoring. The npm_command stays the hosted one: the client verb is the same, and only the handler label distinguishes how it was served.
That puts three codes on the metric only these routes emit, so the closed sets grow with them: handler 9 → 12 and code 37 → 40. The count lives in two places — the npm enumerations, and internal/metrics/cardinality.go, which count-bounds the same label names as literals because the foundation cannot import the npm package. TestRequestLabels_MatchTheCardinalityBudget pins the enumerations against those literals, so a label added on one side without the other fails in this suite rather than on its first production emission.
⚠️ What this does not close
ADR-005's redirect-versus-proxy distinction. A remote tarball read serves both patterns depending on what BlobStore resolves, and separates neither; nor does the hosted download, which records delivery_mode only on its usage-data event rather than on a metric or the completion log. Closing it is a decision about where the dimension belongs, not an emission at one seam, so it is scoped to the observability step and the plan (in 12d) now says so explicitly.
✅ Verification
go build ./...andgo vet ./...: clean.go test -race -count=2 ./internal/format/npm/... ./internal/metrics/...: green.golangci-lint run --max-same-issues=0 --max-issues-per-linter=0 ./internal/format/npm/... ./internal/metrics/...: 0 issues.TestRegisterMetrics_PassesCardinalityAuditemits every declared handler and code and runs the composition root's audit over the result, so the new values are audited here rather than on their first production emission.
📏 Diff size
1747 LOC, past the 500 reviewable-LOC line in docs/dev/development-model.md. Split by file group:
| Group | Files | LOC |
|---|---|---|
| Production | handler.go +32, metrics.go +22, internal/metrics/cardinality.go +13 |
67 |
| Tests | remote_tarball_read_test.go 1559, observe_test.go +72, metrics_test.go +25 |
1656 |
| Docs | metrics.md +24 |
24 |
Tests are 95% of it, and the production half is 67 lines across three files. The suite is the MR.
🧪 E2E scenario impact
None: no route changes behavior, and the emission is unreachable until 12d wires a handler into a slot.
Related to #349 (closed)