feat: apply unit config in rating via mutator - OM-397#4616
Open
rolosp wants to merge 3 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughAdds unit-config-aware billing rating, including a feature flag, a new conversion/rejection mutator pair, updated rate-card validation, and wiring changes through app, tests, and intent snapshots. ChangesUnitConfig Rating Feature
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)
✨ 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 |
Contributor
Greptile SummaryThis PR wires unit_config into usage-based billing on the charges path. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (3): Last reviewed commit: "fix: apply unit_config to subscription c..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
rolosp
changed the title
![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: 1
🤖 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.
Inline comments:
In `@openmeter/billing/rating/service/rate/unitconfig_rating_test.go`:
- Around line 18-155: The test suite in TestUnitConfigRating only covers
zero-start usage paths, so it can miss regressions in PreLinePeriodQty handling.
Add one end-to-end split-line case in this test that uses a non-zero pre-line
quantity (for example a 1400 -> 2700 interval with ceiling rounding) and assert
the converted, rounded billed amount. Use the existing TestUnitConfigRating,
GenerateDetailedLines, and mutator.UnitConfig wiring context to verify the
cumulative start/end behavior is exercised.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fe7d7f63-b6e4-4730-860f-5b0d9525e71b
📒 Files selected for processing (32)
app/common/billing.goapp/common/config.gocmd/billing-worker/wire_gen.gocmd/jobs/internal/wire_gen.gocmd/server/wire_gen.goopenmeter/billing/charges/service/base_test.goopenmeter/billing/charges/service/unitconfig_rating_test.goopenmeter/billing/charges/testutils/service.goopenmeter/billing/charges/usagebased/rating.goopenmeter/billing/charges/usagebased/service/rating/delta/base_test.goopenmeter/billing/charges/usagebased/service/rating/periodpreserving/engine_test.goopenmeter/billing/charges/usagebased/service/rating/service_test.goopenmeter/billing/charges/usagebased/service/rating/subtract/subtract_test.goopenmeter/billing/rating/line.goopenmeter/billing/rating/service/billableperiod.goopenmeter/billing/rating/service/detailedline.goopenmeter/billing/rating/service/mutator/unitconfig.goopenmeter/billing/rating/service/mutator/unitconfig_test.goopenmeter/billing/rating/service/pricer.goopenmeter/billing/rating/service/rate/unitconfig_rating_test.goopenmeter/billing/rating/service/service.goopenmeter/billing/rating/service/testutil/ubptest.goopenmeter/billing/rating/service/unitconfig_registration_test.goopenmeter/billing/stdinvoiceline.goopenmeter/ledger/customerbalance/testenv_test.goopenmeter/productcatalog/price.goopenmeter/productcatalog/price_test.goopenmeter/productcatalog/ratecard.gotest/app/testenv.gotest/billing/suite.gotest/customer/testenv.gotest/subscription/framework_test.go
rolosp
force-pushed
the
feat/om-397-apply-unit-config
branch
from
e8e7cef to
d713239
Compare
turip
reviewed
Jul 1, 2026
Comment on lines
+77
to
+79
| if unitConfigEnabled { | ||
| preCalculationMutators = append(preCalculationMutators, &mutator.UnitConfig{}) | ||
| } |
Member
There was a problem hiding this comment.
Call me paranoid, but instead of this, I would do something like this:
if unitConfigEnabled {
preCalculationMutators = append(preCalculationMutators, &mutator.UnitConfig{})
} else {
preCalculationMutators = append(preCalculationMutators, &mutator.ForbidUnitConfig{})
}
Forbid unit config is a noop, but returns error when the unit config is set on the rateable. This way we are not accidentally ignoring unit configs.
Contributor
Author
There was a problem hiding this comment.
added ForbidUnitConfig
| } | ||
|
|
||
| _, start := unitConfig.Apply(usage.PreLinePeriodQuantity) | ||
| _, end := unitConfig.Apply(usage.PreLinePeriodQuantity.Add(usage.Quantity)) |
Member
There was a problem hiding this comment.
just convert the quantity, don't do calculations here. It is not necessarily true that quantity = perlineperiodquantity + periodquantity (e.g. for exotic meters, and charges rating)
| require.NoError(t, err) | ||
|
|
||
| billingRatingService := billingratingservice.New() | ||
| billingRatingService := billingratingservice.New(billingratingservice.Config{}) |
Member
There was a problem hiding this comment.
enable in all test envs, so that you are validating that there's no regression due to this being enabled.
| return r.Discounts.Clone() | ||
| } | ||
|
|
||
| func (r RateableIntent) GetUnitConfig() *productcatalog.UnitConfig { |
Member
There was a problem hiding this comment.
[P1] Subscription charges drop unit_config before rating
This new rating path only sees unit_config through RateableIntent.GetUnitConfig(), but normal subscription-created usage-based charges still never populate Intent.UnitConfig: newUsageBasedChargeIntent in openmeter/billing/worker/subscriptionsync/service/reconciler/patchchargeusagebased.go copies price and discounts from rateCardMeta but leaves UnitConfig behind. With unitConfig.enabled on, a plan/subscription rate card using unit_config will still invoice raw metered quantity, while only direct charge-intent tests exercise the converted path. Please snapshot rateCardMeta.UnitConfig into the usage-based charge intent, with a subscription round-trip/invoice test.
Contributor
Author
There was a problem hiding this comment.
done, newUsageBasedChargeIntent now snapshots rateCardMeta.UnitConfig onto the usage-based charge intent
turip
reviewed
Jul 1, 2026
| price := l.GetPrice() | ||
| if price == nil || !price.SupportsUnitConfig() { | ||
| return l, nil | ||
| } |
Member
There was a problem hiding this comment.
this should be an error. Don't ignore inconsistencies in billing or rating, instead make them surfaced.
Comment on lines
+59
to
70
| // unitConfigLineAccessor wraps a StandardLine to expose a unit_config, which the | ||
| // StandardLine type itself never carries. | ||
| type unitConfigLineAccessor struct { | ||
| *billing.StandardLine | ||
|
|
||
| unitConfig *productcatalog.UnitConfig | ||
| } | ||
|
|
||
| func (l unitConfigLineAccessor) GetUnitConfig() *productcatalog.UnitConfig { | ||
| return l.unitConfig | ||
| } | ||
|
|
Member
There was a problem hiding this comment.
I would prefer if you add this to the line even if it's not persisted, or add a TODO here to use the line's unit config once it's there.
Given you have done it for charges, you might aswell do this for standard lines.
Contributor
Author
There was a problem hiding this comment.
Added a TODO comment and deferred the standard-line wiring
rolosp
force-pushed
the
feat/om-397-apply-unit-config
branch
from
d713239 to
2e269dd
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
openmeter/billing/charges/service/unitconfig_rating_test.go (2)
128-130: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSecond
clock.FreezeTimecall isn't paired with its owndefer UnFreeze().Line 168 re-freezes time to
invoiceAtwithout a freshdefer clock.UnFreeze()next to it — it only works because the firstdefer(line 129) still fires at the end. As per coding guidelines,**/*_test.go: "When usingclock.FreezeTime(...)in tests, immediately pair it withdefer clock.UnFreeze()in the same scope."Also applies to: 168-168
🤖 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/unitconfig_rating_test.go` around lines 128 - 130, The test in unitconfig_rating_test.go freezes time more than once without pairing each freeze with its own immediate defer UnFreeze call. Update the test around the second clock.FreezeTime usage so it is followed in the same scope by defer clock.UnFreeze(), matching the existing pattern used near the first freeze and keeping the test cleanup local and explicit.Source: Coding guidelines
40-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
TearDownTestoverrides.Both suites override
TearDownTestjust to forward tos.BaseSuite.TearDownTest()— sinceBaseSuiteis embedded, that method is already promoted automatically. These overrides can just be dropped.♻️ Drop the no-op overrides
-func (s *unitConfigRatingEnabledSuite) TearDownTest() { - s.BaseSuite.TearDownTest() -} -(and the equivalent block for
unitConfigRatingDisabledSuite)Also applies to: 83-85
🤖 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/unitconfig_rating_test.go` around lines 40 - 42, Remove the no-op TearDownTest overrides from both unitConfigRatingEnabledSuite and unitConfigRatingDisabledSuite, since the embedded BaseSuite already promotes TearDownTest automatically. Keep the suite types intact and delete the forwarding methods so the test setup/teardown behavior continues to come from BaseSuite without redundant wrappers.
🤖 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/unitconfig_rating_test.go`:
- Around line 128-130: The test in unitconfig_rating_test.go freezes time more
than once without pairing each freeze with its own immediate defer UnFreeze
call. Update the test around the second clock.FreezeTime usage so it is followed
in the same scope by defer clock.UnFreeze(), matching the existing pattern used
near the first freeze and keeping the test cleanup local and explicit.
- Around line 40-42: Remove the no-op TearDownTest overrides from both
unitConfigRatingEnabledSuite and unitConfigRatingDisabledSuite, since the
embedded BaseSuite already promotes TearDownTest automatically. Keep the suite
types intact and delete the forwarding methods so the test setup/teardown
behavior continues to come from BaseSuite without redundant wrappers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b2f31ac0-ae17-4963-ac0f-b6df6f9605ef
📒 Files selected for processing (37)
app/common/billing.goapp/common/config.gocmd/billing-worker/wire_gen.gocmd/jobs/internal/wire_gen.gocmd/server/wire_gen.goopenmeter/billing/charges/service/base_test.goopenmeter/billing/charges/service/unitconfig_rating_test.goopenmeter/billing/charges/testutils/service.goopenmeter/billing/charges/usagebased/rating.goopenmeter/billing/charges/usagebased/service/rating/delta/base_test.goopenmeter/billing/charges/usagebased/service/rating/periodpreserving/engine_test.goopenmeter/billing/charges/usagebased/service/rating/service_test.goopenmeter/billing/charges/usagebased/service/rating/subtract/subtract_test.goopenmeter/billing/rating/line.goopenmeter/billing/rating/service/billableperiod.goopenmeter/billing/rating/service/detailedline.goopenmeter/billing/rating/service/mutator/errors.goopenmeter/billing/rating/service/mutator/forbidunitconfig.goopenmeter/billing/rating/service/mutator/forbidunitconfig_test.goopenmeter/billing/rating/service/mutator/unitconfig.goopenmeter/billing/rating/service/mutator/unitconfig_test.goopenmeter/billing/rating/service/pricer.goopenmeter/billing/rating/service/rate/unitconfig_rating_test.goopenmeter/billing/rating/service/service.goopenmeter/billing/rating/service/testutil/ubptest.goopenmeter/billing/rating/service/unitconfig_registration_test.goopenmeter/billing/stdinvoiceline.goopenmeter/billing/worker/subscriptionsync/service/reconciler/patchchargeusagebased.goopenmeter/billing/worker/subscriptionsync/service/reconciler/patchchargeusagebased_test.goopenmeter/ledger/customerbalance/testenv_test.goopenmeter/productcatalog/price.goopenmeter/productcatalog/price_test.goopenmeter/productcatalog/ratecard.gotest/app/testenv.gotest/billing/suite.gotest/customer/testenv.gotest/subscription/framework_test.go
✅ Files skipped from review due to trivial changes (4)
- openmeter/productcatalog/price_test.go
- cmd/jobs/internal/wire_gen.go
- cmd/server/wire_gen.go
- openmeter/billing/charges/usagebased/service/rating/subtract/subtract_test.go
🚧 Files skipped from review as they are similar to previous changes (20)
- openmeter/billing/rating/service/unitconfig_registration_test.go
- openmeter/ledger/customerbalance/testenv_test.go
- openmeter/productcatalog/ratecard.go
- openmeter/billing/rating/service/detailedline.go
- app/common/billing.go
- openmeter/productcatalog/price.go
- openmeter/billing/rating/service/billableperiod.go
- openmeter/billing/stdinvoiceline.go
- openmeter/billing/charges/usagebased/rating.go
- cmd/billing-worker/wire_gen.go
- openmeter/billing/charges/usagebased/service/rating/periodpreserving/engine_test.go
- test/billing/suite.go
- app/common/config.go
- openmeter/billing/charges/usagebased/service/rating/service_test.go
- openmeter/billing/rating/service/service.go
- openmeter/billing/rating/service/pricer.go
- openmeter/billing/charges/usagebased/service/rating/delta/base_test.go
- openmeter/billing/rating/service/testutil/ubptest.go
- openmeter/billing/rating/line.go
- openmeter/billing/charges/service/base_test.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
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Apply unit_config in rating (charges path)
Wires
unit_configinto the billing rating engine so a usage-based rate card's conversion (operation × factor, then rounding) is applied to the metered quantity before pricing — previously authored and persisted (OM-395, OM-396) but inert at billing time. Dark behindunitConfig.enabled; with the flag off, rating is byte-identical to today.This covers the charges path end-to-end. Standard/legacy invoice lines return no config yet — that's the next ticket (persist
applied_unit_configon invoice lines), where the convert+round also gets applied in the charges line-mapper.Changes
UnitConfigpre-calculation mutator: converts + rounds on the cumulative endpoints (round(convert(pre+qty)) − round(convert(pre))) so progressive splitsround on the cumulative boundary, then hands converted units to the existing
unit/graduated/volume pricers (unchanged, conversion-unaware). Uses
UnitConfig.Apply.DiscountUsage(convert → round → discount), gated by the flag.unitConfig.enabledthreaded into the rating service via DI, defaulting off.GetUnitConfig()on the rating line accessor;RateableIntentreturns thesnapshotted charge-intent config.
Testing
quantity, flag-off bills raw.
Summary by CodeRabbit
New Features
unit_configsupport in billing rating for eligible usage-based prices, including unit conversion with rounding.unit_configfrom rate cards so calculations use the correct units.Bug Fixes
unit_configis present but the feature is disabled, rating is now rejected instead of falling back to raw quantities.Tests