feat(oci): container-remote observability collectors and wide event (S16 plan: 17/26)

Reviewable size

18 files, +4,423 −153, measured three-dot against main3,143 reviewable Go lines with blanks and comment-only lines stripped. Past the 500 ceiling in development-model.md, so the split:

Group Lines
Tests +3,193 −20 — remote_metrics_test.go 1728/0, remote_wideevent_test.go 747/0, remote_list_handler_test.go 431/0, remote_auth_test.go 154/0, remote_list_internal_test.go 69/0, remote_taglist_test.go 35/20, metrics_helpers_test.go 29/0
Production Go — new files +953 −0 — remote_metrics.go 580/0, remote_wideevent.go 373/0
Production Go — call sites +238 −116 — remote_auth.go 103/19, internal/metrics/cardinality.go 41/31, remote_list.go 33/10, remote_tokencache.go 25/21, metrics.go 13/17, remote_list_handler.go 13/3, remote_referrers.go 8/13, remote_taglist.go 2/2
Observability catalog +39 −17

Why not split further. Seven tenths is coverage, and the step has already been split once — this is 17-1 of two, cut at the file-conflict surface rather than at the dependency, for the reason the plan's Step 17 entry gives. The remaining production surface is two new files that reach nothing outside themselves plus 238 lines of call sites across eight files, and there is no second seam inside it: a collector and the label budget that admits it have to land together or AuditCardinality fails, and splitting the budgets out would leave a budgets-only MR red until the collectors arrived.

Against the merged siblings, this is proportionally smaller per metric. npm's whole observability surface — remote_metrics.go, remote_observe.go and both suites — is 2,931 lines for 4 metric families. This MR's four equivalent files are 3,428 lines for 9, and it carries three families npm and Maven have no analogue for at all (token exchange, token cache, challenge discovery).

Why

Step 17-1 of the S16 container remote plan, the first of the step's two MRs. It lands the whole definition surface — nine oci_remote_* collectors, their closed label vocabularies, the cardinality budgets and the oci_remote_proxy wide-event builder — plus the call sites that already exist on main. 17-2 adds the call sites that live in the six files Steps 14 and 15 are still reworking.

Review this against the Step 17 entry in !1939, not main's. main still costs Step 17 as one MR with no per-part table, so it gives no Files list, no criteria allocation and a ~550 estimate for what is now two MRs.

What

  1. Nine collectors, registered and audited. All nine oci_remote_* families the spec's Observability section declares, added to the registration slice in metrics.go and given a budget and a pinned closed set per label name in internal/metrics/cardinality.go.
  2. Five families get their emitters here. token_exchanges_total, token_exchange_duration_seconds, token_cache_events_total, auth_url_rediscoveries_total and auth_discovery_writes_total, wired at the token-exchange, token-cache and challenge-discovery flows Step 12 merged. auth_discovery_writes_total had a spec row and a catalog row since Step 9 and no emitter; this is where it gets one.
  3. Four families are declared but not yet emitted. upstream_responses_total, cache_events_total and the two fill histograms have no call site until 17-2. A Prometheus collector produces no series until its first observation, so a label value declared ahead of its call site changes nothing a scrape returns — and the catalog says, per family, that the absence is this gap rather than a fleet doing no work.
  4. The oci_remote_proxy wide event, with three of its six operations live. token_exchange, tags_list and referrers emit from this branch; fetch, manifest_proxy and blob_proxy are declared for 17-2.
  5. Enum guards that fail closed. Every emitter refuses a label value outside its declared domain and records an Error instead of emitting, so no dynamic string can reach a label. remoteFillArtifact is a distinct type from remoteCacheArtifact, which is what makes tag unrepresentable on the two fill histograms rather than merely untested.

Worth a reviewer's attention

