Count publish index-lookup downgrade events (#68) - #133
Conversation
Reviewer's GuideAdds a lightweight in-process metrics accumulator and uses it to track publish index-lookup downgrade events, documenting the behavior and covering it with targeted unit tests. Sequence diagram for publish index-lookup downgrade metrics recordingsequenceDiagram
participant PublishIndexCheck as publish_index_check
participant Metrics as metrics
participant Atexit as atexit
participant Logger as logging
PublishIndexCheck->>Metrics: increment_counter(INDEX_LOOKUP_DOWNGRADE_METRIC, subcommand, missing_crate)
Note over PublishIndexCheck,Metrics: Called in _handle_index_missing_version downgrade path
Atexit-->>Metrics: emit_summary()
Metrics-->>Logger: _LOGGER.info("lading metrics summary: %s", json.dumps(rendered))
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdd a process-local, thread-safe metrics accumulator with support for counters and duration observations; register a one-time atexit emitter in the CLI; instrument publish index-lookup downgrade success and lockfile operations to increment labelled counters; and add comprehensive unit, property and end-to-end tests, snapshots and documentation covering the accumulator and instrumentation points. ChangesObservability metrics for publish index-lookup downgrades and lockfile operations
Sequence Diagram(s)sequenceDiagram
participant PublishFlow as publish.run
participant IndexCheck as _handle_index_missing_version
participant Metrics as increment_counter
participant Registry as _counters
participant Exit as process exit
PublishFlow->>IndexCheck: CargoIndexLookupFailure (beta crate)
IndexCheck->>Metrics: increment_counter("publish.index_lookup_downgrade", subcommand="package", missing_crate="beta")
Metrics->>Registry: update counter under lock
PublishFlow->>PublishFlow: complete publish run
Exit->>Metrics: emit_summary()
Metrics->>Registry: read counters and durations
Metrics->>Exit: log "lading metrics summary" JSON with downgrade count
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 19 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (19 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
e21f8e6 to
1de95c9
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. lading/commands/publish_index_check.py Comment on lines +329 to +333 metrics.increment_counter(
INDEX_LOOKUP_DOWNGRADE_METRIC,
subcommand=failure.subcommand,
missing_crate=missing_name,
)❌ New issue: Large Method |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lading/utils/metrics.py`:
- Line 32: Replace the assignment-style type alias for _CounterKey with the PEP
695 form: change the current "_CounterKey = tuple[str, tuple[tuple[str, str],
...]]" alias to use the new "type" keyword (i.e., declare the alias using "type
_CounterKey = ...") so it targets Python 3.13 and improves IDE/runtime support;
update any imports or annotations that reference _CounterKey if necessary.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2890e4ef-580a-43a0-8627-e900f30db5aa
📒 Files selected for processing (5)
docs/developers-guide.mdlading/commands/publish_index_check.pylading/utils/metrics.pytests/unit/publish/test_downgrade_metrics.pytests/unit/utils/test_metrics.py
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
This comment was marked as resolved.
This comment was marked as resolved.
|
@coderabbitai pause |
✅ Action performedReviews paused. |
|
@coderabbitai resume |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
|
@coderabbitai review |
✅ Action performedReview finished.
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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
|
@coderabbitai resume |
This comment was marked as resolved.
This comment was marked as resolved.
- 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>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@coderabbitai resume |
This comment was marked as resolved.
This comment was marked as resolved.
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>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai Have the following now been resolved? lading/utils/metrics.py (1)
Proposed patchdef increment_counter(name: str, *, amount: int = 1, **labels: str) -> None:
"""Increment the counter ``name`` for the supplied label values.
Examples
--------
>>> increment_counter("demo.total", subcommand="package")
"""
+ if amount == 0:
+ return
with _LOCK:
_COUNTERS[_counter_key(name, labels)] += amount🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In docs/users-guide.md (1)
Suggested fix-#### publish.index_lookup_downgrade
+### publish.index_lookup_downgrade🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In Source: Coding guidelines docs/adr/004-in-process-metrics-backend.md (1)
Suggested fix## Status
Accepted.
-## Context
+## Date
+
+2026-06-14
+
+## Context and problem statement🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In Source: Coding guidelines |
|
@coderabbitai Have the following now been resolved? docs/developers-guide.md (1)
🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In Source: Coding guidelines |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== docs/developers-guide.md lines 600-635 ==="
sed -n '600,635p' docs/developers-guide.md
echo ""
echo "=== Search for duration_stats in developers-guide.md ==="
rg -n "duration_stats\|counter_value\|snapshot\|reset" docs/developers-guide.md | head -30Length of output: 3392
|
This comment was marked as resolved.
This comment was marked as resolved.
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>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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>
There was a problem hiding this comment.
Gates Passed
See analysis details in CodeScene
Absence of Expected Change Pattern
- lading/lading/cli.py is usually changed with: lading/tests/unit/test_cli.py
Quality Gate Profile: Pay Down Tech Debt
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
Rebasing onto origin/main auto-merged the metrics atexit registration (#133) into cli.main, adjacent to this branch's argument-declaration extraction. The combined hunks left fewer than two blank lines before the following module-level `bump` command definition; ruff format restores the spacing. Whitespace only, no behaviour change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Rebasing onto origin/main auto-merged the metrics atexit registration (#133) into cli.main, adjacent to this branch's argument-declaration extraction. The combined hunks left fewer than two blank lines before the following module-level `bump` command definition; ruff format restores the spacing. Whitespace only, no behaviour change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2 participants
Summary
Closes #68
lading/utils/metrics.py: a process-local metrics accumulator with labelled counters, flushed as one structured JSON log line at interpreter exit (atexit); quiet runs emit nothing.docs/developers-guide.md): a lading invocation is a short-lived CLI process whose logs are already aggregated, so the summary log line is the operational boundary —prometheus_client/statsdwould add a runtime dependency and a network target without a scraper. The in-process registry doubles as a deterministic test seam (counter_value,snapshot,reset)._handle_index_missing_versionincrementspublish.index_lookup_downgradewithsubcommandandmissing_cratelabels.Testing
tests/unit/publish/test_downgrade_metrics.pyverifies the counter increments exactly once on the downgrade path and never on the raise paths (flag disabled; out-of-plan dependency).make check-fmt,make lint,make typecheck, andmake test(568 passed) all green after rebasing onto currentmain.coderabbit review --agent: 0 findings.🤖 Generated with Claude Code
Summary by Sourcery
Introduce a process-local metrics accumulator and track publish index-lookup downgrade events.
New Features:
publish.index_lookup_downgradecounter when index-lookup failures are downgraded to warnings in the publish flow.Documentation:
publish.index_lookup_downgrademetric in the developers guide.Tests: