docs(specs): catalog the two npm remote proxy counters in S15
🎯 Summary
S15's Observability table declared four gitlab_artifact_registry_npm_remote_* metrics.
The npm remote proxy has since grown two more, and the table is the spec surface S15 owns,
so it gains a row each rather than the code deviating from the spec silently.
Split out of !1836 (merged)
so the spec change can be approved on its own terms. S15 is Status: Approved, and guardrail 5
makes merging a spec MR the approval signal, which an amendment riding inside an implementation
MR does not give the spec author.
The MR does two things to one table, and the second was not written down here until review asked for it. It adds the two rows below, and it revises the four original rows to match the label names and value sets the implementation emits.
📊 The two new rows
| Metric | What it reports |
|---|---|
..._npm_remote_response_deadline_arm_failures_total |
A proxied read whose response write deadline could not be armed. The packument, dist-tags, and tarball routes arm through one function and share the counter. Expected rate is flat zero; any sustained rate is a ResponseWriter wiring regression. |
..._npm_remote_tarball_deadline_kills_total |
A proxied tarball response this service cut short mid-body on a timeout-shaped write failure: its own response write deadline elapsing, or an upstream read that timed out well inside that deadline. |
Both are label-less. The repository dimension an alert would want is what S03-B keeps off metric labels, so that dimension stays on the log line.
The deadline-kill counter is the one that carries weight: the outcome code its event records is
internal_server_error, which on the tarball route is shared with the generic internal-error
writer and with a cache-sourced body fault, so gitlab_artifact_registry_npm_request_total
measures the route's 500s and not its truncations. Separating them is what lets the Error line
beside the event be rationed to one per repository per minute per process without losing the count.
🏷️ The four revised rows
Commit d4832c1 renames three of the five label names on the original four rows and brings their
value sets up to what the implementation emits. These came out of
!1878 (merged)'s review, which
implements the metrics; Step 15's Files list in docs/plans/2026-07-15-npm-remote.md does not name
the spec, so the edit had nowhere else it was already scheduled to land.
| Row | Was | Is |
|---|---|---|
..._upstream_responses_total |
operation, four outcome values |
npm_operation, seven outcome values |
..._cache_events_total |
path |
npm_cache_path |
..._cache_fill_bytes |
artifact |
npm_cache_artifact |
outcome and event keep their bare names, because their values do mean the same thing in both
remote proxies, and joining the union is what makes a cross-format query readable.
Nothing is scraping the old names. None of these three metrics exists in code on main yet:
upstream_responses_total, cache_events_total, and cache_fill_bytes are implemented for Maven
only (internal/format/maven/remote_metrics.go), and the npm ones land in !1878 (merged). So these are
renames on a spec-only surface, not a breaking change to a shipped label. That was the reviewer's
stated worry about letting them ride along, and it is the reason it does not apply here.
Why the renames are needed at all. internal/metrics/cardinality.go keys both the cardinality
budget and the pinned closed value set by label name, globally across every package. cache_path
is already pinned there to Maven's {artifact, metadata, sidecar}, and npm's cache paths are
{packument, dist_tags, tarball}, a disjoint set. A bare path or cache_path on the npm rows
would therefore have to join Maven's closed set, widening both and making neither readable.
One caveat on the precedent, since it does not cover all three: maven_operation is a real
precedent for npm_operation, but Maven labels its own cache rows with a bare cache_path, so
npm_cache_path and npm_cache_artifact are not following Maven. They are forced by the
pinned-set collision above. The format-scoped spelling is the fix; Maven's bare spelling is the
thing that made the collision possible.
🔁 Review rounds
Seven Duo findings, six adopted and one declined.
First round, on the two new rows:
- The arm-failure row packed its scope note into the event description. The row now names the three routes that share the counter, which is what an operator wants when the series moves.
- The prose said "the half that leaves no trace" without saying what the split was over, so it only
read to someone who had already opened
armRemoteReadResponseDeadline. Both outcomes are named in the spec now; only the budget arithmetic stays on the doc comment. That reverses this MR's earlier position, which was that the cost belonged on the code alone. - "The request counter" named nothing this spec defines. It is
gitlab_artifact_registry_npm_request_total, cataloged in docs/dev/observability.md, and the sentence now says so and links there. - Declined: re-wrapping the new paragraphs to one sentence per source line. The premise was that
the rest of the document already reads that way, and it does not: 231 of
S15-npm-remote.md's 369 prose lines end mid-sentence and its longest is 102 columns, so it is hard-wrapped at 100. The repo's specs are split on which half of the style guide's rule 14 they honour, and the new prose matches this file. The thread is left open for a maintainer to overrule.
Second round, on the four revised rows:
- The revisions were unannounced in this description, which framed the MR as additive-only. The section above is that callout. Splitting them into their own MR would conflict line-for-line with this one, since both edit the same four table rows, and this MR is already marked as blocking !1878 (merged).
- The
outcomeclause was a comma splice, and it also said the maven counter "pins" the shared set.internal/metrics/cardinality.gois what pins the name globally; the maven counter emits seven of the values in it. The clause now says "emits", which fixes both at once. - "By the read that led the flight" was a one-off against the leader/follower vocabulary the rest of the spec uses. It now names the single-flight leader, and says what the old wording only implied: a follower books no fill.
Third round, on the new prose:
- "The outcome code the same event records" left its antecedent to the reader, and the nearest
candidate is the
npm_remote_proxywide event in the section below. The suggested fix was to name that event, which would have been wrong:recordOutcomeCodestores the code on the per-request observation, where the instrument wrapper reads it asnpm_request_total'scodelabel and the completion log line, and the wide event has no emitter in the tree yet. The sentence now says the tarball route records the code, and the later "theErrorline beside the event" got the same treatment.
Also fixed in the first round, found while re-reading the pair rather than raised by Duo: the kill
row claimed a narrower trigger than the counter has. The arm books any timeout-shaped write
failure, so an upstream body that stalls out upstream.request_total_timeout (30s) increments it
with the armed instant still minutes away, and a rise does not on its own mean the response budget
is too small.
🔗 Merge order
Independent of !1836 (merged) in both directions: this MR touches only docs/specs/S15-npm-remote.md, and
!1836 (merged) does not touch that file. !1836 (merged) has since merged, so the two new counters are already on
main and this MR is the spec catching up to them rather than describing them ahead of time.
That also gives the wording a check: !1836 (merged) added both counters to docs/dev/observability.md as rows attributed to S15, and the two new rows here now agree with that catalog on the kill counter's trigger. Before this round they did not.
The revised rows run the other way round, and this MR is marked as blocking !1878 (merged) for it: merging this first keeps the spec and the emitted label names in agreement, and merging after would leave a window where the table names labels the code no longer emits.
⚠️ For the reviewer
!1834 (merged) and
!1789 (merged) also touch
docs/specs/S15-npm-remote.md, at lines 601/838 and elsewhere respectively; this MR's hunk is in
the Observability section around line 893, so there is no line overlap. Whichever lands second
rebases cleanly.
Related to #793 (closed)