Three calls the spec and the docs left open are decided here. Each disagrees with something, on purpose:

  • Label names stay bareoutcome, event, challenge, operation, artifact. Both merged siblings format-scope their route labels and cardinality.go states a "route vocabulary gets its own name" rule, so this diverges on two labels. The reason is local consistency: every label already in internal/format/oci/metrics.go is bare (manifest_type, push_step, error_code, extract_reason), and a file where two conventions sit side by side is worse than one that picks the wrong one. The cost lands on S32, which is specced to add probe to a then-global operation set. Review asked where a reader finds that argument once the branch is squashed, so it is in the tree now: docs/dev/observability.md gains a Label naming section under ## Authoring metrics carrying the rule and this exception, and cardinality.go cites it. The old citation pointed at a document with no naming section, so it led back to itself.
  • remote_repository_id carries container_remote_repositories.id, not the parent repositories.id. This disagrees with maven_remote_proxy and npm_remote_proxy. docs/dev/observability.md explicitly deferred the choice to whoever wired the emitter; the container request corpus already fixes that id, so matching it is what makes a record joinable to the rest of the corpus.
  • http_status renders as fields.HTTPStatusCode, which is status. LabKit v2.35.8 lists "http_status" in its Deprecated map (fields/fields.go:229), and S03-A calls status LabKit's canonical key for that field (docs/specs/S03-a-logging.md:173-175), so the spec's literal wording is a text defect rather than a constraint. The spec correction lands separately, on !1939, which now also carries two corrections the review of this MR surfaced: the challenge label's gloss and the duration family's coverage. The catalog no longer describes the disagreement as a debt the spec owes — that sentence would have gone stale the moment the amendment merged — and states the durable reason instead: LabKit retired the key, so a record spelling it that way writes a key the library has dropped.

A 17-2 acceptance criterion lands here, deliberately. !1939's Step 17 entry allocates the health-probe exclusion to 17-2, on the grounds that it is vacuous while upstream_responses_total has no emitter at all. TestContainerHealthProbe_IsNotCountedOnUpstreamResponses asserts it in this MR anyway — it drives the container probe against four upstream statuses and asserts the family total does not move. Coverage landing ahead of its part, not the split slipping. Recorded in #873's "already discharged" section so 17-2 does not redo it.

Ten dead //nolint directives are knowingly left in place. This MR deletes three directives that suppress nothing, so the ten it leaves will read as an inconsistency. Eight //nolint:thelper in remote_taglist_test.go and two //nolint:gosec in remote_auth_test.go all predate the branch and all reproduce as unused under nolintlint. Nine of them sit at the foot of a doc comment, and scripts/ci/check-comment-caps.sh is a ratchet: deleting a directive there counts as touching the block, which exposes nine pre-existing prose blocks of 6 to 14 lines against a test-file cap of 2. Keeping the bare // separator dodges the checker until gofmt strips it and puts the failures back — all three states measured. So the real cost of the ten-line deletion is compressing nine test doc comments down to two lines each, in a file this step otherwise barely touches. That belongs to #682.

Four 500 sites now book their envelope code, which review surfaced. A live list answering 500 through writeRemoteReadInternal recorded status=500 with an empty oci_code, while the client's envelope carried INTERNAL. The derived outcome was still error, so nothing was wrong in aggregate — but a reader correlating a record against the response it describes saw a code go missing. writeRemoteListInternal now sits beside writeRemoteListEnvelope as its 500 counterpart and the four sites route through it.

The review note named one of the four. The other three are the same shape and were found by asking which callers run after beginRemoteListProxyRecord, so fixing only the flagged one would have left the catalog's own claim about this gap wrong in a new way. writeRemoteReadInternal itself is still uninstrumented, deliberately: its one remaining list-arm caller runs while the repository row is still resolving, ahead of the record, so there is nothing to book a code on, and instrumenting the shared writer belongs with the manifest and blob arms.

A comment-compression pass rides along inside the files this step touches. The branch carries a commit bringing container-remote comments to the caps in go-style.md; the visible effect is that metrics.go loses a 25-line package doc and remote_referrers.go a 12-line digest-gate rationale. Behavior-neutral, but it is why two files show deletions well past what their call-site edits need.

remote_auth.go adds remote_repository_id to three existing log sites. Request-corpus alignment rather than metric work, so it sits just outside the step's stated scope. It is here because it is the same consistency argument the remote_repository_id decision above makes, and splitting one field across two MRs would leave the corpus half-aligned for the length of the stack.

Two arms are 17-2's even though this MR owns their proxies. The shared relay writer is in remote_relay.go, one of the six files Steps 14 and 15 rework, so the two live lists emit at the proxy rather than inside the writer. observeRemoteUpstreamResponse also needs an OCI classifier that does not exist yet — npm's classifyRemoteUpstream is the model but cannot be lifted, because OCI's outcome domain adds unauthorized, denied and unsupported_content and drops cache_error.

Divergence from the plan's Files list

  • remote_referrers.go is in the diff and 17-1's Files cell does not name it. The plan named remote_taglist.go where the work was; remote_referrers.go is what needed the recordRemoteProxyDigest call past its own digest gate, and the tags_list record begins in remote_list_handler.go. Both files are now in the diff regardless, because the INTERNAL fix below reaches two sites in remote_taglist.go and one in remote_referrers.go.
  • docs/dev/observability.md is in no Step 17 Files entry, and the step's Note says the catalog "already carries every metric the spec declares", so this step consumes those entries rather than adding them. That was very slightly false: the catalog's upstream_responses_total outcome set was missing unsupported_content, which the spec declares. Six places corrected.
  • Five companion test suites are in no Tests entrymetrics_helpers_test.go, remote_list_handler_test.go, remote_list_internal_test.go, remote_auth_test.go, remote_taglist_test.go. Each is the suite for a production file 17-1 edits; 17-1's Tests entry names only the two new files.

All three want a follow-up on !1939 rather than a change here.

Plan changes are in a separate MR

Per the guardrail that step MRs do not edit the plan file, this branch carries none. The Status row and the three corrections above go to !1939, the standing amendment MR for Step 17; !1831 keeps Step 14's.

Merge order

Targets main and is ready to review on its own — nothing precedes it. Every metric it wires is emitted from flows Step 12 merged or from the two list proxies Step 16 merged.

This MR must merge before !1939, or !1939 must be amended first. This branch narrows challenge="unreadable" to a 401 discovery response and rewords the bearer gloss to cover the write that attempted an auth_url. !1939 is Draft and rewrites docs/specs/S16-container-remote.md:1054 and :1065, whose replacement text still defines the label as the parser's outcome and repeats the bearer over-claim. If !1939 lands first, main carries a spec the code disagrees with until the correction reaches it.

17-2 opens once the last of !1873 (merged), !1875, !1894 (merged), !1903 and !1904 has merged. Those five rework remote_operations.go, remote_errors.go, remote_serve.go, remote_relay.go, remote_manifest.go and remote_blob_serve.go between them, which is where 17-2's call sites go. Until then a call site added to any of the six on a branch off main conflicts with whichever change lands second, and no pipeline reports it — which is what drew the cut here.

Open MRs overlapping this branch's paths:

  • !1880 (merged) (docs/dev/observability.md) — npm's equivalent observability step. Both edit the catalog, in different rows.
  • !1896 (merged) (remote_taglist_test.go).

Whichever lands second rebases.

Run recipe

No change owed. The MR adds nine collectors to the registration slice, which touches boot, but it changes nothing about how the service builds or dispatches a request and no recipe row describes the metrics surface. driver.sh smoke passes 72/72 on this branch. The review round that followed that run touched remote_auth.go, remote_list.go and remote_metrics.go and changed no route, boot path, or configuration; it has not been re-run since.

Worth knowing when reading /-/metrics after a boot: none of the nine families appears there until something observes on it, which is how Prometheus vectors behave and not a wiring fault. The five families this MR emits on need a token exchange, a token-cache event or a challenge discovery to have happened first.

Guardrails with nothing to add

  • Conformance tests. The OCI conformance suite runs against hosted repositories and cannot reach a kind=2 route, so it cannot cover this.
  • e2e scenario catalogs. docs/testing/e2e/oci.md puts remote repositories out of scope until the capability ships, so no scenario is added or affected.
  • Configuration reference. No proto/, internal/config/ or config.example.yaml change.
  • Bruno / OpenAPI. No operation added, renamed, or removed.
  • Database. No schema, migration, or query change, so no EXPLAIN evidence is owed.

Related to #288

Edited by Radamanthus Batnag

Merge request reports

Loading
Loading