feat(billing): add period preserving usage rating#4299
Merged
Conversation
Contributor
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a Period-Preserving rating engine and routes GetDetailedRatingForUsage to it; the engine validates multi-period inputs, runs epoch-by-epoch rating preserving original service-period attribution, subtracts prior and already-billed lines, restamps and orders detailed lines, and includes README plus extensive tests. ChangesPeriod-Preserving Rating Engine
Sequence DiagramsequenceDiagram
participant Client
participant Service
participant DetailsHelper
participant PeriodPreservingEngine
participant RatingService
participant BookedLinesStore
Client->>Service: GetDetailedRatingForUsage(intent, mode=PeriodPreserving)
Service->>DetailsHelper: prepare prior/current periods + booked lines
DetailsHelper->>PeriodPreservingEngine: Rate(Input)
PeriodPreservingEngine->>RatingService: rate epoch inputs (per epoch)
RatingService-->>PeriodPreservingEngine: epoch detailed lines
PeriodPreservingEngine->>BookedLinesStore: fetch prior booked lines
PeriodPreservingEngine->>PeriodPreservingEngine: subtract prior/epoch lines, stamp corrections
PeriodPreservingEngine-->>Service: DetailedLines + Totals
Service-->>Client: return combined detailed lines and totals
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 (2)
openmeter/billing/charges/usagebased/service/rating/periodpreserving/engine_test.go (1)
1233-1296: 💤 Low valueTiny note on the phased harness — purely optional. 💭
runLateEventRatingTestCaseregisters all phase subtests inside the same outer test viat.Run, and because the inner subtests don't callt.Parallel(), they run sequentially and the sharedbookedDetailedLinesByPhase/phaseRunIDsstay safe. That's the intended design. If a future contributor ever sprinklest.Parallel()inside the innert.Run, the shared mutable state would silently break under-race. A one-line comment near the loop noting "phases must run sequentially because each phase consumes the previous phase's booked detailed lines" would prevent that footgun without changing behavior.🤖 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/periodpreserving/engine_test.go` around lines 1233 - 1296, The phased test harness runLateEventRatingTestCase uses shared mutable state (bookedDetailedLinesByPhase and phaseRunIDs) across subtests and relies on them running sequentially; add a concise one-line comment above the for loop that iterates tc.phases explaining that subtests must not call t.Parallel() because each phase consumes prior phases' bookedDetailedLines and phaseRunIDs (reference runLateEventRatingTestCase, bookedDetailedLinesByPhase, phaseRunIDs, and t.Run/t.Parallel) to prevent future contributors from making the inner subtests parallel and breaking the harness under -race.openmeter/billing/charges/usagebased/service/rating/periodpreserving/engine.go (1)
140-157: 💤 Low valueHeads-up on the second-granularity assumption. ⏱️
epochClosedPeriodstoresFrom/ToasUnix()seconds andAsClosedPeriodreconstructs them viatime.Unix(e.From, 0).In(time.UTC). That round-trip silently drops sub-second precision and TZ info, and it's also what stampsline.ServicePeriodin the rated output (viaNewDetailedLinesFromBilling). In practice this is fine becauseValidate()requires prior periods to be non-empty when truncated tostreaming.MinimumWindowSizeDuration, and production periods are second-aligned anyway — so the Unix-second key is consistent across the input bucket and the stamped output bucket.The footgun is for any future caller that hands in a
priorPeriod.ServicePeriodwith sub-second precision (say12:00:00.500..12:00:02.000): the validation would still pass, but the stamped line period and therunIDByServicePeriodlookup would both use[12:00:00..12:00:02], quietly losing the.500. Either:
- a one-line comment near
epochClosedPeriodexplicitly stating "service periods are normalized to second precision; sub-second input is intentionally dropped", or- using
UnixNano()for symmetric precision,would prevent future surprises. Not blocking — this is a defensive nudge.
🤖 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/periodpreserving/engine.go` around lines 140 - 157, epochClosedPeriod currently stores From/To as seconds via Unix(), and AsClosedPeriod/closedPeriodToEpochClosedPeriod round-trip using time.Unix(...,0), which silently drops sub-second precision and TZ info; to fix, either (A) update epochClosedPeriod.From/To to be nanoseconds (use time.Unix(0, ns) in AsClosedPeriod and period.From.UnixNano()/UnixNano() in closedPeriodToEpochClosedPeriod) so sub-second precision is preserved, or (B) add a clear one-line comment on epochClosedPeriod stating that service periods are intentionally normalized to second precision and sub-second input will be dropped; modify the functions epochClosedPeriod.AsClosedPeriod and closedPeriodToEpochClosedPeriod accordingly to match the chosen approach.
🤖 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/periodpreserving/engine_test.go`:
- Around line 1233-1296: The phased test harness runLateEventRatingTestCase uses
shared mutable state (bookedDetailedLinesByPhase and phaseRunIDs) across
subtests and relies on them running sequentially; add a concise one-line comment
above the for loop that iterates tc.phases explaining that subtests must not
call t.Parallel() because each phase consumes prior phases' bookedDetailedLines
and phaseRunIDs (reference runLateEventRatingTestCase,
bookedDetailedLinesByPhase, phaseRunIDs, and t.Run/t.Parallel) to prevent future
contributors from making the inner subtests parallel and breaking the harness
under -race.
In
`@openmeter/billing/charges/usagebased/service/rating/periodpreserving/engine.go`:
- Around line 140-157: epochClosedPeriod currently stores From/To as seconds via
Unix(), and AsClosedPeriod/closedPeriodToEpochClosedPeriod round-trip using
time.Unix(...,0), which silently drops sub-second precision and TZ info; to fix,
either (A) update epochClosedPeriod.From/To to be nanoseconds (use time.Unix(0,
ns) in AsClosedPeriod and period.From.UnixNano()/UnixNano() in
closedPeriodToEpochClosedPeriod) so sub-second precision is preserved, or (B)
add a clear one-line comment on epochClosedPeriod stating that service periods
are intentionally normalized to second precision and sub-second input will be
dropped; modify the functions epochClosedPeriod.AsClosedPeriod and
closedPeriodToEpochClosedPeriod accordingly to match the chosen approach.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 941bde20-ccda-480d-b664-db7cce0f50d6
📒 Files selected for processing (7)
openmeter/billing/charges/usagebased/service/rating/details.goopenmeter/billing/charges/usagebased/service/rating/periodpreserving/README.mdopenmeter/billing/charges/usagebased/service/rating/periodpreserving/engine.goopenmeter/billing/charges/usagebased/service/rating/periodpreserving/engine_test.goopenmeter/billing/charges/usagebased/service/rating/periodpreserving/uniquereferenceid.goopenmeter/billing/charges/usagebased/service/rating/service.goopenmeter/billing/charges/usagebased/service/rating/service_test.go
turip
force-pushed
the
split/rating-delta
branch
2 times, most recently
from
d473363 to
fa7912c
Compare
turip
force-pushed
the
split/rating-period-preserving
branch
from
ea108e9 to
44c8bc6
Compare
turip
added
release-note/ignore
tothandras
approved these changes
May 8, 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.
Stacked PR 3/3. Adds the period-preserving rating engine and its service dispatch/tests on top of the delta engine. The production preferred engine remains delta.
This incomplete engine should be only merged, to make sure that the tests are there, so any billingrating changes are tested against this algorithm.
Summary by CodeRabbit
New Features
Documentation
Tests