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 outcome clause was a comma splice, and it also said the maven counter "pins" the shared set. internal/metrics/cardinality.go is 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_proxy wide event in the section below. The suggested fix was to name that event, which would have been wrong: recordOutcomeCode stores the code on the per-request observation, where the instrument wrapper reads it as npm_request_total's code label 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 "the Error line 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)

Edited by Dzmitry (Dima) Meshcharakou

Merge request reports

Loading
Loading