Hypothesis stateful tests for the metrics event reducer (#78) - #247
Hypothesis stateful tests for the metrics event reducer (#78)#247leynos wants to merge 3 commits into
Conversation
MetricsHook.__call__ was a phase-dispatch that reached straight into the collector, with no seam to verify which counters and histograms each event yields across varied phase/order/pid combinations. Extract the pure event-to-operation reducer _metric_operations(event) -> tuple[_MetricOp, ...] (with _CounterOp/_HistogramOp records and a _PHASE_COUNTERS lookup for the unit-counter phases). __call__ now applies the reducer's operations, resolving labels only when there is at least one operation, so plan and unknown phases still compute no labels. The former _increment/_record_stdin_bytes/_record_exit helpers are removed; behaviour is unchanged and the existing test_metrics_adapter.py suite still passes. Add cuprum/unittests/test_metrics_adapter_stateful.py: - property tests pinning the operations produced per phase (unit counters, plan no-op, stdin bytes only when counted, exit failure/ duration only when present, unknown phase raises); - a Hypothesis RuleBasedStateMachine that streams random events through a real MetricsHook/InMemoryMetrics and checks the accumulated counters and histograms against an independent phase-count oracle — proving counters and observations are created exactly when intended. Regenerate the maturin wheel-manifest snapshot for the new test file. Closes #78 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
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:
WalkthroughMetrics collection now reduces execution events into explicit counter or histogram operations before applying them through ChangesMetrics operation pipeline
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 18 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (18 passed)
📋 Issue PlannerLet us write the prompt for your AI agent so you can ship faster (with fewer bugs). View plan for ticket: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Reviewer's GuideRefactors the metrics adapter to introduce a pure event-to-metric-operations reducer and wires MetricsHook.call through it, then adds focused property tests and a Hypothesis rule-based state machine to verify that metrics counters and histograms are produced exactly as intended across all execution phases and event streams. Sequence diagram for MetricsHook event-to-operations reducersequenceDiagram
participant ExecEvent
participant MetricsHook
participant Reducer as _metric_operations
participant Collector as MetricsCollector
ExecEvent ->> MetricsHook: __call__(event)
MetricsHook ->> Reducer: _metric_operations(event)
Reducer -->> MetricsHook: tuple[_MetricOp]
alt no operations
MetricsHook -->> ExecEvent: return
else has operations
MetricsHook ->> MetricsHook: _extract_labels(event)
loop for each operation
MetricsHook ->> MetricsHook: _apply(operation, labels)
alt _CounterOp
MetricsHook ->> Collector: inc_counter(name, value, labels)
else _HistogramOp
MetricsHook ->> Collector: observe_histogram(name, value, labels)
end
end
end
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@cuprum/adapters/metrics_adapter.py`:
- Line 200: Update the _MetricOp type alias to use the PEP 695 type statement
for the _CounterOp | _HistogramOp union, preserving the existing member types
and avoiding the legacy assignment syntax.
- Around line 226-249: Refactor `_metric_operations` to dispatch on
`event.phase` using a `match`/`case` statement rather than the current chained
top-level `if` branches. Preserve the existing behavior for `plan`, mapped
counter phases, `stdin` byte counts, `exit` via `_exit_operations`, and unknown
phases raising `_UnhandledMetricsPhaseError`; keep the nested `stdin` byte-count
check intact.
🪄 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: 4739c9b9-c96c-4693-85fe-4df36863bc7f
📒 Files selected for processing (3)
cuprum/adapters/metrics_adapter.pycuprum/unittests/__snapshots__/test_maturin_build.ambrcuprum/unittests/test_metrics_adapter_stateful.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/shared-actions(auto-detected)leynos/pylint-pypy-shim(auto-detected)leynos/whitaker(auto-detected)
| value: float | ||
|
|
||
|
|
||
| _MetricOp = _CounterOp | _HistogramOp |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Use the PEP 695 type statement for the union alias.
The snapshot confirms requires_python '>=3.12', so declare _MetricOp with the modern type statement instead of a bare assignment.
🔧 Proposed fix
-_MetricOp = _CounterOp | _HistogramOp
+type _MetricOp = _CounterOp | _HistogramOpAs per coding guidelines, "Use the type keyword for type aliases in modern Python; when supporting Python versions before 3.12, retain typing.TypeAlias syntax and add # noqa: UP040."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _MetricOp = _CounterOp | _HistogramOp | |
| type _MetricOp = _CounterOp | _HistogramOp |
🤖 Prompt for 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.
In `@cuprum/adapters/metrics_adapter.py` at line 200, Update the _MetricOp type
alias to use the PEP 695 type statement for the _CounterOp | _HistogramOp union,
preserving the existing member types and avoiding the legacy assignment syntax.
Source: Coding guidelines
| def _metric_operations(event: ExecEvent) -> tuple[_MetricOp, ...]: | ||
| """Map an execution event to the metric operations it should produce. | ||
|
|
||
| This is the pure event-to-operation reducer behind | ||
| [`MetricsHook.__call__`][cuprum.adapters.metrics_adapter.MetricsHook.__call__] | ||
| — the single source of truth for which counters and histogram observations | ||
| each phase yields, so the operations can be verified without a collector. | ||
| Labels are applied by the caller. ``plan`` yields nothing; a ``stdin`` event | ||
| without a byte count yields nothing; an unknown phase is a contract | ||
| violation and raises ``_UnhandledMetricsPhaseError``. | ||
| """ | ||
| phase = event.phase | ||
| if phase == "plan": | ||
| return () | ||
| counter_name = _PHASE_COUNTERS.get(phase) | ||
| if counter_name is not None: | ||
| return (_CounterOp(counter_name, 1.0),) | ||
| if phase == "stdin": | ||
| if event.byte_count is None: | ||
| return () | ||
| return (_CounterOp("cuprum_stdin_bytes_total", float(event.byte_count)),) | ||
| if phase == "exit": | ||
| return _exit_operations(event) | ||
| raise _UnhandledMetricsPhaseError(event.phase) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Dispatch on event.phase with match/case instead of chained ifs.
event.phase is a closed, discriminated literal — exactly the shape structural pattern matching is meant for. The current chain also stacks four sequential branches at the top level before the final raise.
♻️ Proposed refactor
def _metric_operations(event: ExecEvent) -> tuple[_MetricOp, ...]:
"""..."""
- phase = event.phase
- if phase == "plan":
- return ()
- counter_name = _PHASE_COUNTERS.get(phase)
- if counter_name is not None:
- return (_CounterOp(counter_name, 1.0),)
- if phase == "stdin":
- if event.byte_count is None:
- return ()
- return (_CounterOp("cuprum_stdin_bytes_total", float(event.byte_count)),)
- if phase == "exit":
- return _exit_operations(event)
- raise _UnhandledMetricsPhaseError(event.phase)
+ match event.phase:
+ case "plan":
+ return ()
+ case "stdin":
+ if event.byte_count is None:
+ return ()
+ return (_CounterOp("cuprum_stdin_bytes_total", float(event.byte_count)),)
+ case "exit":
+ return _exit_operations(event)
+ case phase if phase in _PHASE_COUNTERS:
+ return (_CounterOp(_PHASE_COUNTERS[phase], 1.0),)
+ case _:
+ raise _UnhandledMetricsPhaseError(event.phase)As per path instructions, "Prefer structural pattern matching over isinstance() or imperative decomposition" and "Move conditionals with >2 branches to predicate/helper functions." As per coding guidelines, "Use structural pattern matching (match/case) for branching over structured data such as enums, discriminated unions, and pattern-rich data structures."
[source_path_instructions,source_coding_guidelines]
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _metric_operations(event: ExecEvent) -> tuple[_MetricOp, ...]: | |
| """Map an execution event to the metric operations it should produce. | |
| This is the pure event-to-operation reducer behind | |
| [`MetricsHook.__call__`][cuprum.adapters.metrics_adapter.MetricsHook.__call__] | |
| — the single source of truth for which counters and histogram observations | |
| each phase yields, so the operations can be verified without a collector. | |
| Labels are applied by the caller. ``plan`` yields nothing; a ``stdin`` event | |
| without a byte count yields nothing; an unknown phase is a contract | |
| violation and raises ``_UnhandledMetricsPhaseError``. | |
| """ | |
| phase = event.phase | |
| if phase == "plan": | |
| return () | |
| counter_name = _PHASE_COUNTERS.get(phase) | |
| if counter_name is not None: | |
| return (_CounterOp(counter_name, 1.0),) | |
| if phase == "stdin": | |
| if event.byte_count is None: | |
| return () | |
| return (_CounterOp("cuprum_stdin_bytes_total", float(event.byte_count)),) | |
| if phase == "exit": | |
| return _exit_operations(event) | |
| raise _UnhandledMetricsPhaseError(event.phase) | |
| def _metric_operations(event: ExecEvent) -> tuple[_MetricOp, ...]: | |
| """Map an execution event to the metric operations it should produce. | |
| This is the pure event-to-operation reducer behind | |
| [`MetricsHook.__call__`][cuprum.adapters.metrics_adapter.MetricsHook.__call__] | |
| — the single source of truth for which counters and histogram observations | |
| each phase yields, so the operations can be verified without a collector. | |
| Labels are applied by the caller. ``plan`` yields nothing; a ``stdin`` event | |
| without a byte count yields nothing; an unknown phase is a contract | |
| violation and raises ``_UnhandledMetricsPhaseError``. | |
| """ | |
| match event.phase: | |
| case "plan": | |
| return () | |
| case "stdin": | |
| if event.byte_count is None: | |
| return () | |
| return (_CounterOp("cuprum_stdin_bytes_total", float(event.byte_count)),) | |
| case "exit": | |
| return _exit_operations(event) | |
| case phase if phase in _PHASE_COUNTERS: | |
| return (_CounterOp(_PHASE_COUNTERS[phase], 1.0),) | |
| case _: | |
| raise _UnhandledMetricsPhaseError(event.phase) |
🤖 Prompt for 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.
In `@cuprum/adapters/metrics_adapter.py` around lines 226 - 249, Refactor
`_metric_operations` to dispatch on `event.phase` using a `match`/`case`
statement rather than the current chained top-level `if` branches. Preserve the
existing behavior for `plan`, mapped counter phases, `stdin` byte counts, `exit`
via `_exit_operations`, and unknown phases raising
`_UnhandledMetricsPhaseError`; keep the nested `stdin` byte-count check intact.
Record the seam this branch extracts in design-doc 8.4, where the telemetry adapter decisions already live. Adds an "Event-to-operation reduction" subsection stating why the split exists — the pure _metric_operations reducer decides what to record and _apply is the only step that reaches the collector, so the mapping is property-testable without one — plus the two consequences worth pinning: labels are projected only when the reducer yields an operation, so a plan event never touches them, and an unrecognized phase raises rather than being silently dropped. The sequence diagram carries a screen-reader caption describing the whole flow in prose, including the empty-tuple early return and which collector call each operation variant becomes. The caption is deliberately unnumbered rather than continuing the Figure N sequence used elsewhere in section 8. PR #245 renumbers the later figures in that section, so any number chosen here would be wrong under one merge order; no prose cross-references figure numbers, and section 13 already uses unnumbered screen-reader captions. Worth a tidying pass once both land. 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.
|
Raised #251 to number this diagram once both PRs land. The §8.4 diagram here is deliberately unnumbered: #245 inserts two figures earlier in §8 and pushes the old 4 and 5 to 6 and 7, so the correct number for this one is #251 records the expected end state (Figures 3–8 with sections and subjects) and asks for a check that #245's renumbering survived the merge, since both PRs touch §8 prose. While auditing for that issue I found three adjacent things and put them in #251 as decisions rather than acting on them here:
None of those are regressions from this PR, so they are flagged rather than folded in. |
Two review findings, both valid against current code. _MetricOp used the legacy assignment form for its union. The project targets Python 3.12 and already declares aliases with the PEP 695 type statement in cuprum/sh.py and cuprum/events.py, so this now matches. The alias is used only in annotations, and the module has postponed evaluation, so the lazy TypeAliasType introduces no runtime concern. _metric_operations dispatched through chained top-level ifs. Restore a match/case on the phase, which is the idiom used elsewhere in this module (_apply) and in _subprocess_timeout. The mapped unit-counter phases stay keyed by _PHASE_COUNTERS behind a guard clause rather than being repeated as a literal alternation in the pattern, so the metric names keep exactly one definition and cannot drift from the table. plan, the nested stdin byte-count check, exit via _exit_operations, and the unknown-phase _UnhandledMetricsPhaseError all behave as before. An earlier revision of this function was converted away from match to satisfy the complexity and return-count lints; keeping the table lookup as a guard clause rather than expanding it into separate cases holds the structure under both limits, and ruff is clean. Behaviour is unchanged, verified by mutation rather than assumed: dropping the stdin byte-count check and silently returning for an unknown phase each fail the existing property and stateful tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Both findings verified against current code and fixed in bf74ced. Neither was stale. 1. PEP 695 aliasValid. The project targets type _MetricOp = _CounterOp | _HistogramOpI checked the one thing that could have made this unsafe: a 2.
|
| Command | Outcome |
|---|---|
uv run pytest cuprum/unittests/test_metrics_adapter.py |
13 passed |
uv run pytest cuprum/unittests/test_metrics_adapter_stateful.py |
9 passed |
make check-fmt |
pass |
make lint |
pass (ruff — no C901/PLR0911 — interrogate 100%, pylint 10.00/10, clippy) |
make typecheck |
pass (ty clean, confirming the lazy alias is fine) |
make test |
pass — Rust nextest 57/57, full Python suite green |
make markdownlint / make nixie |
pass |
2 participants
Summary
MetricsHook.__call__was a phase-dispatch that reached straight into the collector, with no seam to verify which counters and histograms each event yields across varied phase/order/pid combinations (#78).Seam
Extract the pure event-to-operation reducer
_metric_operations(event) -> tuple[_MetricOp, ...]:_CounterOp/_HistogramOprecords describe the intended operations; a_PHASE_COUNTERSlookup collapses the unit-counter phases.MetricsHook.__call__now applies the reducer's operations, resolving labels only when there is at least one operation — soplan(and an unknown phase) still compute no labels, as the existing tests require._increment/_record_stdin_bytes/_record_exithelpers are removed; behaviour is unchanged andtest_metrics_adapter.pystill passes.Tests
cuprum/unittests/test_metrics_adapter_stateful.py:start/stdout/stderr/stdin_error;planyields nothing;stdinyields a bytes counter only when a byte count is present;exitcounts a failure only for a non-zero code and a duration only when measured; an unknown phase raises.RuleBasedStateMachinestreams random events (all seven phases, phase-appropriate fields) through a realMetricsHook/InMemoryMetricsand, after every step, checks the accumulated counters and histograms against an independent phase-count oracle (not the reducer). This proves counters and observations are created exactly when intended, and only then, across arbitrary event orders.Scope note
This addresses the reducer extraction and verification for the named target,
cuprum/adapters/metrics_adapter.py. The metrics hook is stateless per event (no active-span map); the "active maps drain" concern applies to the tracing adapter and is out of scope for this file.Validation
Full gates green:
make check-fmt,make lint(ruff, interrogate 100%, pylint 10.00/10),make test(764 passed / 47 skipped incl. the existing metrics suite; Rust nextest 57/57). Wheel-manifest snapshot regenerated.Closes #78
🤖 Generated with Claude Code
Summary by Sourcery
Extract a pure event-to-metrics reducer for the metrics hook and add property-based and stateful tests to verify metrics behavior across execution phases.
New Features:
_metric_operationsreducer that maps execution events to counter and histogram operations for the metrics hook.Enhancements:
MetricsHook.__call__to delegate to the shared reducer and a generic operation applier, avoiding label computation for no-op events.