refactor: move quantity snapshotting to line engine#4749
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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}
| 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 | ||
| } |
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.
| 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!
1 participant
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 commitGreptile Summary
This PR moves quantity snapshotting out of
billingservice.Serviceand intobillinglineengine.Engine, breaking a prior circular dependency where the service created an engine that called back into the service for snapshot logic. A singlelegacyBillingLineEngineinstance is now constructed externally and injected into both the billing service and the subscription sync service.InvoiceLineServiceis removed from thebilling.Serviceinterface andSnapshotLineQuantitybecomes a method on*Engineinstead;invoiceupdater.Updateris narrowed to depend on a newQuantitySnapshotterinterface backed by the engine.resolveFeatureMeters,featureMetersErrorWrapper, and all parallel-snapshot helpers are moved fromopenmeter/billing/service/quantitysnapshot.gointoopenmeter/billing/lineengine/quantitysnapshot.go; the engine now ownsFeatureService,StreamingConnector, andMaxParallelQuantitySnapshotsdirectly.Confidence Score: 4/5
The refactoring is mechanically straightforward — logic moved verbatim from the service to the engine, and the shared engine instance has no mutable state, making concurrent use safe.
The circular dependency is cleanly broken, all test environments are updated consistently, and the narrower QuantitySnapshotter interface in invoiceupdater is a clear improvement. The only thing worth cleaning up is a now-unreachable guard inside snapshotLineQuantitiesInParallel.
openmeter/billing/lineengine/quantitysnapshot.go — the dead workerCount guard; otherwise no file requires special attention.
Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant Caller as Caller (billing / subsync) participant Factory as NewBillingRegistry / testenv participant Engine as billinglineengine.Engine participant BillingSvc as billingservice.Service participant Reconciler as reconciler.Service participant Updater as invoiceupdater.Updater Factory->>Engine: NewLegacyBillingLineEngine(adapter, rating, feature, streaming, cfg) Factory->>BillingSvc: "billingservice.New(Config{LegacyBillingLineEngine: engine})" Note over BillingSvc: RegisterLineEngine(engine) Factory->>Reconciler: "reconciler.New(Config{LegacyBillingLineEngine: engine})" Reconciler->>Updater: "invoiceupdater.New(Config{QuantitySnapshotter: engine})" Caller->>BillingSvc: OnCollectionCompleted(...) BillingSvc->>Engine: (via LineEngine registry) OnCollectionCompleted Engine->>Engine: SnapshotLineQuantities(invoice, lines) Caller->>Updater: ApplyPatches(...) Updater->>Engine: "SnapshotLineQuantity(SnapshotLineQuantityInput{invoice, line})"%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant Caller as Caller (billing / subsync) participant Factory as NewBillingRegistry / testenv participant Engine as billinglineengine.Engine participant BillingSvc as billingservice.Service participant Reconciler as reconciler.Service participant Updater as invoiceupdater.Updater Factory->>Engine: NewLegacyBillingLineEngine(adapter, rating, feature, streaming, cfg) Factory->>BillingSvc: "billingservice.New(Config{LegacyBillingLineEngine: engine})" Note over BillingSvc: RegisterLineEngine(engine) Factory->>Reconciler: "reconciler.New(Config{LegacyBillingLineEngine: engine})" Reconciler->>Updater: "invoiceupdater.New(Config{QuantitySnapshotter: engine})" Caller->>BillingSvc: OnCollectionCompleted(...) BillingSvc->>Engine: (via LineEngine registry) OnCollectionCompleted Engine->>Engine: SnapshotLineQuantities(invoice, lines) Caller->>Updater: ApplyPatches(...) Updater->>Engine: "SnapshotLineQuantity(SnapshotLineQuantityInput{invoice, line})"Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "refactor: move quantity snapshotting to ..." | Re-trigger Greptile