feat(npm): declare the remote proxy's wide event (S15 plan: 15/16)
What this does
Declares the npm_remote_proxy wide event: one Info-level completion record per proxied read on a kind=remote npm repository, plus the request-scoped accumulator a read fills as it resolves.
No route begins a record yet — part 3 wires the three read routes. This MR lands the record's shape, its lifecycle, and the shared document-serve status stamp.
Where the high-cardinality dimensions live
Part 1 kept namespace_id, package names, and upstream hosts off every metric label. This is where they go instead:
operation outcome duration_s ← the three S03-A baseline fields
namespace_id remote_repository_id upstream_host
package version file_name ← the coordinates the read resolved
cache coalesced status npm_code bytesThe upstream base reaches the record as its host alone — the one component that can never hold a credential — and a base url.Parse refuses is withheld whole as invalid-url. Two tests plant a bearer-token canary in the stored URL's userinfo and assert it is absent from the record.
Three things a reviewer should check
1. status, not http_status
The S15 spec's field list says http_status. That literal is in LabKit's deprecated field map (fields/fields.go, mapped to fields.HTTPStatusCode), and it is what the go-logging-field-validator CI job rejects.
Every other wide event in the service already uses the constant — all seven OCI events, internal/usagedata/tracker.go, and maven_remote_proxy, whose spec also said http_status and which emitted the constant anyway. This MR corrects the spec table to match.
⚠️ That CI job ismerge_request_event-only, so a green branch pipeline does not clear it. Check this MR's own head pipeline.
2. outcome is derived, never passed in
It reads the record's own npm_code and status, so it cannot disagree with the rest of its line.
A record carrying neither is an error, not a success. Every path that answers a client stamps a status, so the only way to reach the emitter without one is a panic unwinding through the read's deferred emit — which runs before the middleware recover() that forces the 500, and writes to a different holder. Nothing else on the record can report it.
Reading that zero as a success would put a panicked read on the success side of any SLO computed off this event. The npm request metric reports the 5xx correctly either way; it is only the wide event that would have lied.
3. recordOutcomeCode becomes a single fan-out sink
Rather than each error writer stamping holders one by one, the package's one outcome-code sink fans the value out to every holder the request installed. So the event's npm_code and the request metric's code label cannot disagree, whichever writer produced them — including a transfer that faulted after its status had already committed, where no envelope is written at all.
A hosted request, and a proxied one refused before its read began, install no record and the extra call is a no-op.
How it is verified
remote_observe_test.go is white-box over the emitter, so the record's shape is pinned once rather than re-asserted at each call site:
- the full field set off one populated record, plus which fields an out-of-scope operation omits
outcomeagainst every answer a client can get, including the no-status panic case- the
cachefield's vocabulary — it is three of the counter's four values, because a revalidated read reports thehit_staleit started from - the credential canary
- the accumulator lifecycle (new in this MR): a verdict recorded anywhere under the read's context reaching both the counter and the field; a read that resolves no verdict booking nothing; every setter no-opping on a context with no record;
coalescedcarrying either value
Reviewing this
internal/format/npm/remote_observe.go is the whole story, and its file header lists what is declared but not yet emitted. The other production files are 3 to 40 line changes at call sites.
Diff size (1669 LOC, over the 500 ceiling)
Measured with git diff --numstat origin/main...HEAD, added plus removed, at cabe53fc.
| Group | LOC |
|---|---|
Wide event + accumulator (remote_observe.go) |
479 |
Tests (remote_observe_test.go, remote_document_serve_test.go, remote_read_errors_test.go) |
1061 |
Call-site stamps (transport.go, remote_document_serve.go, observe.go, packument.go, remote_metrics.go, remote_read_errors.go) |
87 |
Docs (observability.md, metrics.md, S15 spec) |
42 |
64% is tests. The event's field set, its omission rules, and its outcome derivation are one contract; splitting them would land a record whose shape no test pins. This step is already split three ways.
The earlier figures in this section were understated: the total read 1329 against a measured 1365, and every group row was low. Recomputed here rather than carried forward.
Stack
| MR | Target | |
|---|---|---|
| 1 | !1878 (merged) — metric families | main |
| 2 | this one — the wide event | part 1 |
| 3 | !1880 (merged) — the read routes that emit | part 2 |
Merge order matters. This MR uses remoteCacheEvent and remoteCachePath from part 1; part 3 calls beginRemoteProxyRecord from here.
Checklist
- Tests included;
golangci-lint run ./...clean with the counting caps disabled - Conformance tests unaffected: no protocol behaviour changes
- No e2e scenario added or affected (docs/testing/) — this MR adds a log record and changes no response a client can observe
- No ADR conflict; S03-A is the governing contract, and the catalog entry now carries the Not yet a conforming wide event clause naming the typed-emitter deviation, the same one
maven_remote_proxyrecords - No configuration, OpenAPI, or Bruno surface touched
- Spec author sign-off needed — this MR edits
docs/specs/S15-npm-remote.mdto correct thehttp_statusfield name. See the open question below.
Review round 2
Addressed from #note_3731131494 and #note_3731134515.
Behaviour
bytesno longer over-reports a HEAD.net/httpaccepts a HEAD response's body writes and discards them, soio.Copyreturned the document's whole length while nothing left the server, on both metadata routes. The copy still runs, since draining the source commits the cache fill.- The record holds
upstreamHost, projected bybeginRemoteProxyRecord, rather thanupstreamURL. The field comment already promised only the host reaches the record while the field held the whole stored base. - The status stamp moved into
WriteError, the one writer every error envelope shares. It was seven hand-written call sites with six more arriving in part 3, each a silent miss thatoutcome()reads aserror.
Tests. Three mutations survived the suite before this round: dropping the recordRemoteProxyCode fan-out, seeding remote_repository_id from Resolution.NpmRepositoryID, and dropping the httpStatus == 0 arm of outcome(). No test installed a Resolution, so beginRemoteProxyRecord's seeding branch was never entered, and no case carried neither a status nor a code. Six tests and one table row close those, and each of the six behaviours above was checked by re-applying its mutation and confirming the suite goes red.
Docs and comments. The five S13/S17 audit catalog rows are back to their pre-branch wording: four of their rewritten claims contradict the code, and a SOX-relevant audit trail does not belong in a feat(npm) commit. The npm catalog row gains a bytes definition, the 499 case, the corrected omitted-field count, and the S03-A deviation clause. Every present-tense comment about a record production does not create is future tense, metric labels are named as declared, and three pointers to two non-existent test files are gone.
Left out, with reasons
- Item 9 (what the metadata serve books when a copy faults after its 200) stays for the wiring part. This helper holds only
errors.Join(copyErr, closeErr)and cannot separate a client disconnect from a source fault; discriminating them needssourceReadTracker, which arrives with the routes. Tracked in #800. - Item 8 (the plan's Files list, and its sentence that only Steps 11 and 14 list the S15 spec) needs a
docs(plans)branch, which a step MR must not carry.
Three corrections to the review's own references, since a reader checking them will not find what they say:
- The present-tense byte claim is at
remote_document_serve.go:144, not:148. Line 148 carried a separate wrong claim, that the tarball route records its own bytes:remote_document_serve.goholds the package's onlyrecordRemoteProxyBytescall. remote_observe.go:130-131spells no bareoperation=; it wrongly says the metric label has "the same name". The third bare-form site is:58, not in the list.- The remote tarball route is wired in production, at
cmd/artifact-registry/wire_npm_remote.go, soobserve.go's "once the route lands (part 3)" was wrong for the reason given rather than merely stale. What is pending is the record, not the route.
Open question for the spec author
The S15 field list names http_status; this MR changes it to status (fields.HTTPStatusCode) to match the CI validator and every sibling event. Flagging rather than assuming, since it is a spec edit inside an implementation MR.
Related to #350 (closed)