refactor(npm): retire the version field from npm_remote_proxy (S15 observability plan: 2/5)

Dzmitry (Dima) Meshcharakourequested to merge
dmeshcharakou/npm-remote-observability-step-2 into main

What this does

Plan step 2 of the npm remote observability plan: it retires the version field from the npm_remote_proxy wide event.

No code path ever set the field. It was declared as a coordinate whose emitter had yet to be written, and the ### Emitter coverage inventory carried a row saying it is never rendered. There is nothing to close that gap with: the tarball route is the only reader that resolves a version, and it stamps that version on the per-request completion line rather than on the record, deliberately (logPackageVersion, from RemoteTarballHandler.resolveTarget). A metadata route addresses a document rather than a version and has no version to resolve at all.

Keeping the declaration left the inventory promising a coordinate the format cannot supply, which is the shape an operator reads as an instrumentation gap and waits on. The two coordinates the record does carry, package and file_name, already name what each read asked for.

Reviewable size

110 LOC across 7 files (70 added, 40 removed), well under the 500 ceiling.

The branch was rebased with git rebase --onto origin/main 73bf38618 before this MR opened. Its earlier form carried the seven commits of step 1b, which main already has squashed as df27f910d (!2076 (merged)), and measured 1767 LOC. Reviewing this against the step-1b content would be reviewing it twice.

Spec

Amends docs/specs/S15-npm-remote.md, which the merged plan's step-2 Files entry authorizes explicitly. The reversal is recorded in the spec's ## Resolutions rather than applied silently, so a reader cross-referencing the two documents can tell a deliberate correction from a transcription error.

Also drops the version row from docs/dev/observability.md and internal/format/npm/metrics.md, the other two places the field was catalogued.

The goconst directive

internal/format/npm/packument.go carried a //nolint:goconst whose justification named fieldKeyVersion as the constant match-constant had matched. That constant is what this step deletes, and deleting it takes the package's non-test "version" literals from three to two, below min-occurrences: 3. The directive now suppresses nothing, so it is removed rather than reworded.

Measured with the pinned golangci-lint 2.13.2 and the repo config: with the directive stripped, goconst reports nothing on packument.go; with it stripped and two more non-test "version" literals added, it reports packument.go:42: string "version" has 4 occurrences. The second run is the control that proves the linter ran, so the first run's silence is a real absence rather than an invocation that analysed nothing. The count is non-test only, because .golangci.yaml excludes _test.go$ from goconst.

Commit a36ca6b0a's body reaches the opposite conclusion, and 6b08922c3 supersedes it. That body keeps the directive on the grounds that it "still fires" once fieldKeyVersion is gone, because the path: _test.go$ exclusion drops findings located in a test file without taking that file's literals out of the occurrence count or its constants out of the matching. The control run above is what settles it the other way: the count it reports is 4, the two map literals plus the two added, while the package's test files hold dozens more "version" literals. So test-file literals never reach the total and testFieldVersion is not a constant goconst can match. The branch squashes on merge, so neither body reaches main; this note is for anyone reading the commit list, and it is the reading to trust over a36ca6b0a's.

Dropping the directive's paragraph put the whole fullVersionFields doc block through the comment-caps gate, which allows an unexported top-level doc one line. f5e375fe6 names the dist exclusion on the line that survived: it pointed at the spec's "Allowed package.json fields" list, which includes dist, while the map omits it, so read on its own the line was wrong about the set it describes.

Two files the plan's step 2 does not name

The plan's step-2 Files entry names remote_observe.go, metrics.md, observability.md and the S15 spec, and its Tests line names remote_observe_test.go. This MR touches two more, both forced by the retirement rather than picked up alongside it:

  • internal/format/npm/packument.go, for the //nolint:goconst above. Its justification named fieldKeyVersion, the constant this step deletes, and a comment may not cite a symbol its own MR removed.
  • internal/format/npm/remote_metrics_test.go, whose high-cardinality guard told every forbidden label that it "belongs on the npm_remote_proxy wide event". version is in that list and stops being a field of the event here, so the message now reads "an unbounded coordinate belongs on the npm_remote_proxy wide event or nowhere, never on a metric label", which holds for every label in the list. version stays forbidden, because no metric should carry it.

!2189 (merged) widens the step-2 Files entry to name both, alongside the Status row it records.

Testing

TestRemoteProxyEvent_CarriesTheSpecFieldSet pins the whole rendered key set against a literal list, so a widened record fails it; TestRemoteProxyEvent_RendersNoVersionField asserts the absence by name. Both landed ahead of the implementation and the field's removal is what turned them green.

Both assert off fullRemoteProxyEvent, which is one operation="tarball" record. That single record settles every operation, but by construction rather than by enumeration: after this step remoteProxyEvent has no version member and emitRemoteProxyEvent branches on no operation at all, so there is no arm an operation could reach that the fixture does not.

Commit 9caad3c04's body and its spec-coverage table say the absence is asserted "on every operation", which reads as a claim that the test iterates remoteProxyOperations(). It does not, and this paragraph supersedes that wording. The distinction is for whoever adds the first operation-dependent render branch: on that day neither assertion reaches the new arm, and both need a case for it.

go test ./internal/format/npm/ passes. golangci-lint reports 0 issues on the package.

Related

Maintenance, so no issue link is owed.

Edited by Dzmitry (Dima) Meshcharakou

Merge request reports