Instrument lockfile discovery, refresh, and validation (#91) - #134
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideAdds duration aggregation support to the metrics utility and instruments lockfile discovery, refresh, and validation with counters and duration metrics, including tests and developer documentation updates. Sequence diagram for lockfile refresh metrics instrumentationsequenceDiagram
participant Caller
participant lockfile
participant runner
participant metrics
Caller->>lockfile: refresh_lockfile(manifest_path, runner)
lockfile->>runner: runner(cargo generate-lockfile ...)
runner-->>lockfile: exit_code, stdout, stderr
lockfile->>metrics: observe_duration(REFRESH_DURATION_METRIC, elapsed_seconds)
alt exit_code != 0
lockfile->>metrics: increment_counter(REFRESH_METRIC, outcome=failure)
lockfile-->>Caller: raise LockfileRefreshError
else exit_code == 0
lockfile->>metrics: increment_counter(REFRESH_METRIC, outcome=success)
lockfile-->>Caller: Cargo.lock path
end
Sequence diagram for lockfile validation metrics instrumentationsequenceDiagram
participant Caller
participant lockfile
participant runner
participant metrics
Caller->>lockfile: validate_lockfile_freshness(manifest_path, runner)
lockfile->>runner: runner(cargo metadata --locked ...)
runner-->>lockfile: exit_code, stdout, stderr
lockfile->>metrics: observe_duration(VALIDATE_DURATION_METRIC, elapsed_seconds)
lockfile->>lockfile: determine state (fresh|stale|failed)
lockfile->>metrics: increment_counter(VALIDATE_METRIC, outcome=state)
lockfile-->>Caller: ValidationResult(state)
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
e21f8e6 to
1de95c9
Compare
9e0371d to
d79beb9
Compare
2e19cd5 to
2547eaf
Compare
lockfile.py logged decisions but offered no structured metrics, so operators could not track lockfile throughput or error trends. Extend lading.utils.metrics with duration aggregation (observe_duration / duration_stats; count and total seconds per label set, rendered in the exit summary) and instrument the lockfile boundary: - lockfile.discovered counts tracked lockfiles found per discovery - lockfile.refresh counts each refresh_lockfile invocation with an outcome label (success/failure); lockfile.refresh.duration times the cargo generate-lockfile call - lockfile.validate counts each freshness probe with an outcome label (fresh/stale/failed); lockfile.validate.duration times the cargo metadata --locked call Metric names and labels are documented in the developers' guide. Builds on the metrics accumulator introduced for issue #68. Closes #91
d79beb9 to
7a3ad43
Compare
The lint gate obtained ruff two ways: `uv tool install ruff` (latest) and the unpinned dev dependency, which uv.lock resolved to 0.13.3 and `uv sync` placed in the virtualenv. `make lint` runs bare `ruff`, which in CI resolved to the virtualenv's 0.13.3 while developers ran 0.15.12 from PATH. The preview-only rule RUF076 (autouse-fixture warning) exists in 0.13.3 but not 0.15.12, so CI failed on legitimate state-isolation fixtures that lint clean locally. Pin ruff to 0.15.12 in both the dev dependency group and the CI uv-tool install so every environment runs the identical linter, and regenerate uv.lock to match. The autouse fixtures are intentional per-test registry/env isolation and are left unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Count publish index-lookup downgrade events The downgrade decision in _handle_index_missing_version (a crates.io index-lookup failure downgraded to a warning because the missing dependency is part of the publish plan) was visible only in logs, so operators could not track how often releases rely on the override. Add lading.utils.metrics: a process-local accumulator with labelled counters flushed as one structured JSON log line at interpreter exit. The deliberate backend choice over prometheus_client or statsd is documented in the module docstring and developers' guide: a lading run is a short-lived CLI process whose logs are already aggregated, so the summary line is the operational boundary and the registry gives tests a deterministic seam. The downgrade success path increments publish.index_lookup_downgrade with subcommand and missing_crate labels. Unit tests cover the accumulator itself and verify the counter increments exactly once on the downgrade path and never on the raise paths. Closes #68 * Extract _emit_downgrade_success to cut method length _handle_index_missing_version sat at exactly 70 lines, meeting (not beating) CodeScene's large-method threshold. The downgrade-success tail (the metric increment plus its three log statements) is a cohesive unit, so extract it into a new private _emit_downgrade_success helper invoked once the in-plan, override-enabled downgrade has been confirmed. The helper bundles the (current_index, missing_index, missing_canonical_name) triple returned by _validate_dependency_placement as a single placement argument, matching this module's parameter-object convention and keeping the call surface within the four-argument ruff and five-argument Pylint limits without suppression. Behaviour is unchanged: the metric labels and all three log payloads are byte-identical, and the existing publish_index_check tests pass without modification. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Make metrics atexit flush explicit and broaden test coverage Address review feedback on the publish index-lookup downgrade metric. Unit Architecture (error): the metrics summary flush was registered with atexit as an import-time side effect of lading.utils.metrics, making the exit-time behaviour invisible and non-explicit. Move registration into an idempotent register_summary_atexit() helper and call it from the CLI bootstrap (main), so the lifecycle is an explicit decision. Convert the _CounterKey alias to a PEP 695 `type` statement. User-facing documentation: document the "lading metrics summary" JSON INFO line operators may see at exit, and the publish.index_lookup_downgrade counter and its labels, in docs/users-guide.md. Testing (compile-time/UI): snapshot the rendered emit_summary payload with syrupy instead of hand-asserting the parsed structure, locking the exact output format including counter ordering and label-key normalisation. Testing (property/proof): add Hypothesis property tests for the accumulator covering label-order invariance over arbitrary permutations, accumulation over random increment sequences, and snapshot copy isolation. Testing (unit/behavioural): add an end-to-end test driving publish.run() through the real index-failure parsing and downgrade path, then asserting emit_summary() renders the counter. Also unit-test the new idempotent atexit registration. The accumulator framework is retained deliberately: the documented backend decision (issue #68) treats the in-process registry as the operational boundary and the deterministic test seam these tests rely on. Moving the atexit hook into explicit bootstrap resolves the hidden-lifecycle concern without collapsing that seam. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Add Hypothesis property tests for the metrics accumulator Express the accumulator's algebraic invariants as Hypothesis properties in test_metrics.py: - test_label_order_invariance: counter_value is invariant under any permutation of the label kwargs. - test_accumulation_sums_increments: a counter equals the sum of its increment amounts. - test_snapshot_is_immutable_copy: mutating the registry after snapshot() does not disturb the captured copy. Adaptations from the requested draft, required to keep the lint and test gates green: - Draw permutations via Hypothesis (st.permutations) instead of random.shuffle, which trips ruff S311 and is non-reproducible. - Use operator.itemgetter(0) for unique_by (ruff FURB118). - Exclude the reserved label keys "name" and "amount", which collide with increment_counter's own parameters and would otherwise crash the generated examples. - Suppress the function_scoped_fixture health check, since the module's autouse reset fixture is function-scoped while each property resets the registry per example. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Document metrics output under a users-guide Observability section Add a dedicated "Observability" reference subsection to the configuration reference in docs/users-guide.md, covering the structured JSON metrics summary emitted at INFO before process exit and documenting the publish.index_lookup_downgrade counter and its subcommand/missing_crate labels. Consolidate the earlier inline "Metrics summary at exit" note under the --allow-unpublished-workspace-deps section into a one-line cross-reference to the new Observability section, so the format and label reference live in a single canonical place rather than being duplicated. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Fix developers-guide merge artifacts from rebase The rebase onto main left docs/developers-guide.md with a duplicated "Workspace path normalization" section and two double-blank-line runs, which tripped markdownlint MD024 (duplicate heading) and MD012 (multiple consecutive blank lines). Remove the duplicate section and collapse the stray blank lines. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Address review feedback on downgrade metric docs and tests Four review findings, all verified against current code: - docs/users-guide.md: the Observability section referenced a non-existent `lading package` command. The CLI exposes only `bump` and `publish` (cargo package/publish run under `lading publish`), so describe the summary as emitted by `lading publish`. - publish_index_check.py: replace the raw `tuple[int, int, str]` placement contract with a `_DependencyPlacement` NamedTuple (current_index/missing_index/missing_canonical_name). _emit_downgrade_success now reads named fields and _validate_dependency_placement returns the NamedTuple; _IndexMissingVersionHandling and CargoIndexLookupFailure are unchanged. A NamedTuple subclasses tuple, so the contract is explicit without breaking any unpacking. - test_downgrade_metrics.py: drop the brittle metrics.snapshot() internal-key assertion from test_downgrade_path_increments_counter; the public counter_value check covers the behaviour and label normalisation is exercised by the metrics property tests. - test_downgrade_metrics.py: parametrise test_raise_paths_do_not_increment_counter into flag_disabled and out_of_plan cases. The boolean parameter is keyword-only (FBT001-clean; pytest fills test parameters by keyword). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Close TOCTOU race in register_summary_atexit The double-registration guard read _summary_hook_registered.is_set() and called .set() as two separate steps, so two concurrent callers could both observe the event unset and each register emit_summary with atexit. Wrap the check-and-set together under _LOCK so the guard is atomic; atexit.register stays outside the lock (it does not re-enter _LOCK, so there is no deadlock). Behaviour is unchanged for the single-threaded bootstrap path: registration remains idempotent and emit_summary is registered at most once. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Tighten atexit registration locking and metric docs Three review findings, all verified against current code: - metrics.py: move atexit.register(emit_summary) inside the _LOCK critical section in register_summary_atexit. With it outside, a second thread could observe the flag already set and return before the first thread finished registering, so registration was not atomic with the flag flip. atexit.register does not re-enter _LOCK (it only stores the callable; emit_summary acquires the lock later, at exit), so holding the lock here cannot deadlock. - users-guide.md: the metrics summary example used a ```text fence; the documentation style guide mandates `plaintext` for non-code text. - users-guide.md: the publish.index_lookup_downgrade counter increments on each downgrade event, not once per crate (a single crate can be downgraded in both the package and publish phases), so reword "once per crate" to "on each downgrade event". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Instrument lockfile discovery, refresh, and validation (#91) (#134) * Instrument lockfile discovery, refresh, and validation lockfile.py logged decisions but offered no structured metrics, so operators could not track lockfile throughput or error trends. Extend lading.utils.metrics with duration aggregation (observe_duration / duration_stats; count and total seconds per label set, rendered in the exit summary) and instrument the lockfile boundary: - lockfile.discovered counts tracked lockfiles found per discovery - lockfile.refresh counts each refresh_lockfile invocation with an outcome label (success/failure); lockfile.refresh.duration times the cargo generate-lockfile call - lockfile.validate counts each freshness probe with an outcome label (fresh/stale/failed); lockfile.validate.duration times the cargo metadata --locked call Metric names and labels are documented in the developers' guide. Builds on the metrics accumulator introduced for issue #68. Closes #91 * Pin ruff to 0.15.12 across CI, lockfile, and dev PATH The lint gate obtained ruff two ways: `uv tool install ruff` (latest) and the unpinned dev dependency, which uv.lock resolved to 0.13.3 and `uv sync` placed in the virtualenv. `make lint` runs bare `ruff`, which in CI resolved to the virtualenv's 0.13.3 while developers ran 0.15.12 from PATH. The preview-only rule RUF076 (autouse-fixture warning) exists in 0.13.3 but not 0.15.12, so CI failed on legitimate state-isolation fixtures that lint clean locally. Pin ruff to 0.15.12 in both the dev dependency group and the CI uv-tool install so every environment runs the identical linter, and regenerate uv.lock to match. The autouse fixtures are intentional per-test registry/env isolation and are left unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Pin Ruff to 0.15.12 in the Makefile and CI The Makefile invoked a bare `ruff` from PATH and CI installed it unpinned (`uv tool install ruff`), so the Ruff version used for formatting and linting drifted with whatever happened to be installed. Pin it to 0.15.12 in both places: - Makefile: add a RUFF_VERSION variable and resolve Ruff through `uv tool run --from ruff==$(RUFF_VERSION) ruff`, mirroring how PYLINT is pinned via PYLINT_PYPY_SHIM_REF. The fmt, check-fmt, and lint recipes now run the pinned Ruff, and `ruff` is dropped from the PATH-tool existence checks (it is resolved through uv instead). - CI: install `ruff==0.15.12` rather than the latest release. mbake validates the Makefile; `make check-fmt` and `make lint` pass with the pinned 0.15.12 toolchain. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Record metrics backend in an ADR and harden atexit registration Address three review findings: - Developer documentation: add ADR-004 recording the system-design decision for the in-process lading.utils.metrics backend (atexit flush, explicit bootstrap registration, registry-as-test-seam) and the publish.index_lookup_downgrade metric contract. Index it in docs/contents.md and link it from the developers-guide metrics section. - Concurrency / atexit ordering: register_summary_atexit already performed the check-and-register under _LOCK (closing the interleaving the review described as a register-outside-the-lock window), but it set the guard flag before calling atexit.register. Reorder so the flag is set only after a successful atexit.register, leaving a failed registration unset and retryable. Add a failure-path test (register raises -> flag unset -> later call registers) and a concurrency test (8 threads released on a barrier register exactly once). - Observability cardinality: the missing_crate label is kept verbatim rather than bucketed/capped. ADR-004 records why: the accumulator is process-local and flushed once per run, so cardinality is bounded by the crates in that run (not an unbounded external key space), and the crate name is the actionable detail an operator needs. Bucketing would be revisited only if the metrics are ever exported to a long-lived time-series backend. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Address metrics review: guard zero-count, harden tests, dedup properties - users-guide.md: the Observability lead-in scoped the metrics summary to `lading publish`, but register_summary_atexit is wired from CLI bootstrap and flushes process-wide for any command. Reword to "When `lading` runs" and describe a process-wide flush, keeping publish.index_lookup_downgrade as a concrete example. - lockfile.py: guard the discovered-lockfiles counter with `if lockfiles:`. A zero-amount increment created a 0-valued counter key, which made an otherwise silent run emit an exit summary; quiet runs now stay quiet. Add an assertion to test_discover_tracked_lockfiles_returns_empty_result confirming no metric is recorded on empty discovery. - test_downgrade_metrics.py: add `match=` patterns to the parametrised pytest.raises so the flag-disabled and out-of-plan paths assert their distinct error messages, not just the exception type. - test_metrics.py: remove the three @given properties duplicated from the canonical suite in test_metrics_properties.py (label-order invariance, accumulation, snapshot isolation); example-based coverage of these invariants remains in this file. Drop the now-unused hypothesis/operator imports. - test_metrics.py: add a syrupy snapshot test covering counters and duration aggregates together, locking the combined exit-summary serialisation. The "register under concurrency with ThreadPoolExecutor" suggestion is already covered by test_register_summary_atexit_registers_once_under_concurrency, which releases eight threads on a barrier and asserts a single registration; not duplicated. missing_crate cardinality is intentionally left verbatim per ADR-004. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Align ADR-004 with the ADR template and document the Ruff pin Address documentation review findings (all verified against the current code): - docs/adr/004-in-process-metrics-backend.md: the ADR template in documentation-style-guide.md requires sections in the order Status, Date (YYYY-MM-DD), Context and problem statement. Add the missing `## Date` (2026-06-14, the ADR's creation date) and rename `## Context` to `## Context and problem statement`. - docs/developers-guide.md: add `duration_stats` to the lading.utils.metrics test-seam list (it is a public, query-only function in __all__ alongside counter_value, snapshot, and reset). - Makefile: add a comment above RUFF_VERSION explaining that the pin must stay in sync with the ruff== dev dependency in pyproject.toml and the uv tool install ruff== step in CI, and that all three must be bumped together to avoid version-skew lint failures. Skipped: the same Date/heading deviations exist in ADR-003 but are pre-existing and outside this review's scope, so ADR-003 is left unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Guard increment_counter against zero-amount writes Make "quiet runs stay quiet" robust at the source: increment_counter now returns early when amount == 0, so a zero increment never records a 0-valued key (a Counter with a 0-valued entry is truthy, which would otherwise make emit_summary surface a spurious metric). This generalises the call-site guard already present in discover_tracked_lockfiles to every caller. Add a regression test asserting a zero-amount increment records nothing. Other findings in the review were verified and skipped: - users-guide.md `#### publish.index_lookup_downgrade`: already correctly nested one level under `### Observability` (h3 -> h4); markdownlint MD001 passes. Demoting to `###` would make it a sibling of Observability and break the nesting, so it is left as-is. - docs/adr/004: the `## Date` and `## Context and problem statement` headings were already added in a previous commit, so no change is needed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Add regression tests for the zero-amount increment no-op Lock the "quiet runs stay quiet" invariant for increment_counter(amount=0): - test_zero_amount_increment_is_a_noop: a zero-amount increment creates no registry entry (counter_value is 0, snapshot is empty). - test_zero_amount_increment_does_not_break_quiet_run: emit_summary stays silent when only zero-amount increments occurred. These replace the earlier test_increment_counter_ignores_zero_amount, which asserted the same no-op invariant; consolidating avoids duplicate coverage and keeps the canonical test names and placement requested in review. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Document RUFF_VERSION/RUFF in the developers guide The RUFF_VERSION and RUFF Makefile variables that pin Ruff were undocumented in the developers guide, unlike the comparable PYLINT_* variables. Add them to the "relevant Makefile variables" list, mirroring the inline Makefile comment: RUFF_VERSION is the pinned version (kept in sync with the pyproject.toml dev dependency and the CI install, bumped together to avoid version-skew lint failures), and RUFF is the uv-tool-run invocation the fmt, check-fmt, and lint targets use. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary
Closes #91
lading.utils.metricswith duration aggregation:observe_duration/duration_statsrecord count and total seconds per label set, rendered in the exit summary withcountandtotal_secondsfields.lockfile.discovered— incremented by the number of tracked lockfiles each discovery returnslockfile.refresh(outcome=success|failure) +lockfile.refresh.durationaroundcargo generate-lockfilelockfile.validate(outcome=fresh|stale|failed) +lockfile.validate.durationaround thecargo metadata --lockedprobedocs/developers-guide.md.refresh_lockfileas it exists today; if Remove dead lockfile.refresh_lockfile; fix module docstring (#99) #124 (removal ofrefresh_lockfile) lands first, the refresh instrumentation moves to the regeneration path inbump_lockfileson rebase.Testing
make check-fmt,make lint,make typecheck, andmake test(576 passed) all green.coderabbit review --agent: 0 findings.🤖 Generated with Claude Code
Summary by Sourcery
Instrument lockfile operations with metrics and extend the metrics utility to support aggregated duration tracking.
New Features:
Enhancements:
Tests: