refactor: move quantity snapshotting to line engine - #4749
Merged
turip merged 1 commit intoAug 12, 2026
Conversation
Contributor
📝 WalkthroughWalkthroughThe change moves quantity snapshotting into ChangesBilling line engine integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant BillingRegistry
participant BillingService
participant SubscriptionSyncService
participant InvoiceUpdater
participant LegacyBillingLineEngine
BillingRegistry->>LegacyBillingLineEngine: construct engine
BillingRegistry->>BillingService: inject engine
BillingRegistry->>SubscriptionSyncService: inject engine
SubscriptionSyncService->>InvoiceUpdater: configure snapshotter
InvoiceUpdater->>LegacyBillingLineEngine: snapshot invoice line quantity
LegacyBillingLineEngine-->>InvoiceUpdater: return updated line
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
145
to
149
| func (e *Engine) snapshotLineQuantitiesInParallel(ctx context.Context, customer billing.InvoiceCustomer, lines billing.StandardLines, featureMeters feature.FeatureMeters) error { | ||
| workerCount := e.maxParallelQuantitySnapshots | ||
| if workerCount <= 0 { | ||
| workerCount = 1 | ||
| } |
Contributor
There was a problem hiding this comment.
workerCount <= 0 guard is unreachable: Config.Validate() already rejects any MaxParallelQuantitySnapshots < 1, so e.maxParallelQuantitySnapshots is guaranteed to be ≥ 1 by the time this method is called. The fallback can be safely removed.
Suggested change
| func (e *Engine) snapshotLineQuantitiesInParallel(ctx context.Context, customer billing.InvoiceCustomer, lines billing.StandardLines, featureMeters feature.FeatureMeters) error { | |
| workerCount := e.maxParallelQuantitySnapshots | |
| if workerCount <= 0 { | |
| workerCount = 1 | |
| } | |
| func (e *Engine) snapshotLineQuantitiesInParallel(ctx context.Context, customer billing.InvoiceCustomer, lines billing.StandardLines, featureMeters feature.FeatureMeters) error { | |
| workerCount := e.maxParallelQuantitySnapshots |
Prompt To Fix With AI
This is a comment left during a code review.
Path: openmeter/billing/lineengine/quantitysnapshot.go
Line: 145-149
Comment:
The `workerCount <= 0` guard is unreachable: `Config.Validate()` already rejects any `MaxParallelQuantitySnapshots < 1`, so `e.maxParallelQuantitySnapshots` is guaranteed to be ≥ 1 by the time this method is called. The fallback can be safely removed.
```suggestion
func (e *Engine) snapshotLineQuantitiesInParallel(ctx context.Context, customer billing.InvoiceCustomer, lines billing.StandardLines, featureMeters feature.FeatureMeters) error {
workerCount := e.maxParallelQuantitySnapshots
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
turip
force-pushed
the
feat/split-get-subscription-calls-reconstructed
branch
from
August 11, 2026 12:18
97279e3 to
4ef880a
Compare
Base automatically changed from
feat/split-get-subscription-calls-reconstructed
to
main
August 11, 2026 13:30
turip
force-pushed
the
feat/move-quantity-snapshot-to-lineengine-reconstructed
branch
from
August 11, 2026 13:46
ef935d3 to
0b740b7
Compare
turip
marked this pull request as ready for review
August 11, 2026 13:46
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
openmeter/billing/lineengine/engine.go (1)
32-54: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider collecting the config errors instead of returning on the first one.
The repo guideline for
Validate() erroris to accumulate field errors and returnmodels.NewNillableGenericValidationError(errors.Join(errs...)). Here each check returns early, so a caller with two missing dependencies only sees the first one. The newinvoiceupdater.Config.Validatein this same PR already uses the joined form, so aligning would keep the two configs consistent. Not blocking, since this matches the pre-existing style in this file.As per coding guidelines: "In
Validate() error, collect field errors and returnmodels.NewNillableGenericValidationError(errors.Join(errs...)), preserving field context with wrapped errors."♻️ Suggested aggregation
func (c Config) Validate() error { + var errs []error + if c.SplitLineGroupAdapter == nil { - return fmt.Errorf("split line group adapter is required") + errs = append(errs, errors.New("split line group adapter is required")) } if c.RatingService == nil { - return fmt.Errorf("rating service is required") + errs = append(errs, errors.New("rating service is required")) } if c.FeatureService == nil { - return fmt.Errorf("feature service is required") + errs = append(errs, errors.New("feature service is required")) } if c.StreamingConnector == nil { - return fmt.Errorf("streaming connector is required") + errs = append(errs, errors.New("streaming connector is required")) } if c.MaxParallelQuantitySnapshots < 1 { - return fmt.Errorf("max parallel quantity snapshots must be greater than 0") + errs = append(errs, errors.New("max parallel quantity snapshots must be greater than 0")) } - return nil + return models.NewNillableGenericValidationError(errors.Join(errs...)) }🤖 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/lineengine/engine.go` around lines 32 - 54, Update Config.Validate to collect all missing dependency and invalid MaxParallelQuantitySnapshots errors instead of returning on the first failure. Preserve field context by wrapping each validation error, then return models.NewNillableGenericValidationError(errors.Join(errs...)); keep returning nil when no errors are collected.Source: Coding guidelines
🤖 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/lineengine/engine.go`:
- Around line 32-54: Update Config.Validate to collect all missing dependency
and invalid MaxParallelQuantitySnapshots errors instead of returning on the
first failure. Preserve field context by wrapping each validation error, then
return models.NewNillableGenericValidationError(errors.Join(errs...)); keep
returning nil when no errors are collected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ea5f1c9c-5eac-479d-aca8-e83cbbc5791d
📒 Files selected for processing (19)
app/common/billing.goopenmeter/billing/lineengine/engine.goopenmeter/billing/lineengine/quantitysnapshot.goopenmeter/billing/lineengine/stdinvoice.goopenmeter/billing/service.goopenmeter/billing/service/service.goopenmeter/billing/service/stdinvoiceline.goopenmeter/billing/stdinvoiceline.goopenmeter/billing/worker/subscriptionsync/service/base_test.goopenmeter/billing/worker/subscriptionsync/service/reconciler/invoiceupdater/invoiceupdate.goopenmeter/billing/worker/subscriptionsync/service/reconciler/reconciler.goopenmeter/billing/worker/subscriptionsync/service/service.goopenmeter/billing/worker/subscriptionsync/service/sync_credittheninvoice_test.goopenmeter/server/server_test.gotest/app/testenv.gotest/billing/subscription_test.gotest/billing/suite.gotest/customer/testenv.gotest/subscription/framework_test.go
💤 Files with no reviewable changes (4)
- openmeter/billing/service/stdinvoiceline.go
- openmeter/server/server_test.go
- openmeter/billing/stdinvoiceline.go
- openmeter/billing/service.go
turip
force-pushed
the
feat/move-quantity-snapshot-to-lineengine-reconstructed
branch
from
August 11, 2026 14:00
0b740b7 to
e105b4c
Compare
borosr
approved these changes
Aug 11, 2026
turip
deleted the
feat/move-quantity-snapshot-to-lineengine-reconstructed
branch
August 12, 2026 07:55
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.
Summary
Motivation
This aligns quantity snapshot ownership with the line engine and prepares the codebase for moving split-line-group handling out of the main billing path.
Stack
Reconstruction note
This draft reconstructs the original commit from #4746. The existing PRs and branches were intentionally left unchanged.
Validation
make test-nocache: 6,156 passed, 9 skipped on the original commitSummary by CodeRabbit
New Features
Refactor
Greptile Summary
This PR moves quantity snapshot logic from the billing service into the legacy
lineengine.Engine, making the engine self-contained for all standard-invoice line processing. A singleEngineinstance is constructed inNewBillingRegistryand injected into both the billing service and the subscription-sync service, replacing the previous circular dependency where the engine called back into the billing service'sSnapshotLineQuantityvia theQuantitySnapshotterinterface.SnapshotLineQuantities/SnapshotLineQuantityand related helpers (resolveFeatureMeters,featureMetersErrorWrapper,getFeatureUsage) are moved frombilling/servicetobilling/lineengineas methods onEngine, and theQuantitySnapshotterinterface is removed fromstdinvoice.go.InvoiceLineServiceinterface (andSnapshotLineQuantityonbilling.Service) is removed;invoiceupdaternow declares its own narrowQuantitySnapshotterinterface backed directly by the engine.Confidence Score: 5/5
Important Files Changed
Sequence Diagram
sequenceDiagram participant Registry as BillingRegistry participant Engine as LegacyBillingLineEngine participant BillingService as billing.Service participant SubSync as SubscriptionSyncService participant InvoiceUpdater as invoiceupdater.Updater Registry->>Engine: NewLegacyBillingLineEngine(adapter, rating, feature, streaming) Registry->>BillingService: newBillingService(legacyBillingLineEngine) BillingService->>BillingService: RegisterLineEngine(engine) Registry->>SubSync: NewBillingSubscriptionSyncService(legacyBillingLineEngine) SubSync->>InvoiceUpdater: "invoiceupdater.New(Config{QuantitySnapshotter: engine})" Note over Engine: OnCollectionCompleted Engine->>Engine: SnapshotLineQuantities() Engine->>Engine: resolveFeatureMeters() Engine->>Engine: snapshotLineQuantitiesInParallel() Note over InvoiceUpdater: updateMutableStandardInvoice / updateImmutableInvoice InvoiceUpdater->>Engine: SnapshotLineQuantity(input) Engine->>Engine: resolveFeatureMeters() Engine->>Engine: snapshotLineQuantity()Reviews (3): Last reviewed commit: "refactor: move quantity snapshotting to ..." | Re-trigger Greptile