Conversation
turip
added
release-note/feature
Contributor
|
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:
📝 WalkthroughWalkthroughAdds realtime-usage expansion for usage-based charges: new expand constant, model Expands with realtime totals, handler wiring to request/convert realtime expands, service logic to compute and attach realtime totals concurrently, adapter query fix for status filters, and an integration test validating totals. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Handler as API Handler
participant Service as Charges Service
participant Adapter as DB Adapter
participant Rater as Rating Engine
Client->>Handler: ListCharges (expand=realizations,realtime_usage)
Handler->>Service: GetByIDs with ExpandRealtimeUsage
Service->>Adapter: GetByIDs (base charges)
Adapter-->>Service: charges
alt ExpandRealtimeUsage requested
Service->>Service: Deduplicate customers & meters
Service->>Adapter: ResolveFeatureMeters / FetchCustomerOverrides
Adapter-->>Service: meters & overrides
loop per-charge (concurrent, bounded)
Service->>Rater: Compute rating for charge
Rater-->>Service: totals or error
end
Service->>Service: Aggregate totals, attach to charge.Expands.RealtimeUsage
end
Service-->>Handler: charges with Expands.RealtimeUsage
Handler->>Handler: convertUsageBasedChargeTotals (set API Realtime)
Handler-->>Client: Response with realtime totals
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
GAlexIHU
previously approved these changes
Apr 16, 2026
Contributor
GAlexIHU
left a comment
GAlexIHU
left a comment
There was a problem hiding this comment.
why do we need to load live usage for list of charges? (i suppose old line feature-parity but still, these are super expensive)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openmeter/billing/charges/usagebased/charge.go (1)
78-85:⚠️ Potential issue | 🟡 MinorValidate
ExpandsfromCharge.Validate().Right now Lines 78-85 only validate
ChargeBase, so invalidExpands.RealtimeUsagevalues bypass charge-level validation even thoughExpands.Validate()already exists.Suggested fix
func (c Charge) Validate() error { var errs []error if err := c.ChargeBase.Validate(); err != nil { errs = append(errs, fmt.Errorf("charge base: %w", err)) } + + if err := c.Expands.Validate(); err != nil { + errs = append(errs, fmt.Errorf("expands: %w", err)) + } return models.NewNillableGenericValidationError(errors.Join(errs...)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/usagebased/charge.go` around lines 78 - 85, Charge.Validate currently only validates ChargeBase and misses validating the Expands field; update Charge.Validate (the method on type Charge) to also call c.Expands.Validate(), capture and wrap any returned error (e.g., fmt.Errorf("expands: %w", err)) and append it to errs before returning models.NewNillableGenericValidationError(errors.Join(errs...)) so invalid Expands.RealtimeUsage values are caught by charge-level validation.
🧹 Nitpick comments (1)
test/credits/rating_test.go (1)
28-121: Please cover the single-charge expand path too.This is a nice end-to-end check for the list flow. I’d still add a small
GetByIDcase as well, sinceopenmeter/billing/charges/usagebased/service/get.gouses a separateGetCurrentTotalsbranch forExpandRealtimeUsage, and this test won’t catch drift there.As per coding guidelines "Make sure the tests are comprehensive and cover the changes. Keep a strong focus on unit tests and in-code integration tests."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/credits/rating_test.go` around lines 28 - 121, Add a small GetByID path to the existing test: after creating charges and adding streaming events, call s.Charges.GetByID (or the service method that maps to the get.go flow) for one of the created charge IDs with Expands including meta.ExpandRealtimeUsage and meta.ExpandRealizations, then assert the returned charge.AsUsageBasedCharge() has the same RealtimeUsage.Total and Realizations.Sum().Total expectations as the list result; this ensures the separate GetCurrentTotals branch used by ExpandRealtimeUsage in get.go is exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openmeter/billing/charges/usagebased/service/get.go`:
- Around line 104-146: The code calls clock.Now() inside each worker when
building the GetRatingForUsageInput, causing inconsistent StoredAtOffset across
charges; capture a single timestamp once before the for loop (e.g., storedAt :=
clock.Now()) and pass that storedAt value into s.rater.GetRatingForUsage's
StoredAtOffset for every charge so all ratings use the same cutoff; update
references in the loop where StoredAtOffset is set and keep existing
semaphore/waitgroup logic intact.
- Around line 126-149: The code sends the same GetRatingForUsage error twice via
errCh because there's both an explicit errCh <- fmt.Errorf(...) and a deferred
sender that sends err when non-nil; in the wg.Go closure change the explicit
send into setting the local err variable and returning (e.g., err =
fmt.Errorf("rating charge %s: %w", charge.ID, err); return) so the existing
deferred func that does if err != nil { errCh <- err } handles the send exactly
once; reference the wg.Go closure, the local err variable, errCh, and
s.rater.GetRatingForUsage to locate and update the logic.
---
Outside diff comments:
In `@openmeter/billing/charges/usagebased/charge.go`:
- Around line 78-85: Charge.Validate currently only validates ChargeBase and
misses validating the Expands field; update Charge.Validate (the method on type
Charge) to also call c.Expands.Validate(), capture and wrap any returned error
(e.g., fmt.Errorf("expands: %w", err)) and append it to errs before returning
models.NewNillableGenericValidationError(errors.Join(errs...)) so invalid
Expands.RealtimeUsage values are caught by charge-level validation.
---
Nitpick comments:
In `@test/credits/rating_test.go`:
- Around line 28-121: Add a small GetByID path to the existing test: after
creating charges and adding streaming events, call s.Charges.GetByID (or the
service method that maps to the get.go flow) for one of the created charge IDs
with Expands including meta.ExpandRealtimeUsage and meta.ExpandRealizations,
then assert the returned charge.AsUsageBasedCharge() has the same
RealtimeUsage.Total and Realizations.Sum().Total expectations as the list
result; this ensures the separate GetCurrentTotals branch used by
ExpandRealtimeUsage in get.go is exercised.
🪄 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: 45c1a518-4ae1-4362-a7d5-70aae544c96a
📒 Files selected for processing (8)
api/v3/handlers/customers/charges/convert.goapi/v3/handlers/customers/charges/list.goopenmeter/billing/charges/adapter/search.goopenmeter/billing/charges/meta/charge.goopenmeter/billing/charges/service.goopenmeter/billing/charges/usagebased/charge.goopenmeter/billing/charges/usagebased/service/get.gotest/credits/rating_test.go
Member
Author
This is for upcoming charges, we are only loading this if the realtime expand is set (and we should not). This is feature parity with the gathering invoice. Once we have periodic realizations we can make it faster by relying on the previous snapshots for some meter types such as count/sum. |
turip
force-pushed
the
feat/realtime-expand
branch
from
0856ae8 to
33564fd
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openmeter/billing/charges/usagebased/service/get.go`:
- Around line 116-127: After successfully acquiring the semaphore with
sem.Acquire in the loop, ensure you release that slot if ResolveFeatureMeter
fails; currently you break after sending to errCh without releasing and leak the
slot. Modify the block after sem.Acquire so that when
charge.ResolveFeatureMeter(featureMeters) returns an error you call
sem.Release(ctx, 1) (or otherwise free the acquired slot) before sending the
error to errCh and breaking; reference the sem.Acquire/sem.Release calls and the
ResolveFeatureMeter invocation to locate where to add the release.
- Around line 129-155: The anonymous goroutine in wg.Go declares an outer err
but then uses := when calling s.rater.GetRatingForUsage, which shadows err so
the deferred error handler never sees failures; fix by avoiding shadowing—either
predeclare ratingResult (e.g. var ratingResult type) and use = when calling
s.rater.GetRatingForUsage, or replace := with = so the call assigns to the
existing err; keep the deferred panic-to-err and errCh send as-is and ensure you
still call ratingResults.Store(charge.GetChargeID(), ratingResult) only on
success.
🪄 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: 56b54aa2-88c4-49d8-b974-30d0e7d7c1ad
📒 Files selected for processing (2)
openmeter/billing/charges/usagebased/charge.goopenmeter/billing/charges/usagebased/service/get.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
openmeter/billing/charges/usagebased/service/get.go (1)
182-185: Small nit: error message is slightly misleading.The error on line 184 says "not found" but really means the type assertion failed. Not a big deal, just could be clearer if you ever need to debug this path.
Suggested fix
ratingResult, ok := ratingResultAny.(usagebasedrating.GetRatingForUsageResult) if !ok { - return charge, fmt.Errorf("rating result not found for charge %s", charge.ID) + return charge, fmt.Errorf("rating result type assertion failed for charge %s", charge.ID) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/usagebased/service/get.go` around lines 182 - 185, The error message is misleading because the failure is a type assertion on ratingResultAny to usagebasedrating.GetRatingForUsageResult (variables ratingResultAny and ratingResult) rather than a "not found" condition; update the fmt.Errorf call in the type-assertion branch to clearly state the type assertion/unexpected type and include the actual dynamic type (using %T) and the charge.ID to aid debugging (e.g., "unexpected rating result type %T for charge %s"), keeping the same early return path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@openmeter/billing/charges/usagebased/service/get.go`:
- Around line 182-185: The error message is misleading because the failure is a
type assertion on ratingResultAny to usagebasedrating.GetRatingForUsageResult
(variables ratingResultAny and ratingResult) rather than a "not found"
condition; update the fmt.Errorf call in the type-assertion branch to clearly
state the type assertion/unexpected type and include the actual dynamic type
(using %T) and the charge.ID to aid debugging (e.g., "unexpected rating result
type %T for charge %s"), keeping the same early return path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c79ae56b-d69f-4fbb-b3a9-e5896bbd3aa0
📒 Files selected for processing (1)
openmeter/billing/charges/usagebased/service/get.go
GAlexIHU
approved these changes
Apr 16, 2026
This was referenced Apr 17, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Add support for getting the total usage for the charge. The total usage includes the already booked items.
Notes for reviewer
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests