feat(api): add event subject list endpoint#4497
Open
tothandras wants to merge 2 commits into
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThis PR adds /openmeter/events/subjects: TypeSpec contracts, generated JS client and docs, Go HTTP handler and server route, domain/service types and validation, streaming/ClickHouse v2 listing + optional projection, adapter scan/attribution logic, config/wiring, and unit + e2e tests. ChangesEvent Subjects Listing API
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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 docstrings
🧪 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 |
tothandras
force-pushed
the
feat/api-event-subjects
branch
from
9937a40 to
1f3c37e
Compare
tothandras
force-pushed
the
feat/api-event-subjects
branch
from
1f3c37e to
449cad9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
openmeter/meterevent/adapter/subject.go (1)
104-137: ⚡ Quick winTwo separate customer queries for attribution resolution.
The implementation makes two
ListCustomerscalls (lines 114-123) to resolve attributed subjects: one searching byUsageAttributionSubjectKeyand another by customerKey. This is necessary because these are different search fields, and the customer service likely doesn't support OR queries across these fields.While this works correctly, if the customer service API is ever extended to support OR queries or multi-field searches, combining these into a single call could reduce overhead, especially when many subjects need attribution checks.
For now, this is fine—just something to keep in mind for future optimization if attribution checks become a hot path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/meterevent/adapter/subject.go` around lines 104 - 137, The function attributedSubjectKeys currently issues two ListCustomers calls (via a.customerService.ListCustomers) to match keys against UsageAttributionSubjectKey and Key; add a concise TODO comment in attributedSubjectKeys noting that these separate queries are intentional but should be combined into a single OR/multi-field query if/when the customer service gains support for OR searches (or add a feature-flagged path to call a combined search method such as ListCustomersWithOr if implemented), referencing the existing fields UsageAttributionSubjectKey and Key and the ListCustomers call so future maintainers can find and replace the two-query approach with a single call.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@openmeter/meterevent/adapter/subject.go`:
- Around line 104-137: The function attributedSubjectKeys currently issues two
ListCustomers calls (via a.customerService.ListCustomers) to match keys against
UsageAttributionSubjectKey and Key; add a concise TODO comment in
attributedSubjectKeys noting that these separate queries are intentional but
should be combined into a single OR/multi-field query if/when the customer
service gains support for OR searches (or add a feature-flagged path to call a
combined search method such as ListCustomersWithOr if implemented), referencing
the existing fields UsageAttributionSubjectKey and Key and the ListCustomers
call so future maintainers can find and replace the two-query approach with a
single call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7777cf3c-0234-47c6-9ead-9cb9ea1a5d74
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (32)
api/spec/packages/aip-client-javascript/README.mdapi/spec/packages/aip-client-javascript/src/funcs/events.tsapi/spec/packages/aip-client-javascript/src/index.tsapi/spec/packages/aip-client-javascript/src/models/operations/events.tsapi/spec/packages/aip-client-javascript/src/models/schemas.tsapi/spec/packages/aip-client-javascript/src/models/types.tsapi/spec/packages/aip-client-javascript/src/sdk/events.tsapi/spec/packages/aip/src/events/index.tspapi/spec/packages/aip/src/events/operations.tspapi/spec/packages/aip/src/events/subject.tspapi/spec/packages/aip/src/konnect.tspapi/spec/packages/aip/src/openmeter.tspapi/v3/api.gen.goapi/v3/handlers/events/handler.goapi/v3/handlers/events/list_subjects.goapi/v3/server/routes.goapp/common/streaming.goapp/config/aggregation.goe2e/events_subjects_v3_test.goe2e/v3helpers_test.goopenmeter/meterevent/adapter/subject.goopenmeter/meterevent/adapter/subject_test.goopenmeter/meterevent/service.goopenmeter/meterevent/subject.goopenmeter/meterevent/subject_test.goopenmeter/server/server_test.goopenmeter/streaming/clickhouse/connector.goopenmeter/streaming/clickhouse/subject_query.goopenmeter/streaming/clickhouse/subject_query_test.goopenmeter/streaming/connector.goopenmeter/streaming/retry/retry.goopenmeter/streaming/testutils/streaming.go
Contributor
Greptile SummaryThis PR adds a new
Confidence Score: 5/5The change is safe to merge. The new endpoint is marked x-internal and x-unstable, the projection is opt-in and off by default, and the scan-loop logic, cursor pagination, and attribution resolution are all correctly implemented. No correctness bugs were found. The multi-batch scan loop correctly handles all edge cases: empty batches, exact full pages, over-full pages, and the scan-round cap. Attribution resolution via two ListCustomers lookups per batch is correct, and the cursor propagation through both early-return paths and the cap path is sound. No files require special attention beyond the minor suggestions on openmeter/streaming/connector.go and openmeter/streaming/clickhouse/subject_query.go. Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
tothandras
force-pushed
the
feat/api-event-subjects
branch
from
449cad9 to
3bf0c38
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openmeter/meterevent/adapter/subject.go`:
- Around line 128-131: The accumulator currently only adds attribution values
from c.GetUsageAttribution().GetValues(), missing the customer's own key; update
the loop over customerList.Items (variable c) to also insert the customer's key
into the attributed set (e.g., attributed[c.GetKey()] = struct{}{} or the
appropriate getter used in this type), guarding for empty/nil keys if needed so
the customer's own key match path is preserved for filtering.
🪄 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: c094f8c6-a5aa-4659-9ca7-3cba8a9eb910
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (32)
api/spec/packages/aip-client-javascript/README.mdapi/spec/packages/aip-client-javascript/src/funcs/events.tsapi/spec/packages/aip-client-javascript/src/index.tsapi/spec/packages/aip-client-javascript/src/models/operations/events.tsapi/spec/packages/aip-client-javascript/src/models/schemas.tsapi/spec/packages/aip-client-javascript/src/models/types.tsapi/spec/packages/aip-client-javascript/src/sdk/events.tsapi/spec/packages/aip/src/events/index.tspapi/spec/packages/aip/src/events/operations.tspapi/spec/packages/aip/src/events/subject.tspapi/spec/packages/aip/src/konnect.tspapi/spec/packages/aip/src/openmeter.tspapi/v3/api.gen.goapi/v3/handlers/events/handler.goapi/v3/handlers/events/list_subjects.goapi/v3/server/routes.goapp/common/streaming.goapp/config/aggregation.goe2e/events_subjects_v3_test.goe2e/v3helpers_test.goopenmeter/meterevent/adapter/subject.goopenmeter/meterevent/adapter/subject_test.goopenmeter/meterevent/service.goopenmeter/meterevent/subject.goopenmeter/meterevent/subject_test.goopenmeter/server/server_test.goopenmeter/streaming/clickhouse/connector.goopenmeter/streaming/clickhouse/subject_query.goopenmeter/streaming/clickhouse/subject_query_test.goopenmeter/streaming/connector.goopenmeter/streaming/retry/retry.goopenmeter/streaming/testutils/streaming.go
✅ Files skipped from review due to trivial changes (2)
- api/spec/packages/aip/src/konnect.tsp
- api/spec/packages/aip-client-javascript/README.md
🚧 Files skipped from review as they are similar to previous changes (27)
- api/spec/packages/aip/src/openmeter.tsp
- api/spec/packages/aip/src/events/index.tsp
- openmeter/streaming/testutils/streaming.go
- api/spec/packages/aip-client-javascript/src/sdk/events.ts
- openmeter/server/server_test.go
- api/v3/handlers/events/handler.go
- app/config/aggregation.go
- app/common/streaming.go
- api/v3/server/routes.go
- e2e/events_subjects_v3_test.go
- openmeter/streaming/retry/retry.go
- api/spec/packages/aip-client-javascript/src/funcs/events.ts
- api/spec/packages/aip-client-javascript/src/index.ts
- openmeter/meterevent/service.go
- api/spec/packages/aip/src/events/subject.tsp
- e2e/v3helpers_test.go
- openmeter/streaming/connector.go
- api/spec/packages/aip/src/events/operations.tsp
- api/spec/packages/aip-client-javascript/src/models/operations/events.ts
- api/v3/handlers/events/list_subjects.go
- openmeter/streaming/clickhouse/subject_query_test.go
- openmeter/meterevent/adapter/subject_test.go
- openmeter/streaming/clickhouse/connector.go
- openmeter/streaming/clickhouse/subject_query.go
- openmeter/meterevent/subject.go
- api/spec/packages/aip-client-javascript/src/models/schemas.ts
- api/spec/packages/aip-client-javascript/src/models/types.ts
Comment on lines
+128
to
+131
| for _, c := range customerList.Items { | ||
| for _, value := range c.GetUsageAttribution().GetValues() { | ||
| attributed[value] = struct{}{} | ||
| } |
Contributor
There was a problem hiding this comment.
Include customer-key hits in the attributed set.
On Line 128 onward, the accumulator only adds GetUsageAttribution().GetValues(). That misses the “customer’s own key” match path documented above the function, so attributed filtering can drop valid subjects.
💡 Suggested fix
for _, c := range customerList.Items {
+ if c.Key != "" {
+ attributed[c.Key] = struct{}{}
+ }
for _, value := range c.GetUsageAttribution().GetValues() {
attributed[value] = struct{}{}
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openmeter/meterevent/adapter/subject.go` around lines 128 - 131, The
accumulator currently only adds attribution values from
c.GetUsageAttribution().GetValues(), missing the customer's own key; update the
loop over customerList.Items (variable c) to also insert the customer's key into
the attributed set (e.g., attributed[c.GetKey()] = struct{}{} or the appropriate
getter used in this type), guarding for empty/nil keys if needed so the
customer's own key match path is preserved for filtering.
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
1 participant
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.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores