feat(entitlement): expose TotalAvailableGrantedAmount #3556
Conversation
📝 WalkthroughWalkthroughThreads a new TotalAvailableGrantAmount field through the entitlement value chain (API spec, snapshots, metered connector, balance calculation, engine) and captures RunParams in RunResult; also makes BalanceConnector.GetBalanceSinceSnapshot unexported and switches callers to GetBalanceAt. Tests updated to expect the new field. Changes
Sequence Diagram(s)sequenceDiagram
rect rgb(240,248,255)
participant API as API / Handler
participant Conn as Metered Connector
participant Balance as Entitlement Balance
participant Engine as Credit Engine
end
API->>Conn: Request entitlement value
Conn->>Balance: GetEntitlementBalance(owner, meter, at)
Balance->>Engine: BalanceConnector.GetBalanceAt(owner, snap, at)
Engine->>Engine: RunParams.Clone()
Engine->>Engine: Calculate TotalAvailableGrantAmount()
Engine-->>Balance: RunResult{..., RunParams}
Balance->>Conn: EntitlementBalance{..., TotalAvailableGrantAmount}
Conn->>API: Map to MeteredEntitlementValue / EntitlementValue (includes TotalAvailableGrantAmount)
API->>Client: JSON response (totalAvailableGrantAmount)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: 2
🧹 Nitpick comments (2)
api/spec/src/entitlements/subjects.tsp (1)
233-238: Consider clarifying "active" grants in the documentation.The documentation mentions "grant amounts that are active at the time of the query" but doesn't specify what makes a grant active. Consider adding a brief note about whether this includes grants that haven't reached their effective date yet, or have expired, etc. This will help API consumers understand the field better.
e2e/e2e_test.go (1)
1104-1108: Test validates the new field, but could be more thorough.The test correctly expects
TotalAvailableGrantAmountto be 100.0, matching the grant created earlier. However, consider adding test cases for:
- Multiple grants (to verify summation)
- Zero grants (to verify the field handles empty cases)
- Expired or inactive grants (to verify only "active" grants are counted)
This would give more confidence that the calculation logic is robust.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
api/client/go/client.gen.gois excluded by!api/client/**api/client/javascript/src/client/schemas.tsis excluded by!api/client/**api/client/python/openmeter/_generated/models/_models.pyis excluded by!**/_generated/**,!api/client/**api/openapi.cloud.yamlis excluded by!**/openapi.cloud.yamlapi/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (9)
api/spec/src/entitlements/subjects.tsp(1 hunks)e2e/e2e_test.go(1 hunks)openmeter/credit/balance.go(2 hunks)openmeter/credit/engine/engine.go(2 hunks)openmeter/credit/engine/run.go(2 hunks)openmeter/entitlement/driver/parser.go(1 hunks)openmeter/entitlement/metered/balance.go(2 hunks)openmeter/entitlement/metered/connector.go(2 hunks)openmeter/entitlement/snapshot/event.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
⚙️ CodeRabbit configuration file
**/*.go: In general when reviewing the Golang code make readability and maintainability a priority, even potentially suggest restructuring the code to improve them.Performance should be a priority in critical code paths. Anything related to event ingestion, message processing, database operations (regardless of database) should be vetted for potential performance bottlenecks.
Files:
openmeter/credit/engine/run.goopenmeter/entitlement/snapshot/event.goopenmeter/entitlement/metered/balance.goopenmeter/entitlement/metered/connector.goopenmeter/entitlement/driver/parser.goe2e/e2e_test.goopenmeter/credit/engine/engine.goopenmeter/credit/balance.go
**/*_test.go
⚙️ CodeRabbit configuration file
**/*_test.go: Make sure the tests are comprehensive and cover the changes. Keep a strong focus on unit tests and in-code integration tests.
When appropriate, recommend e2e tests for critical changes.
Files:
e2e/e2e_test.go
**/*.tsp
⚙️ CodeRabbit configuration file
**/*.tsp: Review the TypeSpec code for conformity with TypeSpec best practices. When recommending changes also consider the fact that multiple codegeneration toolchains depend on the TypeSpec code, each of which have their idiosyncrasies and bugs.The declared API should be accurate, in parity with the actual implementation, and easy to understand for the user.
Files:
api/spec/src/entitlements/subjects.tsp
🧬 Code graph analysis (6)
openmeter/credit/engine/run.go (2)
openmeter/credit/balance/balance.go (1)
Snapshot(80-85)openmeter/credit/engine/engine.go (1)
RunParams(16-30)
openmeter/entitlement/metered/balance.go (2)
openmeter/credit/balance/balance.go (1)
Snapshot(80-85)openmeter/credit/engine/engine.go (1)
RunParams(16-30)
openmeter/entitlement/metered/connector.go (2)
openmeter/ent/db/balancesnapshot/where.go (2)
Balance(84-86)Overage(89-91)openmeter/ent/db/entitlement/where.go (1)
IsSoftLimit(140-142)
e2e/e2e_test.go (1)
pkg/convert/ptr.go (1)
ToPointer(10-12)
openmeter/credit/engine/engine.go (6)
openmeter/credit/grant/grant.go (1)
Grant(14-60)pkg/timeutil/timeline.go (1)
NewSimpleTimeline(11-18)openmeter/meter/meter.go (1)
Meter(139-148)openmeter/credit/grant/owner_connector.go (1)
ResetBehavior(32-34)openmeter/credit/balance/balance.go (1)
Snapshot(80-85)openmeter/credit/engine/history.go (1)
GrantBurnDownHistory(93-96)
openmeter/credit/balance.go (3)
pkg/models/id.go (1)
NamespacedID(7-10)openmeter/credit/balance/balance.go (1)
Snapshot(80-85)openmeter/credit/engine/engine.go (1)
RunResult(58-65)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Artifacts / Container image
- GitHub Check: Lint
- GitHub Check: Test
- GitHub Check: Migration Checks
- GitHub Check: Code Generators
- GitHub Check: Build
- GitHub Check: Analyze (go)
🔇 Additional comments (10)
openmeter/credit/balance.go (1)
43-44: Nice encapsulation improvement!Making
getBalanceSinceSnapshotprivate is a solid move since it was never exposed through theBalanceConnectorinterface. This keeps implementation details internal while the public API remains clean.openmeter/entitlement/snapshot/event.go (1)
55-56: LGTM! Field addition is clean and consistent.The new
TotalAvailableGrantAmountfield aligns perfectly with the API spec, using the correct Go patterns (pointer for optional field, omitempty JSON tag).openmeter/credit/engine/run.go (1)
17-17: Nice defensive programming with the param cloning!Cloning the input params and returning them in the result ensures the
RunResultcarries an immutable snapshot of the parameters used for the calculation. This makes the result self-contained and easier to reason about.Also applies to: 95-97
openmeter/entitlement/driver/parser.go (1)
160-164: Clean mapping implementation!The field mapping correctly threads
TotalAvailableGrantAmountfrom the internalMeteredEntitlementValueto the API response. Only populating it for metered entitlements makes sense since grants are specific to that entitlement type.openmeter/entitlement/metered/connector.go (1)
49-54: Straightforward field addition, well integrated!The
TotalAvailableGrantAmountfield is cleanly added toMeteredEntitlementValueand correctly populated from the balance. The struct fields are nicely organized alongside other balance-related properties.Also applies to: 132-137
openmeter/credit/engine/engine.go (2)
32-38: Clean implementation of grant amount summation!Using
alpacadecimalfor the intermediate calculation helps maintain precision before converting back to float64. This aligns with the established pattern in the codebase and the acknowledgment on line 75 that float64 limitations are a known concern.
63-64: RunParams field addition makes results more informative!Embedding the exact parameters used to produce the result makes
RunResultself-contained and easier to debug. Nice enhancement to the API.openmeter/entitlement/metered/balance.go (3)
22-29: Nice addition!The new
TotalAvailableGrantAmountfield is properly defined with the correct JSON tag. The structure looks clean and consistent with the existing fields.
83-90: Looking good!The new field is properly populated from
res.RunParams.TotalAvailableGrantAmount(), and all the existing fields are correctly mapped. The balance calculation flow makes sense.
78-78: Method change verified—behavior is equivalent.The refactoring from
GetBalanceSinceSnapshottoGetBalanceAtis solid. TheBalanceConnectorinterface only exposesGetBalanceAt(line 31 inopenmeter/credit/balance.go), and it's explicitly designed to "minimize execution cost by calculating from the latest valid snapshot." That's exactly what the old internalgetBalanceSinceSnapshothelper was doing, so the semantics are identical. The error message "failed to get balance since snapshot" remains accurate sinceGetBalanceAthandles that optimization internally.
| func (p RunParams) Clone() RunParams { | ||
| grants := make([]grant.Grant, len(p.Grants)) | ||
| copy(grants, p.Grants) | ||
|
|
||
| resets := timeutil.NewSimpleTimeline(p.Resets.GetTimes()) | ||
|
|
||
| res := RunParams{ | ||
| Meter: p.Meter, | ||
| Grants: grants, | ||
| Until: p.Until, | ||
| StartingSnapshot: p.StartingSnapshot, | ||
| ResetBehavior: p.ResetBehavior, | ||
| Resets: resets, | ||
| } | ||
|
|
||
| return res | ||
| } |
There was a problem hiding this comment.
Clone method has a shallow copy issue with Meter.GroupBy.
The Clone() method copies the Meter field by value, but Meter.GroupBy is a map[string]string (reference type). If the original Meter.GroupBy is mutated after cloning, it will affect the cloned RunParams too. Consider deep copying the map:
func (p RunParams) Clone() RunParams {
grants := make([]grant.Grant, len(p.Grants))
copy(grants, p.Grants)
resets := timeutil.NewSimpleTimeline(p.Resets.GetTimes())
+
+ // Deep copy Meter.GroupBy map
+ meterCopy := p.Meter
+ if p.Meter.GroupBy != nil {
+ meterCopy.GroupBy = make(map[string]string, len(p.Meter.GroupBy))
+ for k, v := range p.Meter.GroupBy {
+ meterCopy.GroupBy[k] = v
+ }
+ }
res := RunParams{
- Meter: p.Meter,
+ Meter: meterCopy,
Grants: grants,
Until: p.Until,
StartingSnapshot: p.StartingSnapshot,
ResetBehavior: p.ResetBehavior,
Resets: resets,
}
return res
}🤖 Prompt for AI Agents
In openmeter/credit/engine/engine.go around lines 40 to 56, the Clone method
performs a shallow copy of p.Meter so its GroupBy map (map[string]string) is
shared between original and clone; modify Clone to deep-copy the Meter by
creating a new Meter value, allocate a new map for its GroupBy, and copy all
key/value pairs from p.Meter.GroupBy into that new map before assigning the new
Meter to res.Meter so mutations to the original Meter.GroupBy do not affect the
clone.
| res, err := e.balanceConnector.GetBalanceSinceSnapshot(ctx, nsOwner, lastSnap, at) | ||
| res, err := e.balanceConnector.GetBalanceAt(ctx, nsOwner, at) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get balance since snapshot: %w", err) |
There was a problem hiding this comment.
Update the error message to match the method call.
The error message still references "balance since snapshot" but the code now calls GetBalanceAt on line 78. Let's keep them in sync for easier debugging.
Apply this diff:
- return nil, fmt.Errorf("failed to get balance since snapshot: %w", err)
+ return nil, fmt.Errorf("failed to get balance at time: %w", err)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return nil, fmt.Errorf("failed to get balance since snapshot: %w", err) | |
| return nil, fmt.Errorf("failed to get balance at time: %w", err) |
🤖 Prompt for AI Agents
In openmeter/entitlement/metered/balance.go around line 80, the error message
still says "failed to get balance since snapshot" while the code calls
GetBalanceAt on line 78; update the fmt.Errorf message to reference GetBalanceAt
(for example "failed to get balance at snapshot" or "failed to get balance at
specified time") so the log matches the method being called and aids debugging.
GAlexIHU
force-pushed
the
feat/entitlements/tag
branch
from
4d69745 to
2ada658
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
openmeter/entitlement/metered/balance.go (1)
85-85: Update error message to match method call.The error message still references "balance since snapshot" but the code now calls
GetBalanceAton line 83. Let's sync them up for easier debugging.Apply this diff:
- return nil, fmt.Errorf("failed to get balance since snapshot: %w", err) + return nil, fmt.Errorf("failed to get balance at time: %w", err)
🧹 Nitpick comments (1)
openmeter/entitlement/metered/balance.go (1)
29-29: Consider adding documentation for new field.The new
TotalAvailableGrantAmountfield doesn't have a doc comment. Adding a brief comment would help clarify what this represents (e.g., "total amount of all active grants for this entitlement").
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
api/client/go/client.gen.gois excluded by!api/client/**api/client/javascript/src/client/schemas.tsis excluded by!api/client/**api/client/python/openmeter/_generated/models/_models.pyis excluded by!**/_generated/**,!api/client/**api/openapi.cloud.yamlis excluded by!**/openapi.cloud.yamlapi/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (9)
api/spec/src/entitlements/subjects.tsp(1 hunks)e2e/e2e_test.go(1 hunks)openmeter/credit/balance.go(2 hunks)openmeter/credit/engine/engine.go(2 hunks)openmeter/credit/engine/run.go(2 hunks)openmeter/entitlement/driver/parser.go(1 hunks)openmeter/entitlement/metered/balance.go(2 hunks)openmeter/entitlement/metered/connector.go(2 hunks)openmeter/entitlement/snapshot/event.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- openmeter/entitlement/snapshot/event.go
- openmeter/credit/engine/engine.go
- openmeter/entitlement/metered/connector.go
- api/spec/src/entitlements/subjects.tsp
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
⚙️ CodeRabbit configuration file
**/*.go: In general when reviewing the Golang code make readability and maintainability a priority, even potentially suggest restructuring the code to improve them.Performance should be a priority in critical code paths. Anything related to event ingestion, message processing, database operations (regardless of database) should be vetted for potential performance bottlenecks.
Files:
openmeter/credit/engine/run.goopenmeter/credit/balance.goopenmeter/entitlement/driver/parser.goe2e/e2e_test.goopenmeter/entitlement/metered/balance.go
**/*_test.go
⚙️ CodeRabbit configuration file
**/*_test.go: Make sure the tests are comprehensive and cover the changes. Keep a strong focus on unit tests and in-code integration tests.
When appropriate, recommend e2e tests for critical changes.
Files:
e2e/e2e_test.go
🧬 Code graph analysis (4)
openmeter/credit/engine/run.go (1)
openmeter/credit/engine/engine.go (1)
RunParams(16-30)
openmeter/credit/balance.go (3)
pkg/models/id.go (1)
NamespacedID(7-10)openmeter/credit/balance/balance.go (1)
Snapshot(80-85)openmeter/credit/engine/engine.go (1)
RunResult(58-65)
e2e/e2e_test.go (1)
pkg/convert/ptr.go (1)
ToPointer(10-12)
openmeter/entitlement/metered/balance.go (2)
openmeter/credit/balance/balance.go (1)
Snapshot(80-85)openmeter/credit/engine/engine.go (1)
RunParams(16-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Artifacts / Container image
- GitHub Check: Artifacts / Benthos Collector Container image
- GitHub Check: Test
- GitHub Check: Build
- GitHub Check: Migration Checks
- GitHub Check: Code Generators
- GitHub Check: Lint
- GitHub Check: Repository Scan
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
openmeter/credit/balance.go (1)
43-144: LGTM! Clean encapsulation improvement.Nice refactor making
getBalanceSinceSnapshotunexported. The change is consistent across both the definition and the call site, and the comment is properly updated too. This keeps the internal balance calculation details private to the package while still exposing the publicGetBalanceAtinterface.openmeter/entitlement/driver/parser.go (1)
160-164: LGTM! Correct field mapping.The
TotalAvailableGrantAmountfield is properly mapped fromMeteredEntitlementValueto the API response. Makes sense that only metered entitlements have this field since static and boolean entitlements don't use grants.e2e/e2e_test.go (1)
1270-1274: LGTM! Test correctly validates the new field.The test expectation for
TotalAvailableGrantAmount: convert.ToPointer(100.0)matches the grant amount created earlier in the test (line 1117). The values all make sense together: 100.0 total granted, 1.0 used, 99.0 remaining balance.openmeter/entitlement/metered/balance.go (1)
88-95: LGTM! New field properly populated.The
TotalAvailableGrantAmountis correctly populated fromres.RunParams.TotalAvailableGrantAmount()and included in the returned balance. The change aligns well with the PR objective to expose this information.openmeter/credit/engine/run.go (1)
17-17: Looking at the Clone() implementation, I can see it's doing a shallow copy of the grants slice and creating a new SimpleTimeline. The main unknown is whether theGetTimes()call in the SimpleTimeline creation is expensive. Let me check that real quick:Let me search the codebase directly for the SimpleTimeline implementation:
Clone() implementation is appropriate for the use case; verify if capturing params in RunResult is necessary.
The
Clone()method does a shallow copy of the grants slice and creates a new SimpleTimeline—both necessary operations to ensure the cloned params are independent. The concern about performance is worth flagging, but the implementation itself is sound: it uses efficient Go patterns (make+copyfor the slice, and creates a fresh SimpleTimeline from times).The real question is whether capturing the params in
RunResultjustifies the clone on every invocation. If this is needed for auditing or debugging (seems likely given it's stored in the result), then the allocation is warranted. If it's not strictly necessary, you could defer the clone until actually needed or explore using pointers instead. That said, the current approach is a reasonable trade-off for correctness and debuggability on a balance calculation path.
Overview
Exposes new field
Summary by CodeRabbit