chore: use meter service from invoice svc #3821
Conversation
📝 WalkthroughWalkthroughAdds a feature-meter abstraction and resolver, propagates FeatureMeters through invoice/line creation and calculator dependencies, refactors the calculator to accept explicit dependencies, and introduces an ErrSnapshotInvalidDatabaseState error for snapshot-related DB issues. Changes
Sequence Diagram(s)sequenceDiagram
participant S as Billing Service
participant F as Feature Store (features)
participant M as Meter Store (meters)
participant LS as LineService
participant C as Calculator
S->>F: list features for namespace & keys
F-->>S: features (multiple versions)
S->>M: list meters by derived slugs
M-->>S: meters (multiple versions)
S->>S: pick latest feature/version per key
S->>S: pick latest meter/version per slug
S-->>LS: FromEntities(lines, FeatureMeters)
LS-->>S: Lines (line services)
S->>C: Calculate(invoice, CalculatorDependencies{LineService, FeatureMeters})
C-->>S: Calculated invoice
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 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}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openmeter/meter/mockadapter/meter.go (1)
42-58: TotalCount should reflect the full filtered set, not just the page.
Right now Line 55–58 returnslen(meters)after slicing, which makesTotalCountequal to the page size. That’s inconsistent with the no-pagination path and will break page count UIs.✅ Suggested fix
// In memory pagination: case `#2` if there is pagination settings return the paginated dataset + totalCount := len(meters) pageNumberIndex := params.PageNumber - 1 @@ return pagination.Result[meter.Meter]{ Page: params.Page, Items: meters, - TotalCount: len(meters), + TotalCount: totalCount, }, nil
🤖 Fix all issues with AI agents
In `@openmeter/billing/service/featuremeter.go`:
- Around line 26-35: The mapping over lines can dereference a nil UsageBased
(accessing line.UsageBased.FeatureKey) before the filter runs; update the
generation of featuresToResolve so you guard against nil UsageBased (e.g., in
the lo.Map callback or by using lo.Filter/lo.Map order) — check each line for
line.UsageBased != nil before reading FeatureKey (or use lo.FilterMap/Filter
then Map) and then pass the resulting non-empty keys into lo.Uniq; reference the
variables and functions featuresToResolve, StandardLine, UsageBased, FeatureKey
and the lo.Map/lo.Filter calls to locate where to add the nil check.
🧹 Nitpick comments (6)
openmeter/billing/service/lineservice/usagebasedline.go (1)
105-114: Minor inconsistency in nil-meter handling.The explicit nil check at lines 110-112 is redundant since
l.featureMeters.Get(..., true)already returnsErrSnapshotInvalidDatabaseStatewhen the meter is nil. This defensive check is fine to keep, but note thatSnapshotQuantity(line 165) dereferencesfeatureMeter.Meterwithout this same explicit check.For consistency, either:
- Remove the redundant check here (trusting Get's contract), or
- Add the same defensive check in
SnapshotQuantitybefore dereferencingCurrent code is safe since
Get(..., true)guarantees a non-nil meter on success, but the inconsistency could confuse future readers.openmeter/billing/service/featuremeter_test.go (1)
13-48: Consider using a fixed timestamp for determinism.Using
time.Now()here makes ordering a bit implicit. A fixed base time keeps expectations explicit and avoids rare clock-jitter flakes.♻️ Proposed tweak
func TestGetLastFeatures(t *testing.T) { + baseTime := time.Date(2025, 1, 1, 0, 0, 0, 0, time.UTC) tcs := []struct { name string features []feature.Feature expected map[string]string }{ { name: "single-active", features: []feature.Feature{ {ID: "id-active", ArchivedAt: nil, Key: "feature-1-active"}, }, expected: map[string]string{"feature-1-active": "id-active"}, }, { name: "single-archived", features: []feature.Feature{ - {ID: "id-archived", ArchivedAt: lo.ToPtr(time.Now()), Key: "feature-1-archived"}, + {ID: "id-archived", ArchivedAt: lo.ToPtr(baseTime), Key: "feature-1-archived"}, }, expected: map[string]string{"feature-1-archived": "id-archived"}, }, { name: "multi-archived", features: []feature.Feature{ - {ID: "id-archived", ArchivedAt: lo.ToPtr(time.Now()), Key: "feature-1"}, + {ID: "id-archived", ArchivedAt: lo.ToPtr(baseTime), Key: "feature-1"}, {ID: "id-active", ArchivedAt: nil, Key: "feature-1"}, }, expected: map[string]string{"feature-1": "id-active"}, }, { name: "archived-ordering", features: []feature.Feature{ - {ID: "id-archived-1", ArchivedAt: lo.ToPtr(time.Now()), Key: "feature-1"}, - {ID: "id-archived-2", ArchivedAt: lo.ToPtr(time.Now().Add(5 * time.Second)), Key: "feature-1"}, + {ID: "id-archived-1", ArchivedAt: lo.ToPtr(baseTime), Key: "feature-1"}, + {ID: "id-archived-2", ArchivedAt: lo.ToPtr(baseTime.Add(5 * time.Second)), Key: "feature-1"}, }, expected: map[string]string{"feature-1": "id-archived-2"}, }, }openmeter/billing/service/featuremeter.go (2)
108-120: Minor: Variable naming could be more generic.The variable
featuresByKeyon line 109 and the loop variablefeatureson line 114 are named specifically for features, but this is a generic function. Consider renaming toentitiesByKeyandentitiesfor better clarity.♻️ Suggested rename for clarity
func getLastEntity[T any](entities []T, accessor lastEntityAccessor[T]) map[string]T { - featuresByKey := lo.GroupBy(entities, func(entity T) string { + entitiesByKey := lo.GroupBy(entities, func(entity T) string { return accessor.GetKey(entity) }) - out := make(map[string]T, len(featuresByKey)) - for key, features := range featuresByKey { + out := make(map[string]T, len(entitiesByKey)) + for key, grouped := range entitiesByKey { // Let's try to find an unarchived feature - out[key] = latestEntity(features, accessor) + out[key] = latestEntity(grouped, accessor) } return out }
85-92: Silent fallback when meter is missing for a feature with MeterSlug.When a feature has a
MeterSlugbut the corresponding meter isn't found in the database (line 86-92), the code silently creates aFeatureMeterwithout the meter. This could mask data integrity issues where expected meters have been deleted.Consider logging a warning when this happens so issues are visible in monitoring.
openmeter/billing/service/gatheringinvoicependinglines.go (1)
204-206: Consider usinglen(in.FeatureMeters) == 0or keeping nil check consistent.The validation checks
in.FeatureMeters == nilhere and in other places, but an empty map would pass this check. If an emptyFeatureMetersmap is also invalid (no features to resolve), consider checking for both nil and empty. However, if an empty map is valid (invoice has no usage-based lines), then the current check is fine.Just flagging for awareness - the current approach is probably intentional since invoices without usage-based lines would have empty feature meters.
openmeter/billing/service/invoicecalc/calculator.go (1)
47-70: Consider validating required deps to fail fast.
Now that deps are passed in, a lightweight guard (e.g., non‑nilLineService) before running calculations that rely on it would give callers a clear error instead of a potential nil deref. Maybe add this around Line 63 or Line 67.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@openmeter/billing/service/featuremeter.go`:
- Around line 15-105: In resolveFeatureMeters: avoid calling
ListFeatures/ListMeters with empty filter slices which causes unfiltered "list
all" queries; check the computed featuresToResolve after building it and if it's
empty return an empty billing.FeatureMeters (no error), and likewise after
computing metersToResolve skip the call to s.meterService.ListMeters and leave
metersByKey empty (or return early if appropriate) so you don't issue an
unfiltered query; update the logic around featuresToResolve, metersToResolve,
ListFeatures and ListMeters to perform these early returns/short-circuits.
| func (s *Service) resolveFeatureMeters(ctx context.Context, lines billing.StandardLines) (billing.FeatureMeters, error) { | ||
| namespaces := lo.Uniq(lo.Map(lines, func(line *billing.StandardLine, _ int) string { | ||
| return line.Namespace | ||
| })) | ||
|
|
||
| if len(namespaces) != 1 { | ||
| return nil, fmt.Errorf("all lines must be in the same namespace") | ||
| } | ||
|
|
||
| namespace := namespaces[0] | ||
|
|
||
| featuresToResolve := lo.Uniq( | ||
| lo.Filter( | ||
| lo.Map(lines, func(line *billing.StandardLine, _ int) string { | ||
| // Never happens, as StandardLine is always a usage based line, but until we migrate to a new table let's keep it here | ||
| if line.UsageBased == nil { | ||
| return "" | ||
| } | ||
|
|
||
| return line.UsageBased.FeatureKey | ||
| }), | ||
| func(featureKey string, _ int) bool { | ||
| return featureKey != "" | ||
| }, | ||
| ), | ||
| ) | ||
|
|
||
| // Let's resolve the features | ||
| features, err := s.featureService.ListFeatures(ctx, feature.ListFeaturesParams{ | ||
| IDsOrKeys: featuresToResolve, | ||
| Namespace: namespace, | ||
| IncludeArchived: true, | ||
| }) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("listing features: %w", err) | ||
| } | ||
|
|
||
| featuresByKey := getLastFeatures(features.Items) | ||
|
|
||
| metersToResolve := lo.Uniq( | ||
| lo.Filter( | ||
| lo.Map(lo.Values(featuresByKey), func(feature feature.Feature, _ int) string { | ||
| if feature.MeterSlug == nil { | ||
| return "" | ||
| } | ||
|
|
||
| return *feature.MeterSlug | ||
| }), | ||
| func(meterSlug string, _ int) bool { | ||
| return meterSlug != "" | ||
| }, | ||
| ), | ||
| ) | ||
|
|
||
| meters, err := s.meterService.ListMeters(ctx, meter.ListMetersParams{ | ||
| SlugFilter: lo.ToPtr(metersToResolve), | ||
| Namespace: namespace, | ||
| IncludeDeleted: true, | ||
| }) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("listing meters: %w", err) | ||
| } | ||
|
|
||
| metersByKey := getLastMeters(meters.Items) | ||
|
|
||
| out := make(billing.FeatureMeters, len(featuresByKey)) | ||
| for featureKey, feature := range featuresByKey { | ||
| if feature.MeterSlug == nil { | ||
| out[featureKey] = billing.FeatureMeter{ | ||
| Feature: feature, | ||
| } | ||
|
|
||
| continue | ||
| } | ||
|
|
||
| meter, exists := metersByKey[*feature.MeterSlug] | ||
| if !exists { | ||
| out[featureKey] = billing.FeatureMeter{ | ||
| Feature: feature, | ||
| } | ||
|
|
||
| continue | ||
| } | ||
|
|
||
| out[featureKey] = billing.FeatureMeter{ | ||
| Feature: feature, | ||
| Meter: &meter, | ||
| } | ||
| } | ||
|
|
||
| return out, nil |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find feature service ListFeatures implementation
fd -t f -n "feature" --type go | head -20Repository: openmeterio/openmeter
Length of output: 295
🏁 Script executed:
#!/bin/bash
# Search for ListFeatures and ListMeters implementations
rg -n "func.*ListFeatures\(" --type=go -A 5Repository: openmeterio/openmeter
Length of output: 4089
🏁 Script executed:
#!/bin/bash
# Search for ListMeters implementation
rg -n "func.*ListMeters\(" --type=go -A 5Repository: openmeterio/openmeter
Length of output: 6308
🏁 Script executed:
#!/bin/bash
# Read the feature adapter ListFeatures implementation
sed -n '172,230p' openmeter/productcatalog/adapter/feature.goRepository: openmeterio/openmeter
Length of output: 1797
🏁 Script executed:
#!/bin/bash
# Read the meter adapter ListMeters implementation
sed -n '18,80p' openmeter/meter/adapter/meter.goRepository: openmeterio/openmeter
Length of output: 1785
🏁 Script executed:
#!/bin/bash
# Find ListFeaturesParams structure definition
rg -n "type ListFeaturesParams struct" --type=go -A 10Repository: openmeterio/openmeter
Length of output: 2288
🏁 Script executed:
#!/bin/bash
# Find ListMetersParams structure definition
rg -n "type ListMetersParams struct" --type=go -A 10Repository: openmeterio/openmeter
Length of output: 2418
🏁 Script executed:
#!/bin/bash
# Check how KeyIn behaves with empty slice
rg -n "func.*KeyIn\(" --type=go -A 5 | head -30Repository: openmeterio/openmeter
Length of output: 2432
🏁 Script executed:
#!/bin/bash
# Also check if there are any tests showing ListMeters behavior with empty SlugFilter
rg -n "ListMeters.*SlugFilter" --type=go -B2 -A8Repository: openmeterio/openmeter
Length of output: 47
Add early returns to prevent unintended list-all queries when inputs are empty.
When lines is empty or contains items with no UsageBased data, featuresToResolve becomes empty. Calling ListFeatures with an empty IDsOrKeys slice doesn't apply a filter, so it returns all features in the namespace instead of none—a correctness bug in a billing path. Similarly, skipping the meter lookup when there's nothing to resolve avoids unnecessary database hits.
🔧 Suggested fix
func (s *Service) resolveFeatureMeters(ctx context.Context, lines billing.StandardLines) (billing.FeatureMeters, error) {
+ if len(lines) == 0 {
+ return billing.FeatureMeters{}, nil
+ }
namespaces := lo.Uniq(lo.Map(lines, func(line *billing.StandardLine, _ int) string {
return line.Namespace
}))
@@
featuresToResolve := lo.Uniq(
@@
)
+ if len(featuresToResolve) == 0 {
+ return billing.FeatureMeters{}, nil
+ }
@@
- meters, err := s.meterService.ListMeters(ctx, meter.ListMetersParams{
- SlugFilter: lo.ToPtr(metersToResolve),
- Namespace: namespace,
- IncludeDeleted: true,
- })
- if err != nil {
- return nil, fmt.Errorf("listing meters: %w", err)
- }
-
- metersByKey := getLastMeters(meters.Items)
+ metersByKey := map[string]meter.Meter{}
+ if len(metersToResolve) > 0 {
+ meters, err := s.meterService.ListMeters(ctx, meter.ListMetersParams{
+ SlugFilter: lo.ToPtr(metersToResolve),
+ Namespace: namespace,
+ IncludeDeleted: true,
+ })
+ if err != nil {
+ return nil, fmt.Errorf("listing meters: %w", err)
+ }
+ metersByKey = getLastMeters(meters.Items)
+ }🤖 Prompt for AI Agents
In `@openmeter/billing/service/featuremeter.go` around lines 15 - 105, In
resolveFeatureMeters: avoid calling ListFeatures/ListMeters with empty filter
slices which causes unfiltered "list all" queries; check the computed
featuresToResolve after building it and if it's empty return an empty
billing.FeatureMeters (no error), and likewise after computing metersToResolve
skip the call to s.meterService.ListMeters and leave metersByKey empty (or
return early if appropriate) so you don't issue an unfiltered query; update the
logic around featuresToResolve, metersToResolve, ListFeatures and ListMeters to
perform these early returns/short-circuits.
2 participants
Line service feature + meter dependencies
Removing the meter and feature dependencies, and requiring the caller to gather all the required lines. This allows the service to have zero external dependenies.
Given that most of the times the line service is operating on the whole invoice the external resolving makes invoice operations faster, as we don't need to resolve features meters one-by-one.
Notes for reviewer
Full changeset is available here: #3807
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.