feat(npm): declare the remote proxy's metric families (S15 plan: 15/16)
What this does
Declares the npm remote proxy's Prometheus surface: four metric families, their closed label vocabularies, and the composition-root cardinality entries that bound them.
Nothing emits yet. This is the bottom of a three-part stack — the vocabulary has to exist and pass the cardinality audit before anything can reach it.
| Metric | Type | Labels |
|---|---|---|
npm_remote_upstream_responses_total |
Counter | npm_operation, outcome |
npm_remote_cache_events_total |
Counter | npm_cache_path, event |
npm_remote_packument_transform_duration_seconds |
Histogram | — (buckets: DefBuckets + 15, 30) |
npm_remote_cache_fill_bytes |
Histogram | npm_cache_artifact |
Two decisions worth your attention
1. The new label names are format-scoped
npm_operation, npm_cache_path, npm_cache_artifact — not the bare operation, path, artifact.
The cardinality audit keys closed sets by label name globally, so a bare operation would be one set every later format has to join or rename off. maven_operation set that precedent, so S16 stays free.
npm_cache_path is the interesting one, because it goes the opposite way from outcome and event below. maven's equivalent is the unscoped cache_path, and this MR renames off it rather than joining it: cache_path is {artifact, metadata, sidecar} and npm_cache_path is {packument, dist_tags, tarball}, so no value would span the two formats and one shared name would buy no cross-format query. outcome and event are joined because their values do mean the same thing in both proxies. The rule a later format inherits: a verdict vocabulary joins, a route vocabulary gets its own name.
Renaming a scraped label breaks every dashboard and alert built on it, which is why it is settled before anything ships.
2. outcome and event adopt Maven's vocabulary wholesale
These two label names are not new to the audit table. They belong to internal/storage and internal/iam, and the maven-remote proxy already widened both.
npm classifies an upstream answer with the same seven values Maven does:
| Answer | Outcome |
|---|---|
| Relayed body | ok |
Confirming 304 |
not_modified |
404 |
not_found |
Non-404 4xx |
client_error |
5xx |
server_error |
| No usable response | transport_error |
| Fill or single-flight fault | cache_error |
So npm widens neither union by a value.
It does not follow that a sum by (outcome) across the two families reads one thing per value, and the earlier version of this section claimed it did. The value set is shared; the classification is not. Two answers land differently:
| Answer | maven books | npm books |
|---|---|---|
a fallback-ineligible error (context.Canceled, ErrHeaderInjection, ErrNotImplemented, ErrTransformRejected) |
transport_error |
nothing |
| nil error, status below 400 that the fetch could not use (an unfollowed 302) | server_error |
transport_error |
classifyMavenRemoteUpstream books every non-nil error that is not one of the three cache faults; this file gates on remote.FallbackEligible instead. The type comment, the catalog row and metrics.md all say so now, so a cross-format panel is not built on a promise the code does not keep.
One reading an operator is likely to get wrong
classifyRemoteUpstream books nothing when the failure is ours rather than the upstream's — a client that left (context.Canceled), or a request refused before send for header injection.
It does book a security-cover refusal as transport_error, because remote.FallbackEligible admits a BlockedURLError and the read obtained nothing either way. That means a repeatedly blocked upstream is not separable on this series from an unreachable one; the WARN line each refusal writes is what separates them.
The unit table pins that reading explicitly, with the reasoning in a comment, because it is the one most likely to be misread as a bug.
How it is verified
TestRemoteMetrics_PassesCardinalityAuditemits every declared label value and runs the real composition-root audit over the result — so the vocabularies are proved before a route can reach them.TestRemoteMetrics_CarryNoHighCardinalityDimensionasserts no family carriesnamespace_id, a package name, or an upstream host.TestClassifyRemoteUpstream_MapsEveryAnswerToItsOutcomecovers all 12 answer shapes, including the two that book nothing.TestObserveRemoteUpstream_DropsACoalescedFollowerpins the coalescing gate at unit level; part 3 adds the end-to-end composition test.TestObserveRemoteCacheFill_ObservesBytesPerArtifactpins the twin gate on the fill histogram.TestRemoteMetrics_HelpersDropAnOutOfSetEnumValuepins the runtime enum guard: five refused emissions, no invalid value on any gathered family, one Error record each.TestRemoteHistograms_CarryTheirDeclaredBucketFamilypins the transform histogram's family as an extension ofDefBuckets— the prefix survives intact and the two added boundaries are exactly15and30.
Second round: what changed
Thirteen blocking threads and one question from @sylviashen, all addressed. Two are behaviour changes, the rest are corrections to claims the comments and docs were making.
Behaviour
- A runtime enum guard.
remoteCheckEnummirrorsmavenRemoteCheckEnum: it vets each label against npm's own domain, then logs at Error and drops the emission.metrics.AuditCardinalityruns only from tests, so nothing on a request path stopped a converted string from opening an unbounded series. It also closes a gap the audit cannot reach:outcomeandeventare pinned to the cross-package unions, so an npm emission ofdigest_mismatchorcoalescedpasses the audit — the guard rejects both. The three helpers now take acontext.Contextfirst, which parts 2 and 3 must pick up: 7 call sites, listed in the thread. - Transform histogram buckets reach the timeout that bounds them.
DefBucketstops out at 10s against a 30srequest_total_timeout, so p99 pinned at 10 and an 11s rewrite read the same as a 29s one.remoteTransformDurationBucketsadds15and30rather than replacing the family, so everyDefBucketsboundary survives.
Claims that were wrong
| What it said | What is true |
|---|---|
a sum by (outcome) across both proxies reads one thing per value |
the value set is shared, the classification is not (table above) |
| the transform histogram measures every rewrite | it observes only a stream that reached a terminal read; a client disconnect or a stop at exactly Content-Length observes nothing, so rate(_count) is not a denominator and the bias runs the wrong way |
a recovered leader panic books a transport_error per follower |
it books nothing — the coalescing gate fires before the classifier |
BodySizeBuckets brackets a metadata document |
its first boundary is 1024; metadata fills collapse into the first bucket |
the three new names match maven_operation and cache_path |
cache_path is the unscoped one; npm_cache_path renamed off it, deliberately |
| the transform row's family is emitted today | nothing emits on this branch; the row now carries the same clause its three siblings do |
| the read routes land in a later part | the routes are on main already; the calls that emit land later |
the exclusion list (six of FallbackEligible's seven) |
ErrTransformRejected added — the one an npm route actually produces |
remoteCachePaths() points at the ServeResult reason |
the reason is now written on remoteCachePath itself |
| four label names in a type comment and in test assertion messages | spelled post-rename, and the messages interpolate the testLabel* constants |
Moved out of this MR
The S15 spec table rename went to !1882 (merged), which already edits that table — two branches editing adjacent rows cannot both merge on a train, and it belongs in front of a spec reviewer. !1890 (merged) adds the spec to Step 15's Files and fills its Status row.
!1882 (merged) should merge before this MR. While the spec stands reverted here, its table names operation, path and artifact where the code emits the prefixed names.
docs/specs/S15-npm-remote.md is out of this MR's diff entirely — restored from the branch point, not from the main tip, so none of main's later spec edits ride along.
Reviewing this
The whole MR is one package plus one table entry, and the interesting lines are internal/format/npm/remote_metrics.go. Start there — the file header explains why each label name is what it is.
Diff size (1568 LOC, over the 500 ceiling)
Per development-model.md, splitting further does not help here:
| Group | LOC |
|---|---|
Metric declarations, label vocabularies, enum guard (remote_metrics.go) |
666 |
Tests (remote_metrics_test.go, remote_transform_timing_internal_test.go, buffered_test.go) |
1083 |
Cardinality table + registration (cardinality.go, metrics.go, metrics_test.go) |
134 |
Docs (metrics.md, observability.md) |
106 |
goconst rationale comments (handler.go, publish_commit.go, packument.go) |
23 |
54% is tests, and much of remote_metrics.go is comment: the review round added the reasoning behind the bucket family, the enum guard and the cross-format caveat, all of which are the kind of thing a reader needs and grep cannot reconstruct. The production code is a single declaration block: the four families and their five label domains cannot land separately, because the cardinality audit rejects a label absent from its table and the table entry is meaningless without the family. This step was already split three ways — see the stack below.
Stack
| MR | Target | |
|---|---|---|
| 1 | this one — metric families | main |
| 2 | !1879 (merged) — the npm_remote_proxy wide event |
part 1 |
| 3 | !1880 (merged) — the read routes that emit | part 2 |
Merge in order. Part 2 depends on remoteCacheEvent and remoteCachePath from this MR; part 3 calls the emission helpers.
Parts 2 and 3 need one edit each on rebase. The enum guard added a context.Context first parameter to observeRemoteUpstreamResponse, observeRemoteUpstream, observeRemoteCacheEvent and observeRemoteCacheFill. Seven call sites: remote_observe.go:403 and metrics_test.go:277-279 on part 2, those plus remote_tarball.go:579, :872 and :1023 on part 3. Every one has a ctx in scope.
!1882 (merged) should merge before this MR, so the spec's label names and the emitted ones agree. It does not conflict with this branch — the spec file is no longer in this diff.
Checklist
- Tests included;
golangci-lint runclean on./internal/format/npm/... ./internal/metrics/...with--max-same-issues=0 --max-issues-per-linter=0 --uniq-by-line=false, and again under--build-tags=integration(68 findings in that mode, none in a file this MR touches — all pre-existing, tracked by #682) - Every
//nolinttoken was measured, not assumed — thegoconstones fire only once this MR's label constants land - Conformance tests unaffected: no protocol behaviour changes
- No e2e scenario added or affected (docs/testing/) — this MR declares metrics and changes no request handling
- No ADR conflict: 006 endorses LabKit Prometheus metrics, which this uses
- No configuration, OpenAPI, or Bruno surface touched
Related to #350 (closed)