feat(billing): add delta usage rating engine#4298
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughIntroduces a delta-rating path for usage-based billing: a delta engine, subtraction primitives, unique-reference ID generators, wiring into the rating service, many unit/integration tests, and documentation describing the algorithm and subtraction semantics. ChangesDelta Rating Implementation
Sequence DiagramsequenceDiagram
participant Client
participant RatingService
participant DeltaEngine
participant SubtractEngine
participant BillingRatingService
Client->>RatingService: GetDetailedRatingForUsage(intent, snapshot)
RatingService->>RatingService: Build already-billed detailed lines (from realizations)
RatingService->>DeltaEngine: Rate(Input{Intent, CurrentPeriod, AlreadyBilled})
DeltaEngine->>BillingRatingService: GenerateDetailedLines(intent, opts)
BillingRatingService-->>DeltaEngine: currentDetailedLines
DeltaEngine->>DeltaEngine: Strip credits from already-billed lines
DeltaEngine->>SubtractEngine: SubtractRatedRunDetails(currentDetailedLines, sanitizedAlreadyBilled)
SubtractEngine-->>DeltaEngine: deltaDetailedLines
DeltaEngine->>DeltaEngine: Stamp service-period, sort, index, validate uniqueness
DeltaEngine-->>RatingService: Result{DetailedLines, Totals}
RatingService-->>Client: Totals & DetailedLines
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (10)
openmeter/billing/charges/usagebased/service/rating/subtract/uniquereferenceid.go (1)
1-48: ⚡ Quick winMove the mock to a
_test.gofile to avoid importingtestingin production code.The
UniqueReferenceIDGeneratorinterface is clean—nice three-method contract. The thing worth tweaking is havingNewMockUniqueReferenceIDGeneratorand its implementation inuniquereferenceid.go, which imports thetestingpackage. That pullstestinginto the production binary for anyone linking againstsubtract.Since the mock is only used in
subtract_test.go(same package), the simplest move is to shift both the function andmockUniqueReferenceIDGeneratorinto auniquereferenceid_test.gofile. Easy refactor, keeps the build cleaner.The unused
_ testing.TBparameter is totally fine to keep—it future-proofs for lifecycle hooks and signals "this is test-only code."🤖 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 `@openmeter/billing/charges/usagebased/service/rating/subtract/uniquereferenceid.go` around lines 1 - 48, The mock implementation and its constructor (NewMockUniqueReferenceIDGenerator and type mockUniqueReferenceIDGenerator) are test-only but currently live in uniquereferenceid.go and import testing; move both NewMockUniqueReferenceIDGenerator and mockUniqueReferenceIDGenerator into a new uniquereferenceid_test.go file in the same package, remove the testing import from the production file so uniquereferenceid.go only contains the UniqueReferenceIDGenerator interface, and keep the unused _ testing.TB parameter on NewMockUniqueReferenceIDGenerator in the test file.openmeter/billing/charges/usagebased/service/rating/subtract/subtract.go (1)
204-267: 💤 Low valueThoughts on
isZeroDetailedLine: reasonable defensive note, though zero-priced lines appear filtered upstream.The logic reads right: the function drops detailed lines when
Totals.IsZero()returns true. BecauseAmount = PerUnitAmount × Quantity, a$0/unitline with non-zero quantity would indeed have zero totals and get silently dropped here.That said, the pricing layer (tieredgraduated, tieredvolume) appears to prevent zero-priced detailed lines from being constructed in the first place — tiers with nil
UnitPricedon't generate lines. So in practice, lines with$0/unitlikely never reachsubtract. If that assumption holds, the current logic is fine.If there's any scenario where a free-tier detailed line could land here (say, through a future pricing model or edge case), adding an
|| line.Quantity.IsZero()check would be safer for observability — no harm in being explicit. Alternatively, a brief comment confirming "totals zero ⇒ irrelevant" would document the assumption clearly.🤖 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 `@openmeter/billing/charges/usagebased/service/rating/subtract/subtract.go` around lines 204 - 267, The current filtering in subtractDetailedLinesWithSameKey relies on isZeroDetailedLine which only checks Totals.IsZero(), but a $0/unit line with non-zero Quantity could be dropped silently; update isZeroDetailedLine (or its call sites in subtractDetailedLinesWithSameKey) to also treat lines with Quantity.IsZero() as zero (i.e., return true when Totals.IsZero() || Quantity.IsZero()), or add a short comment above subtractDetailedLinesWithSameKey documenting the upstream assumption that zero-priced lines are never produced (reference symbols: isZeroDetailedLine, subtractDetailedLinesWithSameKey, Totals.IsZero, Quantity.IsZero).openmeter/billing/charges/usagebased/service/rating/delta/dynamic_test.go (1)
82-420: ⚡ Quick winPlease add a downward-correction case for dynamic pricing.
This suite covers growth, repricing, rounding, caps, and final min-spend behavior, but it never exercises the inverse path where the cumulative amount drops between phases (for example
15 -> 10or10 -> 0). That’s the branch most likely to shake out “reverse prior cumulative amount, then book the smaller current amount” bugs forDynamicPrice.As per coding guidelines,
**/*_test.go: Make sure the tests are comprehensive and cover the changes.🤖 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 `@openmeter/billing/charges/usagebased/service/rating/delta/dynamic_test.go` around lines 82 - 420, Add a new unit test in this file (e.g. TestDynamicDeltaDownwardCorrection) that mirrors the existing upward/rounding/cap tests but uses a decreasing cumulative snapshot (e.g. first phase meteredQuantity 15, second phase 10 or 0) to assert the code emits a reversal detailed line for the prior higher cumulative amount and then books the smaller current cumulative amount; reuse runDeltaRatingTestCase, deltaRatingTestCase, deltaRatingPhase, billingrating.UsageChildUniqueReferenceID and the "usage#correction:detailed_line_id=..." child ID pattern, and assert expectedDetailedLines and expectedTotals accordingly so the rating logic for reversing prior cumulative amounts then applying the smaller current amount is exercised.openmeter/billing/charges/usagebased/service/rating/delta/engine_test.go (2)
73-82: 💤 Low valueOptional: assert the full expected slice instead of indexed pairs.
The current
ElementsMatchworks since it's order-independent, but indexingout.DetailedLines[0]/[1]will panic with a confusing message if the engine ever returns fewer/more than 2 lines. AToExpectedDetailedLinesWithServicePeriod(or a length assertion first) would localize the failure mode like the other tests in this file do.🤖 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 `@openmeter/billing/charges/usagebased/service/rating/delta/engine_test.go` around lines 73 - 82, The test currently accesses out.DetailedLines[0]/[1] which will panic if the slice length differs; instead assert the slice length and compare the full expected set: first assert len(out.DetailedLines) == 2 (or use the existing helper ToExpectedDetailedLinesWithServicePeriod) and then use require.ElementsMatch with a mapped slice of ChildUniqueReferenceID from out.DetailedLines (or call ToExpectedDetailedLinesWithServicePeriod to get/compare expected IDs) so failures show a clear mismatch instead of an index panic; update the assertions around out.DetailedLines and keep the checks for ratingService.lastOpts.IgnoreMinimumCommitment and DisableCreditsMutator.
340-347: 💤 Low valueStub returns nil for
*timeutil.ClosedPeriod— could mask future contract changes.
ResolveBillablePeriodreturns(nil, nil), which is fine today because the delta engine doesn't call it. If a downstream change starts invoking it, callers may NPE on the dereferenced pointer rather than fail clearly. Consider returning a non-nil zero period or panicking with a "not implemented in stub" message so future regressions surface fast.🤖 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 `@openmeter/billing/charges/usagebased/service/rating/delta/engine_test.go` around lines 340 - 347, The stub method stubRatingService.ResolveBillablePeriod currently returns (nil, nil) which can mask future contract changes; update ResolveBillablePeriod to either return a non-nil zero value of timeutil.ClosedPeriod (constructed with the zero time bounds) or explicitly panic/return an error like "not implemented in stub" so callers fail fast; change the implementation in stubRatingService.ResolveBillablePeriod to create and return a valid *timeutil.ClosedPeriod (or a clear error) instead of nil to avoid unexpected NPEs if it gets called later.openmeter/billing/charges/usagebased/service/rating/delta/engine.go (2)
121-143: 💤 Low valueDuplicate-ID check could be a single pass.
Minor:
lo.GroupByallocates a full map of all reference IDs only to checklen > 1. Since you've already sorted and indexed, you could collapse the duplicate detection into a single pass with an early return — saves one map allocation and a second iteration. Functionally equivalent today, just a small cleanup.Optional single-pass dedup
- remainingDetailedLines.Sort() - for idx := range remainingDetailedLines { - // Indexes are persisted as part of the generated detailed-line contract. - index := idx - remainingDetailedLines[idx].Index = &index - } - - childUniqueReferenceIDs := lo.GroupBy(remainingDetailedLines, func(line usagebased.DetailedLine) string { - return line.ChildUniqueReferenceID - }) - - for id, lines := range childUniqueReferenceIDs { - if len(lines) > 1 { - return Result{}, fmt.Errorf("duplicate child unique reference id: %s", id) - } - } + remainingDetailedLines.Sort() + seen := make(map[string]struct{}, len(remainingDetailedLines)) + for idx := range remainingDetailedLines { + id := remainingDetailedLines[idx].ChildUniqueReferenceID + if _, ok := seen[id]; ok { + return Result{}, fmt.Errorf("duplicate child unique reference id: %s", id) + } + seen[id] = struct{}{} + + // Indexes are persisted as part of the generated detailed-line contract. + index := idx + remainingDetailedLines[idx].Index = &index + }🤖 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 `@openmeter/billing/charges/usagebased/service/rating/delta/engine.go` around lines 121 - 143, The duplicate-child-ID check currently builds a full map via lo.GroupBy on remainingDetailedLines just to detect any ID seen more than once; replace that with a single-pass check over remainingDetailedLines (after remainingDetailedLines.Sort() and indexing) that tracks the previous ChildUniqueReferenceID (or a small seen-set) and returns the same error (fmt.Errorf("duplicate child unique reference id: %s", id)) as soon as a duplicate is detected, and remove the lo.GroupBy block and its subsequent loop; keep references to remainingDetailedLines, remainingDetailedLines.Sort(), the DetailedLine.ChildUniqueReferenceID field and the early return behavior.
73-93: 💤 Low valueAdd a comment clarifying why context isn't propagated to the rating service.
Ratetakes a context but drops it (via_) sincee.ratingService.GenerateDetailedLinesdoesn't currently accept one. This is fine today, but if that method ever gets a context parameter, we'll need to remember to wire it up here. A quick comment like// Context not propagated: GenerateDetailedLines doesn't accept it (yet)would save future readers from wondering if it's an oversight.🤖 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 `@openmeter/billing/charges/usagebased/service/rating/delta/engine.go` around lines 73 - 93, Add a brief inline comment in Engine.Rate explaining why the incoming context is ignored: note that Rate receives a context but passes _ because e.ratingService.GenerateDetailedLines currently does not accept a context; include a short phrasing like "Context not propagated: GenerateDetailedLines doesn't accept it (yet)" near the function signature or right before the call to e.ratingService.GenerateDetailedLines so future changes to GenerateDetailedLines will remind maintainers to wire the context through.openmeter/billing/charges/usagebased/service/rating/delta/tieredgraduated_test.go (1)
14-690: 💤 Low valueComprehensive coverage — tier transitions are well-exercised.
Seven tests covering same-tier subtraction, single/multi-tier jumps, decreases within and across tier boundaries, and flat-tier behavior — that's a thorough matrix. The given/when/then comments at the top of each test make intent obvious, and the deterministic correction reference (
...#correction:detailed_line_id=phase-1-line-2) ties nicely intodetailedLinesBookedForDeltaTest's ID assignment. Minor optional thought: the three-tier price ladder is duplicated across tests; a small helper (e.g.,graduatedTieredPriceWithFlatComponents()) could reduce ~30 lines of boilerplate per test, but the explicit form is also legitimately readable, so totally a matter of taste.🤖 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 `@openmeter/billing/charges/usagebased/service/rating/delta/tieredgraduated_test.go` around lines 14 - 690, Tests duplicate the three-tier price ladder across many cases; extract a small helper to reduce boilerplate. Add a helper function (e.g., graduatedTieredPrice() and optional graduatedTieredPriceWithFlatComponents()) that returns productcatalog.NewPriceFrom(productcatalog.TieredPrice{...}) with the same three tiers used in TestGraduatedTieredDeltaSameTierSubtractsNormally, TestGraduatedTieredDeltaCrossingTierBoundaryBooksOnlyNewTierUsage, TestGraduatedTieredDeltaMultipleTierJumpBooksOnlyNewTierUsage, TestGraduatedTieredDeltaUsageDecreaseWithinSameTierReversesTierQuantity, TestGraduatedTieredDeltaUsageDecreaseAcrossTierBoundaryReversesRemovedTier, TestGraduatedTieredDeltaFlatTierAlreadyBookedDoesNotRepeat and TestGraduatedTieredDeltaCrossingFlatTierBoundaryBooksOnlyNewFlatComponent; replace the repeated inline productcatalog.NewPriceFrom(...) occurrences with the helper calls, keeping behavior identical.openmeter/billing/charges/usagebased/service/rating/details.go (2)
170-179: 💤 Low valueHelper name shadows the local variable — small readability nit.
currentBillingPeriodis both the function name and a local variable inside the function body. Works fine, just makes the body slightly harder to skim. A rename likeperiod := currentRunServicePeriod(and returningperiod) would remove the visual ambiguity.Optional rename
func currentBillingPeriod(currentRunServicePeriod timeutil.ClosedPeriod, eligibleRealizations usagebased.RealizationRuns) timeutil.ClosedPeriod { - currentBillingPeriod := currentRunServicePeriod + period := currentRunServicePeriod for _, realization := range eligibleRealizations { - if realization.ServicePeriodTo.After(currentBillingPeriod.From) { - currentBillingPeriod.From = realization.ServicePeriodTo + if realization.ServicePeriodTo.After(period.From) { + period.From = realization.ServicePeriodTo } } - return currentBillingPeriod + return period }🤖 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 `@openmeter/billing/charges/usagebased/service/rating/details.go` around lines 170 - 179, The local variable inside the function currentBillingPeriod shadows the function name which hurts readability; rename the local variable (e.g., change the variable currently named currentBillingPeriod to period or resultPeriod) and update all references inside the function (the assignment from currentRunServicePeriod, the comparison using realization.ServicePeriodTo.After, the update to period.From, and the return) so the function currentBillingPeriod returns the renamed variable instead of shadowing its own name.
113-139: 💤 Low valueTiny capacity hint mismatch —
makesize doesn't reflect total detailed lines.
alreadyBilledDetailedLinesis sized aslen(eligibleRealizations), but you appendrealization.DetailedLines.OrEmpty()...per realization, which can be many lines each. The slice will grow regardless, so this is purely a small allocation tweak — not a correctness issue. Feel free to skip if it's noise.Optional cap pre-sum
- alreadyBilledDetailedLines := make(usagebased.DetailedLines, 0, len(eligibleRealizations)) - for _, realization := range eligibleRealizations { - alreadyBilledDetailedLines = append(alreadyBilledDetailedLines, realization.DetailedLines.OrEmpty()...) - } + totalLines := 0 + for _, realization := range eligibleRealizations { + totalLines += len(realization.DetailedLines.OrEmpty()) + } + alreadyBilledDetailedLines := make(usagebased.DetailedLines, 0, totalLines) + for _, realization := range eligibleRealizations { + alreadyBilledDetailedLines = append(alreadyBilledDetailedLines, realization.DetailedLines.OrEmpty()...) + }🤖 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 `@openmeter/billing/charges/usagebased/service/rating/details.go` around lines 113 - 139, Pre-size alreadyBilledDetailedLines more accurately: compute totalLines by iterating eligibleRealizations and summing len(realization.DetailedLines.OrEmpty()) and then allocate alreadyBilledDetailedLines := make(usagebased.DetailedLines, 0, totalLines) before the append loop; update the loop that appends realization.DetailedLines.OrEmpty()... to use this pre-sized slice (symbols: alreadyBilledDetailedLines, eligibleRealizations, realization.DetailedLines.OrEmpty()).
🤖 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.
Nitpick comments:
In `@openmeter/billing/charges/usagebased/service/rating/delta/dynamic_test.go`:
- Around line 82-420: Add a new unit test in this file (e.g.
TestDynamicDeltaDownwardCorrection) that mirrors the existing
upward/rounding/cap tests but uses a decreasing cumulative snapshot (e.g. first
phase meteredQuantity 15, second phase 10 or 0) to assert the code emits a
reversal detailed line for the prior higher cumulative amount and then books the
smaller current cumulative amount; reuse runDeltaRatingTestCase,
deltaRatingTestCase, deltaRatingPhase, billingrating.UsageChildUniqueReferenceID
and the "usage#correction:detailed_line_id=..." child ID pattern, and assert
expectedDetailedLines and expectedTotals accordingly so the rating logic for
reversing prior cumulative amounts then applying the smaller current amount is
exercised.
In `@openmeter/billing/charges/usagebased/service/rating/delta/engine_test.go`:
- Around line 73-82: The test currently accesses out.DetailedLines[0]/[1] which
will panic if the slice length differs; instead assert the slice length and
compare the full expected set: first assert len(out.DetailedLines) == 2 (or use
the existing helper ToExpectedDetailedLinesWithServicePeriod) and then use
require.ElementsMatch with a mapped slice of ChildUniqueReferenceID from
out.DetailedLines (or call ToExpectedDetailedLinesWithServicePeriod to
get/compare expected IDs) so failures show a clear mismatch instead of an index
panic; update the assertions around out.DetailedLines and keep the checks for
ratingService.lastOpts.IgnoreMinimumCommitment and DisableCreditsMutator.
- Around line 340-347: The stub method stubRatingService.ResolveBillablePeriod
currently returns (nil, nil) which can mask future contract changes; update
ResolveBillablePeriod to either return a non-nil zero value of
timeutil.ClosedPeriod (constructed with the zero time bounds) or explicitly
panic/return an error like "not implemented in stub" so callers fail fast;
change the implementation in stubRatingService.ResolveBillablePeriod to create
and return a valid *timeutil.ClosedPeriod (or a clear error) instead of nil to
avoid unexpected NPEs if it gets called later.
In `@openmeter/billing/charges/usagebased/service/rating/delta/engine.go`:
- Around line 121-143: The duplicate-child-ID check currently builds a full map
via lo.GroupBy on remainingDetailedLines just to detect any ID seen more than
once; replace that with a single-pass check over remainingDetailedLines (after
remainingDetailedLines.Sort() and indexing) that tracks the previous
ChildUniqueReferenceID (or a small seen-set) and returns the same error
(fmt.Errorf("duplicate child unique reference id: %s", id)) as soon as a
duplicate is detected, and remove the lo.GroupBy block and its subsequent loop;
keep references to remainingDetailedLines, remainingDetailedLines.Sort(), the
DetailedLine.ChildUniqueReferenceID field and the early return behavior.
- Around line 73-93: Add a brief inline comment in Engine.Rate explaining why
the incoming context is ignored: note that Rate receives a context but passes _
because e.ratingService.GenerateDetailedLines currently does not accept a
context; include a short phrasing like "Context not propagated:
GenerateDetailedLines doesn't accept it (yet)" near the function signature or
right before the call to e.ratingService.GenerateDetailedLines so future changes
to GenerateDetailedLines will remind maintainers to wire the context through.
In
`@openmeter/billing/charges/usagebased/service/rating/delta/tieredgraduated_test.go`:
- Around line 14-690: Tests duplicate the three-tier price ladder across many
cases; extract a small helper to reduce boilerplate. Add a helper function
(e.g., graduatedTieredPrice() and optional
graduatedTieredPriceWithFlatComponents()) that returns
productcatalog.NewPriceFrom(productcatalog.TieredPrice{...}) with the same three
tiers used in TestGraduatedTieredDeltaSameTierSubtractsNormally,
TestGraduatedTieredDeltaCrossingTierBoundaryBooksOnlyNewTierUsage,
TestGraduatedTieredDeltaMultipleTierJumpBooksOnlyNewTierUsage,
TestGraduatedTieredDeltaUsageDecreaseWithinSameTierReversesTierQuantity,
TestGraduatedTieredDeltaUsageDecreaseAcrossTierBoundaryReversesRemovedTier,
TestGraduatedTieredDeltaFlatTierAlreadyBookedDoesNotRepeat and
TestGraduatedTieredDeltaCrossingFlatTierBoundaryBooksOnlyNewFlatComponent;
replace the repeated inline productcatalog.NewPriceFrom(...) occurrences with
the helper calls, keeping behavior identical.
In `@openmeter/billing/charges/usagebased/service/rating/details.go`:
- Around line 170-179: The local variable inside the function
currentBillingPeriod shadows the function name which hurts readability; rename
the local variable (e.g., change the variable currently named
currentBillingPeriod to period or resultPeriod) and update all references inside
the function (the assignment from currentRunServicePeriod, the comparison using
realization.ServicePeriodTo.After, the update to period.From, and the return) so
the function currentBillingPeriod returns the renamed variable instead of
shadowing its own name.
- Around line 113-139: Pre-size alreadyBilledDetailedLines more accurately:
compute totalLines by iterating eligibleRealizations and summing
len(realization.DetailedLines.OrEmpty()) and then allocate
alreadyBilledDetailedLines := make(usagebased.DetailedLines, 0, totalLines)
before the append loop; update the loop that appends
realization.DetailedLines.OrEmpty()... to use this pre-sized slice (symbols:
alreadyBilledDetailedLines, eligibleRealizations,
realization.DetailedLines.OrEmpty()).
In `@openmeter/billing/charges/usagebased/service/rating/subtract/subtract.go`:
- Around line 204-267: The current filtering in subtractDetailedLinesWithSameKey
relies on isZeroDetailedLine which only checks Totals.IsZero(), but a $0/unit
line with non-zero Quantity could be dropped silently; update isZeroDetailedLine
(or its call sites in subtractDetailedLinesWithSameKey) to also treat lines with
Quantity.IsZero() as zero (i.e., return true when Totals.IsZero() ||
Quantity.IsZero()), or add a short comment above
subtractDetailedLinesWithSameKey documenting the upstream assumption that
zero-priced lines are never produced (reference symbols: isZeroDetailedLine,
subtractDetailedLinesWithSameKey, Totals.IsZero, Quantity.IsZero).
In
`@openmeter/billing/charges/usagebased/service/rating/subtract/uniquereferenceid.go`:
- Around line 1-48: The mock implementation and its constructor
(NewMockUniqueReferenceIDGenerator and type mockUniqueReferenceIDGenerator) are
test-only but currently live in uniquereferenceid.go and import testing; move
both NewMockUniqueReferenceIDGenerator and mockUniqueReferenceIDGenerator into a
new uniquereferenceid_test.go file in the same package, remove the testing
import from the production file so uniquereferenceid.go only contains the
UniqueReferenceIDGenerator interface, and keep the unused _ testing.TB parameter
on NewMockUniqueReferenceIDGenerator in the test file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 412b58f7-4ab3-43de-9204-4f8be1b021b4
📒 Files selected for processing (24)
openmeter/billing/charges/service/invoicable_test.goopenmeter/billing/charges/usagebased/service/rating/delta/README.mdopenmeter/billing/charges/usagebased/service/rating/delta/base_test.goopenmeter/billing/charges/usagebased/service/rating/delta/dynamic_test.goopenmeter/billing/charges/usagebased/service/rating/delta/engine.goopenmeter/billing/charges/usagebased/service/rating/delta/engine_test.goopenmeter/billing/charges/usagebased/service/rating/delta/package_test.goopenmeter/billing/charges/usagebased/service/rating/delta/tieredgraduated_test.goopenmeter/billing/charges/usagebased/service/rating/delta/tieredvolume_test.goopenmeter/billing/charges/usagebased/service/rating/delta/uniquereferenceid.goopenmeter/billing/charges/usagebased/service/rating/delta/unit_test.goopenmeter/billing/charges/usagebased/service/rating/detailedline.goopenmeter/billing/charges/usagebased/service/rating/details.goopenmeter/billing/charges/usagebased/service/rating/service.goopenmeter/billing/charges/usagebased/service/rating/service_test.goopenmeter/billing/charges/usagebased/service/rating/subtract/README.mdopenmeter/billing/charges/usagebased/service/rating/subtract/subtract.goopenmeter/billing/charges/usagebased/service/rating/subtract/subtract_test.goopenmeter/billing/charges/usagebased/service/rating/subtract/uniquereferenceid.goopenmeter/billing/charges/usagebased/service/rating/testutils/testutils.goopenmeter/billing/charges/usagebased/service/rating/uniqueref.goopenmeter/ledger/chargeadapter/usagebased_test.gotest/credits/credit_then_invoice_test.gotest/credits/sanity_test.go
💤 Files with no reviewable changes (2)
- openmeter/billing/charges/usagebased/service/rating/uniqueref.go
- openmeter/billing/charges/usagebased/service/rating/detailedline.go
turip
force-pushed
the
split/rating-scaffolding
branch
from
d43c0a7 to
a3df829
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (10)
openmeter/billing/charges/usagebased/service/rating/delta/engine.go (1)
112-143: 💤 Low valueDuplicate uniqueness check between
subtract.SubtractRatedRunDetailsand the engine.
subtract.SubtractRatedRunDetailsalready validatesChildUniqueReferenceIDuniqueness when called withoutWithUniqueReferenceIDValidationIgnored()(defaults tovalidateUniqueChildReferenceIDs: true), so thelo.GroupBycheck at lines 135–143 re-does the same work. Two easy ways to dedupe:
- Drop the second check here and rely on subtract's validation (simplest), or
- Pass
subtract.WithUniqueReferenceIDValidationIgnored()so subtract skips it, and keep the post-stamp validation here as the single source of truth.Since the only mutations after subtract are
ServicePeriod/CorrectsRunID/Index(none of which touchChildUniqueReferenceID), option 1 is fine.♻️ Proposed change (option 1)
remainingDetailedLines.Sort() for idx := range remainingDetailedLines { // Indexes are persisted as part of the generated detailed-line contract. index := idx remainingDetailedLines[idx].Index = &index } - childUniqueReferenceIDs := lo.GroupBy(remainingDetailedLines, func(line usagebased.DetailedLine) string { - return line.ChildUniqueReferenceID - }) - - for id, lines := range childUniqueReferenceIDs { - if len(lines) > 1 { - return Result{}, fmt.Errorf("duplicate child unique reference id: %s", id) - } - } - return Result{ DetailedLines: remainingDetailedLines, }, nil…and drop the
"github.com/samber/lo"import if it ends up unused after the change.🤖 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 `@openmeter/billing/charges/usagebased/service/rating/delta/engine.go` around lines 112 - 143, The engine is redundantly re-checking ChildUniqueReferenceID uniqueness after calling subtract.SubtractRatedRunDetails (which already validates uniqueness by default); remove the post-subtract grouping/dup-check (the lo.GroupBy block that iterates childUniqueReferenceIDs and errors on duplicates) and rely on subtract.SubtractRatedRunDetails for validation, then remove the lo import if it becomes unused; alternatively, if you prefer to keep validation here, call subtract.SubtractRatedRunDetails with subtract.WithUniqueReferenceIDValidationIgnored() so subtract skips validation and this file remains the single source of truth.openmeter/billing/charges/usagebased/service/rating/subtract/subtract.go (1)
95-108: 💤 Low valueTiny comment hint for future readers.
The clone +
Quantity.Neg()pattern is doing "validate as if the line were positive" so negative correction inputs surviveline.Validate(). That intent isn't obvious at a glance — a one-liner comment ("we allow negative-quantity correction inputs as arithmetic; flip sign so Validate doesn't reject them") would save the next reader a detour. Note thatTotalsare intentionally not normalized here, which is fine becauseValidate()doesn't require sign consistency, but worth documenting alongside.🤖 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 `@openmeter/billing/charges/usagebased/service/rating/subtract/subtract.go` around lines 95 - 108, Add a one-line explanatory comment inside validateDetailedLinesForSubtract above the clone/negation block clarifying that we intentionally flip negative line.Quantity values (using line.Clone(), Quantity.IsNegative() and Quantity.Neg()) so we "validate as if the line were positive" and allow negative-quantity correction inputs to pass line.Validate(); also note in the same comment that Totals are intentionally not normalized here because Validate() does not require sign consistency.openmeter/billing/charges/usagebased/service/rating/details.go (1)
170-179: 💤 Low valueTiny readability nit: local var shadows the enclosing function name.
currentBillingPeriod(the variable) shadowscurrentBillingPeriod(the function) inside its own body. It compiles fine, but a different local name (e.g.outorperiod) reads a touch nicer.♻️ Tiny rename
func currentBillingPeriod(currentRunServicePeriod timeutil.ClosedPeriod, eligibleRealizations usagebased.RealizationRuns) timeutil.ClosedPeriod { - currentBillingPeriod := currentRunServicePeriod + period := currentRunServicePeriod for _, realization := range eligibleRealizations { - if realization.ServicePeriodTo.After(currentBillingPeriod.From) { - currentBillingPeriod.From = realization.ServicePeriodTo + if realization.ServicePeriodTo.After(period.From) { + period.From = realization.ServicePeriodTo } } - return currentBillingPeriod + return period }🤖 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 `@openmeter/billing/charges/usagebased/service/rating/details.go` around lines 170 - 179, The local variable inside the currentBillingPeriod function shadows the function name; rename the variable (e.g., to period or out) to avoid shadowing, update all references inside the function (assignment and the return) to the new name, and ensure the function still returns the renamed variable of type timeutil.ClosedPeriod; specifically modify the variable declaration currently named currentBillingPeriod and its From update inside currentBillingPeriod(...) to use the new identifier.openmeter/billing/charges/usagebased/service/rating/subtract/uniquereferenceid.go (1)
1-48: ⚡ Quick winMove the mock and
testingimport to a sibling test package to avoid linking the test runtime into production builds.Right now
uniquereferenceid.goisn't a test file, but it importstestingand exportsNewMockUniqueReferenceIDGenerator. This pulls thetestingpackage into every production binary that depends onsubtract— not idiomatic Go. The mock is only used by tests (16 call sites, all insubtract_test.go), so it's a clean move.Keep the
UniqueReferenceIDGeneratorinterface here, but move the mock and its constructor to something likesubtract/subtracttest/uniquereferenceid.gowith its ownpackage subtracttest. Tests import the sibling package and call the mock from there.♻️ Suggested split
Keep in original file:
package subtract import "github.com/openmeterio/openmeter/openmeter/billing/charges/usagebased" type UniqueReferenceIDGenerator interface { CurrentOnly(line usagebased.DetailedLine) (string, error) MatchedDelta(current, previous usagebased.DetailedLine) (string, error) PreviousOnlyReversal(line usagebased.DetailedLine) (string, error) }Move to
subtract/subtracttest/uniquereferenceid.go:package subtracttest import ( "fmt" "testing" "github.com/openmeterio/openmeter/openmeter/billing/charges/usagebased" "github.com/openmeterio/openmeter/openmeter/billing/charges/usagebased/service/rating/subtract" ) func NewMockUniqueReferenceIDGenerator(_ testing.TB) subtract.UniqueReferenceIDGenerator { return mockUniqueReferenceIDGenerator{} } type mockUniqueReferenceIDGenerator struct{} // ... methods🤖 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 `@openmeter/billing/charges/usagebased/service/rating/subtract/uniquereferenceid.go` around lines 1 - 48, The file currently exports UniqueReferenceIDGenerator and also defines/testing-imports the test-only NewMockUniqueReferenceIDGenerator and mockUniqueReferenceIDGenerator, which pulls the testing package into production builds; keep the UniqueReferenceIDGenerator interface in this file but remove the mock and testing import and move them into a sibling test package (e.g., package subtracttest) in a new file under subtract/subtracttest/uniquereferenceid.go; implement NewMockUniqueReferenceIDGenerator, mockUniqueReferenceIDGenerator and its methods (CurrentOnly, MatchedDelta, PreviousOnlyReversal) there, returning a subtract.UniqueReferenceIDGenerator, and update tests to import subtracttest to obtain the mock.openmeter/billing/charges/usagebased/service/rating/testutils/testutils.go (1)
22-68: 💤 Low valueHeads up: float64 conversions can mask precision drift in expectations.
ExpectedDetailedLine/ExpectedTotalsmodel amounts asfloat64and rely onInexactFloat64()for both actuals and expectations. This is fine for the current dollar-and-cent fixtures, but once tests start using prices like0.001(already happening inunit_test.go's rounding cases), tiny IEEE-754 drift can sneak in and produce confusing diffs that look like "0.34 != 0.34". Worth a quick mental note while authoring future cases — if you ever hit that, switching to string comparison viaDecimal.String()keeps things deterministic.🤖 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 `@openmeter/billing/charges/usagebased/service/rating/testutils/testutils.go` around lines 22 - 68, The test helpers use float64 fields (ExpectedDetailedLine and ExpectedTotals) and InexactFloat64() conversions which can mask IEEE-754 drift; change the amount fields in ExpectedTotals (Amount, ChargesTotal, DiscountsTotal, TaxesInclusiveTotal, TaxesExclusiveTotal, TaxesTotal, CreditsTotal, Total) and the PerUnitAmount/Quantity in ExpectedDetailedLine to string, and in ToExpectedTotals and ToExpectedDetailedLinesWithServicePeriod convert using the decimal value's String() (e.g., Decimal.String()) instead of InexactFloat64(), so tests assert deterministic decimal string equality; update any callers in tests to compare the string representations accordingly.openmeter/billing/charges/service/invoicable_test.go (1)
1389-1393: 💤 Low valueTiny drive-by: another spot worth pinning to
RatingEngineDelta.You added
s.Equal(usagebased.RatingEngineDelta, fetched.State.RatingEngine)to similar "fetched after Create" sections at Line 849 and Line 1093, but this nearly identical block here inTestUsageBasedCreditThenInvoiceFullyCreditedDoesNotAccrueInvoiceUsagedoesn't get the same assertion. Adding it would make the rating-engine wiring assertions consistent across the lifecycle suites — totally optional cleanup though.🤖 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 `@openmeter/billing/charges/service/invoicable_test.go` around lines 1389 - 1393, Add the missing rating-engine assertion in the "fetched after Create" section of TestUsageBasedCreditThenInvoiceFullyCreditedDoesNotAccrueInvoiceUsage: assert that fetched.State.RatingEngine equals usagebased.RatingEngineDelta (i.e., add s.Equal(usagebased.RatingEngineDelta, fetched.State.RatingEngine) next to the other post-create checks for fetched in that test) so the rating-engine wiring assertions are consistent with the similar checks in the other tests.openmeter/billing/charges/usagebased/service/rating/service_test.go (1)
37-50: 💤 Low valueQuick thought: this test now lives a bit awkwardly.
TestFormatDetailedLineChildUniqueReferenceIDexercises a pure helper that moved into thetestutilspackage — keeping its test inratingrather than next to the helper means a reader looking attestutils/testutils.gowon't see any direct coverage. Not a blocker, just feels a touch out of place; you could move this to atestutils_test.go(using a separate_testpackage) or drop it given the helper is exercised heavily downstream.🤖 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 `@openmeter/billing/charges/usagebased/service/rating/service_test.go` around lines 37 - 50, The test TestFormatDetailedLineChildUniqueReferenceID lives in the rating package but exercises the pure helper ratingtestutils.FormatDetailedLineChildUniqueReferenceID; move this test next to the helper by creating a new test file (e.g., testutils_test.go) in the testutils package (use a separate _test package if you prefer) and copy the test there (or alternatively remove the test here if you prefer relying on downstream coverage), ensuring the test still calls ratingtestutils.FormatDetailedLineChildUniqueReferenceID with the same servicePeriod setup and assertions so coverage is colocated with the helper.openmeter/billing/charges/usagebased/service/rating/delta/base_test.go (1)
37-73: 💤 Low valueTiny organizational nit:
detailedLinesBookedForDeltaTestlives in the wrong file.
runDeltaRatingTestCasehere callsdetailedLinesBookedForDeltaTest, but that helper is defined inengine_test.go(Line 320). Since this is the shared test scaffolding consumed byunit_test.go,package_test.go, etc., the helper feels like it belongs next to its only caller inbase_test.go. Engine-specific tests can keep using it via the package, but readers of the harness wouldn't have to jump files to understand how booked IDs get assigned. Cosmetic only.🤖 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 `@openmeter/billing/charges/usagebased/service/rating/delta/base_test.go` around lines 37 - 73, The helper detailedLinesBookedForDeltaTest is defined separately from its sole caller runDeltaRatingTestCase; move the detailedLinesBookedForDeltaTest function so it is declared alongside runDeltaRatingTestCase (in the same test file/package scope), update any references/imports if needed, and ensure visibility (package-level test helper) remains correct so engine-specific tests still compile and the shared harness is easier to read.openmeter/billing/charges/usagebased/service/rating/delta/tieredvolume_test.go (1)
76-152: ⚡ Quick winAdd the inverse volume-tier repricing case.
Nice coverage on
15 -> 16. The missing high-risk path is the reverse move (16 -> 15or16 -> 14), where delta rating has to undo the cheaper cumulative tier and emit a positive adjustment for the now-more-expensive lower tier. That sign flip is easy to get wrong, and this suite doesn’t exercise it yet. As per coding guidelines,**/*_test.go: Make sure the tests are comprehensive and cover the changes.🤖 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 `@openmeter/billing/charges/usagebased/service/rating/delta/tieredvolume_test.go` around lines 76 - 152, Add a complementary test to cover the inverse volume‑tier crossing (e.g., 16 -> 15 or 16 -> 14) so delta rating undoes the cheaper cumulative tier and emits a positive adjustment for the higher per‑unit lower tier; copy the structure of TestVolumeTieredDeltaUnitTierCrossingRepricesCurrentPeriod (or create TestVolumeTieredDeltaUnitTierCrossingRepricesBackward) using deltaRatingTestCase with phases that start in a higher cumulative tier (meteredQuantity ~16) then drop to a lower cumulative tier (meteredQuantity ~15 or 14), and assert expectedDetailedLines includes a positive adjustment line referencing the original child ID (similar to "volume-tiered-price#correction:...") with correct PerUnitAmount, positive Quantity/Amount and updated expectedTotals so the sign flip path is exercised.openmeter/billing/charges/usagebased/service/rating/delta/dynamic_test.go (1)
82-154: ⚡ Quick winAdd a lower-snapshot dynamic delta test.
This suite covers equal and increasing cumulative usage, but not a later lower snapshot like
15 -> 10. That’s a core delta path too, and it’s usually where negative correction signs and rounding behavior go sideways. A simple decrease case here would tighten confidence a lot. As per coding guidelines,**/*_test.go: Make sure the tests are comprehensive and cover the changes.🤖 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 `@openmeter/billing/charges/usagebased/service/rating/delta/dynamic_test.go` around lines 82 - 154, Add a new test case that covers a later lower snapshot (e.g., 15 -> 10) similar to TestDynamicDeltaAdditionalUsageRepricesCumulativeAmount: create a deltaRatingTestCase passed to runDeltaRatingTestCase with productcatalog.NewPriceFrom(productcatalog.DynamicPrice{Multiplier: alpacadecimal.NewFromInt(1)}), two deltaRatingPhase entries where period1 has meteredQuantity 15 and period2 has meteredQuantity 10, and assert expectedDetailedLines include a negative correction line (ChildUniqueReferenceID like "usage#correction:...") reversing the prior cumulative amount and the new cumulative booking; set expectedTotals on phase2 to reflect the net decrease (Amount -5, Total -5) so delta rating and rounding behavior for decreases are exercised by the test.
🤖 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.
Nitpick comments:
In `@openmeter/billing/charges/service/invoicable_test.go`:
- Around line 1389-1393: Add the missing rating-engine assertion in the "fetched
after Create" section of
TestUsageBasedCreditThenInvoiceFullyCreditedDoesNotAccrueInvoiceUsage: assert
that fetched.State.RatingEngine equals usagebased.RatingEngineDelta (i.e., add
s.Equal(usagebased.RatingEngineDelta, fetched.State.RatingEngine) next to the
other post-create checks for fetched in that test) so the rating-engine wiring
assertions are consistent with the similar checks in the other tests.
In `@openmeter/billing/charges/usagebased/service/rating/delta/base_test.go`:
- Around line 37-73: The helper detailedLinesBookedForDeltaTest is defined
separately from its sole caller runDeltaRatingTestCase; move the
detailedLinesBookedForDeltaTest function so it is declared alongside
runDeltaRatingTestCase (in the same test file/package scope), update any
references/imports if needed, and ensure visibility (package-level test helper)
remains correct so engine-specific tests still compile and the shared harness is
easier to read.
In `@openmeter/billing/charges/usagebased/service/rating/delta/dynamic_test.go`:
- Around line 82-154: Add a new test case that covers a later lower snapshot
(e.g., 15 -> 10) similar to
TestDynamicDeltaAdditionalUsageRepricesCumulativeAmount: create a
deltaRatingTestCase passed to runDeltaRatingTestCase with
productcatalog.NewPriceFrom(productcatalog.DynamicPrice{Multiplier:
alpacadecimal.NewFromInt(1)}), two deltaRatingPhase entries where period1 has
meteredQuantity 15 and period2 has meteredQuantity 10, and assert
expectedDetailedLines include a negative correction line (ChildUniqueReferenceID
like "usage#correction:...") reversing the prior cumulative amount and the new
cumulative booking; set expectedTotals on phase2 to reflect the net decrease
(Amount -5, Total -5) so delta rating and rounding behavior for decreases are
exercised by the test.
In `@openmeter/billing/charges/usagebased/service/rating/delta/engine.go`:
- Around line 112-143: The engine is redundantly re-checking
ChildUniqueReferenceID uniqueness after calling subtract.SubtractRatedRunDetails
(which already validates uniqueness by default); remove the post-subtract
grouping/dup-check (the lo.GroupBy block that iterates childUniqueReferenceIDs
and errors on duplicates) and rely on subtract.SubtractRatedRunDetails for
validation, then remove the lo import if it becomes unused; alternatively, if
you prefer to keep validation here, call subtract.SubtractRatedRunDetails with
subtract.WithUniqueReferenceIDValidationIgnored() so subtract skips validation
and this file remains the single source of truth.
In
`@openmeter/billing/charges/usagebased/service/rating/delta/tieredvolume_test.go`:
- Around line 76-152: Add a complementary test to cover the inverse volume‑tier
crossing (e.g., 16 -> 15 or 16 -> 14) so delta rating undoes the cheaper
cumulative tier and emits a positive adjustment for the higher per‑unit lower
tier; copy the structure of
TestVolumeTieredDeltaUnitTierCrossingRepricesCurrentPeriod (or create
TestVolumeTieredDeltaUnitTierCrossingRepricesBackward) using deltaRatingTestCase
with phases that start in a higher cumulative tier (meteredQuantity ~16) then
drop to a lower cumulative tier (meteredQuantity ~15 or 14), and assert
expectedDetailedLines includes a positive adjustment line referencing the
original child ID (similar to "volume-tiered-price#correction:...") with correct
PerUnitAmount, positive Quantity/Amount and updated expectedTotals so the sign
flip path is exercised.
In `@openmeter/billing/charges/usagebased/service/rating/details.go`:
- Around line 170-179: The local variable inside the currentBillingPeriod
function shadows the function name; rename the variable (e.g., to period or out)
to avoid shadowing, update all references inside the function (assignment and
the return) to the new name, and ensure the function still returns the renamed
variable of type timeutil.ClosedPeriod; specifically modify the variable
declaration currently named currentBillingPeriod and its From update inside
currentBillingPeriod(...) to use the new identifier.
In `@openmeter/billing/charges/usagebased/service/rating/service_test.go`:
- Around line 37-50: The test TestFormatDetailedLineChildUniqueReferenceID lives
in the rating package but exercises the pure helper
ratingtestutils.FormatDetailedLineChildUniqueReferenceID; move this test next to
the helper by creating a new test file (e.g., testutils_test.go) in the
testutils package (use a separate _test package if you prefer) and copy the test
there (or alternatively remove the test here if you prefer relying on downstream
coverage), ensuring the test still calls
ratingtestutils.FormatDetailedLineChildUniqueReferenceID with the same
servicePeriod setup and assertions so coverage is colocated with the helper.
In `@openmeter/billing/charges/usagebased/service/rating/subtract/subtract.go`:
- Around line 95-108: Add a one-line explanatory comment inside
validateDetailedLinesForSubtract above the clone/negation block clarifying that
we intentionally flip negative line.Quantity values (using line.Clone(),
Quantity.IsNegative() and Quantity.Neg()) so we "validate as if the line were
positive" and allow negative-quantity correction inputs to pass line.Validate();
also note in the same comment that Totals are intentionally not normalized here
because Validate() does not require sign consistency.
In
`@openmeter/billing/charges/usagebased/service/rating/subtract/uniquereferenceid.go`:
- Around line 1-48: The file currently exports UniqueReferenceIDGenerator and
also defines/testing-imports the test-only NewMockUniqueReferenceIDGenerator and
mockUniqueReferenceIDGenerator, which pulls the testing package into production
builds; keep the UniqueReferenceIDGenerator interface in this file but remove
the mock and testing import and move them into a sibling test package (e.g.,
package subtracttest) in a new file under
subtract/subtracttest/uniquereferenceid.go; implement
NewMockUniqueReferenceIDGenerator, mockUniqueReferenceIDGenerator and its
methods (CurrentOnly, MatchedDelta, PreviousOnlyReversal) there, returning a
subtract.UniqueReferenceIDGenerator, and update tests to import subtracttest to
obtain the mock.
In `@openmeter/billing/charges/usagebased/service/rating/testutils/testutils.go`:
- Around line 22-68: The test helpers use float64 fields (ExpectedDetailedLine
and ExpectedTotals) and InexactFloat64() conversions which can mask IEEE-754
drift; change the amount fields in ExpectedTotals (Amount, ChargesTotal,
DiscountsTotal, TaxesInclusiveTotal, TaxesExclusiveTotal, TaxesTotal,
CreditsTotal, Total) and the PerUnitAmount/Quantity in ExpectedDetailedLine to
string, and in ToExpectedTotals and ToExpectedDetailedLinesWithServicePeriod
convert using the decimal value's String() (e.g., Decimal.String()) instead of
InexactFloat64(), so tests assert deterministic decimal string equality; update
any callers in tests to compare the string representations accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aee026d7-6cf9-47eb-81b0-9483e060ec6c
📒 Files selected for processing (21)
openmeter/billing/charges/service/invoicable_test.goopenmeter/billing/charges/usagebased/service/rating/delta/README.mdopenmeter/billing/charges/usagebased/service/rating/delta/base_test.goopenmeter/billing/charges/usagebased/service/rating/delta/dynamic_test.goopenmeter/billing/charges/usagebased/service/rating/delta/engine.goopenmeter/billing/charges/usagebased/service/rating/delta/engine_test.goopenmeter/billing/charges/usagebased/service/rating/delta/package_test.goopenmeter/billing/charges/usagebased/service/rating/delta/tieredgraduated_test.goopenmeter/billing/charges/usagebased/service/rating/delta/tieredvolume_test.goopenmeter/billing/charges/usagebased/service/rating/delta/uniquereferenceid.goopenmeter/billing/charges/usagebased/service/rating/delta/unit_test.goopenmeter/billing/charges/usagebased/service/rating/detailedline.goopenmeter/billing/charges/usagebased/service/rating/details.goopenmeter/billing/charges/usagebased/service/rating/service.goopenmeter/billing/charges/usagebased/service/rating/service_test.goopenmeter/billing/charges/usagebased/service/rating/subtract/README.mdopenmeter/billing/charges/usagebased/service/rating/subtract/subtract.goopenmeter/billing/charges/usagebased/service/rating/subtract/subtract_test.goopenmeter/billing/charges/usagebased/service/rating/subtract/uniquereferenceid.goopenmeter/billing/charges/usagebased/service/rating/testutils/testutils.goopenmeter/billing/charges/usagebased/service/rating/uniqueref.go
💤 Files with no reviewable changes (2)
- openmeter/billing/charges/usagebased/service/rating/detailedline.go
- openmeter/billing/charges/usagebased/service/rating/uniqueref.go
turip
force-pushed
the
split/rating-scaffolding
branch
from
4dbbe0e to
fa80b79
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (7)
openmeter/billing/charges/usagebased/service/rating/subtract/subtract.go (1)
168-202: 💤 Low valueTiny preallocation nit on the output slice.
outis sized tolen(keys), but each key can contribute multiple per-unit-amount rows (current-only, matched-delta, previous-only-reversal). Not a correctness issue, just a couple of extra growslice calls in volume-tier-style scenarios. Pre-sizing tolen(current)+len(previous)would match the inner helper's bound.♻️ Optional preallocation tweak
- out := make(usagebased.DetailedLines, 0, len(keys)) + out := make(usagebased.DetailedLines, 0, len(current)+len(previous))🤖 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 `@openmeter/billing/charges/usagebased/service/rating/subtract/subtract.go` around lines 168 - 202, The output slice preallocation in subtractDetailedLinesByKey currently uses len(keys) but each key can emit multiple lines; change the out allocation to use len(current)+len(previous) to avoid extra grows (referencing the subtractDetailedLinesByKey function, the out variable, and the helper subtractDetailedLinesWithSameKey which can return multiple entries per key). Keep the rest of the logic the same; this simple preallocation tweak reduces reallocations in volume-tier scenarios.openmeter/billing/charges/usagebased/service/rating/delta/uniquereferenceid.go (1)
19-25: 💤 Low valueConsider also guarding
PricerReferenceIDin the reversal ID.
PreviousOnlyReversalreturns an error whenline.IDis empty, but the formatted output also embedsline.PricerReferenceID. If a previous line ever lacksPricerReferenceID, you'd silently produce a malformed reference like#correction:detailed_line_id=…(and any uniqueness check downstream might still consider that "valid"). A small guard here would catch the misuse close to the source.♻️ Optional defensive check
func (uniqueReferenceIDGenerator) PreviousOnlyReversal(line usagebased.DetailedLine) (string, error) { if line.ID == "" { return "", fmt.Errorf("detailed line id is required") } + if line.PricerReferenceID == "" { + return "", fmt.Errorf("pricer reference id is required") + } return fmt.Sprintf("%s#correction:detailed_line_id=%s", line.PricerReferenceID, line.ID), nil }🤖 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 `@openmeter/billing/charges/usagebased/service/rating/delta/uniquereferenceid.go` around lines 19 - 25, PreviousOnlyReversal currently only validates line.ID but then formats line.PricerReferenceID into the returned string; add a guard in PreviousOnlyReversal to also validate that line.PricerReferenceID is non-empty and return an error (e.g., "pricer reference id is required") if missing before calling fmt.Sprintf; update the error path so callers get a clear failure instead of producing a malformed "#correction:..." reference.openmeter/billing/charges/usagebased/service/rating/subtract/uniquereferenceid.go (1)
1-48: ⚡ Quick winMove the mock to the testutils subpackage to keep test doubles out of production code.
The
testingpackage is imported here in a non-_test.gofile, which pulls test infrastructure and debug flags into any build that uses thesubtractpackage. The codebase already follows a clear pattern:openmeter/billing/charges/usagebased/service/rating/testutils/testutils.gois the dedicated spot for test helpers (mocks, constructors that taketesting.TB, etc.). MoveNewMockUniqueReferenceIDGeneratorand themockUniqueReferenceIDGeneratorimplementation there alongside the other test constructors, keepingUniqueReferenceIDGeneratorinterface right here where it belongs.🤖 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 `@openmeter/billing/charges/usagebased/service/rating/subtract/uniquereferenceid.go` around lines 1 - 48, The mock implementation and its constructor (NewMockUniqueReferenceIDGenerator, mockUniqueReferenceIDGenerator and its methods CurrentOnly, MatchedDelta, PreviousOnlyReversal) should be moved out of the production subtract package into the testutils subpackage used for test doubles; keep only the UniqueReferenceIDGenerator interface in this file, remove the testing import and any mock code here, then create a new file in the testutils package (e.g., testutils.go) that defines NewMockUniqueReferenceIDGenerator(t testing.TB) and mockUniqueReferenceIDGenerator with the same method implementations so tests can import testutils.NewMockUniqueReferenceIDGenerator without polluting production builds.openmeter/billing/charges/usagebased/service/rating/delta/engine.go (1)
40-53: ⚡ Quick winKeep explicit validation for
CurrentPeriod.ServicePeriod.Right now this only checks containment inside the intent period. That makes zero/reversed periods depend on whatever
ContainsPeriodInclusivehappens to do, which is a pretty loose contract for a new input type. Restoring the directIsZero()/Validate()checks keeps bad inputs from reaching rating/subtraction with harder-to-diagnose errors.Suggested change
func (i Input) Validate() error { var errs []error if err := i.Intent.Validate(); err != nil { errs = append(errs, fmt.Errorf("intent: %w", err)) } + + if i.CurrentPeriod.ServicePeriod.IsZero() { + errs = append(errs, fmt.Errorf("current period service period is zero")) + } else if err := i.CurrentPeriod.ServicePeriod.Validate(); err != nil { + errs = append(errs, fmt.Errorf("current period service period validation failed: %w", err)) + } intentServicePeriod := i.Intent.ServicePeriod if !intentServicePeriod.ContainsPeriodInclusive(i.CurrentPeriod.ServicePeriod) { errs = append(errs, fmt.Errorf("current period service period must be contained in intent service period: [%s..%s] vs [%s..%s]", intentServicePeriod.From.Format(time.RFC3339), intentServicePeriod.To.Format(time.RFC3339),🤖 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 `@openmeter/billing/charges/usagebased/service/rating/delta/engine.go` around lines 40 - 53, The Validate method currently only checks containment via intentServicePeriod.ContainsPeriodInclusive which is insufficient; update Input.Validate to explicitly validate CurrentPeriod.ServicePeriod by first checking if i.CurrentPeriod.ServicePeriod.IsZero() and adding a clear error if so, then call i.CurrentPeriod.ServicePeriod.Validate() (or equivalent validation method) and append any returned error before performing the containment check with intentServicePeriod.ContainsPeriodInclusive; keep the existing intent validation (i.Intent.Validate()) and the final return using models.NewNillableGenericValidationError(errors.Join(errs...)). Ensure you reference the same symbols: Input.Validate, i.CurrentPeriod.ServicePeriod, i.Intent.ServicePeriod, ContainsPeriodInclusive, IsZero, Validate, and models.NewNillableGenericValidationError.openmeter/billing/charges/usagebased/service/rating/delta/tieredgraduated_test.go (1)
26-47: ⚡ Quick winConsider extracting shared tiered price fixtures to reduce duplication.
The repeated
productcatalog.TieredPricesetup makes the tests harder to scan and maintain. A small helper (e.g.,newGraduatedTieredPrice()/newGraduatedTieredPriceWithFlat()) would keep each test focused on the behavior being asserted.♻️ Example refactor sketch
+func newGraduatedTieredPrice() productcatalog.Price { + return *productcatalog.NewPriceFrom(productcatalog.TieredPrice{ + Mode: productcatalog.GraduatedTieredPrice, + Tiers: []productcatalog.PriceTier{ + { + UpToAmount: lo.ToPtr(alpacadecimal.NewFromInt(5)), + UnitPrice: &productcatalog.PriceTierUnitPrice{Amount: alpacadecimal.NewFromInt(1)}, + }, + { + UpToAmount: lo.ToPtr(alpacadecimal.NewFromInt(10)), + UnitPrice: &productcatalog.PriceTierUnitPrice{Amount: alpacadecimal.NewFromInt(2)}, + }, + { + UnitPrice: &productcatalog.PriceTierUnitPrice{Amount: alpacadecimal.NewFromInt(3)}, + }, + }, + }) +} ... - price: *productcatalog.NewPriceFrom(productcatalog.TieredPrice{ ... }), + price: newGraduatedTieredPrice(),As per coding guidelines: "
**/*.go: In general when reviewing the Golang code make readability and maintainability a priority."Also applies to: 107-128, 199-220, 302-323, 383-404, 486-513, 584-611
🤖 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 `@openmeter/billing/charges/usagebased/service/rating/delta/tieredgraduated_test.go` around lines 26 - 47, Extract the repeated TieredPrice fixtures into one or two helpers (e.g., newGraduatedTieredPrice and newGraduatedTieredPriceWithFlat) and replace the inline productcatalog.NewPriceFrom(productcatalog.TieredPrice{...}) blocks in tests with calls to those helpers; implement the helpers to return *productcatalog.Price (or the exact type used by productcatalog.NewPriceFrom) built from the same Tiers (including UpToAmount and UnitPrice values) so tests at locations referencing productcatalog.TieredPrice, productcatalog.NewPriceFrom, and the specific tier Amounts can simply call the helper to reduce duplication and improve readability.openmeter/billing/charges/usagebased/service/rating/delta/dynamic_test.go (2)
60-77: ⚡ Quick winHelper builders for expected lines would make these tests easier to read.
There’s a lot of repeated
ExpectedDetailedLineconstruction for usage and correction rows; small helpers would cut noise and keep each test focused on scenario intent.As per coding guidelines: "
**/*.go: In general when reviewing the Golang code make readability and maintainability a priority."Also applies to: 102-113, 123-145, 175-186, 223-234, 244-266, 300-311, 321-344, 379-390, 400-411
🤖 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 `@openmeter/billing/charges/usagebased/service/rating/delta/dynamic_test.go` around lines 60 - 77, Replace repeated verbose constructions of ratingtestutils.ExpectedDetailedLine and ratingtestutils.ExpectedTotals in dynamic_test.go with small helper builders (e.g., in the test file or ratingtestutils) such as a function like newExpectedLine(childID string, category stddetailedline.Category, servicePeriod *periodType, perUnit float64, qty int, amount float64) that returns ratingtestutils.ExpectedDetailedLine and a newExpectedTotals(amount, total float64) that returns ratingtestutils.ExpectedTotals; update the test cases (the blocks creating ExpectedDetailedLine with billingrating.UsageChildUniqueReferenceID, stddetailedline.CategoryRegular, lo.ToPtr(periods.period1), PerUnitAmount, Quantity, and Totals) to call these helpers to reduce duplication and improve readability.
82-201: ⚡ Quick winAdd one explicit dynamic snapshot-decrease test case.
You already cover increase and unchanged snapshots; adding a decrease path (e.g., phase1
15→ phase210) would lock in negative-delta behavior for dynamic pricing too.As per coding guidelines: "
**/*_test.go: Make sure the tests are comprehensive and cover the changes."🤖 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 `@openmeter/billing/charges/usagebased/service/rating/delta/dynamic_test.go` around lines 82 - 201, Add a new test function (e.g., TestDynamicDeltaAdditionalUsageHandlesDecrease) that mirrors the existing dynamic tests but exercises a decreased cumulative snapshot (phase1 meteredQuantity 15 → phase2 meteredQuantity 10); use runDeltaRatingTestCase with a price created via productcatalog.NewPriceFrom(productcatalog.DynamicPrice{Multiplier: alpacadecimal.NewFromInt(1)}), construct two deltaRatingPhase entries with the first booking the 15 cumulative amount and the second providing a smaller current snapshot 10, and assert the expectedDetailedLines include a negative correction line (Quantity -1, PerUnitAmount equal to the prior 15, with appropriate Totals) and expectedTotals reflecting the negative delta so the deltaRatingTestCase behavior for decreases is covered.
🤖 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.
Nitpick comments:
In `@openmeter/billing/charges/usagebased/service/rating/delta/dynamic_test.go`:
- Around line 60-77: Replace repeated verbose constructions of
ratingtestutils.ExpectedDetailedLine and ratingtestutils.ExpectedTotals in
dynamic_test.go with small helper builders (e.g., in the test file or
ratingtestutils) such as a function like newExpectedLine(childID string,
category stddetailedline.Category, servicePeriod *periodType, perUnit float64,
qty int, amount float64) that returns ratingtestutils.ExpectedDetailedLine and a
newExpectedTotals(amount, total float64) that returns
ratingtestutils.ExpectedTotals; update the test cases (the blocks creating
ExpectedDetailedLine with billingrating.UsageChildUniqueReferenceID,
stddetailedline.CategoryRegular, lo.ToPtr(periods.period1), PerUnitAmount,
Quantity, and Totals) to call these helpers to reduce duplication and improve
readability.
- Around line 82-201: Add a new test function (e.g.,
TestDynamicDeltaAdditionalUsageHandlesDecrease) that mirrors the existing
dynamic tests but exercises a decreased cumulative snapshot (phase1
meteredQuantity 15 → phase2 meteredQuantity 10); use runDeltaRatingTestCase with
a price created via
productcatalog.NewPriceFrom(productcatalog.DynamicPrice{Multiplier:
alpacadecimal.NewFromInt(1)}), construct two deltaRatingPhase entries with the
first booking the 15 cumulative amount and the second providing a smaller
current snapshot 10, and assert the expectedDetailedLines include a negative
correction line (Quantity -1, PerUnitAmount equal to the prior 15, with
appropriate Totals) and expectedTotals reflecting the negative delta so the
deltaRatingTestCase behavior for decreases is covered.
In `@openmeter/billing/charges/usagebased/service/rating/delta/engine.go`:
- Around line 40-53: The Validate method currently only checks containment via
intentServicePeriod.ContainsPeriodInclusive which is insufficient; update
Input.Validate to explicitly validate CurrentPeriod.ServicePeriod by first
checking if i.CurrentPeriod.ServicePeriod.IsZero() and adding a clear error if
so, then call i.CurrentPeriod.ServicePeriod.Validate() (or equivalent validation
method) and append any returned error before performing the containment check
with intentServicePeriod.ContainsPeriodInclusive; keep the existing intent
validation (i.Intent.Validate()) and the final return using
models.NewNillableGenericValidationError(errors.Join(errs...)). Ensure you
reference the same symbols: Input.Validate, i.CurrentPeriod.ServicePeriod,
i.Intent.ServicePeriod, ContainsPeriodInclusive, IsZero, Validate, and
models.NewNillableGenericValidationError.
In
`@openmeter/billing/charges/usagebased/service/rating/delta/tieredgraduated_test.go`:
- Around line 26-47: Extract the repeated TieredPrice fixtures into one or two
helpers (e.g., newGraduatedTieredPrice and newGraduatedTieredPriceWithFlat) and
replace the inline productcatalog.NewPriceFrom(productcatalog.TieredPrice{...})
blocks in tests with calls to those helpers; implement the helpers to return
*productcatalog.Price (or the exact type used by productcatalog.NewPriceFrom)
built from the same Tiers (including UpToAmount and UnitPrice values) so tests
at locations referencing productcatalog.TieredPrice,
productcatalog.NewPriceFrom, and the specific tier Amounts can simply call the
helper to reduce duplication and improve readability.
In
`@openmeter/billing/charges/usagebased/service/rating/delta/uniquereferenceid.go`:
- Around line 19-25: PreviousOnlyReversal currently only validates line.ID but
then formats line.PricerReferenceID into the returned string; add a guard in
PreviousOnlyReversal to also validate that line.PricerReferenceID is non-empty
and return an error (e.g., "pricer reference id is required") if missing before
calling fmt.Sprintf; update the error path so callers get a clear failure
instead of producing a malformed "#correction:..." reference.
In `@openmeter/billing/charges/usagebased/service/rating/subtract/subtract.go`:
- Around line 168-202: The output slice preallocation in
subtractDetailedLinesByKey currently uses len(keys) but each key can emit
multiple lines; change the out allocation to use len(current)+len(previous) to
avoid extra grows (referencing the subtractDetailedLinesByKey function, the out
variable, and the helper subtractDetailedLinesWithSameKey which can return
multiple entries per key). Keep the rest of the logic the same; this simple
preallocation tweak reduces reallocations in volume-tier scenarios.
In
`@openmeter/billing/charges/usagebased/service/rating/subtract/uniquereferenceid.go`:
- Around line 1-48: The mock implementation and its constructor
(NewMockUniqueReferenceIDGenerator, mockUniqueReferenceIDGenerator and its
methods CurrentOnly, MatchedDelta, PreviousOnlyReversal) should be moved out of
the production subtract package into the testutils subpackage used for test
doubles; keep only the UniqueReferenceIDGenerator interface in this file, remove
the testing import and any mock code here, then create a new file in the
testutils package (e.g., testutils.go) that defines
NewMockUniqueReferenceIDGenerator(t testing.TB) and
mockUniqueReferenceIDGenerator with the same method implementations so tests can
import testutils.NewMockUniqueReferenceIDGenerator without polluting production
builds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e23875f1-4101-4511-b173-483460fd4c62
📒 Files selected for processing (23)
openmeter/billing/charges/service/invoicable_test.goopenmeter/billing/charges/usagebased/service/rating/delta/README.mdopenmeter/billing/charges/usagebased/service/rating/delta/base_test.goopenmeter/billing/charges/usagebased/service/rating/delta/dynamic_test.goopenmeter/billing/charges/usagebased/service/rating/delta/engine.goopenmeter/billing/charges/usagebased/service/rating/delta/engine_test.goopenmeter/billing/charges/usagebased/service/rating/delta/package_test.goopenmeter/billing/charges/usagebased/service/rating/delta/tieredgraduated_test.goopenmeter/billing/charges/usagebased/service/rating/delta/tieredvolume_test.goopenmeter/billing/charges/usagebased/service/rating/delta/uniquereferenceid.goopenmeter/billing/charges/usagebased/service/rating/delta/unit_test.goopenmeter/billing/charges/usagebased/service/rating/detailedline.goopenmeter/billing/charges/usagebased/service/rating/details.goopenmeter/billing/charges/usagebased/service/rating/service.goopenmeter/billing/charges/usagebased/service/rating/service_test.goopenmeter/billing/charges/usagebased/service/rating/subtract/README.mdopenmeter/billing/charges/usagebased/service/rating/subtract/subtract.goopenmeter/billing/charges/usagebased/service/rating/subtract/subtract_test.goopenmeter/billing/charges/usagebased/service/rating/subtract/uniquereferenceid.goopenmeter/billing/charges/usagebased/service/rating/testutils/testutils.goopenmeter/billing/charges/usagebased/service/rating/uniqueref.gotest/credits/credit_then_invoice_test.gotest/credits/sanity_test.go
💤 Files with no reviewable changes (2)
- openmeter/billing/charges/usagebased/service/rating/uniqueref.go
- openmeter/billing/charges/usagebased/service/rating/detailedline.go
✅ Files skipped from review due to trivial changes (1)
- openmeter/billing/charges/usagebased/service/rating/subtract/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- openmeter/billing/charges/usagebased/service/rating/delta/unit_test.go
- openmeter/billing/charges/usagebased/service/rating/subtract/subtract_test.go
tothandras
approved these changes
May 7, 2026
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.
Delta Rating
Delta rating is the temporary production detailed-line engine for usage-based
charges. It rates the latest cumulative meter snapshot, subtracts every detailed
line already booked for the charge, and books the remaining delta on the current
run's service period.
The engine intentionally does not preserve the original service period of
corrections. That keeps downstream invoicing in the simpler "current invoice
period only" shape while the period-preserving engine and invoicing support for
corrections evolve.
Repricing is expected with non-linear prices. For example, a volume-tiered price
can rate
15units at one unit amount and16units at a different unitamount for the whole quantity. Delta rating handles this by emitting a reversal
for the previously booked price component and a current line for the newly rated
price component, both on the current run's service period.
Algorithm
Input:
Calculation:
The snapshot covers
[intent.ServicePeriod.From, currentPeriod.To).service-period snapshot.
PricerReferenceIDstores the billing-rating child reference used forarithmetic matching.
ChildUniqueReferenceIDstarts from the generated childreference, but generated billing-rating output can contain duplicates and the
subtraction generator can rewrite it for correction lines.
Credits are allocated after rating, so credit changes must not look like
usage or pricing changes.
subtract.SubtractRatedRunDetails(current, alreadyBilled, generator).CorrectsRunID.ChildUniqueReferenceIDvalues are unique.The result totals are the sum of the emitted detailed lines.
Example: Additional Unit Usage
Price: unit price
10.Already billed in period 1:
Current cumulative snapshot at period 2:
8units.Billing rating generates the cumulative state:
Delta subtracts what was already booked:
Output:
Example: Volume-Tiered Repricing Correction
Price: volume-tiered price where
1..15units rate at$10/unit, and16+units rate at
$5/unitfor the whole quantity.Already billed:
Current cumulative rating:
Delta output:
The two lines share the same
PricerReferenceID, but theirPerUnitAmountdiffers. Subtraction therefore treats this as repricing instead of a quantity
delta. Both output lines are stamped to the current run service period. The
correction child reference is deterministic and points at the previous detailed
line ID so persistence can distinguish the reversal from the current generated
line.
Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests
Summary by CodeRabbit
New Features
Tests