-
Notifications
You must be signed in to change notification settings - Fork 91
feat(events): add v2 api definition #2485
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 update introduces extensive modifications to pagination functionality across the API’s various components. Schema names for listing responses have been updated, and pagination parameters have transitioned from legacy models (e.g., PaginatedQuery) to new ones (e.g., QueryPagination and PaginatedResponse). The changes span client-side schemas, code generation configuration, OpenAPI definitions, and specification files. In addition, new cursor-based pagination models are introduced, a new events listing endpoint (ListEventsV2) is added, and isolated non‐pagination updates (such as modifications in a Calculator class) are applied. Changes
Possibly related PRs
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.64.8)level=warning msg="The linter 'tenv' is deprecated (since v1.64.0) due to: Duplicate feature in another linter. Replaced by usetesting." Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 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 (
|
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: 2
🧹 Nitpick comments (5)
api/openapi.yaml (2)
8989-9017
: ReviewAppPaginatedResponse
Schema Descriptions
The updated schema forAppPaginatedResponse
now specifies required fields and properties for pagination. Consider revisiting the property descriptions—for example, the term "The items in the current page" may not accurately reflecttotalCount
(which typically represents the total number of records across all pages).
14161-14191
: ReviewMarketplaceListingPaginatedResponse
Schema Descriptions
The properties (totalCount
,page
,pageSize
, anditems
) all use the same description ("The items in the current page"). It would be clearer to:
- Indicate that
totalCount
represents the total number of available items.- Clarify that
page
andpageSize
are pagination controls.- Describe
items
as the collection for the current page.api/openapi.cloud.yaml (2)
9047-9072
: Clarify Descriptions in AppPaginatedResponse SchemaThe
AppPaginatedResponse
schema is correctly defined with required pagination fields. However, the descriptions fortotalCount
,page
, andpageSize
are all set to "The items in the current page." To avoid confusion, consider revising these descriptions as follows:
totalCount
: "Total number of items available."page
: "Current page number."pageSize
: "Number of items per page."
13849-13880
: Refine Descriptions in MarketplaceListingPaginatedResponse SchemaThe structure for
MarketplaceListingPaginatedResponse
is properly implemented; however, the property descriptions are not specific:
- For
totalCount
,page
, andpageSize
, the description "The items in the current page." is misleading.
Consider updating the descriptions to:
totalCount
: "Total number of marketplace listings."page
: "Current page number."pageSize
: "Number of listings per page."- Additionally, review the description for
items
if more detail is necessary.api/spec/src/customer.tsp (1)
114-116
: Consider documenting the pagination model changeWhile the code changes are consistent, there's no documentation explaining the difference between the old and new pagination models, or how they should be used.
Consider adding documentation comments to explain:
- What
QueryPagination
provides vs. the previousPaginatedQuery
- What clients should expect in the
PaginatedResponse
structure- Any changes in behavior that API consumers should be aware of
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
api/client/javascript/src/client/schemas.ts
(11 hunks)api/codegen.yaml
(2 hunks)api/openapi.cloud.yaml
(7 hunks)api/openapi.yaml
(11 hunks)api/spec/src/app/app.tsp
(2 hunks)api/spec/src/app/marketplace.tsp
(1 hunks)api/spec/src/app/stripe.tsp
(0 hunks)api/spec/src/customer.tsp
(3 hunks)api/spec/src/events.tsp
(1 hunks)api/spec/src/pagination.tsp
(0 hunks)api/spec/src/query.tsp
(1 hunks)openmeter/app/httpdriver/app.go
(1 hunks)openmeter/app/httpdriver/marketplace.go
(1 hunks)openmeter/server/router/event.go
(1 hunks)
💤 Files with no reviewable changes (2)
- api/spec/src/app/stripe.tsp
- api/spec/src/pagination.tsp
🧰 Additional context used
🧬 Code Definitions (3)
openmeter/server/router/event.go (3)
openmeter/meterevent/adapter/event.go (1) (1)
a
(12-62)openmeter/server/router/router.go (1) (1)
Router
(171-191)api/api.gen.go (17) (17)
params
(10168-10168)params
(10328-10328)params
(10469-10469)params
(10546-10546)params
(10715-10715)params
(10900-10900)params
(11015-11015)params
(11067-11067)params
(11189-11189)params
(11309-11309)params
(11345-11345)params
(11469-11469)params
(11593-11593)params
(11690-11690)params
(11837-11837)params
(11992-11992)ListEventsV2Params
(6514-6530)
openmeter/app/httpdriver/marketplace.go (3)
api/api.gen.go (1) (1)
MarketplaceListingPaginatedResponse
(3642-3654)api/client/go/client.gen.go (1) (1)
MarketplaceListingPaginatedResponse
(3320-3332)api/client/javascript/src/client/schemas.ts (1) (1)
MarketplaceListingPaginatedResponse
(8333-8334)
api/client/javascript/src/client/schemas.ts (2)
api/client/go/client.gen.go (2) (2)
AppPaginatedResponse
(712-724)MarketplaceListingPaginatedResponse
(3320-3332)api/api.gen.go (2) (2)
AppPaginatedResponse
(813-825)MarketplaceListingPaginatedResponse
(3642-3654)
🔇 Additional comments (37)
api/client/javascript/src/client/schemas.ts (11)
1855-1874
: New schemaAppPaginatedResponse
correctly implements pagination response structure.The added schema includes all necessary pagination fields: totalCount, page, pageSize, and the actual items array. This aligns with the Go implementation seen in the relevant code snippets from
api/client/go/client.gen.go
andapi/api.gen.go
.
5234-5253
: New schemaMarketplaceListingPaginatedResponse
maintains pagination consistency.The structure mirrors
AppPaginatedResponse
, providing the same pagination fields (totalCount, page, pageSize, items) for marketplace listings. This ensures consistent pagination behavior across different resource types.
8071-8071
: Appropriate type export forAppPaginatedResponse
.This export makes the new paginated response type available to consumers of the API client.
8333-8334
: ExportedMarketplaceListingPaginatedResponse
type maintains naming consistency.The exported type follows the same pattern as other exported types, making the new pagination response type available to consumers.
8658-8665
: Well-documented pagination parameters forlistApps
.The pagination parameters include clear descriptions that explain their purpose. The JSDoc comments include formatting details and default values which will be helpful for API consumers.
8679-8679
: Updated response type forlistApps
to use new pagination structure.The response content type correctly references the new
AppPaginatedResponse
schema, ensuring API consumers receive the structured pagination data.
11224-11231
: Consistent pagination parameters added tolistCustomers
endpoint.The pagination parameters follow the same pattern established in other endpoints, maintaining API consistency.
11423-11430
: Pagination parameters forlistCustomerAppData
match the established pattern.The pagination implementation is consistent with other endpoints, which is good for developer experience.
11816-11823
: Pagination parameters added tolistCustomerSubscriptions
endpoint.These parameters follow the same format as other endpoints, maintaining consistency across the API.
13473-13480
: Consistent pagination parameters forlistMarketplaceListings
.The pagination parameters follow the established pattern, providing a consistent API experience.
13494-13494
: Updated response type forlistMarketplaceListings
to use new pagination structure.The response content type now correctly references the new
MarketplaceListingPaginatedResponse
schema.openmeter/app/httpdriver/app.go (1)
20-20
: LGTM: Type alias updated to use new pagination schemaThe change from
api.AppList
toapi.AppPaginatedResponse
aligns with the standardization of pagination schemas across the API, while maintaining backward compatibility with the existing implementation. The function that returns this type still creates a response with the same fields, so this change is safe.api/codegen.yaml (2)
1-1
: LGTM: Added schema reference for YAML validationAdding the schema reference helps with validation during development, providing better IDE support and catching potential configuration errors earlier.
13-14
:✅ Verification successful
Verify the impact of
skip-prune: true
on generated code sizeThe
skip-prune: true
option prevents the code generator from removing unused elements, which could lead to larger generated files but may prevent issues with type references that appear unused but are actually needed indirectly.Consider running the following command to see how this affects the generated code size:
🏁 Script executed:
#!/bin/bash # Check the file size before and after adding skip-prune: true # Get current file size current_size=$(wc -l ./api/api.gen.go | awk '{print $1}') echo "Current api.gen.go size: $current_size lines" # Look for any new types that might be included due to skip-prune: true echo "Looking for potentially new types in generated code:" grep -n "type.*struct" ./api/api.gen.go | grep -i "paginated" | head -n 10Length of output: 838
Verification Outcome: Confirmed Impact of
skip-prune: true
The executed script confirms that enabling
skip-prune: true
leads to a larger generated file—currently 15,403 lines—with several extra paginated type definitions (such as AppPaginatedResponse, BillingProfilePaginatedResponse, CustomerPaginatedResponse, etc.) retained in the generated code. This behavior is consistent with the intended purpose of preventing the removal of indirectly used types.
- File Size Verification: The generated file size is 15,403 lines.
- Type Inclusion: Multiple paginated response types are present, confirming that less-used (but potentially necessary) code is preserved.
- Recommendation: Confirm that the increased file size does not negatively impact performance or maintainability. If it does, consider re-evaluating whether
skip-prune: true
is the best option or if selective pruning might be viable.api/openapi.yaml (8)
42-56
: Update Pagination Parameters and Response Schema for List Apps Endpoint
The endpoint now uses the new pagination parameters (Pagination.page
andPagination.pageSize
) and updates the response schema reference toAppPaginatedResponse
per the new API specification. Please double-check that all client references are updated accordingly.
2063-2070
: Update Pagination Parameters for List Customers Endpoint
TheList customers
endpoint now references the newPagination.page
andPagination.pageSize
parameters. Ensure that the backend and client-side logic correctly interpret these parameters in line with the overall pagination update.
2137-2144
: Integrate Pagination Parameters in Endpoint Query
New pagination references are added in this endpoint (lines 2140–2141). Verify that the ordering of parameters and their validations do not conflict with any existing behavior.
2442-2449
: Apply New Pagination Parameters for the Endpoint
The updates add the new pagination parameters into the parameter list. Confirm that the associated business logic properly processes these values and that there is consistency across endpoints.
3742-3756
: Update Marketplace Listing Endpoint with Revised Pagination and Schema
This hunk updates the listing of available apps for the marketplace by adding new pagination parameters and changing the response schema reference toMarketplaceListingPaginatedResponse
. Ensure that these changes are consistent with the overall refactoring of pagination handling.
8064-8174
: New Endpoint/api/v2/events
with Cursor-Based Pagination
A new endpoint for listing ingested events is introduced with advanced filtering and cursor-based pagination. The parameters for cursor (CursorPagination.cursor
) and limit are defined alongside additional filtering options. Please verify that:
- The deepObject style for filter parameters meets client expectations.
- The response schema (
IngestedEventCursorPaginatedResponse
) correctly models the paginated event data.
8277-8301
: Define CursorPagination Parameters in Components Section
The new definitions forCursorPagination.cursor
andCursorPagination.limit
offer clear constraints (e.g., integer range for limit) and defaults for cursor-based pagination. Confirm that these components are correctly referenced in the new events endpoint and elsewhere if needed.
12378-12397
: IntroduceIngestedEventCursorPaginatedResponse
Schema
The new schema for cursor-based pagination in events is well-structured with requireditems
andnextCursor
fields. Verify that this response model meets the consumption needs and that additional metadata (if necessary) is addressed elsewhere.api/spec/src/events.tsp (1)
134-175
:❓ Verification inconclusive
Well-structured new V2 events API with cursor pagination
The new
EventsV2
interface provides a good enhancement to the existing API by implementing cursor-based pagination and advanced filtering capabilities. The implementation properly extends the existing patterns and makes use of the new cursor pagination models.A few observations:
- The interface is marked with
@extension("x-internal", true)
, suggesting it might be intended for internal use - is this intentional?- The filter parameters use
deepObject
style for more complex filtering, which is appropriate for the use case.- The response type uses the new
CursorPaginatedResponse
model, which aligns with the cursor pagination pattern.
🏁 Script executed:
#!/bin/bash # Check if there are any existing implementations or consumers of this new endpoint rg -A 3 "ListEventsV2" --type goLength of output: 1740
New V2 events API integrates cursor pagination and advanced filtering effectively
The
EventsV2
interface is well structured:
- It properly leverages cursor-based pagination via the
...QueryCursorPagination
spread and theCursorPaginatedResponse
type.- The use of
deepObject
formatting for filter parameters is appropriate for complex queries.- Our review confirms that the new endpoint is referenced in the codebase (e.g., in
openmeter/server/router/event.go
andapi/api.gen.go
).However, the interface is marked with
@extension("x-internal", true)
, which suggests it might be for internal use. Please confirm if this was intentional.api/spec/src/query.tsp (2)
131-149
: Good implementation of cursor pagination query parametersThe
QueryCursorPagination
model is well-designed with appropriate constraints. Cursor-based pagination is generally more efficient than offset-based pagination for large datasets and avoids the "skipped items" problem.The limit constraints (min 1, max 1000) with a default of 100 are reasonable values that balance between performance and usability.
151-167
: Well-designed cursor pagination response modelThe
CursorPaginatedResponse
model is properly structured with items and nextCursor fields. The@maxItems(1000)
constraint on the items field is a good practice to prevent excessive data transfer.The model follows the TypeSpec naming convention with
{name}CursorPaginatedResponse
, which allows for type-specific naming when used with generics.openmeter/app/httpdriver/marketplace.go (1)
23-23
:✅ Verification successful
Type alias updated to use new pagination response model
This change aligns the
ListMarketplaceListingsResponse
type with the new pagination models introduced in the API. The existing implementation already handles pagination data (page, pageSize, totalCount, items), so this change is consistent with the actual structure being used.This update is part of the broader effort to standardize pagination across the API.
🏁 Script executed:
#!/bin/bash # Check if all return values are compatible with the new type rg -A 5 "ListMarketplaceListingsResponse\{" --type goLength of output: 1102
Update Verified: Type alias correctly uses the new pagination response model
- The alias
ListMarketplaceListingsResponse
now directly referencesapi.MarketplaceListingPaginatedResponse
.- The grep output confirms that return statements in
openmeter/app/httpdriver/marketplace.go
properly construct the response using the new pagination fields (Page
,PageSize
,TotalCount
,Items
).- Usage in
api/client/go/client.gen.go
further supports consistent adoption of the updated type across the codebase.No additional changes are needed.
api/openapi.cloud.yaml (5)
43-57
: API V2 Pagination Update for List Apps EndpointThe updated parameters now reference
Pagination.page
andPagination.pageSize
, and the response schema is updated toAppPaginatedResponse
. These changes correctly align with the new pagination design for listing apps.
2064-2071
: Customer Listing Pagination UpdateThe addition of
Pagination.page
andPagination.pageSize
parameters in the customer listing endpoint ensures consistency with the new pagination model. This helps standardize how pagination is handled across endpoints.
2138-2145
: Pagination Parameters Integration in Query SectionThe inclusion of
Pagination.page
andPagination.pageSize
after the string validation block ensures that this query configuration now benefits from uniform pagination properties. The update is well incorporated.
2443-2450
: Consistent Pagination in Endpoint ParametersThe addition of the pagination parameters here (using
$ref
toPagination.page
andPagination.pageSize
) maintains consistency with the other endpoints. This aids in a uniform and predictable API behavior.
3743-3757
: Marketplace Listing Pagination UpdateThe changes update the "List available apps" endpoint by incorporating the new pagination parameters and updating the response schema to
MarketplaceListingPaginatedResponse
. This update reflects the intended API evolution for marketplace listings.api/spec/src/app/marketplace.tsp (1)
27-29
:❓ Verification inconclusive
API change: Updated pagination model
The change from
PaginatedQuery
toQueryPagination
and fromPaginated<T>
toPaginatedResponse<T>
appears to be part of a larger refactoring of the pagination mechanism across the API. This standardization is good, but it represents a potential breaking change for API consumers.Has this breaking change been communicated to API consumers? Consider adding a migration guide or deprecation notice if not already done.
Also, check that the OpenAPI generator is properly handling this type change in generated clients:
🏁 Script executed:
#!/bin/bash # Verify that OpenAPI specs have been updated with the new pagination types grep -r "PaginatedResponse" api/openapi*.yamlLength of output: 4777
API Pagination Update Verified — Please Confirm Consumer Communication
The OpenAPI specs have been updated to reference the new pagination types (e.g.
MarketplaceListingPaginatedResponse
is now present in bothapi/openapi.cloud.yaml
andapi/openapi.yaml
). However, please verify that this breaking change has been communicated to API consumers—if no migration guide or deprecation notice exists yet, consider adding one.
- Verified OpenAPI specs include the new pagination types.
- Check that API consumers have received clear communication regarding this breaking change via a migration guide or deprecation notice.
api/spec/src/app/app.tsp (2)
31-31
: Pagination return type updated for consistencyThe return type change from
Paginated<App>
toPaginatedResponse<App>
aligns with the pagination standardization effort across the API.
71-72
: Updated pagination model in ListAppsRequestReplacing
PaginatedQuery
withQueryPagination
maintains consistency with the pagination changes made throughout the codebase.api/spec/src/customer.tsp (3)
115-116
: Updated pagination parameter for customer subscription listingThe change from
PaginatedQuery
toQueryPagination
is consistent with the API-wide pagination standardization.
137-138
: Updated pagination model in ListCustomersParamsReplacing
PaginatedQuery
withQueryPagination
maintains consistency with the pagination changes throughout the API.
195-196
: Updated pagination model in ListCustomerAppDataParamsThe change from
PaginatedQuery
toQueryPagination
is consistent with the pagination standardization across the API.
Type name change due to using multiple pagination types previously.
Summary by CodeRabbit
New Features
Refactor