refactor: thresholds for balance threshold events #3269
Conversation
📝 WalkthroughWalkthroughAdds three new balance-threshold value types (balance_value, usage_percentage, usage_value) across TypeSpec/OpenAPI/JS clients and Zod validators; updates Go consumer logic for normalized threshold evaluation, dedup hashing (V1/V2), tests, constants, and validation to support the new types. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
chrisgacsal
force-pushed
the
refactor/notifications
branch
from
0ddc32b to
5c888fd
Compare
![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
🧹 Nitpick comments (7)
openmeter/notification/consumer/entitlementbalancethreshold.go (7)
329-339: Have Iter() skip nil candidates to simplify callers and avoid mistakesYielding nil entries makes call sites easy to misuse (as above). Filter them out in Iter() so only real candidates are yielded.
func (a activeThresholds) Iter() iter.Seq2[int, *notification.BalanceThreshold] { - thresholds := []*notification.BalanceThreshold{a.Usage, a.Balance} - - return func(yield func(int, *notification.BalanceThreshold) bool) { - for i := 0; i <= len(thresholds)-1; i++ { - if !yield(i, thresholds[i]) { - return - } - } - } + thresholds := make([]*notification.BalanceThreshold, 0, 2) + if a.Usage != nil { + thresholds = append(thresholds, a.Usage) + } + if a.Balance != nil { + thresholds = append(thresholds, a.Balance) + } + + return func(yield func(int, *notification.BalanceThreshold) bool) { + for i := 0; i < len(thresholds); i++ { + if !yield(i, thresholds[i]) { + return + } + } + } }
373-386: Unify tie-breaker logic for Usage thresholds (percent vs value)For UsagePercentage you use <= to allow later equal thresholds to win; for UsageValue you use <. This inconsistency is subtle and can produce different winners for equal numeric thresholds across types.
- case api.NotificationRuleBalanceThresholdValueTypeUsageValue: + case api.NotificationRuleBalanceThresholdValueTypeUsageValue: if usage == nil { usage = numThreshold - } else if usage.ThresholdValue < numThreshold.ThresholdValue { + } else if usage.ThresholdValue <= numThreshold.ThresholdValue { usage = numThreshold }If you prefer “first wins” semantics, flip both branches to use < consistently; either way, keep them aligned.
90-107: Reduce DB load: fetch last events once per rule evaluationInside the loop you call ListEvents for each candidate threshold. The dedupe hash and period are the same across candidates, so you can fetch once and reuse.
- periodDedupeHash := b.getPeriodsDeduplicationHash(balSnapshot, rule.ID) - - // TODO[issue-1364]: this must be cached to prevent going to the DB for each balance.snapshot event - lastEvents, err := b.Notification.ListEvents(ctx, notification.ListEventsInput{ + periodDedupeHash := b.getPeriodsDeduplicationHash(balSnapshot, rule.ID) + // Fetch once per rule evaluation; reuse for all candidates + lastEvents, err := b.Notification.ListEvents(ctx, notification.ListEventsInput{ Page: pagination.Page{ PageSize: 1, PageNumber: 1, }, Namespaces: []string{balSnapshot.Namespace.ID}, From: balSnapshot.Entitlement.CurrentUsagePeriod.From, To: balSnapshot.Entitlement.CurrentUsagePeriod.To, DeduplicationHashes: []string{periodDedupeHash}, OrderBy: notification.OrderByCreatedAt, Order: sortx.OrderDesc, })Alternatively, hoist periodDedupeHash and lastEvents above the loop; pass them into createEventInput and reuse.
137-145: Revisit ErrNoBalanceAvailable handling for last event comparisonIf the last event’s stored value yields ErrNoBalanceAvailable (e.g., zero grants at event time), you currently skip creating a new event even if a valid threshold is active now. That may prevent the “first meaningful” event from being emitted for the period.
Two options:
- Treat ErrNoBalanceAvailable as “unknown previous threshold” and proceed to create the event.
- Or fall back to comparing only the threshold.Type/Value (ignoring derived numeric ThresholdValue) to decide whether to emit.
Example tweak:
- if errors.Is(err, ErrNoBalanceAvailable) { - b.Logger.Warn("no balance available skipping event creation", "last_event_id", lastEvent.ID) - return nil - } + if errors.Is(err, ErrNoBalanceAvailable) { + b.Logger.Warn("no balance available in last event; emitting first valid threshold event", "last_event_id", lastEvent.ID) + return b.createEvent(ctx, createEventInput) + }Please confirm intended behavior for this edge case.
30-32: Remove unused EntitlementSnapshotHandlerStateThis type isn’t referenced in the file; consider deleting to reduce noise.
-type EntitlementSnapshotHandlerState struct { - TotalGrants float64 `json:"totalGrants"` -} +// (removed: unused EntitlementSnapshotHandlerState)
281-322: Input validation guardrails in getNumericThresholdNon-sensical inputs (negative values; percentage outside [0,100]) are likely validated upstream, but a quick sanity clamp/guard here would make the function more defensive, preventing odd Active states from malformed rules.
case api.NotificationRuleBalanceThresholdValueTypeUsagePercentage: - totalGrants := lo.FromPtr(value.Balance) + lo.FromPtr(value.Usage) - lo.FromPtr(value.Overage) + pct := threshold.Value + if pct < 0 { + pct = 0 + } else if pct > 100 { + pct = 100 + } + totalGrants := lo.FromPtr(value.Balance) + lo.FromPtr(value.Usage) - lo.FromPtr(value.Overage) ... - thresholdValue := totalGrants * threshold.Value / 100 + thresholdValue := totalGrants * pct / 100If validation is guaranteed elsewhere, feel free to skip; add a brief comment linking that validator.
125-131: Logging style consistency (slog)You use ErrorContext with typed attrs and Warn with untyped key/value pairs. Consider using the same style everywhere for readability and consistent structured logs.
- b.Logger.ErrorContext(ctx, "last event is not a balance threshold event", slog.String("event_id", lastEvent.ID)) + b.Logger.ErrorContext(ctx, "last event is not a balance threshold event", slog.String("event_id", lastEvent.ID)) ... - b.Logger.Warn("no balance available skipping event creation", "last_event_id", lastEvent.ID) + b.Logger.Warn("no balance available skipping event creation", slog.String("last_event_id", lastEvent.ID))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
api/client/javascript/src/client/schemas.ts(1 hunks)api/client/javascript/src/zod/index.ts(2 hunks)api/openapi.cloud.yaml(1 hunks)api/openapi.yaml(1 hunks)api/spec/src/notification/entitlements.tsp(1 hunks)openmeter/notification/consumer/balancethreshold_test.go(0 hunks)openmeter/notification/consumer/entitlementbalancethreshold.go(4 hunks)openmeter/notification/consumer/entitlementbalancethreshold_test.go(1 hunks)openmeter/notification/entitlements.go(2 hunks)
💤 Files with no reviewable changes (1)
- openmeter/notification/consumer/balancethreshold_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- openmeter/notification/consumer/entitlementbalancethreshold_test.go
- api/client/javascript/src/client/schemas.ts
- openmeter/notification/entitlements.go
- api/client/javascript/src/zod/index.ts
- api/openapi.yaml
- api/spec/src/notification/entitlements.tsp
- api/openapi.cloud.yaml
🧰 Additional context used
🧬 Code graph analysis (1)
openmeter/notification/consumer/entitlementbalancethreshold.go (3)
openmeter/notification/entitlements.go (1)
BalanceThreshold(43-43)openmeter/notification/event.go (1)
ListEventsInput(67-88)api/api.gen.go (7)
Entitlement(2551-2553)EntitlementValue(2963-2978)NotificationRuleBalanceThresholdValueTypeNumber(549-549)NotificationRuleBalanceThresholdValueTypeUsageValue(552-552)NotificationRuleBalanceThresholdValueTypePercent(550-550)NotificationRuleBalanceThresholdValueTypeUsagePercentage(551-551)NotificationRuleBalanceThresholdValueTypeBalanceValue(548-548)
⏰ 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: E2E
- GitHub Check: Quickstart
- GitHub Check: Test
- GitHub Check: Build
- GitHub Check: Lint
- GitHub Check: Code Generators
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
openmeter/notification/consumer/entitlementbalancethreshold.go (1)
8-8: Gogo.moddeclares Go 1.24.1, which supportsiter.Seq2usage
The repository’sgo.modfile specifiesgo 1.24.1(≥ 1.22), so the import and use ofiter.Seq2is fully supported. No updates to the Go toolchain or CI images are required.
chrisgacsal
force-pushed
the
refactor/notifications
branch
2 times, most recently
from
a8cab23 to
fd7c1cf
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openmeter/notification/entitlements.go (1)
61-75: Tighten validation semantics: allow BalanceValue = 0 and bound UsagePercentage ≤ 100Today all types require Value > 0, which:
- disallows a legitimate BalanceValue threshold at exactly 0 (notify when balance hits zero), and
- doesn’t constrain UsagePercentage to a sensible upper bound.
Recommend explicit per-type checks and avoid fallthrough so constraints are self-documenting.
Apply this diff to the switch:
- switch threshold.Type { - case BalanceThresholdTypeNumber, BalanceThresholdTypePercent: - fallthrough - case BalanceThresholdTypeBalanceValue, BalanceThresholdTypeUsageValue, BalanceThresholdTypeUsagePercentage: - if threshold.Value <= 0 { - return ValidationError{ - Err: fmt.Errorf("invalid threshold with type %s: value must be greater than 0: %.2f", - threshold.Type, - threshold.Value, - ), - } - } - default: - return fmt.Errorf("unknown balance threshold type: %s", threshold.Type) - } + switch threshold.Type { + // Usage value (absolute) + case BalanceThresholdTypeNumber, BalanceThresholdTypeUsageValue: + if threshold.Value <= 0 { + return ValidationError{ + Err: fmt.Errorf("invalid %s threshold: value must be > 0: %.2f", threshold.Type, threshold.Value), + } + } + // Usage percentage + case BalanceThresholdTypePercent, BalanceThresholdTypeUsagePercentage: + if threshold.Value <= 0 || threshold.Value > 100 { + return ValidationError{ + Err: fmt.Errorf("invalid %s threshold: percentage must be in (0, 100]: %.2f", threshold.Type, threshold.Value), + } + } + // Remaining balance value + case BalanceThresholdTypeBalanceValue: + if threshold.Value < 0 { + return ValidationError{ + Err: fmt.Errorf("invalid %s threshold: value must be >= 0: %.2f", threshold.Type, threshold.Value), + } + } + default: + return fmt.Errorf("unknown balance threshold type: %s", threshold.Type) + }Optionally, consider emitting a deprecation warning when NUMBER/PERCENT are used to aid migration.
♻️ Duplicate comments (1)
openmeter/notification/consumer/entitlementbalancethreshold.go (1)
82-86: Nil candidate now skipped instead of aborting — LGTMThis matches the earlier review resolution and ensures Balance-only/Usage-only scenarios are evaluated.
🧹 Nitpick comments (3)
openmeter/notification/consumer/entitlementbalancethreshold.go (3)
329-339: Simplify Iter implementation and avoid off-by-one patternsUse range over the slice inside the generator for clarity.
Apply this diff:
-func (a activeThresholds) Iter() iter.Seq2[int, *notification.BalanceThreshold] { - thresholds := []*notification.BalanceThreshold{a.Usage, a.Balance} - - return func(yield func(int, *notification.BalanceThreshold) bool) { - for i := 0; i <= len(thresholds)-1; i++ { - if !yield(i, thresholds[i]) { - return - } - } - } -} +func (a activeThresholds) Iter() iter.Seq2[int, *notification.BalanceThreshold] { + thresholds := []*notification.BalanceThreshold{a.Usage, a.Balance} + return func(yield func(int, *notification.BalanceThreshold) bool) { + for i, th := range thresholds { + if !yield(i, th) { + return + } + } + } +}
369-386: Unify “usage” comparator logic for clarity and determinismPercent and number usage branches use slightly different tie-breaking (<= vs <). A single “max ThresholdValue wins” rule is easier to reason about and consistent across both Usage types.
Apply this diff:
- // Deprecated: obsoleted by api.NotificationRuleBalanceThresholdValueTypeUsagePercentage - case api.NotificationRuleBalanceThresholdValueTypePercent: - fallthrough - case api.NotificationRuleBalanceThresholdValueTypeUsagePercentage: - if usage == nil { - usage = numThreshold - } else if usage.ThresholdValue <= numThreshold.ThresholdValue { - usage = numThreshold - } - // Deprecated: obsoleted by api.NotificationRuleBalanceThresholdValueTypeUsageValue - case api.NotificationRuleBalanceThresholdValueTypeNumber: - fallthrough - case api.NotificationRuleBalanceThresholdValueTypeUsageValue: - if usage == nil { - usage = numThreshold - } else if usage.ThresholdValue < numThreshold.ThresholdValue { - usage = numThreshold - } + // Usage thresholds (value or percentage): pick the highest threshold value reached + case api.NotificationRuleBalanceThresholdValueTypePercent, + api.NotificationRuleBalanceThresholdValueTypeUsagePercentage, + api.NotificationRuleBalanceThresholdValueTypeNumber, + api.NotificationRuleBalanceThresholdValueTypeUsageValue: + if usage == nil || numThreshold.ThresholdValue >= usage.ThresholdValue { + usage = numThreshold + }
319-321: Include the unknown type in the error for easier diagnosisApply this diff:
- return nil, errors.New("unknown threshold type") + return nil, fmt.Errorf("unknown threshold type: %s", threshold.Type)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
openmeter/notification/consumer/balancethreshold_test.go(0 hunks)openmeter/notification/consumer/entitlementbalancethreshold.go(4 hunks)openmeter/notification/consumer/entitlementbalancethreshold_test.go(1 hunks)openmeter/notification/entitlements.go(2 hunks)
💤 Files with no reviewable changes (1)
- openmeter/notification/consumer/balancethreshold_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- openmeter/notification/consumer/entitlementbalancethreshold_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
openmeter/notification/consumer/entitlementbalancethreshold.go (3)
openmeter/notification/entitlements.go (2)
BalanceThreshold(43-43)EventTypeBalanceThreshold(11-11)openmeter/notification/event.go (1)
ListEventsInput(67-88)api/api.gen.go (7)
Entitlement(2551-2553)EntitlementValue(2963-2978)NotificationRuleBalanceThresholdValueTypeNumber(549-549)NotificationRuleBalanceThresholdValueTypeUsageValue(552-552)NotificationRuleBalanceThresholdValueTypePercent(550-550)NotificationRuleBalanceThresholdValueTypeUsagePercentage(551-551)NotificationRuleBalanceThresholdValueTypeBalanceValue(548-548)
openmeter/notification/entitlements.go (2)
api/api.gen.go (5)
NotificationRuleBalanceThresholdValueTypeNumber(549-549)NotificationRuleBalanceThresholdValueTypePercent(550-550)NotificationRuleBalanceThresholdValueTypeUsagePercentage(551-551)NotificationRuleBalanceThresholdValueTypeBalanceValue(548-548)NotificationRuleBalanceThresholdValueTypeUsageValue(552-552)api/client/go/client.gen.go (5)
NotificationRuleBalanceThresholdValueTypeNumber(506-506)NotificationRuleBalanceThresholdValueTypePercent(507-507)NotificationRuleBalanceThresholdValueTypeUsagePercentage(508-508)NotificationRuleBalanceThresholdValueTypeBalanceValue(505-505)NotificationRuleBalanceThresholdValueTypeUsageValue(509-509)
⏰ 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: Quickstart
- GitHub Check: Lint
- GitHub Check: Build
- GitHub Check: Test
- GitHub Check: Code Generators
- GitHub Check: Migration Checks
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
openmeter/notification/entitlements.go (1)
34-38: New enum aliases are clear and backward-compatible — LGTMThe exported constants cleanly mirror the API enum and retain support for deprecated NUMBER/PERCENT. Good alignment with the PR objective.
chrisgacsal
force-pushed
the
refactor/notifications
branch
from
fd7c1cf to
09ddbb1
Compare
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/notification/consumer/entitlementbalancethreshold.go (1)
200-223: Enforce and handle idempotency onHandlerDeduplicationHashinCreateEventI didn’t find any unique constraint on the
handler_deduplication_hashcolumn in our SQL schema (noUNIQUEindex on that field), so callingCreateEventtwice with the same dedupe hash will happily insert duplicate events (or potentially error without recovery) rather than returning the original event. To makeCreateEventtruly idempotent:• Add a database migration to enforce uniqueness
– e.g.ALTER TABLE notification_events ADD CONSTRAINT uq_notification_dedup_hash UNIQUE (handler_deduplication_hash);• Update the insert logic in
openmeter/notification/adapter/event.go(insidefunc (a *adapter) CreateEvent)- Or directly in the repository implementation in
openmeter/notification/repository.go
to catch the unique‐violation error (e.g. SQLState23505) onhandler_deduplication_hashand, in that case, issue aSELECT * FROM notification_events WHERE handler_deduplication_hash = $1to return the existing record withnilerror.• Alternatively, perform a pre‐insert lookup in the repository:
existing, err := repo.GetEvent(ctx, GetEventInput{HandlerDeduplicationHash: params.HandlerDeduplicationHash}) if err != nil && !IsNotFound(err) { return nil, err } if existing != nil { return existing, nil } // proceed with INSERT...This will ensure downstream handlers never see duplicate‐error failures and always receive the same event object on retries.
♻️ Duplicate comments (1)
openmeter/notification/consumer/entitlementbalancethreshold.go (1)
85-89: Good fix: skip nil candidates instead of exiting earlySwitching to continue avoids suppressing valid Balance-only scenarios. This aligns with the intended two-candidate evaluation flow.
🧹 Nitpick comments (6)
openmeter/notification/consumer/entitlementbalancethreshold.go (6)
77-83: Guard against malformed snapshot events at the entry pointhandleRule dereferences balSnapshot.Value (and relies on Balance/Usage being set) but does not short-circuit on invalid events. Add a cheap precondition using the existing predicate to avoid nil-deref paths and unnecessary work.
func (b *EntitlementSnapshotHandler) handleRule(ctx context.Context, balSnapshot snapshot.SnapshotEvent, rule notification.Rule) error { - // Check 1: do we have a threshold we should create an event for? + // Ensure the snapshot contains all fields required for threshold evaluation. + if !b.isBalanceThresholdEvent(balSnapshot) { + return nil + } + + // Check 1: do we have a threshold we should create an event for?
146-156: Use WarnContext with structured fields for consistency and observabilityOther logs in this function use ErrorContext. Prefer WarnContext here and include contextual fields to make triage easier.
- if errors.Is(err, ErrNoBalanceAvailable) { - // In case there are no grants, percentage all percentage rules would match, so let's instead - // wait until we have some credits to calculate the actual value - b.Logger.Warn("no balance available skipping event creation", "last_event_id", lastEvent.ID) - - continue - } + if errors.Is(err, ErrNoBalanceAvailable) { + // In case there are no grants, percentage rules would all match. + // Skip this candidate; allow other candidates to be evaluated. + b.Logger.WarnContext( + ctx, + "no balance available; skipping event creation for this candidate", + slog.String("event.id", lastEvent.ID), + slog.String("rule.id", rule.ID), + slog.String("threshold.type", string(threshold.Type)), + slog.Float64("threshold.value", threshold.Value), + ) + continue + }
385-402: Deterministic tie-breaking across UsagePercentage vs UsageValueCurrently UsagePercentage uses “>=” (via <= on the stored value) while UsageValue uses “>” for replacement. This makes outcomes depend on evaluation order when threshold values are numerically equal. Use the same comparator to make selection stable across types.
case api.NotificationRuleBalanceThresholdValueTypeUsagePercentage: if usage == nil { usage = numThreshold - } else if usage.ThresholdValue <= numThreshold.ThresholdValue { + } else if usage.ThresholdValue <= numThreshold.ThresholdValue { usage = numThreshold } @@ case api.NotificationRuleBalanceThresholdValueTypeUsageValue: if usage == nil { usage = numThreshold - } else if usage.ThresholdValue < numThreshold.ThresholdValue { + } else if usage.ThresholdValue <= numThreshold.ThresholdValue { usage = numThreshold }Alternatively, collapse both Usage branches and compare only on ThresholdValue to reduce branching.
348-354: Minor: use range for clarity (avoid index arithmetic)The explicit i <= len(thresholds)-1 is correct but less idiomatic and slightly more error-prone than range.
- return func(yield func(int, *notification.BalanceThreshold) bool) { - for i := 0; i <= len(thresholds)-1; i++ { - if !yield(i, thresholds[i]) { - return - } - } - } + return func(yield func(int, *notification.BalanceThreshold) bool) { + for i, t := range thresholds { + if !yield(i, t) { + return + } + } + }
335-337: Include the unknown type in the error for faster triageReturning a typed error with the offending value speeds up debugging and alert correlation.
- return nil, errors.New("unknown threshold type") + return nil, fmt.Errorf("unknown threshold type: %q", threshold.Type)
250-281: Optional: subject identity in dedupe sourceIf subjects can have mutable keys or non-unique keys across namespaces, consider including a stable Subject.Id (when available) in the dedupe source as well to avoid cross-key collisions within a period.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
openmeter/notification/consumer/balancethreshold_test.go(0 hunks)openmeter/notification/consumer/entitlementbalancethreshold.go(5 hunks)openmeter/notification/consumer/entitlementbalancethreshold_test.go(1 hunks)openmeter/notification/entitlements.go(2 hunks)
💤 Files with no reviewable changes (1)
- openmeter/notification/consumer/balancethreshold_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- openmeter/notification/entitlements.go
- openmeter/notification/consumer/entitlementbalancethreshold_test.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-26T10:30:37.866Z
Learnt from: chrisgacsal
PR: openmeterio/openmeter#3269
File: openmeter/notification/consumer/entitlementbalancethreshold.go:8-8
Timestamp: 2025-08-26T10:30:37.866Z
Learning: The `iter` package is part of the Go standard library starting from Go 1.23, providing basic definitions for working with iterators over sequences, including `iter.Seq[V any]` and `iter.Seq2[K, V any]` types.
Applied to files:
openmeter/notification/consumer/entitlementbalancethreshold.go
🧬 Code graph analysis (1)
openmeter/notification/consumer/entitlementbalancethreshold.go (3)
api/api.gen.go (7)
Entitlement(2551-2553)EntitlementValue(2963-2978)NotificationRuleBalanceThresholdValueTypeNumber(549-549)NotificationRuleBalanceThresholdValueTypeUsageValue(552-552)NotificationRuleBalanceThresholdValueTypePercent(550-550)NotificationRuleBalanceThresholdValueTypeUsagePercentage(551-551)NotificationRuleBalanceThresholdValueTypeBalanceValue(548-548)openmeter/notification/entitlements.go (2)
BalanceThreshold(43-43)EventTypeBalanceThreshold(11-11)openmeter/notification/event.go (1)
ListEventsInput(67-88)
⏰ 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: Code Generators
- GitHub Check: Lint
- GitHub Check: Test
- GitHub Check: Build
- GitHub Check: Migration Checks
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
openmeter/notification/consumer/entitlementbalancethreshold.go (1)
8-8: iter usage LGTMUsing iter.Seq2 in Iter() cleanly models the two-candidate enumeration and pairs well with range-over-func. No action needed.
Note: Leveraging team’s learning that iter is available with newer Go toolchains. Thanks for keeping this consistent.
chrisgacsal
force-pushed
the
refactor/notifications
branch
from
028fe20 to
06c6c9d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (11)
openmeter/notification/consumer/entitlementbalancethreshold_test.go (4)
56-415: Great table coverage; add a couple of clarifications and one more edge-case
- Coverage is solid across deprecated and new types, overage, exact matches, and mixed-type prioritization.
- Two minor naming nits: the following case titles look swapped:
- Line 264: "Last threshold" uses NUMBER (deprecated).
- Line 278: "Last threshold (deprecated)" uses UsageValue (new).
- Consider adding 1 more case to assert that a BalanceValue and a Usage* threshold can both be active simultaneously and are returned in their respective slots (already implicitly covered, but an explicit case with equal tie values helps guard regressions).
Would you like me to push a tiny rename diff and an extra test vector?
417-424: Assert message could include the actual thresholds for faster triageCurrent "must be equal" is terse. Including both expected/actual thresholds in the message accelerates failure diagnosis in CI.
- assert.Equalf(t, test.Expected, actual, "must be equal") + assert.Equalf(t, test.Expected, actual, "thresholds mismatch\nexpected=%+v\nactual=%+v", test.Expected, actual)
437-609: Dedup hash tests are good; add a BalanceValue case to cement semantics
- Nice to see both V1 (sha256) and V2 (xxh3/128) asserted.
- The "Balance threshold" case actually uses a UsageValue threshold. Either rename it to "Usage threshold" or add a real BalanceValue case to ensure the hash deliberately ignores threshold type (as designed).
Add this additional test case (expected hashes identical to existing ones):
@@ }{ + { + Name: "BalanceValue threshold", + Snapshot: tests[0].Snapshot, // reuse same snapshot + RuleID: "01K3K3JFZ9M36YD287YGAMZX5X", + Threshold: newBalanceValueThreshold(10.0), + ExpectedV1: "bsnap_v1_b8a36575f7f70393b3b23a032b7acf17202f767f11f061bb17815fd04e7ca24a", + ExpectedV2: "bsnap_v2_787b64062d91d73166e0ea8993947741", + }, }
518-521: Inconsistent period in Snapshot.CurrentUsagePeriod (To < From)Both cases set CurrentUsagePeriod.To to 2021-09-29 while From is 2025-08-25. It doesn’t affect the hash (which uses Entitlement.CurrentUsagePeriod), but it’s confusing.
- To: MustParseISOTime(t, "2021-09-29T12:00:00Z"), + To: MustParseISOTime(t, "2025-09-25T15:32:00Z"),Also applies to: 599-602
openmeter/notification/consumer/entitlementbalancethreshold.go (7)
81-84: Defensive: clarify preconditions or guard nil ValuehandleRule assumes balSnapshot.Value and its inner pointers are non-nil. Upstream code typically guarantees this, but attaching a brief precondition comment (or an early return using isBalanceThresholdEvent) would make the contract explicit and future-proof.
99-116: Use normalized period from the dedup-hash to avoid potential nil dereferenceListEvents uses balSnapshot.Entitlement.CurrentUsagePeriod directly; if it’s ever nil, this panics. You already normalize the period inside NewBalanceEventDedupHash. Prefer using that normalized period here.
- From: balSnapshot.Entitlement.CurrentUsagePeriod.From, - To: balSnapshot.Entitlement.CurrentUsagePeriod.To, + From: dedupHash.currentUsagePeriodFrom, + To: dedupHash.currentUsagePeriodTo,
154-160: Prefer WarnContext and add context fields for observabilityAlign with ErrorContext and enrich logs with rule/threshold/hash context.
- b.Logger.Warn("no balance available skipping event creation", "last_event_id", lastEvent.ID) + b.Logger.WarnContext( + ctx, + "no balance available; skipping this balance threshold candidate", + slog.String("last_event_id", lastEvent.ID), + slog.String("rule_id", rule.ID), + slog.String("dedup_v1", dedupHash.V1()), + slog.String("dedup_v2", dedupHash.V2()), + slog.String("threshold.type", string(threshold.Type)), + slog.Float64("threshold.value", threshold.Value), + )
165-171: Threshold equality is a strict struct compare; consider tolerant float compareIf a last event’s threshold value was serialized/deserialized through JSON, minor float drifts can cause inequality. Comparing on (Type, Value within epsilon) avoids spurious re-emits.
- if lastEventActualValue.BalanceThreshold != *threshold { + if !equalThresholds(lastEventActualValue.BalanceThreshold, *threshold, 1e-9) { // The last event was triggered by a different threshold, so we need to trigger a new event err = b.createEvent(ctx, createEventInput) if err != nil { return fmt.Errorf("failed to create event: %w", err) } }Add alongside helpers:
func equalThresholds(a, b notification.BalanceThreshold, eps float64) bool { if a.Type != b.Type { return false } // For Balance/Usage values, compare with tolerance if a.Value == b.Value { return true } d := a.Value - b.Value if d < 0 { d = -d } return d <= eps }
314-360: Memoization ineffective: V1/V2 have value receiversThe intent is to cache the computed strings in v1/v2, but value receivers assign on a copy. Switch to pointer receivers so the memoized values persist across calls.
-func (d BalanceEventDedupHash) V1() string { +func (d *BalanceEventDedupHash) V1() string { if d.v1 != nil { return *d.v1 } @@ - d.v1 = lo.ToPtr(fmt.Sprintf("bsnap_v1_%x", h.Sum(nil))) + d.v1 = lo.ToPtr(fmt.Sprintf("bsnap_v1_%x", h.Sum(nil))) return *d.v1 } -func (d BalanceEventDedupHash) V2() string { +func (d *BalanceEventDedupHash) V2() string { if d.v2 != nil { return *d.v2 } @@ - d.v2 = lo.ToPtr(fmt.Sprintf("bsnap_v2_%x", xxh3.HashString128(source).Bytes())) + d.v2 = lo.ToPtr(fmt.Sprintf("bsnap_v2_%x", xxh3.HashString128(source).Bytes())) return *d.v2 }
373-417: Improve error context for unknown threshold typesThe default case returns a generic error. Including the actual type string helps diagnostics.
- default: - return nil, errors.New("unknown threshold type") + default: + return nil, fmt.Errorf("unknown threshold type: %s", threshold.Type)
424-434: Simplify iterator with rangeFunctionally identical, more idiomatic.
func (a activeThresholds) Iter() iter.Seq2[int, *notification.BalanceThreshold] { thresholds := []*notification.BalanceThreshold{a.Usage, a.Balance} return func(yield func(int, *notification.BalanceThreshold) bool) { - for i := 0; i <= len(thresholds)-1; i++ { - if !yield(i, thresholds[i]) { + for i, th := range thresholds { + if !yield(i, th) { return } } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
openmeter/notification/consumer/entitlementbalancethreshold.go(5 hunks)openmeter/notification/consumer/entitlementbalancethreshold_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-26T10:30:37.866Z
Learnt from: chrisgacsal
PR: openmeterio/openmeter#3269
File: openmeter/notification/consumer/entitlementbalancethreshold.go:8-8
Timestamp: 2025-08-26T10:30:37.866Z
Learning: The `iter` package is part of the Go standard library starting from Go 1.23, providing basic definitions for working with iterators over sequences, including `iter.Seq[V any]` and `iter.Seq2[K, V any]` types.
Applied to files:
openmeter/notification/consumer/entitlementbalancethreshold.go
🧬 Code graph analysis (2)
openmeter/notification/consumer/entitlementbalancethreshold_test.go (5)
openmeter/entitlement/snapshot/event.go (1)
SnapshotEvent(58-72)openmeter/entitlement/entitlement.go (1)
GenericProperties(320-342)pkg/models/model.go (2)
NamespacedModel(204-206)ManagedModel(119-125)pkg/timeutil/closedperiod.go (1)
ClosedPeriod(8-11)openmeter/notification/consumer/entitlementbalancethreshold.go (1)
NewBalanceEventDedupHash(289-312)
openmeter/notification/consumer/entitlementbalancethreshold.go (6)
api/api.gen.go (9)
Entitlement(2551-2553)NotificationRuleBalanceThresholdValueTypeBalanceValue(548-548)NotificationRuleBalanceThresholdValueTypePercent(550-550)NotificationRuleBalanceThresholdValueTypeNumber(549-549)NotificationRuleBalanceThresholdValueTypeUsagePercentage(551-551)NotificationRuleBalanceThresholdValueTypeUsageValue(552-552)Subject(6323-6348)Feature(3000-3029)EntitlementValue(2963-2978)openmeter/notification/entitlements.go (2)
BalanceThreshold(43-43)EventTypeBalanceThreshold(11-11)openmeter/notification/event.go (1)
ListEventsInput(67-88)openmeter/entitlement/snapshot/event.go (1)
SnapshotEvent(58-72)pkg/timeutil/closedperiod.go (1)
ClosedPeriod(8-11)openmeter/subject/subject.go (1)
Subject(8-16)
⏰ 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: Quickstart
- GitHub Check: E2E
- GitHub Check: Lint
- GitHub Check: Test
- GitHub Check: Code Generators
- GitHub Check: Build
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
openmeter/notification/consumer/entitlementbalancethreshold_test.go (2)
21-55: Helper constructors read well and cover all threshold variantsThe helpers make the test tables concise and clearly map deprecated vs. new types.
426-435: MustParseISOTime is fineHelper is straightforward; keeping it local to the test file is appropriate.
openmeter/notification/consumer/entitlementbalancethreshold.go (3)
86-91: Good: skip nil candidates to allow Balance-only/Usage-only scenariosThis addresses the earlier early-return bug and ensures both candidates are evaluated.
261-272: Explicit mapping is clear; keep itThe ThresholdKind mapping is straightforward and aligns deprecated and new types under "usage".
301-312: Constructor uses measured/normalized period and measureUsageFrom—goodStoring normalized period and measureUsageFrom in the hash inputs provides stable dedup across timezones.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
openmeter/notification/consumer/entitlementbalancethreshold.go (1)
86-90: Nice fix: skip nil candidates instead of exitingContinuing when a candidate is nil ensures Balance-only scenarios aren’t skipped. Matches the intent from the earlier discussion.
🧹 Nitpick comments (6)
openmeter/notification/consumer/entitlementbalancethreshold.go (6)
99-116: Guard against nil CurrentUsagePeriod when building the last-events query
balSnapshot.Entitlement.CurrentUsagePeriodis a pointer. Here it’s dereferenced directly forFrom/To. Unless you can guarantee it’s always populated for every snapshot handled by this code path, this can panic. You already uselo.FromPtrOrfor the same field in the dedup hash constructor—do the same here.Apply this diff to make the query resilient:
- // TODO[issue-1364]: this must be cached to prevent going to the DB for each balance.snapshot event - lastEvents, err := b.Notification.ListEvents(ctx, notification.ListEventsInput{ + // TODO[issue-1364]: this must be cached to prevent going to the DB for each balance.snapshot event + period := lo.FromPtrOr(balSnapshot.Entitlement.CurrentUsagePeriod, timeutil.ClosedPeriod{}) + lastEvents, err := b.Notification.ListEvents(ctx, notification.ListEventsInput{ Page: pagination.Page{ PageSize: 1, PageNumber: 1, }, Namespaces: []string{balSnapshot.Namespace.ID}, - - From: balSnapshot.Entitlement.CurrentUsagePeriod.From, - To: balSnapshot.Entitlement.CurrentUsagePeriod.To, + From: period.From, + To: period.To, DeduplicationHashes: []string{dedupHash.V1(), dedupHash.V2()}, OrderBy: notification.OrderByCreatedAt, Order: sortx.OrderDesc, })Also applies to: 107-109
94-116: Dedupe hash v2 distinguishes kind (usage vs balance) but not threshold identity; confirm intended semanticsRight now V2 uses only the threshold “kind” (usage/balance), not the specific value/type. Within a period, multiple usage thresholds (e.g., 50%, 80%, 90%) will all share the same dedupe hash. The code compensates by emitting a new event when the last event’s stored threshold differs, but downstream systems that rely on
HandlerDeduplicationHashfor de-duping/idempotency may collapse or otherwise conflate those events.If the intent is “one event per threshold per period,” include the threshold identity in the V2 hash (e.g., kind + type + value). If the intent is “one event per kind per period,” this is fine, but please confirm CreateEvent/delivery semantics so a second, valid event doesn’t get suppressed.
Minimal change to make V2 per-threshold while preserving V1 compatibility:
@@ type BalanceEventDedupHash struct { @@ thresholdKind ThresholdKind + // thresholdKey makes V2 per-threshold (kind+type+value) + thresholdKey string @@ } @@ func NewBalanceEventDedupHash(snapshot snapshot.SnapshotEvent, ruleID string, threshold notification.BalanceThreshold) (*BalanceEventDedupHash, error) { @@ - thresholdKind, err := thresholdKindFromThreshold(threshold) + thresholdKind, err := thresholdKindFromThreshold(threshold) if err != nil { return nil, err } @@ return &BalanceEventDedupHash{ @@ thresholdKind: thresholdKind, + thresholdKey: fmt.Sprintf("%s/%s:%g", thresholdKind, threshold.Type, threshold.Value), @@ }, nil } @@ -func (d BalanceEventDedupHash) V2() string { +func (d *BalanceEventDedupHash) V2() string { if d.v2 != nil { return *d.v2 } - source := strings.Join([]string{ - string(d.thresholdKind), + source := strings.Join([]string{ + d.thresholdKey, d.notificationRuleID, d.namespaceID, d.currentUsagePeriodFrom.UTC().Format(time.RFC3339), d.currentUsagePeriodTo.UTC().Format(time.RFC3339), d.subjectKey, d.entitlementID, d.featureID, d.measureUsageFrom.UTC().Format(time.RFC3339), - }, "") + }, "/") d.v2 = lo.ToPtr(fmt.Sprintf("bsnap_v2_%x", xxh3.HashString128(source).Bytes())) return *d.v2 }If you prefer to keep “one per kind,” consider renaming the annotation to reflect that scope and add a test asserting that multiple usage thresholds within a period are intended to share the same dedupe hash.
Also applies to: 118-123, 125-133, 137-148, 150-171
154-160: Use WarnContext for consistency and preserve request context in logsThe other log here uses
ErrorContext. Use the context-aware variant for warnings too.- b.Logger.Warn("no balance available skipping event creation", "last_event_id", lastEvent.ID) + b.Logger.WarnContext(ctx, "no balance available; skipping event creation for this candidate", + slog.String("last_event_id", lastEvent.ID))
314-339: Make V1/V2 methods pointer receivers so the memoization actually works; also add a delimiter in V2 to avoid ambiguous concatenationsBoth
V1()andV2()cache intod.v1/d.v2, but the receiver is by value, so the cached pointers are discarded on return. Switch to pointer receivers to benefit from memoization. While at it, use a delimiter in the V2sourceto avoid accidental collisions from concatenation without separators.Apply this diff:
-func (d BalanceEventDedupHash) V1() string { +func (d *BalanceEventDedupHash) V1() string { if d.v1 != nil { return *d.v1 } @@ - d.v1 = lo.ToPtr(fmt.Sprintf("bsnap_v1_%x", h.Sum(nil))) + d.v1 = lo.ToPtr(fmt.Sprintf("bsnap_v1_%x", h.Sum(nil))) return *d.v1 } -func (d BalanceEventDedupHash) V2() string { +func (d *BalanceEventDedupHash) V2() string { if d.v2 != nil { return *d.v2 } - source := strings.Join([]string{ - string(d.thresholdKind), + source := strings.Join([]string{ + string(d.thresholdKind), d.notificationRuleID, d.namespaceID, d.currentUsagePeriodFrom.UTC().Format(time.RFC3339), d.currentUsagePeriodTo.UTC().Format(time.RFC3339), d.subjectKey, d.entitlementID, d.featureID, d.measureUsageFrom.UTC().Format(time.RFC3339), - }, "") + }, "/") d.v2 = lo.ToPtr(fmt.Sprintf("bsnap_v2_%x", xxh3.HashString128(source).Bytes())) return *d.v2 }Also applies to: 341-361
425-434: Slightly simplify iterator implementationUse a
rangeover the local slice for clarity.func (a activeThresholds) Iter() iter.Seq2[int, *notification.BalanceThreshold] { thresholds := []*notification.BalanceThreshold{a.Usage, a.Balance} return func(yield func(int, *notification.BalanceThreshold) bool) { - for i := 0; i <= len(thresholds)-1; i++ { - if !yield(i, thresholds[i]) { - return - } - } + for i, t := range thresholds { + if !yield(i, t) { + return + } + } } }
437-499: Selection policy for “highest priority” thresholds looks sound; small consistency tweak optional
- BalanceValue: picking the smallest active balance threshold first makes sense.
- UsagePercentage: picking the largest active threshold first is consistent.
- UsageValue: uses
<vs.<=used for percentage; consider using the same comparator unless you intentionally want a tie-breaker difference.No change required unless you want comparator consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
openmeter/notification/consumer/entitlementbalancethreshold.go(5 hunks)openmeter/notification/consumer/entitlementbalancethreshold_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- openmeter/notification/consumer/entitlementbalancethreshold_test.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-26T10:30:37.866Z
Learnt from: chrisgacsal
PR: openmeterio/openmeter#3269
File: openmeter/notification/consumer/entitlementbalancethreshold.go:8-8
Timestamp: 2025-08-26T10:30:37.866Z
Learning: The `iter` package is part of the Go standard library starting from Go 1.23, providing basic definitions for working with iterators over sequences, including `iter.Seq[V any]` and `iter.Seq2[K, V any]` types.
Applied to files:
openmeter/notification/consumer/entitlementbalancethreshold.go
🧬 Code graph analysis (1)
openmeter/notification/consumer/entitlementbalancethreshold.go (7)
api/api.gen.go (9)
Entitlement(2551-2553)NotificationRuleBalanceThresholdValueTypeBalanceValue(548-548)NotificationRuleBalanceThresholdValueTypePercent(550-550)NotificationRuleBalanceThresholdValueTypeNumber(549-549)NotificationRuleBalanceThresholdValueTypeUsagePercentage(551-551)NotificationRuleBalanceThresholdValueTypeUsageValue(552-552)Subject(6323-6348)Feature(3000-3029)EntitlementValue(2963-2978)openmeter/notification/entitlements.go (2)
BalanceThreshold(43-43)EventTypeBalanceThreshold(11-11)openmeter/notification/event.go (1)
ListEventsInput(67-88)pkg/pagination/pagination.go (1)
Page(18-21)openmeter/entitlement/snapshot/event.go (1)
SnapshotEvent(58-72)pkg/timeutil/closedperiod.go (1)
ClosedPeriod(8-11)openmeter/subject/subject.go (1)
Subject(8-16)
⏰ 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). (3)
- GitHub Check: E2E
- GitHub Check: Quickstart
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
openmeter/notification/consumer/entitlementbalancethreshold.go (2)
67-71: Good error wrapping with contextual fieldsThe enriched error adds namespace, rule ID, and entitlement ID, which will simplify triage. No changes requested.
81-84: Active-threshold computation call looks correctThe separation of threshold activation logic into
getActiveThresholdsWithHighestPriorityimproves readability and testability. The error wrapping is appropriate.
chrisgacsal
force-pushed
the
refactor/notifications
branch
from
06c6c9d to
2ebcbd7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
openmeter/notification/consumer/entitlementbalancethreshold.go (1)
86-90: Correct: skip nil candidate instead of returningChanging early return to continue ensures Balance-only scenarios aren’t skipped when the Usage candidate is absent. This prevents false negatives during iteration.
🧹 Nitpick comments (13)
api/spec/src/notification/entitlements.tsp (3)
138-144: Clarify semantics, units, and valid ranges in the docsGood expansion. To reduce ambiguity for API consumers, consider explicitly stating units and trigger semantics (>= vs <=) and the valid range for percentages.
Apply this doc-only diff to append clarifications:
/** * Type of the rule in the balance threshold specification: * * `balance_value`: threshold defined by the remaining balance value based on usage and the total of grants in the current usage period * * `usage_percentage`: threshold defined by the usage percentage compared to the total of grants in the current usage period * * `usage_value`: threshold defined by the usage value in the current usage period * * `NUMBER` (**deprecated**): see `usage_value` * * `PERCENT` (**deprecated**): see `usage_percentage` + * + * Notes: + * * `usage_percentage` is expressed on a 0–100 scale (inclusive). Decimals are allowed. + * * `usage_value` and `balance_value` must be non-negative and use the same unit as the feature’s meter. + * * Trigger semantics: + * - `usage_value`/`usage_percentage`: triggers when usage crosses the threshold upward (>=). + * - `balance_value`: triggers when remaining balance crosses the threshold downward (<=). + * If your consumer logic differs, please adjust this description accordingly. */
151-157: Mark legacy enum members as deprecated in TypeSpecKeeping
PERCENTandNUMBERensures backward compatibility. To surface deprecation in docs and generated clients that consume TypeSpec directly, annotate the legacy members with@deprecated.Apply this diff:
enum NotificationRuleBalanceThresholdValueType { - PERCENT, - NUMBER, + @deprecated("Deprecated. Use 'usage_percentage' instead.") + PERCENT, + @deprecated("Deprecated. Use 'usage_value' instead.") + NUMBER, balance_value, usage_percentage, usage_value, }
121-135: Optional: model type-specific value constraints via a discriminated unionRight now,
value: float64is uniform across all types. If you want the schema to enforce ranges (e.g.,0..100forusage_percentage, non-negative for others), consider a discriminated union keyed bytype. This tightens validation and improves generated docs without changing wire format.Example sketch:
union NotificationRuleBalanceThresholdValue { BalanceValue: { value: float64 & @minValue(0); type: "balance_value"; }; UsageValue: { value: float64 & @minValue(0); type: "usage_value"; }; UsagePercentage: { value: float64 & @minValue(0) & @maxValue(100); type: "usage_percentage"; }; // Deprecated aliases (optional): DeprecatedNumber: { @deprecated("Use 'usage_value' instead.") value: float64 & @minValue(0); type: "NUMBER"; }; DeprecatedPercent: { @deprecated("Use 'usage_percentage' instead.") value: float64 & @minValue(0) & @maxValue(100); type: "PERCENT"; }; }If this is too heavy for now, adding the doc clarifications in the previous comment is a good middle ground.
api/client/javascript/src/client/schemas.ts (2)
7613-7616: Clarify units and percentage representation in the JSDocTo reduce ambiguity for API consumers, explicitly state the unit for value-based types and how percentages should be expressed (75 vs 0.75). If this file is generated, please update the OpenAPI/TypeSpec descriptions and regenerate; the diff below is illustrative.
/** * @description Type of the rule in the balance threshold specification: - * * `balance_value`: threshold defined by the remaining balance value based on usage and the total of grants in the current usage period - * * `usage_percentage`: threshold defined by the usage percentage compared to the total of grants in the current usage period - * * `usage_value`: threshold defined by the usage value in the current usage period + * * `balance_value`: threshold defined by the remaining balance value (same unit as the entitlement) based on usage and the total of grants in the current usage period + * * `usage_percentage`: threshold defined by the usage percentage compared to the total of grants in the current usage period (e.g., 75 means 75%, not 0.75) + * * `usage_value`: threshold defined by the usage value in the current usage period (same unit as the entitlement) * * `NUMBER` (**deprecated**): see `usage_value` * * `PERCENT` (**deprecated**): see `usage_percentage` */
7621-7626: Deprecate legacy enum values and reorder for codegenThe generated TypeSpec and OpenAPI schemas currently list
PERCENTandNUMBERfirst, with no deprecation metadata. To guide clients away from these legacy values and highlight the preferred options, please:
In api/spec/src/notification/entitlements.tsp
• Reorder the enum members to list the preferred values first (balance_value,usage_percentage,usage_value), then the legacy ones.
• Add@deprecatedJSDoc comments toPERCENTandNUMBER.enum NotificationRuleBalanceThresholdValueType {PERCENT,
NUMBER,
- /** @deprecated Use
usage_percentageinstead. */- PERCENT,
- /** @deprecated Use
usage_valueinstead. */- NUMBER,
- balance_value,
- usage_percentage,
- usage_value,
}- In **api/openapi.yaml** (and **api/openapi.cloud.yaml**) • Reorder the `enum` array so the three recommended values come first. • Add an `x-enum-deprecated` vendor extension for the legacy entries. ```diff NotificationRuleBalanceThresholdValueType: type: string - enum: [PERCENT, NUMBER, balance_value, usage_percentage, usage_value] + enum: [balance_value, usage_percentage, usage_value, NUMBER, PERCENT] x-enum-varnames: [BalanceValue, UsagePercentage, UsageValue, NUMBER, PERCENT] + x-enum-deprecated: + NUMBER: true + PERCENT: true
- In api/client/javascript/src/client/schemas.ts
No manual edits needed—regenerate the client after updating the OpenAPI spec to pick up the new ordering.Please also audit the codebase for any hard-coded uses of the legacy values outside of BC shims or tests. For example:
rg -nP '\b(PERCENT|NUMBER)\b' --glob '!**/{test,tests,examples}/**'Ensure these occurrences are only in backward-compatibility layers.
api/openapi.cloud.yaml (2)
19462-19474: Surface deprecation status at the enum-member levelOpenAPI doesn’t natively support per-enum deprecation, but several generators (openapi-generator, swagger-codegen) honor x-enum-deprecated and x-enum-descriptions. Adding these will propagate deprecation markers into SDKs and docs, reducing confusion during the migration period.
Apply this diff under the enum to annotate deprecated members and provide per-member docs (arrays must align 1:1 with enum order):
enum: - PERCENT - NUMBER - balance_value - usage_percentage - usage_value + x-enum-deprecated: + - true # PERCENT + - true # NUMBER + - false # balance_value + - false # usage_percentage + - false # usage_value + x-enum-descriptions: + - "Deprecated: use usage_percentage." + - "Deprecated: use usage_value." + - "Threshold defined by remaining balance value based on usage and the total of grants in the current usage period." + - "Threshold defined by the usage percentage compared to the total of grants in the current usage period." + - "Threshold defined by the usage value in the current usage period."
19467-19474: Minor copy tweak to reduce ambiguity (optional)“Type of the rule in the balance threshold specification” could be read as the rule type rather than the value type. Consider explicitly saying “Type of the threshold value”. Not blocking.
- Type of the rule in the balance threshold specification: + Type of the threshold value in the balance threshold specification:openmeter/notification/consumer/entitlementbalancethreshold.go (4)
137-148: Enrich log context when last event is not a balance-threshold eventMinor observability nit: include rule ID and dedupe hashes in the error log to speed up triage in case of data inconsistencies.
- b.Logger.ErrorContext(ctx, "last event is not a balance threshold event", slog.String("event.id", lastEvent.ID)) + b.Logger.ErrorContext( + ctx, + "last event is not a balance threshold event", + slog.String("event.id", lastEvent.ID), + slog.String("rule.id", rule.ID), + slog.String("dedup.v1", dedupHash.V1()), + slog.String("dedup.v2", dedupHash.V2()), +)
150-160: Use WarnContext and add structured fields for the no-balance caseThe warning currently drops context. Prefer WarnContext and include identifiers to make log lines actionable.
- b.Logger.Warn("no balance available skipping event creation", "last_event_id", lastEvent.ID) + b.Logger.WarnContext( + ctx, + "no balance available; skipping event creation for this candidate", + slog.String("last_event_id", lastEvent.ID), + slog.String("rule.id", rule.ID), + slog.String("dedup.v2", dedupHash.V2()), +)
346-359: Low collision risk, but prefer explicit separators for V2 hash inputConcatenating components without delimiters can, in theory, create ambiguous inputs. Using a non-printable delimiter (or “/” like V1) reduces the risk without changing semantics.
- source := strings.Join([]string{ + source := strings.Join([]string{ string(d.thresholdKind), d.notificationRuleID, d.namespaceID, d.currentUsagePeriodFrom.UTC().Format(time.RFC3339), d.currentUsagePeriodTo.UTC().Format(time.RFC3339), d.subjectKey, d.entitlementID, d.featureID, d.measureUsageFrom.UTC().Format(time.RFC3339), -}, "") +}, "/")Note: Adopting this would require updating the golden strings in tests. If you intend to keep the existing V2 values stable for compatibility, consider leaving as-is and adding a comment explaining the choice.
437-499: Tie-breaker behavior is implicit; add a short comment and keep testsThe <= comparison for percentage vs < for value ensures usage_percentage wins when the computed numeric thresholds are equal. Good that tests cover this. Consider adding a brief comment in code to document the intended tie-breaker.
- } else if usage.ThresholdValue <= numThreshold.ThresholdValue { + // Prefer percentage over value when the computed numeric threshold is equal + } else if usage.ThresholdValue <= numThreshold.ThresholdValue {openmeter/notification/consumer/entitlementbalancethreshold_test.go (2)
198-231: Clarity: test name “Mixed number and percentage thresholds (deprecated)” vs. “Mixed number and percentage thresholds”The two cases are easy to confuse. Consider appending the exact types in the name (e.g., “deprecated NUMBER+PERCENT” vs “usage_value+usage_percentage”) to aid future maintenance.
447-608: Misleading test name and missing BalanceValue coverage in dedup-hash tests
- The case named “Balance threshold” actually passes a UsageValue threshold. Rename to avoid confusion.
- Add a third case that uses BalanceValue to assert V2 differs when ThresholdKind changes (by design, V2 includes the kind).
Proposed adjustments:
- { - Name: "Balance threshold", + { + Name: "Usage threshold", ... - Threshold: newUsageValueThreshold(100.0), + Threshold: newUsageValueThreshold(100.0), ExpectedV1: "bsnap_v1_b8a36575f7f70393b3b23a032b7acf17202f767f11f061bb17815fd04e7ca24a", ExpectedV2: "bsnap_v2_521777eff8de3eb39301c492ff75620e", }, + { + Name: "Balance threshold (BalanceValue changes V2 kind)", + Snapshot: /* reuse the same snapshot as above for determinism */, + RuleID: "01K3K3JFZ9M36YD287YGAMZX5X", + Threshold: newBalanceValueThreshold(10.0), + ExpectedV1: "bsnap_v1_b8a36575f7f70393b3b23a032b7acf17202f767f11f061bb17815fd04e7ca24a", + // ExpectedV2 should differ from the usage-kind hash; compute once and freeze the value. + ExpectedV2: "<fill with computed value>", + },If you’d like, I can run a quick helper to compute the new ExpectedV2 from the same inputs so we can lock the golden.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
api/client/javascript/src/client/schemas.ts(1 hunks)api/client/javascript/src/zod/index.ts(2 hunks)api/openapi.cloud.yaml(1 hunks)api/openapi.yaml(1 hunks)api/spec/src/notification/entitlements.tsp(1 hunks)openmeter/notification/consumer/balancethreshold_test.go(0 hunks)openmeter/notification/consumer/entitlementbalancethreshold.go(5 hunks)openmeter/notification/consumer/entitlementbalancethreshold_test.go(1 hunks)openmeter/notification/entitlements.go(2 hunks)
💤 Files with no reviewable changes (1)
- openmeter/notification/consumer/balancethreshold_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- openmeter/notification/entitlements.go
- api/client/javascript/src/zod/index.ts
- api/openapi.yaml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-26T10:30:37.866Z
Learnt from: chrisgacsal
PR: openmeterio/openmeter#3269
File: openmeter/notification/consumer/entitlementbalancethreshold.go:8-8
Timestamp: 2025-08-26T10:30:37.866Z
Learning: The `iter` package is part of the Go standard library starting from Go 1.23, providing basic definitions for working with iterators over sequences, including `iter.Seq[V any]` and `iter.Seq2[K, V any]` types.
Applied to files:
openmeter/notification/consumer/entitlementbalancethreshold.go
🧬 Code graph analysis (2)
openmeter/notification/consumer/entitlementbalancethreshold_test.go (4)
openmeter/notification/entitlements.go (1)
BalanceThreshold(43-43)openmeter/entitlement/snapshot/event.go (1)
SnapshotEvent(58-72)pkg/timeutil/closedperiod.go (1)
ClosedPeriod(8-11)openmeter/notification/consumer/entitlementbalancethreshold.go (1)
NewBalanceEventDedupHash(289-312)
openmeter/notification/consumer/entitlementbalancethreshold.go (4)
api/api.gen.go (9)
Entitlement(2551-2553)NotificationRuleBalanceThresholdValueTypeBalanceValue(548-548)NotificationRuleBalanceThresholdValueTypePercent(550-550)NotificationRuleBalanceThresholdValueTypeNumber(549-549)NotificationRuleBalanceThresholdValueTypeUsagePercentage(551-551)NotificationRuleBalanceThresholdValueTypeUsageValue(552-552)Subject(6323-6348)Feature(3000-3029)EntitlementValue(2963-2978)openmeter/notification/event.go (1)
ListEventsInput(67-88)pkg/pagination/pagination.go (1)
Page(18-21)openmeter/entitlement/snapshot/event.go (1)
SnapshotEvent(58-72)
⏰ 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). (8)
- GitHub Check: Artifacts / Benthos Collector Container image
- 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 (12)
api/spec/src/notification/entitlements.tsp (2)
133-135: Example updated to new type — LGTMSwitching the example to
usage_valuealigns with the rename and helps steer new integrations toward the non-deprecated values.
147-150: Usex-deprecated(notx-enum-deprecated) to mark deprecated enum valuesOpenAPI Generator only recognizes
x-enum-varnamesandx-enum-descriptionsout of the box, but recent versions also support a customx-deprecatedextension (a list of enum values to deprecate) for marking specific enum items with@Deprecatedin generated Java code (bookstack.cn, github.com). There is no built-in support for anx-enum-deprecatedboolean list.Apply this diff instead:
@extension( "x-enum-varnames", #["Percent", "Number", "BalanceValue", "UsagePercentage", "UsageValue"] ) +@extension( + "x-deprecated", + #["Percent", "Number"] +)Please verify your OpenAPI toolchain (OpenAPI Generator and any SDK generators in CI) supports the
x-deprecatedextension. If it doesn’t, you’ll either need to drop it or customize your templates.Likely an incorrect or invalid review comment.
api/client/javascript/src/client/schemas.ts (1)
7613-7618: Enum rename + new type are clear and backward-compatibleDocumentation bullets correctly explain the new semantics, and the union keeps deprecated values to avoid breaking consumers. The example switched to usage_value, which aligns with the new guidance. Looks good.
Also applies to: 7621-7626
api/openapi.cloud.yaml (2)
19457-19457: Enum expansion and example update look goodThe new values (balance_value, usage_percentage, usage_value) are added without removing the legacy ones (NUMBER, PERCENT). The example is aligned to the new naming. x-enum-varnames order matches enum order, which many generators rely on. Good backward-compat move.
Also applies to: 19462-19466, 19478-19480
19454-19458: Examples & Descriptions Confirmed in JavaScript Client
- The enum definition for
NotificationRuleBalanceThresholdValueTypeis identical inapi/openapi.yamlandapi/openapi.cloud.yaml, so there’s no drift in the source specs.- In the generated JavaScript client, both the
example: usage_valueand the accompanying description are preserved:
- In
api/client/javascript/src/zod/index.ts(lines 9857–9864), you can see the three values (balance_value,usage_percentage,usage_value) and the full documentation comment survived the allOf resolution.- In
api/client/javascript/src/client/schemas.ts(lines 7607–7617), the@example usage_valueannotation and the detailed bullet-list description are intact.No further changes are required for the JavaScript client; your pipeline correctly handles allOf siblings here. If you also generate other language clients or run a documentation renderer, please double-check those outputs—if any drop the sibling fields, consider inlining this schema or moving the example/description into the referenced component schema.
openmeter/notification/consumer/entitlementbalancethreshold.go (3)
67-71: Good contextual error wrapping for per-rule failuresWrapping rule-handling errors with namespace, rule ID, and entitlement ID materially improves diagnosability.
99-116: Listing last events with both V1 and V2 dedup hashes is a solid migration strategyQuerying with both versions preserves backward compatibility while writing new events with V2. Nicely done.
374-418: Threshold normalization and activation conditions look correct
- usage_value/NUMBER and usage_percentage/PERCENT compute thresholds against usage as expected.
- balance_value activates when balance is below or equal to the configured value, which matches the new semantics.
openmeter/notification/consumer/entitlementbalancethreshold_test.go (4)
56-415: Broad, high-signal coverage for active-threshold selectionGreat table-driven tests across deprecated and new types, including overage and edge cases. These will prevent regressions in the selection logic.
292-305: Intentional tie-breaker covered — good“Same threshold in percentage and number (deprecated)” validates the designed preference for percentage. Nice.
337-351: Duplicate-vs-equal scenario covered — good“Exact threshold match” with both percent and usage_value present confirms the intended winner. Solid.
7-19: Import hygiene: two different models packages in tests can be confusingYou import both pkg/models (as pkgmodels) and openmeter/openmeter/event/models (as models). If both export NamespaceID et al., ambiguity can creep in. Verify SnapshotEvent.Namespace uses the correct models package; otherwise consider aliasing the second import (e.g., eventmodels) to avoid misreads.
Would you like me to scan the repo to confirm SnapshotEvent.Namespace’s exact package path and adjust the import/usage accordingly?
3 participants
Overview
In order to resolve confusion around how entitlement balance threshold events work we decided to make the threshold types more descriptive where current types are renamed (while still supporting the old names) to
usage_value(previouslyNUMBER) andusage_percentage(previouslyPERCENT) while adding a new threshold type calledbalance_value.Former two types work the same as before while the new type allows triggering balance notifications based on the remaining balance instead of usage.
The
NUMBERandPERCENTthreshold types are deprecated and will be removed in the future while migrating exiting rules to use the new types.Note
Documenttation will be extended on this topic to provide more detailed explanation how the different threshold types work.
Summary by CodeRabbit
New Features
Deprecations
Documentation