feat(npm): book the probe's upstream response and its record (S15 observability plan: 4/5)

What

Closes the last of the three npm-remote observability gaps: the health probe now has an npm-side producer, so npm_remote_upstream_responses_total{npm_operation="probe"} and the npm_remote_proxy record for operation="probe" both emit.

remoteProbeObserver (internal/format/npm/remote_health_observe.go) implements remote.HealthProbeObserver, and the composition root hands it to the npm health monitor and to no other format's, on both the scheduled sweep and the on-demand connection test.

The record's outcome reports the reachability verdict

Worth a reviewer's attention, because it is the one place this step departs from how the three read routes derive the same field.

A probe answered 401, 403 or 404 is reachable per S13's taxonomy, so the monitor records the remote healthy. npm contributes no health-check path, so the probe is a HEAD against the bare base URL, which is exactly the request an auth-gated upstream answers 401 or 404. Deriving the record's outcome from the status would have booked a failure on every sweep of such a remote, on the field both catalogs name as the SLO input, while last_health_status read healthy throughout.

So remoteProxyEvent gains a non-rendered probePassed, and outcome consults it on an operation="probe" record and on no other. The read routes keep the status-derived arm unchanged: for a read, a relayed 4xx is its client's own failed operation. TestRemoteProxyEvent_ReadRoutesKeepStatusDerivedOutcome pins that.

The counter keeps following the status, because it counts what the upstream answered. So one 404 probe books not_found on the counter and outcome=success on the record. The two answer different questions about the same event; internal/format/npm/metrics.md, "What decides a record's outcome", carries the reasoning, and the spec's Health probe section carries the ruling.

Plan and stack

# Step MR
1a the metadata read observer and its standalone seam !2075 (merged) (merged)
1b the metadata routes install their read observer !2076 (merged) (merged)
2 retire the version field from npm_remote_proxy !2187 (merged) (merged)
3 HealthMonitor reports each probe to a format-scoped observer !2071 (merged) (merged)
4 npm books the probe's upstream response and its record this MR

Every predecessor is merged, so this targets main directly. !2205 (merged) has merged too, so the plan's ## Research Findings and its step-4 entry on main now describe the verdict-derived arm this MR implements rather than the status-derived reuse it replaces, and the plan carries the ## Spec Reference pointer this step's spec change needs. The S15 spec amendment the plan's ## Dependencies requires as a merge gate on this step has already landed.

Reviewer notes

The observer consults its deadline nowhere, deliberately. remote.HealthProbeObserver bounds each report by healthObserveTimeout and warns that an implementation ignoring the deadline holds one of the sweep's probe slots open. A round-2 revision added a select on ctx.Done(), and review then established that it could not fire: HealthMonitor.Probe defers observeProbe on a context.WithoutCancel of the caller's context and observeProbe opens a fresh budget on that detached context, so the context is unexpired on entry and the only work ahead of the check was one counter Inc. A pre-check also bounds entry into the slog write rather than its duration, which is the hazard that was raised. The guard is gone, and ObserveProbe's body is one counter Inc and one log write. npm is the first implementation of this interface, so this is the template Maven and container will copy.

Two internal/remote changes, neither behavioural. probeNoHostMarker is now exported as remote.ProbeNoHostMarker, so npm's own remoteProxyInvalidHost can be pinned equal to it by a test instead of by a comment asking the next editor to change both: same value, same two call sites. And healthRecordTimeout's doc said it equalled jobsriver's forceCancelGrace "leaving zero margin", which held only while no format wired a probe observer. Wiring npm's makes the detached tail 5s + 2s against a 5s grace. check-comment-caps.sh caps an unexported doc at one line and scores the whole block, so the corrected reasoning moved to internal/remote/health.md, this package's first sidecar, with both constants left pointing at it. That follows the internal/format/npm pattern where metrics.md and packument_cache.md carry what a one-line doc cannot.

The branch has been force-pushed, with a lease each time. The current head is 4ae82ab8a, and it is 20 commits on top of main. SHAs quoted in the review threads predate it and have moved, so read any of them as a commit since replayed rather than one still reachable; the thread that asked for the commit-message retype carries the mapping for that round. Two commit messages were retyped there, and the longest title on the branch is 71 characters, inside gitlint's limit.

File overlap with a merged MR, and the gate that settled it. !2193 (merged) touched docs/dev/observability.md, docs/specs/S15-npm-remote.md and internal/format/npm/metrics.md with hunks adjacent to this MR's, and conflicted with this branch in the first two. It was hard-gated ahead of this MR through the /blocks endpoint, it has merged, and this branch is rebased onto main past it. Different subject matter (its cache-event derivation against this one's probe), so no duplicated work.

Diff size. 1166 reviewable LOC (1056 added, 110 removed), past the 500 the development model asks a justification for. Measured at 4ae82ab8a with git diff origin/main...HEAD --numstat; no vendored or generated paths appear in the diff. The split: 653 tests (remote_health_observe_test.go 581, wire_remote_test.go 68, remote_observe_test.go 4), 342 Markdown (metrics.md 233, health.md 46, S15-npm-remote.md 55, observability.md 8), and 171 production Go (remote_health_observe.go 73, remote_observe.go 55, health.go 25, wire_remote.go 10, remote_metrics.go 8). Splitting would separate a 73-line emitter from the suite that pins it and the catalogs that document it, so the reviewable unit is smaller than the count suggests. Review commits move these figures, so read them as of that SHA rather than as of the current head.

Log volume. The probe is the first npm_remote_proxy producer no client request drives, so it writes at a rate the npm-remote fleet size and health_check.scheduled_interval set rather than traffic, and a healthy fleet was previously silent on the sweep path. The records are unsampled: internal/logging's sampler acts only on the access message. internal/format/npm/metrics.md, "What the probe arm adds to the log stream", states both.

Tests

  • internal/format/npm/remote_health_observe_test.go: the status-to-outcome mapping across the counter's domain, the record's whole field set, the verdict-derived outcome in both directions, coalesced rendering false, the omitted fields, and the host taken from the observation rather than from a stored credential-bearing base.
  • The same file pins the catch-all's two readings against each other: an unfollowed 3xx, the one member whose counter and record disagree, beside a 1xx member where they agree, both asserted from a single ObserveProbe call. It also pins remoteProxyInvalidHost equal to remote.ProbeNoHostMarker, so the invalid-url marker cannot split across its two importable producers. The split table drives all four classes the catch-all admits, including a status at or above 600, and it was falsified before being kept: swapping the catch-all's return value fails the table, and dropping the < 600 bound from the 5xx arm fails the 600 row alone.
  • cmd/artifact-registry/wire_remote_test.go: TestBuildHealthSources_OnlyNpmObservesProbes pins which of the six monitors observes. It reads remote.HealthMonitor's unexported observer through reflect, because no exported accessor reports it and a nil observer probes exactly like a wired one. Verified by mutation: nil on either npm call fails the npm subtest, and handing the observer to maven or container fails theirs.

Ran locally at 4ae82ab8a: go build ./..., go test ./... -count=1 and golangci-lint run ./... all clean under the .tool-versions-pinned toolchain (Go 1.26.7, golangci-lint 2.13, which mise resolves to 2.13.2 here; the repo pins no patch version), plus -race on internal/format/npm/... and internal/remote/..., and scripts/ci/check-comment-caps.sh --base origin/main over the whole branch diff. Also go vet -tags=integration and golangci-lint --build-tags=integration on the touched packages, because .golangci.yaml sets no run.build-tags and CI's lint never compiles those files: no finding falls in a file this MR touches. The three TestWireStorage_CloudCDN* cases in cmd/artifact-registry need GOOGLE_APPLICATION_CREDENTIALS, so the suite ran with it set, the way the pre-commit hook does.

e2e scenarios

No docs/testing/ scenario is added or affected. The change emits observability for a probe the scheduled sweep and the connection test already performed; it alters no request path, response, or stored state, and the e2e catalogs describe client-visible behavior.

Related to #800 (closed)

Steps 1a through 3 closed that work item's other acceptance items, and this MR settles its probe question, so #800 (closed) is ready to close once this merges. Left open deliberately rather than closed from here, because the evidence for its first two acceptance items sits in !2075 (merged) and !2076 (merged) rather than in this MR.

Edited by Dzmitry (Dima) Meshcharakou

Merge request reports

Loading
Loading