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-derivedoutcomein both directions,coalescedrenderingfalse, 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 a1xxmember where they agree, both asserted from a singleObserveProbecall. It also pinsremoteProxyInvalidHostequal toremote.ProbeNoHostMarker, so theinvalid-urlmarker cannot split across its two importable producers. The split table drives all four classes the catch-all admits, including a status at or above600, and it was falsified before being kept: swapping the catch-all's return value fails the table, and dropping the< 600bound from the5xxarm fails the600row alone. cmd/artifact-registry/wire_remote_test.go:TestBuildHealthSources_OnlyNpmObservesProbespins which of the six monitors observes. It readsremote.HealthMonitor's unexportedobserverthroughreflect, because no exported accessor reports it and a nil observer probes exactly like a wired one. Verified by mutation:nilon 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.