feat(oci): container-remote observability collectors and wide event (S16 plan: 17/26)
Reviewable size
18 files, +4,423 −153, measured three-dot against main — 3,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
- Nine collectors, registered and audited. All nine
oci_remote_*families the spec's Observability section declares, added to the registration slice inmetrics.goand given a budget and a pinned closed set per label name ininternal/metrics/cardinality.go. - Five families get their emitters here.
token_exchanges_total,token_exchange_duration_seconds,token_cache_events_total,auth_url_rediscoveries_totalandauth_discovery_writes_total, wired at the token-exchange, token-cache and challenge-discovery flows Step 12 merged.auth_discovery_writes_totalhad a spec row and a catalog row since Step 9 and no emitter; this is where it gets one. - Four families are declared but not yet emitted.
upstream_responses_total,cache_events_totaland 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. - The
oci_remote_proxywide event, with three of its six operations live.token_exchange,tags_listandreferrersemit from this branch;fetch,manifest_proxyandblob_proxyare declared for 17-2. - 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.
remoteFillArtifactis a distinct type fromremoteCacheArtifact, which is what makestagunrepresentable 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 bare —
outcome,event,challenge,operation,artifact. Both merged siblings format-scope their route labels andcardinality.gostates a "route vocabulary gets its own name" rule, so this diverges on two labels. The reason is local consistency: every label already ininternal/format/oci/metrics.gois 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 addprobeto a then-globaloperationset. Review asked where a reader finds that argument once the branch is squashed, so it is in the tree now:docs/dev/observability.mdgains a Label naming section under## Authoring metricscarrying the rule and this exception, andcardinality.gocites it. The old citation pointed at a document with no naming section, so it led back to itself. remote_repository_idcarriescontainer_remote_repositories.id, not the parentrepositories.id. This disagrees withmaven_remote_proxyandnpm_remote_proxy.docs/dev/observability.mdexplicitly 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_statusrenders asfields.HTTPStatusCode, which isstatus. LabKit v2.35.8 lists"http_status"in itsDeprecatedmap (fields/fields.go:229), and S03-A callsstatusLabKit'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: thechallengelabel'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.gois in the diff and 17-1's Files cell does not name it. The plan namedremote_taglist.gowhere the work was;remote_referrers.gois what needed therecordRemoteProxyDigestcall past its own digest gate, and thetags_listrecord begins inremote_list_handler.go. Both files are now in the diff regardless, because theINTERNALfix below reaches two sites inremote_taglist.goand one inremote_referrers.go.docs/dev/observability.mdis 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'supstream_responses_totaloutcome set was missingunsupported_content, which the spec declares. Six places corrected.- Five companion test suites are in no Tests entry —
metrics_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=2route, so it cannot cover this. - e2e scenario catalogs.
docs/testing/e2e/oci.mdputs remote repositories out of scope until the capability ships, so no scenario is added or affected. - Configuration reference. No
proto/,internal/config/orconfig.example.yamlchange. - Bruno / OpenAPI. No operation added, renamed, or removed.
- Database. No schema, migration, or query change, so no
EXPLAINevidence is owed.
Related to #288