Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplaces the single rater method Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
turip
added
release-note/ignore
![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: 3
🧹 Nitpick comments (3)
openmeter/billing/charges/usagebased/service/run/create.go (1)
112-120: Tiny nit: the error wrap still says "get rating for usage".Not a bug, just a stale breadcrumb — the call is now
GetDetailedLinesForUsage, so an error here reads a little confusingly in logs. Feel free to ignore if you want to keep the message stable for grep-ability across history.🧽 Optional wording tweak
- return CreateRatedRunResult{}, fmt.Errorf("get rating for usage: %w", err) + return CreateRatedRunResult{}, fmt.Errorf("get detailed lines for usage: %w", err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/usagebased/service/run/create.go` around lines 112 - 120, The error wrap message is stale: change the returned error in the GetDetailedLinesForUsage call so it reflects the current method name; replace fmt.Errorf("get rating for usage: %w", err) with a message like fmt.Errorf("get detailed lines for usage: %w", err) (in the block where s.rater.GetDetailedLinesForUsage is called and returning CreateRatedRunResult{} on error) so logs correctly describe s.rater.GetDetailedLinesForUsage failures.openmeter/billing/charges/usagebased/service/rating/totals.go (1)
11-36: Docstring overstates the optimization, and there's near-duplicate logic withdetails.go.Two small things worth tightening up here:
The comment says "It avoids generating detailed lines, so prefer it over
GetDetailedLinesForUsagewhen only totals are needed." But the body still callss.ratingService.GenerateDetailedLines(...)— the detailed lines are generated, they're just thrown away. Functionally fine, but the docstring sets an expectation the code doesn't meet. Either update the comment to be honest ("returns only the totals portion of the rating result") or swap in a cheaper totals-only path onratingServiceif one exists.Everything above the final
returnis identical toGetDetailedLinesForUsageindetails.go(validate →snapshotQuantity→GenerateDetailedLines). A tiny shared helper would keep the two APIs from drifting.♻️ One way to dedupe
// in a shared file (e.g. rating/service.go) func (s *Service) rate(ctx context.Context, in GetRatingForUsageInput) (billingrating.GenerateDetailedLinesResult, alpacadecimal.Decimal, error) { if err := in.Validate(); err != nil { return billingrating.GenerateDetailedLinesResult{}, alpacadecimal.Decimal{}, err } q, err := s.snapshotQuantity(ctx, snapshotQuantityInput{ Customer: in.Customer.Customer, FeatureMeter: in.FeatureMeter, ServicePeriod: in.Charge.Intent.ServicePeriod, StoredAtOffset: in.StoredAtOffset, }) if err != nil { return billingrating.GenerateDetailedLinesResult{}, alpacadecimal.Decimal{}, fmt.Errorf("get snapshot quantity: %w", err) } res, err := s.ratingService.GenerateDetailedLines(usagebased.RateableIntent{ Intent: in.Charge.Intent, MeterValue: q, }) if err != nil { return billingrating.GenerateDetailedLinesResult{}, alpacadecimal.Decimal{}, fmt.Errorf("rating: %w", err) } return res, q, nil }Then both
GetDetailedLinesForUsageandGetTotalsForUsagebecome 3-liners.As per coding guidelines: "In general when reviewing the Golang code make readability and maintainability a priority, even potentially suggest restructuring the code to improve them." And re: performance — since rating sits on a billing hot path, the docstring currently promising "avoids generating detailed lines" is the kind of thing that'll cause a reviewer to mis-pick the "faster" API.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/usagebased/service/rating/totals.go` around lines 11 - 36, The docstring for GetTotalsForUsage incorrectly claims it avoids generating detailed lines while the body calls s.ratingService.GenerateDetailedLines; update the comment to state it "returns only the totals portion of the rating result" (or, if a totals-only method exists on ratingService, call that instead). Also factor the duplicated logic (input validation → snapshotQuantity → GenerateDetailedLines) into a shared helper on Service (e.g., rate or rateForUsage) and have both GetTotalsForUsage and GetDetailedLinesForUsage call that helper to return the rating result and snapshot quantity, keeping error wrapping behavior (use the same error messages like "get snapshot quantity" and "rating" or "rating totals") consistent.openmeter/billing/charges/usagebased/service/creditheninvoice.go (1)
186-194: Same stale error wording as inrun/create.go.The method is now
GetDetailedLinesForUsagebut the wrap still reads"get rating for usage". Worth a matching rename for log consistency, or leave it if you prefer stable message strings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/usagebased/service/creditheninvoice.go` around lines 186 - 194, The error wrap message is stale: when calling s.Rater.GetDetailedLinesForUsage(...) the returned error is wrapped with "get rating for usage"; update the wrap to reflect the actual method name (e.g., "get detailed lines for usage" or similar) for consistency and easier tracing—locate the call to GetDetailedLinesForUsage in creditheninvoice.go (the block that assigns ratingResult, err) and change the fmt.Errorf string accordingly.
🤖 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/currenttotals.go`:
- Around line 61-64: GetCurrentTotalsResult is returned without populating
Quantity causing TestChargeFeatureIDs to fail; update the logic in
GetCurrentTotals (where you currently call GetTotalsForUsage) to call
GetDetailedLinesForUsage instead and extract the metered amount from the
returned GetRatingForUsageResult.Quantity, then set
GetCurrentTotalsResult.Quantity before returning; alternatively if you
intentionally want to drop Quantity, remove the field from
GetCurrentTotalsResult and adjust tests, but to satisfy current tests implement
the first option using GetDetailedLinesForUsage and the Quantity symbol from
GetRatingForUsageResult.
In `@openmeter/billing/charges/usagebased/service/get.go`:
- Around line 177-190: The two error returns in the mapping lambda are
indistinguishable; change the second error (the type-assertion failure after
dueTotalsAny.(totals.Totals)) to a distinct message indicating an invalid type
(e.g., "invalid totals type for charge %s") so callers can tell apart a missing
entry from a wrong-type entry; locate the anonymous function passed to
slicesx.MapWithErr (using ratingResults.Load, charge.GetChargeID(),
totals.Totals, and charge.Expands.RealtimeUsage) and update only the fmt.Errorf
for the failed type assertion.
In `@openmeter/billing/charges/usagebased/service/rating/details.go`:
- Around line 10-12: The doc comment for GetDetailedLinesForUsage wrongly claims
GetTotalsForUsage is faster even though GetTotalsForUsage currently calls
s.ratingService.GenerateDetailedLines(...) and does the same work; either
remove/soften the "Prefer GetTotalsForUsage when only totals are needed because
it is faster" phrasing in the comment of GetDetailedLinesForUsage, or implement
a true fast path by changing GetTotalsForUsage to avoid calling
s.ratingService.GenerateDetailedLines (e.g., add a totals-only method on
ratingService or a flag to GenerateDetailedLines that returns only totals), and
update references to GetTotalsForUsage/GetDetailedLinesForUsage and
s.ratingService.GenerateDetailedLines accordingly.
---
Nitpick comments:
In `@openmeter/billing/charges/usagebased/service/creditheninvoice.go`:
- Around line 186-194: The error wrap message is stale: when calling
s.Rater.GetDetailedLinesForUsage(...) the returned error is wrapped with "get
rating for usage"; update the wrap to reflect the actual method name (e.g., "get
detailed lines for usage" or similar) for consistency and easier tracing—locate
the call to GetDetailedLinesForUsage in creditheninvoice.go (the block that
assigns ratingResult, err) and change the fmt.Errorf string accordingly.
In `@openmeter/billing/charges/usagebased/service/rating/totals.go`:
- Around line 11-36: The docstring for GetTotalsForUsage incorrectly claims it
avoids generating detailed lines while the body calls
s.ratingService.GenerateDetailedLines; update the comment to state it "returns
only the totals portion of the rating result" (or, if a totals-only method
exists on ratingService, call that instead). Also factor the duplicated logic
(input validation → snapshotQuantity → GenerateDetailedLines) into a shared
helper on Service (e.g., rate or rateForUsage) and have both GetTotalsForUsage
and GetDetailedLinesForUsage call that helper to return the rating result and
snapshot quantity, keeping error wrapping behavior (use the same error messages
like "get snapshot quantity" and "rating" or "rating totals") consistent.
In `@openmeter/billing/charges/usagebased/service/run/create.go`:
- Around line 112-120: The error wrap message is stale: change the returned
error in the GetDetailedLinesForUsage call so it reflects the current method
name; replace fmt.Errorf("get rating for usage: %w", err) with a message like
fmt.Errorf("get detailed lines for usage: %w", err) (in the block where
s.rater.GetDetailedLinesForUsage is called and returning CreateRatedRunResult{}
on error) so logs correctly describe s.rater.GetDetailedLinesForUsage failures.
🪄 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: a08cecb6-85b0-4860-a254-eb43a5c2f8c3
📒 Files selected for processing (8)
openmeter/billing/charges/usagebased/service/creditheninvoice.goopenmeter/billing/charges/usagebased/service/creditsonly.goopenmeter/billing/charges/usagebased/service/currenttotals.goopenmeter/billing/charges/usagebased/service/get.goopenmeter/billing/charges/usagebased/service/rating/details.goopenmeter/billing/charges/usagebased/service/rating/service.goopenmeter/billing/charges/usagebased/service/rating/totals.goopenmeter/billing/charges/usagebased/service/run/create.go
💤 Files with no reviewable changes (1)
- openmeter/billing/charges/usagebased/service/rating/service.go
turip
force-pushed
the
chore/split-rating
branch
from
588a903 to
791abcd
Compare
tothandras
approved these changes
Apr 21, 2026
This was referenced Apr 22, 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
Note: code duplication is intentional, these two methods will diverge over time.
Split
openmeter/billing/charges/usagebased/service/ratingintoGetTotalsForUsageintotals.goandGetDetailedLinesForUsageindetails.go, with comments documenting the return values and that totals-only lookupshould be preferred because it is faster.
Refactored the totals-only consumers to use
GetTotalsForUsagein realtime expand (openmeter/billing/charges/usagebased/service/get.go) andGetCurrentTotals(openmeter/billing/charges/usagebased/service/ currenttotals.go), while state-machine/run paths now callGetDetailedLinesForUsage, and I reverted the temporary ledger API changes.Updated
openmeter/billing/charges/service/featureid_test.goto assert on due totals instead of quantity, then ran focused verification successfully:go test -count=1 -tags=dynamic ./openmeter/billing/charges/service ./ openmeter/billing/charges/usagebased/service ./openmeter/billing/charges/usagebased/service/run ./openmeter/billing/charges/usagebased/service/rating.Notes for reviewer
Summary by CodeRabbit