Open
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR implements a comprehensive usage-based charge advancement system with credit-only settlement mode, introducing charge listing by customer, new realization run lifecycle management with collection-period timing, credit allocation workflows, and state machine-driven status transitions. Major adapter refactoring splits concerns into specialized interfaces (ChargeAdapter, RealizationRunAdapter, RealizationRunCreditAllocationAdapter), while service layer orchestrates advancement, rating, and persistence across the charge lifecycle. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ChargeService as Charge<br/>Service
participant MetaAdapter as Meta<br/>Adapter
participant UsageBasedService as UsageBased<br/>Service
participant Locker
participant RatingService as Rating<br/>Service
participant Adapter as UsageBased<br/>Adapter
participant Handler
Client->>ChargeService: AdvanceCharges(customer)
ChargeService->>MetaAdapter: ListByCustomer(customer)
MetaAdapter-->>ChargeService: [non-final charges]
ChargeService->>ChargeService: Filter UsageBased type
ChargeService->>UsageBasedService: GetByMetas + FeatureMeter resolution
UsageBasedService-->>ChargeService: [usage-based charges]
loop For each usage-based charge
ChargeService->>UsageBasedService: AdvanceCharge(chargeID, override, meter)
UsageBasedService->>Adapter: GetByID(chargeID, expand realizations)
Adapter-->>UsageBasedService: charge
UsageBasedService->>Locker: LockForTX(chargeID)
Locker-->>UsageBasedService: lock acquired
alt SettlementMode == CreditOnly
UsageBasedService->>UsageBasedService: NewCreditsOnlyStateMachine
loop Until state stable
UsageBasedService->>RatingService: Rate charge
RatingService-->>UsageBasedService: rating result
UsageBasedService->>Handler: OnCollectionStarted/Finalized
Handler-->>UsageBasedService: credit allocations
UsageBasedService->>Adapter: CreateRealizationRun + CreateRunCreditAllocations
Adapter-->>UsageBasedService: run created
UsageBasedService->>Adapter: UpdateCharge(new state)
Adapter-->>UsageBasedService: updated charge
end
UsageBasedService-->>ChargeService: advanced charge
else SettlementMode != CreditOnly
UsageBasedService-->>ChargeService: not implemented error
end
end
ChargeService-->>Client: [advanced charges]
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate 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
force-pushed
the
feat/usage-based-charges
branch
from
91086aa to
14bf466
Compare
turip
force-pushed
the
feat/usage-based-charges-lifecycle
branch
from
3edee44 to
ce93c06
Compare
turip
force-pushed
the
feat/usage-based-charges-lifecycle
branch
from
ce93c06 to
b7578f0
Compare
turip
force-pushed
the
feat/usage-based-charges-lifecycle
branch
from
b7578f0 to
b5ca17e
Compare
turip
force-pushed
the
feat/usage-based-charges-lifecycle
branch
from
b5ca17e to
ab9b345
Compare
turip
force-pushed
the
feat/usage-based-charges-lifecycle
branch
from
ab9b345 to
53b86a4
Compare
turip
force-pushed
the
feat/usage-based-charges-lifecycle
branch
from
53b86a4 to
73e1d91
Compare
turip
force-pushed
the
feat/usage-based-charges-lifecycle
branch
from
73e1d91 to
be402c2
Compare
turip
force-pushed
the
feat/usage-based-charges-lifecycle
branch
from
be402c2 to
f9ed2d5
Compare
turip
force-pushed
the
feat/usage-based-charges-lifecycle
branch
from
f9ed2d5 to
8c4d723
Compare
turip
force-pushed
the
feat/usage-based-charges-lifecycle
branch
from
8c4d723 to
6e2cd0d
Compare
turip
force-pushed
the
feat/usage-based-charges-lifecycle
branch
from
6e2cd0d to
0a8fa53
Compare
turip
force-pushed
the
feat/usage-based-charges-lifecycle
branch
from
0a8fa53 to
a5cf573
Compare
turip
force-pushed
the
feat/usage-based-charges-lifecycle
branch
from
a5cf573 to
413f856
Compare
turip
force-pushed
the
feat/usage-based-charges-lifecycle
branch
4 times, most recently
from
340eb94 to
c9a4b8d
Compare
turip
changed the title
turip
force-pushed
the
feat/usage-based-charges-lifecycle
branch
from
c9a4b8d to
06965ce
Compare
turip
force-pushed
the
feat/usage-based-charges-lifecycle
branch
from
06965ce to
248e508
Compare
![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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openmeter/billing/charges/service/invoice.go (1)
68-86:⚠️ Potential issue | 🔴 CriticalMissing switch cases for usage-based and credit-purchase charges.
The
handleChargeEventfunction only handlesChargeTypeFlatFee. Since you've wired upusageBasedandcreditPurchaseprocessors inprocessorByType, any charge of those types will hit thedefaultcase and error with "unsupported charge type".You'll need to add cases for
meta.ChargeTypeUsageBasedandmeta.ChargeTypeCreditPurchaseto actually invoke those processors.🐛 Proposed fix to add missing switch cases
for _, lineWithCharge := range linesWithCharges { switch lineWithCharge.Charge.Type() { case meta.ChargeTypeFlatFee: flatFee, err := lineWithCharge.Charge.AsFlatFeeCharge() if err != nil { return err } if processorByType.flatFee == nil { return fmt.Errorf("flat fee payment post processor is not supported") } err = processorByType.flatFee(ctx, flatFee, lineWithCharge.StandardLineWithInvoiceHeader) if err != nil { return err } + case meta.ChargeTypeUsageBased: + usageBased, err := lineWithCharge.Charge.AsUsageBasedCharge() + if err != nil { + return err + } + + if processorByType.usageBased == nil { + return fmt.Errorf("usage based payment post processor is not supported") + } + + err = processorByType.usageBased(ctx, usageBased, lineWithCharge.StandardLineWithInvoiceHeader) + if err != nil { + return err + } + case meta.ChargeTypeCreditPurchase: + creditPurchase, err := lineWithCharge.Charge.AsCreditPurchaseCharge() + if err != nil { + return err + } + + if processorByType.creditPurchase == nil { + return fmt.Errorf("credit purchase payment post processor is not supported") + } + + err = processorByType.creditPurchase(ctx, creditPurchase, lineWithCharge.StandardLineWithInvoiceHeader) + if err != nil { + return err + } default: return fmt.Errorf("unsupported charge type: %s", lineWithCharge.Charge.Type()) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/service/invoice.go` around lines 68 - 86, The switch in handleChargeEvent currently only handles meta.ChargeTypeFlatFee and returns an error for other types; add cases for meta.ChargeTypeUsageBased and meta.ChargeTypeCreditPurchase that mirror the flat-fee flow: call the appropriate type conversion methods (e.g., AsUsageBasedCharge and AsCreditPurchaseCharge) on lineWithCharge.Charge, check errors, verify the corresponding processor exists on processorByType (processorByType.usageBased and processorByType.creditPurchase), then invoke the processor with ctx, the typed charge, and lineWithCharge.StandardLineWithInvoiceHeader, returning any error from the processor; keep the same error patterns/messages used for flat-fee when a processor is missing or conversion fails.
🧹 Nitpick comments (7)
pkg/statelessx/actions.go (1)
13-24: Skip nil actions in this shared combinator.Now that
AllOfis reusable, a single optional callback left nil will panic the whole activation path. Treating nil entries as no-ops makes this helper a lot safer for future callers.♻️ Suggested tweak
func AllOf(fn ...ActionFn) ActionFn { return func(ctx context.Context) error { var outErr error for _, f := range fn { + if f == nil { + continue + } if err := f(ctx); err != nil { outErr = errors.Join(outErr, err) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/statelessx/actions.go` around lines 13 - 24, AllOf currently panics if any entry in the variadic fn slice is nil; update the combinator (function AllOf and its use of ActionFn) to treat nil actions as no-ops by skipping nil entries in the for loop (i.e., if f == nil continue) so callers can pass optional callbacks safely, while preserving the existing error-joining logic (errors.Join) and return behavior.openmeter/billing/charges/usagebased/service/quantitysnapshot.go (1)
68-75: Minor: Consider precision implications ofNewFromFloat.
alpacadecimal.NewFromFloat(row.Value)converts a float64 to Decimal. For meter values that are large or have many decimal places, this conversion may introduce subtle precision artifacts since float64 can't represent all decimal values exactly.If
meter.MeterQueryRow.Valueis always expected to be a "nice" number (e.g., counts), this is fine. But if high precision is needed, you might want to verify the source data or consider whether the meter API could return a string/decimal representation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/usagebased/service/quantitysnapshot.go` around lines 68 - 75, summarizeMeterQueryRow currently builds the sum by calling alpacadecimal.NewFromFloat(row.Value), which can introduce precision artifacts; update summarizeMeterQueryRow to construct decimals from a string or exact representation instead (e.g., use alpacadecimal.NewFromString on a string field or formatted value) for meter.MeterQueryRow.Value, and handle/propagate any conversion errors; if you cannot change the source type immediately, add a conversion branch that prefers NewFromString when a string representation is available and falls back to NewFromFloat only as a last resort, ensuring the logic lives in summarizeMeterQueryRow and references meter.MeterQueryRow.Value and alpacadecimal.NewFromString/NewFromFloat.openmeter/billing/charges/usagebased/handler.go (1)
14-41: Looks good overall!The
AllocateCreditsInputstruct and its validation are well-structured. Theerrors.Joinpattern is idiomatic Go for aggregating multiple validation errors.One small nit: Line 29's error message says "as of is required" but the field is actually named
AllocateAt. Might be worth aligning the error message with the field name for clearer debugging.if i.AllocateAt.IsZero() { - errs = append(errs, fmt.Errorf("as of is required")) + errs = append(errs, fmt.Errorf("allocate at is required")) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/usagebased/handler.go` around lines 14 - 41, Update the validation error message in AllocateCreditsInput.Validate: replace the message "as of is required" with one that references the actual field name (e.g., "allocateAt is required" or "AllocateAt is required") so the error clearly points to AllocateCreditsInput.AllocateAt when the timestamp is missing; adjust the string in the Validate method where i.AllocateAt.IsZero() is checked..agents/skills/charges/SKILL.md (2)
285-286: Minor grammar fix.The compound adjective "merged-profile-based" should use hyphens when modifying "collection-period resolution":
-- preserve merged-profile based collection-period resolution +- preserve merged-profile-based collection-period resolution🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/charges/SKILL.md around lines 285 - 286, The phrase "merged-profile based collection-period resolution" in the SKILL.md doc is missing hyphens; update the compound adjective to "merged-profile-based collection-period resolution" to correctly hyphenate the modifier, preserving the existing bullet that also mentions `AdvanceCharge(...)` and the "nil means noop" contract without changing meaning.
70-73: Minor wording nit.The static analysis flagged "only supports
CreditOnly" as repetitive. SinceCreditOnlyis an enum value, it's unavoidable, but you could rephrase slightly if it bothers you:-- `usagebased.Service.AdvanceCharge(...)` only supports `CreditOnly` +- `usagebased.Service.AdvanceCharge(...)` supports only the `CreditOnly` settlement modeTotally optional - the meaning is clear either way.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/charges/SKILL.md around lines 70 - 73, Rephrase the repetitive wording in the documentation: update the sentence that reads "usagebased.Service.AdvanceCharge(...) only supports `CreditOnly`" to a less repetitive form such as "usagebased.Service.AdvanceCharge(...) supports the CreditOnly mode" (or "only supports the CreditOnly mode") in SKILL.md so the meaning stays identical but avoids the awkward repetition of "only supports `CreditOnly`".openmeter/billing/charges/usagebased/service/triggers.go (1)
38-46: Switch pattern note.The empty
case productcatalog.CreditOnlySettlementMode:followed by code after the switch block is valid Go, but it might be slightly clearer to add a brief comment:switch charge.Intent.SettlementMode { case productcatalog.CreditOnlySettlementMode: // Handled below via credits-only state machine case productcatalog.CreditThenInvoiceSettlementMode: return nil, models.NewGenericNotImplementedError(...) default: return nil, fmt.Errorf(...) }This is purely a readability suggestion - the current code is correct.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/usagebased/service/triggers.go` around lines 38 - 46, Add a brief clarifying comment inside the empty case for productcatalog.CreditOnlySettlementMode in the switch on charge.Intent.SettlementMode (in triggers.go) to indicate why it’s intentionally empty and that the credits-only path is handled later (e.g., "Handled below via credits-only state machine"); this preserves the current behavior while improving readability for future maintainers.openmeter/billing/charges/service/invoicable_test.go (1)
407-575: Worth pinning down the persistedCollectionEndinvariant here.This flow keeps the billing profile fixed from
#3.1through#4.2, so it still passes if finalization accidentally recomputes the collection window from the latest profile instead of the value stored on the run. Changing the collection interval after the first realization starts would make that regression visible.As per coding guidelines,
**/*_test.go: Make sure the tests are comprehensive and cover the changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/service/invoicable_test.go` around lines 407 - 575, The test doesn't assert that the persisted CollectionEnd on the realization run remains unchanged across advances/finalization (allowing a regression where finalization recomputes it from a new billing profile); capture the initial run.CollectionEnd (e.g. after obtaining currentRun in "#3.1") into a variable like initialCollectionEnd and then assert equality against usageBasedFromDB.Realizations[...] .CollectionEnd (or finalRun.CollectionEnd) in subsequent subtests "#3.2", "#4.1" and "#4.2" to ensure the stored CollectionEnd is invariant; update the assertions around currentRun.CollectionEnd and finalRun.CollectionEnd to compare to initialCollectionEnd (using the same UTC/time equality checks used elsewhere) so the test fails if finalization recomputes the window.
🤖 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/service/advance_test.go`:
- Around line 34-36: TearDownTest in AdvanceChargesTestSuite must clear any test
clock overrides set by clock.SetTime (seen at lines 55 and 177); update
AdvanceChargesTestSuite.TearDownTest to reset the clock (e.g., call
clock.Reset(), or if that API is unavailable restore to real time with
clock.SetTime(time.Now())) before delegating to s.BaseSuite.TearDownTest() so
clock state cannot leak between tests.
In `@openmeter/billing/charges/usagebased/adapter/charge.go`:
- Around line 164-179: The loop assumes entities from query.All(ctx) line up
with input.Charges by index, causing mismatched ChargeBase; instead build a map
from input.Charges keyed by their ID (the same ID field used on DB entity), then
iterate entities and look up the corresponding meta using entity.ID (do not use
idx/input.Charges[idx]); call MapChargeBaseFromDB(entity, foundMeta) and
MapRealizationRunsFromDB(entity) as before, and return an error if a matching
meta is missing so KeyBy and subsequent steps get correctly paired data (refer
to MapChargeBaseFromDB, MapRealizationRunsFromDB, input.Charges, entities, and
KeyBy).
In `@openmeter/billing/charges/usagebased/realizationrun.go`:
- Around line 82-97: UpdateRealizationRunInput.Validate currently omits
validating the Totals field, allowing malformed totals to be persisted; modify
UpdateRealizationRunInput.Validate to call the same validation used elsewhere
for totals (e.g., invoke Totals.Validate or the equivalent Totals validation
helper) and append any returned error to errs (with context like "totals: %w")
before returning errors.Join(errs...), ensuring Totals are checked on updates
just like on create and in the persisted model.
In `@openmeter/billing/charges/usagebased/service/statemachine.go`:
- Around line 82-105: The external-storage callback passed into
stateless.NewStateMachineWithExternalStorage persists the new state before
activation side-effects run, causing failed activations (e.g.,
StartFinalRealizationRun invoked from OnActive) to be skipped on retry; update
the state machine flow so persistence only occurs after a successful ActivateCtx
(or call ActivateCtx on the persisted state before computing the next
transition). Concretely, modify the storage callback / surrounding
AdvanceUntilStateStable loop to either (A) defer calling
out.Adapter.UpdateStatus (usagebased.UpdateStatusInput / out.Charge.ChargeBase
assignment) until after ActivateCtx and any OnActive handlers complete
successfully, or (B) immediately invoke ActivateCtx for the currently persisted
state before checking CanFireCtx/FireCtx so idempotent activation retries run;
ensure newStatus.Validate() still runs and error handling remains intact.
In `@openmeter/ent/schema/chargesusagebased.go`:
- Around line 129-131: Make the collection_end field immutable so it cannot be
changed after creation: in the Ent schema where
field.Time("collection_end").Optional().Nillable() is declared (schema
ChargesUsageBased / the field named "collection_end"), add the Immutable()
modifier to the field declaration (e.g.,
field.Time("collection_end")....Immutable()) and re-run Ent codegen/migrations
so updates are rejected at the ORM layer.
---
Outside diff comments:
In `@openmeter/billing/charges/service/invoice.go`:
- Around line 68-86: The switch in handleChargeEvent currently only handles
meta.ChargeTypeFlatFee and returns an error for other types; add cases for
meta.ChargeTypeUsageBased and meta.ChargeTypeCreditPurchase that mirror the
flat-fee flow: call the appropriate type conversion methods (e.g.,
AsUsageBasedCharge and AsCreditPurchaseCharge) on lineWithCharge.Charge, check
errors, verify the corresponding processor exists on processorByType
(processorByType.usageBased and processorByType.creditPurchase), then invoke the
processor with ctx, the typed charge, and
lineWithCharge.StandardLineWithInvoiceHeader, returning any error from the
processor; keep the same error patterns/messages used for flat-fee when a
processor is missing or conversion fails.
---
Nitpick comments:
In @.agents/skills/charges/SKILL.md:
- Around line 285-286: The phrase "merged-profile based collection-period
resolution" in the SKILL.md doc is missing hyphens; update the compound
adjective to "merged-profile-based collection-period resolution" to correctly
hyphenate the modifier, preserving the existing bullet that also mentions
`AdvanceCharge(...)` and the "nil means noop" contract without changing meaning.
- Around line 70-73: Rephrase the repetitive wording in the documentation:
update the sentence that reads "usagebased.Service.AdvanceCharge(...) only
supports `CreditOnly`" to a less repetitive form such as
"usagebased.Service.AdvanceCharge(...) supports the CreditOnly mode" (or "only
supports the CreditOnly mode") in SKILL.md so the meaning stays identical but
avoids the awkward repetition of "only supports `CreditOnly`".
In `@openmeter/billing/charges/service/invoicable_test.go`:
- Around line 407-575: The test doesn't assert that the persisted CollectionEnd
on the realization run remains unchanged across advances/finalization (allowing
a regression where finalization recomputes it from a new billing profile);
capture the initial run.CollectionEnd (e.g. after obtaining currentRun in
"#3.1") into a variable like initialCollectionEnd and then assert equality
against usageBasedFromDB.Realizations[...] .CollectionEnd (or
finalRun.CollectionEnd) in subsequent subtests "#3.2", "#4.1" and "#4.2" to
ensure the stored CollectionEnd is invariant; update the assertions around
currentRun.CollectionEnd and finalRun.CollectionEnd to compare to
initialCollectionEnd (using the same UTC/time equality checks used elsewhere) so
the test fails if finalization recomputes the window.
In `@openmeter/billing/charges/usagebased/handler.go`:
- Around line 14-41: Update the validation error message in
AllocateCreditsInput.Validate: replace the message "as of is required" with one
that references the actual field name (e.g., "allocateAt is required" or
"AllocateAt is required") so the error clearly points to
AllocateCreditsInput.AllocateAt when the timestamp is missing; adjust the string
in the Validate method where i.AllocateAt.IsZero() is checked.
In `@openmeter/billing/charges/usagebased/service/quantitysnapshot.go`:
- Around line 68-75: summarizeMeterQueryRow currently builds the sum by calling
alpacadecimal.NewFromFloat(row.Value), which can introduce precision artifacts;
update summarizeMeterQueryRow to construct decimals from a string or exact
representation instead (e.g., use alpacadecimal.NewFromString on a string field
or formatted value) for meter.MeterQueryRow.Value, and handle/propagate any
conversion errors; if you cannot change the source type immediately, add a
conversion branch that prefers NewFromString when a string representation is
available and falls back to NewFromFloat only as a last resort, ensuring the
logic lives in summarizeMeterQueryRow and references meter.MeterQueryRow.Value
and alpacadecimal.NewFromString/NewFromFloat.
In `@openmeter/billing/charges/usagebased/service/triggers.go`:
- Around line 38-46: Add a brief clarifying comment inside the empty case for
productcatalog.CreditOnlySettlementMode in the switch on
charge.Intent.SettlementMode (in triggers.go) to indicate why it’s intentionally
empty and that the credits-only path is handled later (e.g., "Handled below via
credits-only state machine"); this preserves the current behavior while
improving readability for future maintainers.
In `@pkg/statelessx/actions.go`:
- Around line 13-24: AllOf currently panics if any entry in the variadic fn
slice is nil; update the combinator (function AllOf and its use of ActionFn) to
treat nil actions as no-ops by skipping nil entries in the for loop (i.e., if f
== nil continue) so callers can pass optional callbacks safely, while preserving
the existing error-joining logic (errors.Join) and return behavior.
🪄 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: ffb1f89a-51b5-441a-b96f-26537e5737b5
⛔ Files ignored due to path filters (9)
openmeter/ent/db/chargeusagebasedruns.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruns/chargeusagebasedruns.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruns/where.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruns_create.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruns_update.gois excluded by!**/ent/db/**openmeter/ent/db/migrate/schema.gois excluded by!**/ent/db/**openmeter/ent/db/mutation.gois excluded by!**/ent/db/**openmeter/ent/db/setorclear.gois excluded by!**/ent/db/**tools/migrate/migrations/atlas.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (48)
.agents/skills/charges/SKILL.mdopenmeter/billing/charges/lock/service.goopenmeter/billing/charges/meta/adapter.goopenmeter/billing/charges/meta/adapter/get.goopenmeter/billing/charges/models/creditrealization/models.goopenmeter/billing/charges/service.goopenmeter/billing/charges/service/advance.goopenmeter/billing/charges/service/advance_test.goopenmeter/billing/charges/service/base_test.goopenmeter/billing/charges/service/create.goopenmeter/billing/charges/service/creditpurchase_test.goopenmeter/billing/charges/service/handlers_test.goopenmeter/billing/charges/service/helpers.goopenmeter/billing/charges/service/invoicable_test.goopenmeter/billing/charges/service/invoice.goopenmeter/billing/charges/service/service.goopenmeter/billing/charges/usagebased/adapter.goopenmeter/billing/charges/usagebased/adapter/charge.goopenmeter/billing/charges/usagebased/adapter/creditallocation.goopenmeter/billing/charges/usagebased/adapter/mapper.goopenmeter/billing/charges/usagebased/adapter/realizationrun.goopenmeter/billing/charges/usagebased/charge.goopenmeter/billing/charges/usagebased/const.goopenmeter/billing/charges/usagebased/errors.goopenmeter/billing/charges/usagebased/handler.goopenmeter/billing/charges/usagebased/rating.goopenmeter/billing/charges/usagebased/realizationrun.goopenmeter/billing/charges/usagebased/service.goopenmeter/billing/charges/usagebased/service/creditsonly.goopenmeter/billing/charges/usagebased/service/get.goopenmeter/billing/charges/usagebased/service/mutations.goopenmeter/billing/charges/usagebased/service/quantitysnapshot.goopenmeter/billing/charges/usagebased/service/rating.goopenmeter/billing/charges/usagebased/service/service.goopenmeter/billing/charges/usagebased/service/statemachine.goopenmeter/billing/charges/usagebased/service/stdinvoice.goopenmeter/billing/charges/usagebased/service/triggers.goopenmeter/billing/charges/usagebased/statemachine.goopenmeter/billing/service/stdinvoicestate.goopenmeter/ent/schema/chargesusagebased.goopenmeter/entitlement/metered/lateevents_test.goopenmeter/streaming/testutils/streaming.goopenmeter/streaming/testutils/streaming_test.gopkg/statelessx/actions.gopkg/statelessx/conditions.gotest/billing/suite.gotools/migrate/migrations/20260322083731_add_usage_based_run_collection_end.down.sqltools/migrate/migrations/20260322083731_add_usage_based_run_collection_end.up.sql
turip
force-pushed
the
feat/usage-based-charges-lifecycle
branch
from
248e508 to
1549f0b
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openmeter/billing/charges/service/invoice.go (1)
68-86:⚠️ Potential issue | 🔴 CriticalMissing switch case for usage-based charges — invoice events will fail!
The
processorByTypestruct was extended withusageBasedprocessor (line 31), and it's properly wired inhandleStandardInvoiceUpdate. However, thehandleChargeEventswitch only handlesChargeTypeFlatFee— there's no case forChargeTypeUsageBased.Any usage-based charge will fall through to the default case (line 85) and return
"unsupported charge type: usage_based".🐛 Add the missing switch case for usage-based charges
case meta.ChargeTypeFlatFee: flatFee, err := lineWithCharge.Charge.AsFlatFeeCharge() if err != nil { return err } if processorByType.flatFee == nil { return fmt.Errorf("flat fee payment post processor is not supported") } err = processorByType.flatFee(ctx, flatFee, lineWithCharge.StandardLineWithInvoiceHeader) if err != nil { return err } + case meta.ChargeTypeUsageBased: + usageBased, err := lineWithCharge.Charge.AsUsageBasedCharge() + if err != nil { + return err + } + + if processorByType.usageBased == nil { + return fmt.Errorf("usage based payment post processor is not supported") + } + + err = processorByType.usageBased(ctx, usageBased, lineWithCharge.StandardLineWithInvoiceHeader) + if err != nil { + return err + } default: return fmt.Errorf("unsupported charge type: %s", lineWithCharge.Charge.Type())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/service/invoice.go` around lines 68 - 86, The switch in handleChargeEvent iterates linesWithCharges but only handles meta.ChargeTypeFlatFee, causing UsageBased charges to hit the default error; add a new case for meta.ChargeTypeUsageBased that calls lineWithCharge.Charge.AsUsageBasedCharge(), checks processorByType.usageBased is non-nil (return a descriptive error if nil), then invoke processorByType.usageBased(ctx, usageBased, lineWithCharge.StandardLineWithInvoiceHeader) and propagate any error returned—mirroring the flat-fee branch's error handling and flow.
🧹 Nitpick comments (5)
openmeter/billing/charges/usagebased/adapter/creditallocation.go (1)
36-39: Consider adding context to the error for easier debugging.The bulk create error doesn't include context about what was being created. This is pretty minor, but wrapping with context can help when troubleshooting.
💡 Optional: wrap error with context
dbEntities, err := tx.db.ChargeUsageBasedRunCreditAllocations.CreateBulk(creates...).Save(ctx) if err != nil { - return nil, err + return nil, fmt.Errorf("creating credit allocations for run [id=%s]: %w", runID.ID, err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/usagebased/adapter/creditallocation.go` around lines 36 - 39, The error returned from tx.db.ChargeUsageBasedRunCreditAllocations.CreateBulk(...).Save(ctx) lacks context; update the error handling in that block (around CreateBulk/Save and variable dbEntities) to wrap or annotate the original err with a descriptive message—e.g., "creating ChargeUsageBasedRunCreditAllocations bulk"—using fmt.Errorf("%s: %w", ...) or your project's error-wrapping utility before returning..agents/skills/charges/SKILL.md (1)
286-286: Minor grammar fix.Per the static analysis hint, "merged-profile based" should use a hyphen to form the compound adjective.
📝 Suggested fix
-- preserve merged-profile based collection-period resolution +- preserve merged-profile-based collection-period resolution🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/charges/SKILL.md at line 286, Update the compound adjective in the phrase "preserve merged-profile based collection-period resolution" to use a hyphen: change the text "preserve merged-profile based collection-period resolution" to "preserve merged-profile-based collection-period resolution" in SKILL.md so the compound modifier "merged-profile-based" is grammatically correct.openmeter/billing/charges/usagebased/service/triggers.go (1)
38-46: Consider restructuring the switch for readability.The empty
case productcatalog.CreditOnlySettlementMode:followed by other cases that return errors is valid Go but reads a bit awkwardly. A small refactor could make the intent clearer.♻️ Optional: Explicit early-exit pattern
- switch charge.Intent.SettlementMode { - case productcatalog.CreditOnlySettlementMode: - case productcatalog.CreditThenInvoiceSettlementMode: + if charge.Intent.SettlementMode == productcatalog.CreditThenInvoiceSettlementMode { return nil, models.NewGenericNotImplementedError( fmt.Errorf("advancing usage based charge with settlement mode %s is not supported [charge_id=%s]", charge.Intent.SettlementMode, charge.ID), ) - default: + } + + if charge.Intent.SettlementMode != productcatalog.CreditOnlySettlementMode { return nil, fmt.Errorf("unsupported settlement mode %s [charge_id=%s]", charge.Intent.SettlementMode, charge.ID) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/usagebased/service/triggers.go` around lines 38 - 46, The switch over charge.Intent.SettlementMode is awkward due to an empty case; update it to explicitly handle the supported CreditOnly path and return nil there, and make the CreditThenInvoice case explicitly return models.NewGenericNotImplementedError (using fmt.Errorf with the same message) so intent is clear; keep the default branch returning fmt.Errorf for other unsupported modes. Target the switch that references charge.Intent.SettlementMode and the constants productcatalog.CreditOnlySettlementMode and productcatalog.CreditThenInvoiceSettlementMode and the error constructor models.NewGenericNotImplementedError.openmeter/billing/charges/service/create.go (1)
165-181: Consider adding observability for advancement failures.When
AdvanceChargesfails for a customer, the error is returned immediately, stopping advancement for remaining customers. This is fine for correctness, but you might want to log or emit metrics here so operators can see which customer/charge caused issues during the auto-advance step.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/service/create.go` around lines 165 - 181, When AdvanceCharges(ctx, charges.AdvanceChargesInput{Customer: custID}) returns an error, augment the current behavior by emitting observability before returning: call the service logger (e.g., s.logger or s.Log) and/or increment a metric (e.g., s.metrics.Inc or s.recordError) with the customer identifier and the error details, then return the original fmt.Errorf(...) as before; locate the AdvanceCharges call and the surrounding loop over customerIDs and add the logging/metric emit right inside the if err != nil block so operators can see which custID caused the failure.openmeter/billing/charges/usagebased/adapter/charge.go (1)
236-243: Consider usingtx.GetByMetasfor consistency.Inside the
TransactingRepocallback, you have access totx *adapterbut calla.GetByMetas(the outer adapter). While this works sinceentutils.TransactingReporeuses the transaction from context, usingtx.GetByMetaswould be more explicit and consistent with patterns elsewhere in this file (e.g.,tx.metaAdapterat line 225).♻️ Optional fix
- charges, err := a.GetByMetas(ctx, usagebased.GetByMetasInput{ + charges, err := tx.GetByMetas(ctx, usagebased.GetByMetasInput{ Namespace: input.ChargeID.Namespace, Charges: meta.Charges{metas[0]}, Expands: input.Expands, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/usagebased/adapter/charge.go` around lines 236 - 243, Inside the TransactingRepo callback you call a.GetByMetas (outer adapter) but should use the transaction-bound adapter to be explicit; change the call to tx.GetByMetas so the query uses the same transaction instance (mirrors usage of tx.metaAdapter and follows the file's pattern). Locate the block where charges, err := a.GetByMetas(ctx, usagebased.GetByMetasInput{...}) is invoked and replace the receiver a with tx, leaving the input and error handling unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@openmeter/billing/charges/service/invoice.go`:
- Around line 68-86: The switch in handleChargeEvent iterates linesWithCharges
but only handles meta.ChargeTypeFlatFee, causing UsageBased charges to hit the
default error; add a new case for meta.ChargeTypeUsageBased that calls
lineWithCharge.Charge.AsUsageBasedCharge(), checks processorByType.usageBased is
non-nil (return a descriptive error if nil), then invoke
processorByType.usageBased(ctx, usageBased,
lineWithCharge.StandardLineWithInvoiceHeader) and propagate any error
returned—mirroring the flat-fee branch's error handling and flow.
---
Nitpick comments:
In @.agents/skills/charges/SKILL.md:
- Line 286: Update the compound adjective in the phrase "preserve merged-profile
based collection-period resolution" to use a hyphen: change the text "preserve
merged-profile based collection-period resolution" to "preserve
merged-profile-based collection-period resolution" in SKILL.md so the compound
modifier "merged-profile-based" is grammatically correct.
In `@openmeter/billing/charges/service/create.go`:
- Around line 165-181: When AdvanceCharges(ctx,
charges.AdvanceChargesInput{Customer: custID}) returns an error, augment the
current behavior by emitting observability before returning: call the service
logger (e.g., s.logger or s.Log) and/or increment a metric (e.g., s.metrics.Inc
or s.recordError) with the customer identifier and the error details, then
return the original fmt.Errorf(...) as before; locate the AdvanceCharges call
and the surrounding loop over customerIDs and add the logging/metric emit right
inside the if err != nil block so operators can see which custID caused the
failure.
In `@openmeter/billing/charges/usagebased/adapter/charge.go`:
- Around line 236-243: Inside the TransactingRepo callback you call a.GetByMetas
(outer adapter) but should use the transaction-bound adapter to be explicit;
change the call to tx.GetByMetas so the query uses the same transaction instance
(mirrors usage of tx.metaAdapter and follows the file's pattern). Locate the
block where charges, err := a.GetByMetas(ctx, usagebased.GetByMetasInput{...})
is invoked and replace the receiver a with tx, leaving the input and error
handling unchanged.
In `@openmeter/billing/charges/usagebased/adapter/creditallocation.go`:
- Around line 36-39: The error returned from
tx.db.ChargeUsageBasedRunCreditAllocations.CreateBulk(...).Save(ctx) lacks
context; update the error handling in that block (around CreateBulk/Save and
variable dbEntities) to wrap or annotate the original err with a descriptive
message—e.g., "creating ChargeUsageBasedRunCreditAllocations bulk"—using
fmt.Errorf("%s: %w", ...) or your project's error-wrapping utility before
returning.
In `@openmeter/billing/charges/usagebased/service/triggers.go`:
- Around line 38-46: The switch over charge.Intent.SettlementMode is awkward due
to an empty case; update it to explicitly handle the supported CreditOnly path
and return nil there, and make the CreditThenInvoice case explicitly return
models.NewGenericNotImplementedError (using fmt.Errorf with the same message) so
intent is clear; keep the default branch returning fmt.Errorf for other
unsupported modes. Target the switch that references
charge.Intent.SettlementMode and the constants
productcatalog.CreditOnlySettlementMode and
productcatalog.CreditThenInvoiceSettlementMode and the error constructor
models.NewGenericNotImplementedError.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fecfaa69-a022-4282-b6ba-d7fddc5b6076
⛔ Files ignored due to path filters (8)
go.sumis excluded by!**/*.sum,!**/*.sumopenmeter/ent/db/chargeusagebasedruns.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruns/chargeusagebasedruns.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruns/where.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruns_create.gois excluded by!**/ent/db/**openmeter/ent/db/migrate/schema.gois excluded by!**/ent/db/**openmeter/ent/db/mutation.gois excluded by!**/ent/db/**tools/migrate/migrations/atlas.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (45)
.agents/skills/charges/SKILL.mdopenmeter/billing/charges/lock.goopenmeter/billing/charges/meta/adapter.goopenmeter/billing/charges/meta/adapter/get.goopenmeter/billing/charges/models/creditrealization/models.goopenmeter/billing/charges/service.goopenmeter/billing/charges/service/advance.goopenmeter/billing/charges/service/advance_test.goopenmeter/billing/charges/service/base_test.goopenmeter/billing/charges/service/create.goopenmeter/billing/charges/service/creditpurchase_test.goopenmeter/billing/charges/service/handlers_test.goopenmeter/billing/charges/service/helpers.goopenmeter/billing/charges/service/invoicable_test.goopenmeter/billing/charges/service/invoice.goopenmeter/billing/charges/service/service.goopenmeter/billing/charges/usagebased/adapter.goopenmeter/billing/charges/usagebased/adapter/charge.goopenmeter/billing/charges/usagebased/adapter/creditallocation.goopenmeter/billing/charges/usagebased/adapter/mapper.goopenmeter/billing/charges/usagebased/adapter/realizationrun.goopenmeter/billing/charges/usagebased/charge.goopenmeter/billing/charges/usagebased/const.goopenmeter/billing/charges/usagebased/errors.goopenmeter/billing/charges/usagebased/handler.goopenmeter/billing/charges/usagebased/rating.goopenmeter/billing/charges/usagebased/realizationrun.goopenmeter/billing/charges/usagebased/service.goopenmeter/billing/charges/usagebased/service/creditsonly.goopenmeter/billing/charges/usagebased/service/get.goopenmeter/billing/charges/usagebased/service/mutations.goopenmeter/billing/charges/usagebased/service/quantitysnapshot.goopenmeter/billing/charges/usagebased/service/rating.goopenmeter/billing/charges/usagebased/service/service.goopenmeter/billing/charges/usagebased/service/statemachine.goopenmeter/billing/charges/usagebased/service/stdinvoice.goopenmeter/billing/charges/usagebased/service/triggers.goopenmeter/billing/charges/usagebased/statemachine.goopenmeter/billing/service/stdinvoicestate.goopenmeter/ent/schema/chargesusagebased.gopkg/statelessx/actions.gopkg/statelessx/conditions.gotest/billing/suite.gotools/migrate/migrations/20260323090205_charges-run-collection-period.down.sqltools/migrate/migrations/20260323090205_charges-run-collection-period.up.sql
✅ Files skipped from review due to trivial changes (11)
- openmeter/billing/charges/usagebased/const.go
- tools/migrate/migrations/20260323090205_charges-run-collection-period.down.sql
- openmeter/billing/charges/service/service.go
- tools/migrate/migrations/20260323090205_charges-run-collection-period.up.sql
- openmeter/billing/charges/models/creditrealization/models.go
- openmeter/ent/schema/chargesusagebased.go
- pkg/statelessx/conditions.go
- openmeter/billing/charges/usagebased/service/rating.go
- openmeter/billing/charges/usagebased/errors.go
- openmeter/billing/charges/usagebased/service/creditsonly.go
- openmeter/billing/charges/usagebased/statemachine.go
🚧 Files skipped from review as they are similar to previous changes (14)
- openmeter/billing/charges/service/helpers.go
- test/billing/suite.go
- openmeter/billing/charges/meta/adapter/get.go
- pkg/statelessx/actions.go
- openmeter/billing/charges/usagebased/service/stdinvoice.go
- openmeter/billing/charges/service/advance.go
- openmeter/billing/charges/usagebased/adapter/realizationrun.go
- openmeter/billing/charges/usagebased/service/quantitysnapshot.go
- openmeter/billing/charges/usagebased/service.go
- openmeter/billing/charges/usagebased/handler.go
- openmeter/billing/charges/usagebased/service/service.go
- openmeter/billing/charges/usagebased/realizationrun.go
- openmeter/billing/service/stdinvoicestate.go
- openmeter/billing/charges/usagebased/adapter/mapper.go
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
1 participant
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
Implements the full lifecycle state machine for credit-only usage-based charges, from creation through final realization and settlement.
What's in this PR
State machine (usagebased/service/)
A statelessx-backed state machine drives usage-based charge advancement through seven states:
created → active → active.final_realization.started
→ active.final_realization.waiting_for_collection
→ active.final_realization.processing
→ active.final_realization.completed → final
AdvanceUntilStateStable loops the machine until no further transitions are possible in a single call, so a charge that is ready to skip multiple states (e.g. created during an already-elapsed service period) lands in the
correct final state in one advance.
Realization runs (usagebased/realizationrun.go, usagebased/adapter/realizationrun.go)
A new RealizationRun entity tracks the metered quantity snapshot and credit totals for each collection window. Key invariants:
Collection period semantics
AdvanceCharges facade (charges/service/advance.go)
Customer-scoped orchestration: lists non-final usage-based charges, resolves the merged billing profile and feature meters once per customer, then calls usagebased.Service.AdvanceCharge per charge. CreditThenInvoice
settlement mode is explicitly rejected with a not-implemented error.
Auto-advance on create (charges/service/create.go)
After the create transaction commits, autoAdvanceCreatedCharges collects the unique customer IDs of any newly created credit-only usage-based charges and calls AdvanceCharges for each. This means a charge created inside an
active service period is returned from Create already in active status. The auto-advance runs outside the create transaction so that the persisted creation record survives even if the advance fails — a background worker
can retry.
Limitations
Credit Then invoice is not implemented yet
Summary by CodeRabbit
New Features
Documentation