refactor: list entitlements for ingested events#4259
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
Changes
Sequence Diagram(s)sequenceDiagram
participant Worker as Worker (balance worker)
participant Adapter as Entitlement Adapter
participant DB as Database
Worker->>Adapter: ListEntitlementsAffectedByIngestEvents(ns, subject, meters...)
Adapter->>DB: Exec EntitlementsByIngestedEventsQuery(dialect, ns, subject, meters)
DB-->>Adapter: rows stream (entitlement rows)
Adapter->>Adapter: rows.Next()/rows.Scan() -> map to response (normalize UTC)
Adapter-->>Worker: ListAffectedEntitlementsResponse (entitlement IDs + metadata)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
chrisgacsal
force-pushed
the
refactor/entitlement-query
branch
from
ccbb032 to
c30f887
Compare
chrisgacsal
force-pushed
the
refactor/entitlement-query
branch
from
c30f887 to
e285e99
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openmeter/entitlement/adapter/entitlement.go`:
- Around line 324-331: Within the entutils.TransactingRepo closure the raw query
uses the base client (a.db) so the read can escape an existing transaction;
switch both the dialect lookup and the QueryContext call to the
transaction-bound client (use repo.db.GetConfig().Driver.Dialect() and
repo.db.QueryContext(ctx, query, args...)) where
EntitlementsByIngestedEventsQuery and the rows retrieval are performed to ensure
the query runs within the transaction.
- Around line 285-290: The WHERE clause uses sql.In(mt.C(db_meter.FieldKey),
lo.ToAnySlice(meters)...) which generates SQL "IN ($1)" with an array param and
fails on Postgres; replace that predicate with an ANY-style comparison e.g.
sql.EQ(mt.C(db_meter.FieldKey), sql.Any(lo.ToAnySlice(meters))) (update the
Where(sql.And(...)) clause) so the generated SQL becomes "col = ANY($1)" and
accepts the array parameter correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6dd04ba9-9e60-46b1-b642-bc39e644190b
📒 Files selected for processing (2)
openmeter/entitlement/adapter/entitlement.goopenmeter/entitlement/adapter/entitlement_test.go
chrisgacsal
force-pushed
the
refactor/entitlement-query
branch
from
e285e99 to
0402e10
Compare
chrisgacsal
force-pushed
the
refactor/entitlement-query
branch
from
0402e10 to
630b152
Compare
Signed-off-by: Krisztian Gacsal <chrisgacsal@users.noreply.github.com>
chrisgacsal
force-pushed
the
refactor/entitlement-query
branch
from
630b152 to
a8cbfbb
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
openmeter/entitlement/adapter/entitlement.go (1)
324-331:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the transaction-bound client here.
This closure is already inside
entutils.TransactingRepo, but the raw SQL still goes througha.db. That can escape the caller's transaction and read a different snapshot than the rest of the balance-worker flow.Small fix
- query, args := EntitlementsByIngestedEventsQuery( - a.db.GetConfig().Driver.Dialect(), + query, args := EntitlementsByIngestedEventsQuery( + repo.db.GetConfig().Driver.Dialect(), eventFilter.Namespace, eventFilter.EventSubject, eventFilter.MeterSlugs..., ) - rows, err := a.db.QueryContext(ctx, query, args...) + rows, err := repo.db.QueryContext(ctx, query, args...)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/entitlement/adapter/entitlement.go` around lines 324 - 331, The SQL query is executed against a.db which can bypass the caller's transaction from entutils.TransactingRepo; change the call to use the transaction-bound client provided by the transacting repo (e.g., tx or repo provided inside the entutils.TransactingRepo closure) so EntitlementsByIngestedEventsQuery is executed with tx.QueryContext(...) (replace a.db.QueryContext with the transaction client) to ensure the read uses the same transactional snapshot as the balance-worker flow.
🧹 Nitpick comments (1)
openmeter/entitlement/adapter/entitlement_test.go (1)
47-161: ⚡ Quick winThread the test context through setup/helpers.
setupand the helper inserts still start fromcontext.Background(). Usingt.Context()here keeps cancellations and timeouts attached to the test harness instead of letting setup work keep running after a test is already done.Small cleanup
- if err := dbClient.Schema.Create(context.Background()); err != nil { + if err := dbClient.Schema.Create(t.Context()); err != nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/entitlement/adapter/entitlement_test.go` around lines 47 - 161, The tests use context.Background() and context.WithCancel() instead of the test-scoped context, which can let work continue after a test finishes; update setup, createCustomerWithSubject, and createCustomerWithSubjectAndKey to use t.Context() (propagate the test context) for DB migrations and all repository/service calls (replace context.Background() and the context.WithCancel pattern with ctx := t.Context(), and remove the manual cancel where used) so cancellations/timeouts are tied to the test runner.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@openmeter/entitlement/adapter/entitlement.go`:
- Around line 324-331: The SQL query is executed against a.db which can bypass
the caller's transaction from entutils.TransactingRepo; change the call to use
the transaction-bound client provided by the transacting repo (e.g., tx or repo
provided inside the entutils.TransactingRepo closure) so
EntitlementsByIngestedEventsQuery is executed with tx.QueryContext(...) (replace
a.db.QueryContext with the transaction client) to ensure the read uses the same
transactional snapshot as the balance-worker flow.
---
Nitpick comments:
In `@openmeter/entitlement/adapter/entitlement_test.go`:
- Around line 47-161: The tests use context.Background() and
context.WithCancel() instead of the test-scoped context, which can let work
continue after a test finishes; update setup, createCustomerWithSubject, and
createCustomerWithSubjectAndKey to use t.Context() (propagate the test context)
for DB migrations and all repository/service calls (replace context.Background()
and the context.WithCancel pattern with ctx := t.Context(), and remove the
manual cancel where used) so cancellations/timeouts are tied to the test runner.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8ccffff2-a8c6-4296-819c-afb205c310cd
📒 Files selected for processing (2)
openmeter/entitlement/adapter/entitlement.goopenmeter/entitlement/adapter/entitlement_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
openmeter/entitlement/adapter/entitlement.go (1)
319-329: ⚡ Quick winShort-circuit when no meter slugs are provided.
If
eventFilter.MeterSlugsis empty, returning early avoids building/executing a query that cannot produce useful matches.♻️ Suggested change
func (a *entitlementDBAdapter) ListEntitlementsAffectedByIngestEvents(ctx context.Context, eventFilter balanceworker.IngestEventQueryFilter) ([]balanceworker.ListAffectedEntitlementsResponse, error) { return entutils.TransactingRepo[[]balanceworker.ListAffectedEntitlementsResponse, *entitlementDBAdapter]( ctx, a, func(ctx context.Context, repo *entitlementDBAdapter) ([]balanceworker.ListAffectedEntitlementsResponse, error) { + if len(eventFilter.MeterSlugs) == 0 { + return []balanceworker.ListAffectedEntitlementsResponse{}, nil + } + query, args := EntitlementsByIngestedEventsQuery( repo.db.GetConfig().Driver.Dialect(), eventFilter.Namespace, eventFilter.EventSubject, eventFilter.MeterSlugs..., )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/entitlement/adapter/entitlement.go` around lines 319 - 329, In ListEntitlementsAffectedByIngestEvents, add a short-circuit that checks if eventFilter.MeterSlugs is empty and immediately returns an empty []balanceworker.ListAffectedEntitlementsResponse and nil error to avoid building/executing EntitlementsByIngestedEventsQuery with no meter slugs; place the check at the start of the transacting function (or before calling EntitlementsByIngestedEventsQuery) so the function exits early when eventFilter.MeterSlugs has length 0.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openmeter/entitlement/adapter/entitlement.go`:
- Around line 349-360: The CreatedAt timestamp on the scanned entitlement row is
not being normalized to UTC like the other timestamps; after scanning into r
(the entitlement row variable) call r.CreatedAt = convert.SafeToUTC(r.CreatedAt)
alongside the existing r.DeletedAt/ActiveFrom/ActiveTo conversions so all
timestamps use UTC. Update the same block where the scan occurs (the code that
scans into &r.CreatedAt, &r.DeletedAt, &r.ActiveFrom, &r.ActiveTo) to include
this conversion using convert.SafeToUTC.
---
Nitpick comments:
In `@openmeter/entitlement/adapter/entitlement.go`:
- Around line 319-329: In ListEntitlementsAffectedByIngestEvents, add a
short-circuit that checks if eventFilter.MeterSlugs is empty and immediately
returns an empty []balanceworker.ListAffectedEntitlementsResponse and nil error
to avoid building/executing EntitlementsByIngestedEventsQuery with no meter
slugs; place the check at the start of the transacting function (or before
calling EntitlementsByIngestedEventsQuery) so the function exits early when
eventFilter.MeterSlugs has length 0.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6dd4c9b7-5061-47b9-ac6c-fc42c82d275d
📒 Files selected for processing (2)
openmeter/entitlement/adapter/entitlement.goopenmeter/entitlement/adapter/entitlement_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- openmeter/entitlement/adapter/entitlement_test.go
turip
previously approved these changes
Apr 30, 2026
gergely-kurucz-konghq
previously approved these changes
Apr 30, 2026
chrisgacsal
dismissed stale reviews from gergely-kurucz-konghq and turip
via
4f87342
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
openmeter/entitlement/adapter/entitlement.go (1)
324-329: ⚡ Quick winConsider a small fast-path for empty/duplicate meter slugs.
Quick win here: dedupe
eventFilter.MeterSlugsand return early when the list is empty before building/executing SQL. It keeps query args smaller and avoids unnecessary DB work in high-throughput ingestion batches.⚙️ Suggested tweak
+ meters := lo.Uniq(eventFilter.MeterSlugs) + if len(meters) == 0 { + return []balanceworker.ListAffectedEntitlementsResponse{}, nil + } + query, args := EntitlementsByIngestedEventsQuery( repo.db.GetConfig().Driver.Dialect(), eventFilter.Namespace, eventFilter.EventSubject, - eventFilter.MeterSlugs..., + meters..., )As per coding guidelines: "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."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/entitlement/adapter/entitlement.go` around lines 324 - 329, Dedupe eventFilter.MeterSlugs and short-circuit when empty before calling EntitlementsByIngestedEventsQuery: in the function containing the shown snippet, create a small deduping step for eventFilter.MeterSlugs (e.g., build a temporary map/set and produce a compact slice of unique slugs), replace uses of eventFilter.MeterSlugs with that deduped slice, and if the deduped slice is empty return immediately (the same zero/empty result the function would otherwise produce) to avoid building/executing the SQL; ensure you update references to EntitlementsByIngestedEventsQuery to pass the deduped slice.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@openmeter/entitlement/adapter/entitlement.go`:
- Around line 324-329: Dedupe eventFilter.MeterSlugs and short-circuit when
empty before calling EntitlementsByIngestedEventsQuery: in the function
containing the shown snippet, create a small deduping step for
eventFilter.MeterSlugs (e.g., build a temporary map/set and produce a compact
slice of unique slugs), replace uses of eventFilter.MeterSlugs with that deduped
slice, and if the deduped slice is empty return immediately (the same zero/empty
result the function would otherwise produce) to avoid building/executing the
SQL; ensure you update references to EntitlementsByIngestedEventsQuery to pass
the deduped slice.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 97e83879-23d3-4f08-8aef-cfcf0991cf55
📒 Files selected for processing (1)
openmeter/entitlement/adapter/entitlement.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openmeter/entitlement/adapter/entitlement.go`:
- Around line 299-310: The query in the entitlement adapter is incorrectly
filtering out deleted rows by using
sql.IsNull(et.C(db_entitlement.FieldDeletedAt)), which prevents DeletedAt from
being scanned into ListAffectedEntitlementsResponse and breaks
GetEntitlementActivityPeriod; remove that hard filter so deleted entitlements
are included (i.e., stop adding sql.IsNull on db_entitlement.FieldDeletedAt) in
the Select/Where building (the block that
Select(...).From(et).Join(cbs)...Where(sql.And(...)) in this file), and make the
same change for the other identical query occurrence around the 349-363 region
so deleted entitlements are returned and their DeletedAt values can be used.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: edda6542-0772-45eb-90a1-eaa84c709c52
📒 Files selected for processing (3)
openmeter/entitlement/adapter/entitlement.goopenmeter/entitlement/adapter/entitlement_test.goopenmeter/entitlement/balanceworker/repository.go
🚧 Files skipped from review as they are similar to previous changes (1)
- openmeter/entitlement/adapter/entitlement_test.go
chrisgacsal
force-pushed
the
refactor/entitlement-query
branch
from
317e924 to
eb220f8
Compare
tothandras
approved these changes
Apr 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Improve fetching entitlements affected by ingested events. It combines 3 separate database calls into one reducing the time/resources to get affected entitlements. This helps with improving the overall performance of the balance worker which heavily relies on this call in order to figure out which entitlements need to be tracked/their balance updated.
Summary by CodeRabbit
Refactor
New Features
Tests