-
Notifications
You must be signed in to change notification settings - Fork 91
feat(api): add /api/v2/events route specification #2419
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a new Changes
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
c288e65
to
8bc3188
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🧹 Nitpick comments (6)
api/openapi.yaml (2)
11689-11708
: IngestedEventCursorList Schema
The addition of theIngestedEventCursorList
schema effectively supports cursor-based pagination by mandating bothitems
andnextCursor
. One consideration is whethernextCursor
should be optional to accommodate scenarios where there are no subsequent pages. Providing an example usage in the schema documentation might also benefit API consumers.
13381-13393
: Installed Apps Query Parameters Update
The updated properties for listing installed apps—specifically, thepage
andpageSize
parameters—are appropriately defined with numerical constraints and defaults. If the new/api/v2/events
endpoint is intended to move toward cursor-based pagination exclusively, it might be worthwhile to review whether the page-based parameters should be consolidated or marked for deprecation in the future for consistency.api/client/javascript/src/client/schemas.ts (2)
7956-7959
: Appropriate cursor pagination parametersThe pagination query parameters (
cursor
andlimit
) are well-defined with clear documentation. Consider adding validation constraints for thelimit
parameter (e.g., minimum and maximum values) in the implementation to prevent abuse.
18332-18419
: Comprehensive operation definition with thorough error handlingThe
listEventsV2
operation is well-defined with:
- Appropriate query parameters for filtering and pagination
- Clear documentation for each parameter
- Thorough response definitions for successful cases (200) and various error scenarios (400, 401, 403, 500, 503, default)
The error handling follows RESTful API best practices by using appropriate HTTP status codes and structured problem+json responses.
One enhancement to consider: Add a parameter for sorting the results, as users often expect to control the order of returned items (e.g., by time ascending/descending).
api/spec/src/events.tsp (2)
280-280
: Consider explaining the purpose of x-internal extensionThe
@extension("x-internal", true)
annotation suggests this API might be for internal use. It would be helpful to add a comment explaining the purpose of this extension and any usage restrictions.
281-311
: Consider adding documentation for filter parametersThe filter parameters (
id
,source
,subject
,type
,time
,ingestedAt
) have minimal documentation. Consider adding more detailed descriptions explaining the available filter operations and expected format.Example for one parameter:
@query(#{ explode: true, style: "deepObject" }) + /** + * Filter events by ID. + * Supports operations: eq, contains + * Example: ?id[eq]=abc123 or ?id[contains]=abc + */ id?: OpenMeter.FilterString,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
api/spec/pnpm-lock.yaml
is excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (15)
api/client/javascript/src/client/schemas.ts
(8 hunks)api/openapi.cloud.yaml
(6 hunks)api/openapi.yaml
(6 hunks)api/spec/package.json
(0 hunks)api/spec/src/billing/invoices/main.tsp
(0 hunks)api/spec/src/billing/main.tsp
(0 hunks)api/spec/src/cloud/main.tsp
(1 hunks)api/spec/src/entitlements/grant.tsp
(0 hunks)api/spec/src/entitlements/main.tsp
(0 hunks)api/spec/src/events.tsp
(2 hunks)api/spec/src/main.tsp
(0 hunks)api/spec/src/notification/main.tsp
(0 hunks)api/spec/src/pagination.tsp
(2 hunks)api/spec/src/productcatalog/subscription.tsp
(0 hunks)openmeter/server/router/event.go
(1 hunks)
💤 Files with no reviewable changes (8)
- api/spec/src/notification/main.tsp
- api/spec/package.json
- api/spec/src/billing/main.tsp
- api/spec/src/productcatalog/subscription.tsp
- api/spec/src/entitlements/main.tsp
- api/spec/src/billing/invoices/main.tsp
- api/spec/src/entitlements/grant.tsp
- api/spec/src/main.tsp
🔇 Additional comments (18)
api/openapi.cloud.yaml (4)
7642-7742
: New/api/v2/events
Endpoint DefinitionThe new endpoint is clearly defined with its operationId
listEventsV2
, concise summary, and detailed description. The query parameters for filtering (such asclientId
,id
,source
,subject
,type
,time
, andingestedAt
) are well specified with appropriate schemas (usingFilterString
andFilterTime
where needed). The response definitions for various HTTP statuses are thorough, and the inclusion of thex-internal
flag and proper security configuration is appropriate for internal API endpoints.
7849-7873
: Cursor-Based Pagination Parameter DefinitionsThe new reusable parameters for cursor-based pagination (
CursorPaginatedQuery.cursor
andCursorPaginatedQuery.limit
) are well defined with clear descriptions, types, and validation (including minimum, maximum, and default values). This addition will support advanced pagination effectively.
11294-11404
: New Schema Definitions for Query FilteringThe additions of the
FilterString
andFilterTime
schemas provide a comprehensive, operator-rich filtering mechanism. The use of various operators ($eq
,$ne
,$in
,$nin
,$like
, and so on) with clear descriptions and appropriate handling of nullable values is well executed. This will allow precise and flexible query filtering across the API.
11622-11641
: IngestedEventCursorList Schema for Cursor PaginationThe new
IngestedEventCursorList
schema is well structured for supporting cursor-based pagination. It correctly enforces required fields (items
andnextCursor
), limits the number of items, and documents the purpose of each field. This schema aligns effectively with the new/api/v2/events
endpoint requirements.api/openapi.yaml (4)
7411-7511
: New /api/v2/events Endpoint Specification
The new/api/v2/events
endpoint is well-defined. The operationId (listEventsV2
), summary, and detailed description clearly outline its purpose. The filtering parameters (includingclientId
,id
,source
,subject
,type
,time
, andingestedAt
) are properly referenced via schemas, and the use of thedeepObject
style is appropriate for complex query structures. Ensure that all API clients support thedeepObject
style and that this behavior is clearly documented.
7615-7637
: Cursor-Based Pagination Parameters
The definitions forCursorPaginatedQuery.cursor
andCursorPaginatedQuery.limit
are clear and include useful constraints such as data type, minimum/maximum limits, and default values. This setup should help enforce consistency on pagination behavior. Just verify that the backend enforces these constraints as defined.
8022-8027
: Page Number Parameter Definition
ThePaginatedQuery.page
parameter is clearly documented with a description, a minimum value of 1, and a default of 1. This provides a sensible and user-friendly mechanism for page-based pagination.
8032-8039
: Page Size Parameter Definition
ThePagination.pageSize
parameter is well-configured with constraints ensuring a minimum value of 1, a maximum of 1000, and a default value of 100. This helps maintain consistency and control over the volume of data per page.api/client/javascript/src/client/schemas.ts (5)
1762-1781
: Well-structured API path definitionThe new
/api/v2/events
endpoint is clearly defined with appropriate JSDoc comments explaining its purpose. The explicit marking of supported (GET) and unsupported HTTP methods follows the pattern used throughout the codebase.
3927-3957
: Comprehensive string filtering capabilitiesThe
FilterString
type provides a rich set of filtering operators including equality, negation, inclusion, pattern matching (with case sensitivity options), and comparison operators. The inclusion of logical operators ($and
,$or
) enables the construction of complex filtering expressions. Each field has clear documentation explaining its purpose.
3958-3984
: Well-designed time filtering capabilitiesThe
FilterTime
type appropriately provides comparison operators for date-time values with clear JSDoc documentation. The inclusion of logical operators ($and
,$or
) enables complex time-based filtering expressions. The consistent structure betweenFilterString
andFilterTime
makes the API intuitive to use.
4152-4158
: Good implementation of cursor-based paginationThe
IngestedEventCursorList
type follows best practices for cursor-based pagination, including an array of items and anextCursor
field for retrieving the next page of results. This approach is more reliable than offset-based pagination for large datasets.
8272-8273
: Proper type exportsThe newly created types and parameters are correctly exported, making them available to consumers of this module and ensuring type safety throughout the application.
Also applies to: 8290-8291, 8591-8594
api/spec/src/cloud/main.tsp (1)
109-111
: Interface extension looks goodThe
EventsV2
interface properly extendsOpenMeter.EventsV2
and is correctly tagged with the/api/v2/events
route, following the same pattern as other interfaces in the file.api/spec/src/events.tsp (1)
272-312
: EventsV2 interface implementation looks goodThe
EventsV2
interface and itslist
method are well-designed with appropriate query parameters for filtering and pagination. The method is correctly annotated and the return type properly uses the cursor-based pagination model.api/spec/src/pagination.tsp (3)
14-15
: Good addition of validation constraintsAdding min/max value constraints to pagination parameters is a good practice to prevent performance issues from excessive page sizes or invalid page numbers.
Also applies to: 21-23
55-73
: Well-designed cursor pagination query modelThe
CursorPaginatedQuery
model is well-structured with appropriate documentation and constraints. The implementation follows best practices for cursor-based pagination.
75-91
: Well-designed cursor pagination response modelThe
CursorPagedList<T>
model properly implements the cursor-based pagination response pattern with the@pageItems
annotation and appropriate constraints.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
api/spec/src/events.tsp (1)
270-312
: Provide usage examples for advanced filteringThe new endpoint is well structured for deep-object filtering. However, consider adding example requests that demonstrate how to filter by time, source, and other properties. This helps API consumers understand the expected query shapes for
OpenMeter.FilterString
andOpenMeter.FilterTime
.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
api/spec/src/events.tsp
(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Artifacts / Benthos Collector Container image
- GitHub Check: Artifacts / Container image
- GitHub Check: Quickstart
- GitHub Check: Test
- GitHub Check: Migration Checks
- GitHub Check: Lint
- GitHub Check: E2E
- GitHub Check: Developer environment
- GitHub Check: CI
- GitHub Check: Build
- GitHub Check: Commit hooks
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
api/spec/src/events.tsp (1)
4-4
: Import looks goodThanks for adding the import for pagination; it clearly aligns with the spread usage of
CursorPaginatedQuery
introduced below.
Summary by CodeRabbit