fix(bidengine): increment reservation counter metrics#409
Conversation
The reservation success/fail counters called WithLabelValues without a terminal .Inc(), so the returned counter was discarded and the metric was never recorded. Every other counter in this file calls .Inc(); these four were the exception, leaving reservation open/close metrics flat.
WalkthroughThree missing ChangesReservation Metrics Fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?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 `@bidengine/order_metrics_test.go`:
- Line 34: The Unreserve method in the cluster interface takes only one argument
(orderID), but the AssertCalled assertion on the scaffold.cluster mock is
passing two arguments: scaffold.orderID and mock.Anything. Remove the
mock.Anything argument from the AssertCalled invocation so that it only includes
the method name "Unreserve" and the single expected argument scaffold.orderID,
matching the actual method signature.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 492025d5-4d1f-4a79-9dab-c18d8b42efab
📒 Files selected for processing (3)
bidengine/order.gobidengine/order_metrics_test.gogo.mod
|
|
||
| // Shutting down triggers the unreserve path. | ||
| order.lc.Shutdown(nil) | ||
| scaffold.cluster.AssertCalled(t, "Unreserve", scaffold.orderID, mock.Anything) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Run the new metrics test to verify it detects the assertion mismatch
cd bidengine
go test -v -run Test_ReservationCounterIncrementsRepository: akash-network/provider
Length of output: 152
🏁 Script executed:
# Find and examine the cluster service interface to verify Unreserve signature
fd -t f service.go | grep cluster | head -5Repository: akash-network/provider
Length of output: 117
🏁 Script executed:
# Search for Unreserve method signature
rg "Unreserve" -A 2 -B 2 --type-listRepository: akash-network/provider
Length of output: 456
🏁 Script executed:
# Examine the test file at line 34 specifically
fd -t f order_metrics_test.go -x wc -l {}Repository: akash-network/provider
Length of output: 103
🏁 Script executed:
# Search for Unreserve method definition in cluster service
rg "Unreserve" -A 1 cluster/service.goRepository: akash-network/provider
Length of output: 296
🏁 Script executed:
# Examine the test file around line 34 to see the exact assertion
cat -n bidengine/order_metrics_test.go | sed -n '30,40p'Repository: akash-network/provider
Length of output: 603
🏁 Script executed:
# Check the actual call to Unreserve in order.go around line 481
rg "Unreserve" -B 2 -A 2 bidengine/order.go | head -20Repository: akash-network/provider
Length of output: 343
Remove extraneous mock.Anything argument from Unreserve assertion.
The Unreserve method takes only one argument (orderID), but this assertion expects two arguments. According to the cluster interface signature (cluster/service.go), Unreserve(mtypes.OrderID) error has a single parameter, and the production call site in order.go invokes it as o.cluster.Unreserve(reservation.OrderID()). The extra mock.Anything will cause this assertion to fail.
🔧 Proposed fix
- scaffold.cluster.AssertCalled(t, "Unreserve", scaffold.orderID, mock.Anything)
+ scaffold.cluster.AssertCalled(t, "Unreserve", scaffold.orderID)📝 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.
| scaffold.cluster.AssertCalled(t, "Unreserve", scaffold.orderID, mock.Anything) | |
| scaffold.cluster.AssertCalled(t, "Unreserve", scaffold.orderID) |
🤖 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 `@bidengine/order_metrics_test.go` at line 34, The Unreserve method in the
cluster interface takes only one argument (orderID), but the AssertCalled
assertion on the scaffold.cluster mock is passing two arguments:
scaffold.orderID and mock.Anything. Remove the mock.Anything argument from the
AssertCalled invocation so that it only includes the method name "Unreserve" and
the single expected argument scaffold.orderID, matching the actual method
signature.
1 participant
What
Four
reservationCounter.WithLabelValues(...)calls inbidengine/order.gowere missing a terminal.Inc(), so the returned counter was discarded and the metric was never recorded.Why
Every other counter in this file (
orderCompleteCounter,shouldBidCounter,bidCounter) calls.Inc(); these four (reservation open/close × success/fail) were the exception. As a result the reservation metrics stayed flat at zero, so operators had no visibility into reservation success/failure rates.Testing
Added
Test_ReservationCounterIncrements: runs a full reserve + unreserve cycle and asserts the open-success and close-success counters each advance by one. Fails without the.Inc()calls. (go mod tidyregisteredprometheus/.../testutil's indirect depkylelemons/godebug; go.sum unchanged.)