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_PassesCardinalityAudit emits 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_CarryNoHighCardinalityDimension asserts no family carries namespace_id, a package name, or an upstream host.
  • TestClassifyRemoteUpstream_MapsEveryAnswerToItsOutcome covers all 12 answer shapes, including the two that book nothing.
  • TestObserveRemoteUpstream_DropsACoalescedFollower pins the coalescing gate at unit level; part 3 adds the end-to-end composition test. TestObserveRemoteCacheFill_ObservesBytesPerArtifact pins the twin gate on the fill histogram.
  • TestRemoteMetrics_HelpersDropAnOutOfSetEnumValue pins the runtime enum guard: five refused emissions, no invalid value on any gathered family, one Error record each.
  • TestRemoteHistograms_CarryTheirDeclaredBucketFamily pins the transform histogram's family as an extension of DefBuckets — the prefix survives intact and the two added boundaries are exactly 15 and 30.

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. remoteCheckEnum mirrors mavenRemoteCheckEnum: it vets each label against npm's own domain, then logs at Error and drops the emission. metrics.AuditCardinality runs 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: outcome and event are pinned to the cross-package unions, so an npm emission of digest_mismatch or coalesced passes the audit — the guard rejects both. The three helpers now take a context.Context first, which parts 2 and 3 must pick up: 7 call sites, listed in the thread.
  • Transform histogram buckets reach the timeout that bounds them. DefBuckets tops out at 10s against a 30s request_total_timeout, so p99 pinned at 10 and an 11s rewrite read the same as a 29s one. remoteTransformDurationBuckets adds 15 and 30 rather than replacing the family, so every DefBuckets boundary 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 run clean 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 //nolint token was measured, not assumed — the goconst ones 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)

Edited by Dzmitry (Dima) Meshcharakou

Merge request reports

Loading
Loading