feat(api): events v2 API #2495
feat(api): events v2 API #2495
Conversation
📝 WalkthroughWalkthroughThis pull request implements comprehensive pagination updates across the entire API. It introduces a new endpoint for listing ingested events at Changes
Possibly related PRs
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (23)
api/openapi.cloud.yaml (2)
9044-9073: New AppPaginatedResponse Schema Definition.The new schema requires
totalCount,page,pageSize, anditems. However, the descriptions fortotalCount,page, andpageSizeall state "The items in the current page." It is recommended to update these descriptions as follows:
- totalCount: "Total number of items available."
- page: "Current page number."
- pageSize: "Number of items per page."
This clarification will guide API consumers more accurately.
13849-13878: MarketplaceListingPaginatedResponse Schema Updates.Similar to the AppPaginatedResponse, the new schema for marketplace listings includes pagination fields with identical descriptions that seem to incorrectly describe all fields as "The items in the current page." Please update the descriptions for:
- totalCount to indicate the total number of items,
- page to indicate the current page,
- pageSize to indicate the number of items per page.
This adjustment will enhance clarity for consumers of the API.api/openapi.yaml (6)
2137-2144: Add Missing Pagination for Customer Filter.
The inclusion of pagination (Pagination.pageandPagination.pageSize) in this snippet helps standardize the endpoint behavior. Please ensure that the filter parameter (queryCustomerList.type) integrates smoothly with these pagination parameters and that their definitions (e.g., example patterns) meet the intended requirements.
8064-8174: Review New/api/v2/eventsEndpoint for Event Listing.
This new endpoint is well-detailed with extensive query parameters for filtering (using deepObject style) and cursor-based pagination viaCursorPagination.cursorandCursorPagination.limit. Please ensure that:
- The filter schemas (
FilterStringandFilterTime) are accurately defined.- The response schema (
IngestedEventCursorPaginatedResponse) fully accommodates the cursor pagination model.- All error response definitions maintain consistency with other endpoints.
Overall, this implementation meets advanced filtering needs while supporting robust error handling.
8277-8301: Define Cursor-Based Pagination Parameters.
The new definitions forCursorPagination.cursorandCursorPagination.limitare clear and incorporate proper validation (e.g., min/max values and default settings). It would be good to ensure that similar naming conventions are used across all pagination implementations for consistency.
8989-9020: Review and ValidateAppPaginatedResponseSchema.
The schema correctly lists required properties with example values. Consider refining the descriptions to be more precise—for instance, update:
- page: "The current page number."
- pageSize: "The number of items per page."
This can improve clarity for API consumers.
12378-12397: Define Schema for Cursor-Based Event Pagination Response.
The newIngestedEventCursorPaginatedResponseschema is appropriately structured for cursor-based pagination by requiring bothitemsandnextCursor. Verify that themaxItems: 1000constraint is intended and that the description ofnextCursorclearly indicates its role as a pointer for subsequent queries.
14161-14192: ReviewMarketplaceListingPaginatedResponseSchema Descriptions.
The descriptions for the properties (totalCount,page,pageSize, anditems) seem to be generic and repetitive ("The items in the current page."). It would be clearer to revise them as follows:
- totalCount: "Total number of available items."
- page: "Current page number."
- pageSize: "Number of items per page."
- items: "List of marketplace listings."
This will provide better clarity to API consumers.api/client/javascript/src/client/schemas.ts (2)
1855-1874: Schema field descriptions are inconsistent with their purposeThe
AppPaginatedResponseschema has been implemented correctly with the necessary pagination fields (totalCount,page,pageSize,items), but the JSDoc descriptions for most fields incorrectly state "The items in the current page."Each field should have a description that accurately reflects its purpose:
totalCountshould describe the total number of items across all pagespageshould describe the current page numberpageSizeshould describe the number of items per pageitemshas the correct description/** - * @description The items in the current page. + * @description The total number of items across all pages. * @example 500 */ totalCount: number /** - * @description The items in the current page. + * @description The current page number. * @example 1 */ page: number /** - * @description The items in the current page. + * @description The number of items per page. * @example 100 */ pageSize: number
5234-5253: Schema field descriptions are inconsistent with their purposeSimilar to the
AppPaginatedResponseschema, theMarketplaceListingPaginatedResponseschema has field descriptions that don't match their actual purpose./** - * @description The items in the current page. + * @description The total number of items across all pages. * @example 500 */ totalCount: number /** - * @description The items in the current page. + * @description The current page number. * @example 1 */ page: number /** - * @description The items in the current page. + * @description The number of items per page. * @example 100 */ pageSize: numberopenmeter/meterevent/adapter/event.go (1)
65-115: Reduce code duplication between ListEvents and ListEventsV2.The new method implements the V2 API correctly but contains significant duplicated code from the original
ListEventsmethod. Consider extracting the common validation logic into a shared helper function to improve maintainability.+func (a *adapter) validateEventsAgainstMeters(events []streaming.RawEvent, meters []meter.Meter) []meterevent.Event { + validatedEvents := make([]meterevent.Event, len(events)) + for idx, event := range events { + validatedEvent := meterevent.Event{ + ID: event.ID, + Type: event.Type, + Source: event.Source, + Subject: event.Subject, + Time: event.Time, + Data: event.Data, + IngestedAt: event.IngestedAt, + StoredAt: event.StoredAt, + ValidationErrors: make([]error, 0), + } + + for _, m := range meters { + if event.Type == m.EventType { + _, err := meter.ParseEventString(m, event.Data) + if err != nil { + validatedEvent.ValidationErrors = append(validatedEvent.ValidationErrors, err) + } + } + } + + validatedEvents[idx] = validatedEvent + } + return validatedEvents +} func (a *adapter) ListEventsV2(ctx context.Context, params meterevent.ListEventsV2Params) (pagination.Result[meterevent.Event], error) { // Validate input if err := params.Validate(); err != nil { return pagination.Result[meterevent.Event]{}, models.NewGenericValidationError( fmt.Errorf("validate input: %w", err), ) } // Get all events events, err := a.streamingConnector.ListEventsV2(ctx, params) if err != nil { return pagination.Result[meterevent.Event]{}, fmt.Errorf("query events: %w", err) } // Get all meters meters, err := meter.ListAll(ctx, a.meterService, meter.ListMetersParams{ Namespace: params.Namespace, }) if err != nil { return pagination.Result[meterevent.Event]{}, fmt.Errorf("get meters: %w", err) } - // Validate events against meters - validatedEvents := make([]meterevent.Event, len(events)) - for idx, event := range events { - validatedEvent := meterevent.Event{ - ID: event.ID, - Type: event.Type, - Source: event.Source, - Subject: event.Subject, - Time: event.Time, - Data: event.Data, - IngestedAt: event.IngestedAt, - StoredAt: event.StoredAt, - ValidationErrors: make([]error, 0), - } - - for _, m := range meters { - if event.Type == m.EventType { - _, err = meter.ParseEventString(m, event.Data) - if err != nil { - validatedEvent.ValidationErrors = append(validatedEvent.ValidationErrors, err) - } - } - } - - validatedEvents[idx] = validatedEvent - } + validatedEvents := a.validateEventsAgainstMeters(events, meters) return pagination.NewResult(validatedEvents), nil }openmeter/streaming/clickhouse/event_query_v2_test.go (1)
96-98: Consider adding a test case for the ascending time order.The tests currently only cover the default descending time order. Since ordering is important for pagination, it would be valuable to also test the case where results are ordered by time in ascending order.
openmeter/meterevent/httphandler/event_v2.go (1)
30-33: Consider adding logging for validation errors.When parameter validation fails, adding debug-level logging with the specific validation errors would help with troubleshooting API usage issues without exposing internal details in the HTTP response.
func(ctx context.Context, r *http.Request, params ListEventsV2Params) (ListEventsV2Request, error) { ns, err := h.resolveNamespace(ctx) if err != nil { return ListEventsV2Request{}, err } p, err := convertListEventsV2Params(params, ns) if err != nil { + logger := h.logger.With().Str("operation", "listEventsV2").Logger() + logger.Debug().Err(err).Msg("invalid parameters for list events v2") return ListEventsV2Request{}, models.NewGenericValidationError(err) } return p, nil },pkg/filter/filter.go (1)
77-111: Consider extracting common validation logic.The implementations of
ValidateWithComplexityacross different filter types contain significant duplication. Consider extracting the common logic into a helper function to improve maintainability.You could create a generic helper that handles the common validation pattern:
// validateFilterComplexity is a helper for implementing ValidateWithComplexity across filter types func validateFilterComplexity[T Filter]( filter T, maxDepth int, hasLogicalOps bool, validateAndFilters func(int) error, validateOrFilters func(int) error, ) error { // First validate the filter itself if err := filter.Validate(); err != nil { return err } // Check if we're at a logical operator and need to validate the depth if hasLogicalOps { if maxDepth <= 0 { return errors.New("filter complexity exceeds maximum allowed depth") } // Validate nested filters with decremented depth if err := validateAndFilters(maxDepth - 1); err != nil { return err } if err := validateOrFilters(maxDepth - 1); err != nil { return err } } return nil }This would reduce duplication and make it easier to maintain consistent behavior across filter types.
Also applies to: 193-227, 296-330, 398-432
pkg/filter/filter_test.go (5)
1009-1152: Thorough coverage for validating string filters with depth constraints.The table-driven tests appear correct and comprehensive. As a small improvement, consider adding a boundary test for non-positive maxDepth (e.g., 0 or -1) to confirm how the code behaves in those scenarios.
1154-1276: Comprehensive depth validation tests for integer filters.These tests mirror the string-filter approach and seem consistent with established patterns. Same nitpick as above: you could add a test case for non-positive maxDepth to document expected behavior more clearly.
1278-1374: Good test coverage for float filter depth constraints.The nested filter tests align well with the integer and string filter tests. Consider also verifying negative or zero maxDepth outcomes if relevant to your application’s requirements.
1376-1409: Sufficient boolean filter complexity tests.This set is minimal compared to string/integer/float/time but appears sufficient. For completeness, you might add an out-of-scope test if negative or zero maxDepth is possible or expected.
1411-1509: Time filter complexity tests look robust.All typical nesting scenarios are tested thoroughly. Similar to other tests, consider verifying behavior for non-positive maxDepth values if that edge case is relevant.
openmeter/meterevent/httphandler/mapping.go (1)
31-34: Trivial namespace conversion function.Currently, it’s a pass-through. If you don’t plan to transform the namespace in the future, this function may be unnecessary formalism.
openmeter/streaming/clickhouse/event_query_v2.go (1)
101-138: Selective filter application for toCountRowSQL.Excluding certain filters for performance is a valid approach, but please document that the reported row count may omit ID/source/ingested_at constraints, causing possible count overestimation.
openmeter/apiconverter/filter.go (2)
1-3: Consider pinning your code generation tool versions.The
go:generatedirective looks good, but you might want to pin a specific version ofgoverterto ensure reproducible builds. Otherwise, future upstream updates might introduce breaking changes.
12-23: Ensure thorough testing for each conversion variable.You’ve declared multiple conversion function variables (
ConvertString,ConvertInt, etc.). Make sure they’re covered by unit tests, especially since filter logic can be complex. This testing ensures that surrogates like pointer-to-struct toggles, edge cases (e.g., nil inputs), and real-world filter usage are verified.Would you like assistance creating unit tests that validate each of these conversions under edge conditions?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (36)
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)go.mod(2 hunks)openmeter/apiconverter/cursor.go(1 hunks)openmeter/apiconverter/filter.gen.go(1 hunks)openmeter/apiconverter/filter.go(1 hunks)openmeter/app/httpdriver/app.go(1 hunks)openmeter/app/httpdriver/marketplace.go(1 hunks)openmeter/meterevent/adapter/event.go(2 hunks)openmeter/meterevent/httphandler/event.go(1 hunks)openmeter/meterevent/httphandler/event_v2.go(1 hunks)openmeter/meterevent/httphandler/handler.go(1 hunks)openmeter/meterevent/httphandler/mapping.gen.go(1 hunks)openmeter/meterevent/httphandler/mapping.go(3 hunks)openmeter/meterevent/service.go(4 hunks)openmeter/server/router/event.go(1 hunks)openmeter/server/server_test.go(1 hunks)openmeter/streaming/clickhouse/connector.go(2 hunks)openmeter/streaming/clickhouse/event_query_v2.go(1 hunks)openmeter/streaming/clickhouse/event_query_v2_test.go(1 hunks)openmeter/streaming/connector.go(1 hunks)openmeter/streaming/testutils/streaming.go(1 hunks)pkg/filter/filter.go(6 hunks)pkg/filter/filter_test.go(1 hunks)pkg/pagination/v2/cursor.go(1 hunks)pkg/pagination/v2/cursor_test.go(1 hunks)tools.go(1 hunks)
💤 Files with no reviewable changes (2)
- api/spec/src/pagination.tsp
- api/spec/src/app/stripe.tsp
🧰 Additional context used
🧬 Code Definitions (18)
openmeter/streaming/connector.go (3)
api/api.gen.go (1) (1)
ListEventsV2Params(6509-6525)openmeter/meterevent/service.go (1) (1)
ListEventsV2Params(121-142)openmeter/meterevent/httphandler/event_v2.go (1) (1)
ListEventsV2Params(15-15)
openmeter/apiconverter/cursor.go (1)
pkg/pagination/v2/cursor.go (2) (2)
Cursor(10-13)DecodeCursor(39-48)
openmeter/streaming/testutils/streaming.go (3)
openmeter/meterevent/service.go (1) (1)
ListEventsV2Params(121-142)openmeter/meterevent/httphandler/event_v2.go (1) (1)
ListEventsV2Params(15-15)openmeter/streaming/connector.go (1) (1)
RawEvent(25-35)
openmeter/meterevent/adapter/event.go (2)
openmeter/meterevent/service.go (2) (2)
ListEventsV2Params(121-142)Event(46-65)openmeter/streaming/testutils/streaming.go (15) (15)
m(37-40)m(42-48)m(50-55)m(57-59)m(69-71)m(73-75)m(77-79)m(81-83)m(85-87)m(89-91)m(95-114)m(116-118)m(120-122)m(125-196)m(198-200)
openmeter/meterevent/httphandler/mapping.gen.go (5)
openmeter/meterevent/httphandler/mapping.go (2) (2)
convertListEventsV2Params(28-28)convertNamespace(32-34)openmeter/meterevent/httphandler/event_v2.go (1) (1)
ListEventsV2Params(15-15)openmeter/meterevent/service.go (1) (1)
ListEventsV2Params(121-142)openmeter/apiconverter/cursor.go (1) (1)
ConvertCursorPtr(12-18)openmeter/apiconverter/filter.go (2) (2)
ConvertStringPtr(14-14)ConvertTimePtr(20-20)
openmeter/server/server_test.go (3)
openmeter/meterevent/service.go (1) (1)
ListEventsV2Params(121-142)openmeter/meterevent/httphandler/event_v2.go (1) (1)
ListEventsV2Params(15-15)openmeter/streaming/connector.go (1) (1)
RawEvent(25-35)
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(8328-8329)
openmeter/apiconverter/filter.go (1)
pkg/filter/filter.go (5) (5)
FilterString(30-45)FilterInteger(154-163)FilterFloat(258-267)FilterTime(361-368)FilterBoolean(459-461)
openmeter/streaming/clickhouse/event_query_v2_test.go (4)
openmeter/streaming/clickhouse/event_query_v2.go (1) (1)
queryEventsTableV2(12-16)openmeter/meterevent/service.go (1) (1)
ListEventsV2Params(121-142)pkg/filter/filter.go (7) (7)
FilterString(30-45)FilterTime(361-368)_(22-22)_(23-23)_(24-24)_(25-25)_(26-26)pkg/pagination/v2/cursor.go (1) (1)
Cursor(10-13)
openmeter/server/router/event.go (3)
openmeter/meterevent/adapter/event.go (2) (2)
a(13-63)a(65-115)openmeter/server/router/router.go (1) (1)
Router(171-191)api/api.gen.go (17) (17)
params(10044-10044)params(10204-10204)params(10345-10345)params(10422-10422)params(10591-10591)params(10776-10776)params(10891-10891)params(10943-10943)params(11065-11065)params(11185-11185)params(11221-11221)params(11345-11345)params(11469-11469)params(11566-11566)params(11713-11713)params(11868-11868)ListEventsV2Params(6509-6525)
openmeter/streaming/clickhouse/connector.go (4)
openmeter/streaming/testutils/streaming.go (3) (3)
c(61-63)c(65-67)_(17-17)openmeter/streaming/connector.go (2) (2)
Connector(37-50)RawEvent(25-35)openmeter/meterevent/service.go (1) (1)
ListEventsV2Params(121-142)openmeter/streaming/clickhouse/event_query_v2.go (1) (1)
queryEventsTableV2(12-16)
openmeter/meterevent/httphandler/event_v2.go (3)
openmeter/meterevent/service.go (2) (2)
ListEventsV2Params(121-142)p(145-199)openmeter/meterevent/httphandler/handler.go (2) (2)
h(30-37)handler(24-28)openmeter/meterevent/httphandler/mapping.go (2) (2)
convertListEventsV2Params(28-28)convertListEventsV2Response(70-84)
openmeter/streaming/clickhouse/event_query_v2.go (3)
openmeter/meterevent/httphandler/event_v2.go (1) (1)
ListEventsV2Params(15-15)openmeter/meterevent/service.go (1) (1)
ListEventsV2Params(121-142)pkg/pagination/v2/cursor.go (1) (1)
Cursor(10-13)
pkg/filter/filter.go (1)
api/api.gen.go (5) (5)
FilterString(2423-2465)FilterInteger(2396-2420)FilterFloat(2369-2393)FilterTime(2468-2486)FilterBoolean(2363-2366)
pkg/filter/filter_test.go (1)
pkg/filter/filter.go (5) (5)
FilterString(30-45)FilterInteger(154-163)FilterFloat(258-267)FilterBoolean(459-461)FilterTime(361-368)
openmeter/apiconverter/filter.gen.go (2)
openmeter/apiconverter/filter.go (10) (10)
ConvertBoolean(21-21)ConvertBooleanPtr(22-22)ConvertFloat(17-17)ConvertFloatPtr(18-18)ConvertInt(15-15)ConvertIntPtr(16-16)ConvertString(13-13)ConvertStringPtr(14-14)ConvertTime(19-19)ConvertTimePtr(20-20)pkg/filter/filter.go (5) (5)
FilterBoolean(459-461)FilterFloat(258-267)FilterInteger(154-163)FilterString(30-45)FilterTime(361-368)
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)
openmeter/meterevent/httphandler/mapping.go (2)
openmeter/meterevent/service.go (4) (4)
ListEventsV2Params(121-142)Event(46-65)i(76-118)e(68-73)openmeter/meterevent/httphandler/event_v2.go (1) (1)
ListEventsV2Params(15-15)
🔇 Additional comments (67)
api/openapi.cloud.yaml (5)
43-57: Updated Pagination for List Apps Endpoint.The endpoint now references
Pagination.pageandPagination.pageSizefor its query parameters and uses theAppPaginatedResponseschema for responses. Please ensure that both the client and documentation are updated accordingly to reflect these changes.
2064-2071: Updated Pagination for List Customers Endpoint.The parameters now include
Pagination.pageandPagination.pageSizealongside existing ordering and filtering parameters. Confirm that this update is consistent with similar endpoints and that downstream validations/processes are adjusted as needed.
2138-2145: Enhanced Query Parameters for Customer List Filtering.The integration of pagination parameters (
Pagination.pageandPagination.pageSize) with the customer type query offers better control of result sets. Verify the expected behavior when these parameters interact with other query filters.
2443-2450: Refined Pagination in Parameterized Endpoint.Here, new pagination parameters have been added in a context where an ID field is defined with a specific pattern and example. Double-check that introducing pagination here does not conflict with the validation constraints of the ID field and that schema validation behaves as expected.
3743-3757: Updated Pagination for Marketplace Listings Endpoint.The changes add
Pagination.pageandPagination.pageSizeparameters and update the response schema toMarketplaceListingPaginatedResponse. Make sure that the new response schema fully encapsulates the pagination details and that client-side code consuming this endpoint is updated.tools.go (1)
8-8: Good addition of Goverter dependencyAdding Goverter is a good choice for implementing type converters, which will help with the event API implementations.
pkg/pagination/v2/cursor.go (1)
26-36: Good addition of cursor validationAdding validation is essential for cursor-based pagination to prevent invalid cursors from being used. The implementation correctly checks both the time and ID fields with clear error messages.
openmeter/meterevent/httphandler/event.go (1)
1-1: Consistent package naming updateThe package rename from
httpdrivertohttphandleraligns with the broader standardization of HTTP handling components across the codebase.api/codegen.yaml (2)
1-1: Good addition of YAML schema referenceAdding the schema reference improves editor support and validation for this configuration file.
13-14: Appropriate use of skip-prune optionSetting
skip-prune: trueensures all components from the OpenAPI spec are generated, which is important when developing new API endpoints like the v2 events API.openmeter/meterevent/httphandler/handler.go (1)
1-1: Package name change aligns with architectural refactoringThe package name change from
httpdrivertohttphandlerappears to be part of a broader architectural refactoring across the codebase. This change better reflects the purpose of the package and creates more consistent naming conventions.openmeter/streaming/connector.go (1)
42-42:✅ Verification successful
New V2 API method added to interface
The addition of
ListEventsV2method to theConnectorinterface introduces a newer, more structured approach to event listing. This method properly consolidates parameters into a singleListEventsV2Paramsobject, which includes the namespace as a field rather than a separate parameter. The method supports the new V2 events API endpoint with enhanced filtering and cursor-based pagination capabilities.
🏁 Script executed:
#!/bin/bash # Verify that all implementations of the Connector interface have been updated # to include the new ListEventsV2 method echo "Checking for implementations of Connector interface that need to be updated:" rg --type go "type \w+ struct" -A 5 -B 5 | rg "Connector" | rg -v "interface" | rg -v "Mock" echo -e "\nChecking for implementations of the ListEventsV2 method:" rg --type go "func \(\w+ \*\w+\) ListEventsV2\("Length of output: 8842
V2 API Method Update Verified
The new
ListEventsV2method is correctly added to theConnectorinterface, consolidating parameters into theListEventsV2Paramsobject as intended. Verification confirms that all relevant implementations—including those in production code and tests—now support this method and its enhanced filtering and cursor-based pagination capabilities.api/openapi.yaml (4)
42-56: Ensure Consistent Pagination for List Apps Endpoint.
The addition of the pagination parameters (Pagination.pageandPagination.pageSize) and the use of theAppPaginatedResponseschema appears consistent. Please verify that the referenced schema is fully aligned with the new pagination model and that any default values or validations are in sync.
2063-2070: Confirm Pagination and Ordering for Customer Listing.
Adding pagination parameters to the "List customers" endpoint, alongside ordering options, is in line with the updated patterns. Double-check that the ordering parameters (CustomerOrderByOrdering.orderandCustomerOrderByOrdering.orderBy) and the additional parameter (queryCustomerList.includeDeleted) are defined as expected in the components section.
2442-2449: Standardize Pagination for Filtered ID Endpoint.
The integration of pagination parameters in this segment promotes consistency. Verify that the pagination references in the parameters are correct and that no old or deprecated parameters remain.
3742-3756: Ensure Consistent Pagination for Marketplace Listing.
The "List available apps" endpoint now leverages pagination parameters and a revised response schema (MarketplaceListingPaginatedResponse). Please confirm that the new schema accurately reflects the necessary pagination details and that the endpoint's response documentation is comprehensive.api/client/javascript/src/client/schemas.ts (4)
8066-8066: LGTM! Type export for new paginated responseThe type export for
AppPaginatedResponseis properly defined and references the schema correctly.
8328-8329: LGTM! Type export for new paginated responseThe type export for
MarketplaceListingPaginatedResponseis properly defined and references the schema correctly.
8672-8672: LGTM! Updated response type to use paginated structureThe response type has been correctly updated to use the new
AppPaginatedResponseschema.
13487-13487: LGTM! Updated response type to use paginated structureThe response type has been correctly updated to use the new
MarketplaceListingPaginatedResponseschema.openmeter/streaming/testutils/streaming.go (1)
77-79: Implementation of ListEventsV2 is a suitable stub for testingThe new
ListEventsV2method correctly implements the interface method with the appropriate signature. As this is in a test utilities package, the minimal implementation returning an empty slice and nil error is appropriate, allowing test cases to customize the behavior as needed.Consider adding a documentation comment explaining the purpose of this method and its relation to the updated API endpoints.
go.mod (2)
40-40: Addition of goverter dependency is appropriate for the code generation needsThe
github.com/jmattheis/goverterlibrary is being used for code generation in the new and modified converter files (likeopenmeter/apiconverter/filter.go). This is a suitable choice for generating type conversion code, which is heavily used in the new pagination and filtering functionality.
117-117: Indirect dependency added correctlyThe
github.com/dave/jenniferlibrary is correctly marked as an indirect dependency, as it's likely required by the goverter library for code generation features.openmeter/apiconverter/cursor.go (2)
8-10: Well-implemented cursor conversion functionThe
ConvertCursorfunction correctly delegates to the pagination package'sDecodeCursormethod, maintaining a clear separation of concerns and avoiding code duplication.
12-18: Proper nil handling in cursor pointer conversionThe
ConvertCursorPtrfunction properly handles nil input by returning nil values, preventing nil pointer dereferences and following best practices for optional parameter handling.openmeter/server/server_test.go (1)
602-616: Implementation of ListEventsV2 method looks goodThe
ListEventsV2method properly implements the test mock for the new v2 events API. It follows the same pattern as the existingListEventsmethod while adapting to the new parameter type (ListEventsV2Params), which supports the enhanced filtering capabilities introduced in this PR.openmeter/app/httpdriver/marketplace.go (1)
23-23: Response type updated to support paginationGood update changing the response type from a simple list to a paginated response structure, which aligns with the broader pagination improvements in this PR. The implementation in
ListMarketplaceListings()already returns the response with the required pagination fields, so this change improves type consistency.openmeter/meterevent/httphandler/mapping.gen.go (1)
1-34: Generated conversion function for the new v2 API looks goodThis auto-generated file correctly implements the conversion from API parameters to internal service parameters for the new v2 events API. It includes proper error handling for cursor conversion and correctly maps all filter fields. The implementation aligns well with the enhanced filtering capabilities introduced in this endpoint.
openmeter/app/httpdriver/app.go (1)
20-20: Response type updated to support paginationGood update to change the response type from
api.AppListtoapi.AppPaginatedResponse, which aligns with the broader pagination improvements in this PR. The implementation inListApps()already returns a response with the required pagination fields, so this change improves type consistency.api/spec/src/query.tsp (2)
130-149: Well-implemented cursor pagination model with appropriate constraints.The
QueryCursorPaginationmodel is well-designed with:
- Descriptive documentation
- Optional cursor parameter
- Appropriate limit constraints (1-1000)
- Sensible default limit (100)
This implementation follows best practices for cursor-based pagination.
151-167: Good implementation of cursor pagination response model.The
CursorPaginatedResponse<T>model provides a clear contract for paginated responses with:
- Properly annotated
@pageItemsfor the items collection- Appropriate
@maxItems(1000)constraint- Clear documentation for the
nextCursorfieldThis model complements the existing pagination models and provides a modern alternative to offset/page-based pagination.
api/spec/src/app/app.tsp (2)
31-31: Updated return type to use the new pagination response model.The change from
Paginated<App>toPaginatedResponse<App>correctly implements the updated pagination model, ensuring consistency across the API.
71-71: Updated to use new pagination model consistently.Replacing
...PaginatedQuerywith...QueryPaginationmaintains consistency with the pagination model changes across the API.api/spec/src/events.tsp (1)
134-175: Well-designed new V2 events endpoint with advanced filtering and cursor pagination.The new
EventsV2interface provides significant improvements:
- Uses cursor-based pagination for better performance with large datasets
- Offers more flexible filtering with
FilterStringandFilterTimetypes- Properly documented with clear method description
- Correctly marked as internal with
@extension("x-internal", true)- Maintains API versioning best practices by creating a new
/api/v2/eventsrouteThe implementation effectively balances backward compatibility (by keeping the original
Eventsinterface) while introducing enhanced functionality.api/spec/src/customer.tsp (3)
115-115: Updated to use new pagination model.The change from
...PaginatedQueryto...QueryPaginationcorrectly implements the updated pagination model for customer subscriptions.
137-137: Updated to use new pagination model.The change from
...PaginatedQueryto...QueryPaginationcorrectly implements the updated pagination model for customer listing parameters.
195-195: Updated to use new pagination model.The change from
...PaginatedQueryto...QueryPaginationcorrectly implements the updated pagination model for customer app data parameters.api/spec/src/app/marketplace.tsp (1)
27-29: Update to modern pagination pattern looks good.The change from
PaginatedQuerytoQueryPaginationand fromPaginated<MarketplaceListing>toPaginatedResponse<MarketplaceListing>aligns well with the broader API pagination updates across the system.pkg/pagination/v2/cursor_test.go (1)
190-239: Well-structured test for cursor validation.The test covers the key validation scenarios (valid cursor, zero time, empty ID) using a clean table-driven approach. The error message validation is appropriately implemented.
openmeter/streaming/clickhouse/event_query_v2_test.go (3)
14-124: Comprehensive test coverage for the SQL generation functionality.The test cases cover a good range of scenarios including basic queries, filtering by ID, subject, time, cursor, and ingested_at fields. Each test verifies both the generated SQL string and the expected arguments.
126-200: Good separation of tests for SQL generation and count operations.This test function properly validates that count queries are generated correctly for different filter combinations. The separation from the main query tests improves test clarity.
203-206: Clean helper function implementation.This utility function makes the test code more readable by abstracting the creation of string pointers.
openmeter/meterevent/httphandler/event_v2.go (2)
14-19: Good use of type aliases for clarity.The type aliases provide clear documentation of the handler's input/output types and make the code more readable. This approach maintains consistency with other handlers in the codebase.
21-51: Well-structured handler with proper error handling.The handler follows the established pattern in the codebase, with clear separation between parameter conversion and service interaction. The error handling is appropriate, converting errors to the right types for HTTP responses.
openmeter/meterevent/service.go (6)
8-11: Appropriate package imports for the new functionality.The import of filter package and pagination/v2 package aligns with the new functionality being added.
20-20: Interface updated to support the new listing method.Adding the
ListEventsV2method to the Service interface enhances the API with cursor-based pagination and advanced filtering capabilities.
67-73: Effective cursor implementation for pagination.The
Cursormethod efficiently creates a pagination cursor from the Event's ID and Time fields, which enables straightforward implementation of cursor-based pagination throughout the system.
120-142: Well-designed parameter structure for the V2 API.The
ListEventsV2Paramsstruct provides a comprehensive set of filtering options with proper field documentation. The use of pointer types for optional parameters is appropriate.
144-199: Thorough validation implementation.The validation method checks all necessary conditions, including:
- Required namespace
- Cursor validation
- Filter complexity validation
- Mutual exclusion between time and ingested_at filters
- Clear error messages with proper context
This ensures robust input validation before query execution.
182-184: Good constraint on mutually exclusive filters.Preventing the simultaneous use of time and ingested_at filters is a sensible constraint that helps maintain query clarity and performance.
pkg/filter/filter.go (4)
15-16: Interface expanded with complexity validation method.Adding
ValidateWithComplexityto the Filter interface provides a standardized way to control query complexity across all filter types.
77-111: Well-implemented complexity validation for string filters.The implementation:
- First validates the basic filter structure
- Checks if logical operators are present
- Validates the depth limit
- Recursively validates nested filters with decremented depth
This effectively limits query complexity while maintaining flexibility.
193-227: Consistent implementation across filter types.The
ValidateWithComplexitymethod follows the same pattern for all filter types, maintaining consistency in the codebase. This makes the code easier to understand and maintain.Also applies to: 296-330, 398-432
468-472: Simplified implementation for boolean filters.The implementation for
FilterBooleancorrectly recognizes that boolean filters don't have nested structures and simply delegates to the basic validation method.openmeter/meterevent/httphandler/mapping.go (4)
1-2: No issues with go:generate directive and package declaration.This setup for code generation is conventional and looks good.
14-14: New pagination import reference.This addition for v2 pagination is concise and logical. No concerns here.
17-29: Goverter variable definitions are properly configured.They match the intended code-generation workflows, with clear directives for namespace handling and extension mappings.
70-84: Implementation of convertListEventsV2Response.Logic for converting and aggregating events, plus setting the cursor, looks neat. Partial results are discarded on error, which is acceptable in typical “convert all or fail” flows.
openmeter/streaming/clickhouse/event_query_v2.go (3)
1-2: New package and file introduction.The file layout is standard, and the package name is consistent with existing ClickHouse integrations.
3-16: Struct definition and imports for queryEventsTableV2.No immediate issues spotted. Fields match the
ListEventsV2Paramscontract, and imports are minimal.
19-99:✅ Verification successful
toSQL implements descending pagination logic.
The approach to building the query using “time” or “ingested_at” and applying cursor-based filtering appears correct. Suggest verifying that strictly using
<(rather than<=) for IDs is desired, as equality might be hypothetically included for tie cases.
🏁 Script executed:
#!/bin/bash # Confirm all references to ID-based pagination logic reflect strict < usage rg 'LessThan\(\"id\"' -A 2Length of output: 121
I'll generate a revised shell script to re-run the verification with corrected escaping so we can confirm that all ID-based comparisons are indeed using strict
<as expected.Attention: Verify Strict '<' Usage for ID Pagination
The
toSQL()method implements descending pagination using strict<for IDs. Please confirm that using a strict comparison (i.e. not including equality) for IDs is indeed the intended behavior when two events share the same timestamp. The updated verification script above should help ensure that all ID comparisons use<exclusively.
🏁 Script executed:
#!/bin/bash # Re-verify that ID-based pagination logic uses strict "<" comparison. # This command searches for any usage of "LessThan" with the "id" field. rg "LessThan\\(\"id\"" -A 2Length of output: 351
Strict
<Comparison for ID Pagination Verified
- The
toSQL()method inopenmeter/streaming/clickhouse/event_query_v2.gocorrectly applies a strict<comparison for theidfield when used in cursor-based pagination.- Verification confirms that the line
enforces strict ordering, ensuring tie-breaking for events sharing the same timestamp.query.LessThan("id", q.Params.Cursor.ID)No changes are required unless a design decision is made to include equality for IDs.
openmeter/apiconverter/filter.go (1)
9-11: No immediate concerns with goverter directives.These annotations (
// goverter:variables,// goverter:skipCopySameType, and// goverter:output:file) look fine. Just be sure that your build or CI pipeline properly runsgo generateso these instructions stay in sync with your main codebase.openmeter/apiconverter/filter.gen.go (5)
12-24: Validate pointer usage for Boolean conversions.The pointer logic in
ConvertBooleanPtrcorrectly handles nil checks. This is critical for optional filter fields. The approach is safe for a straightforward mapping. No changes needed.
25-62: Double-check the float boundary conditions.The float conversion code looks correct for typical operations. However, watch out for boundary edge cases such as NaN or ±Inf if your upstream input can contain invalid float data. You might want to add validation or mention these edge conditions in documentation.
63-100: Integer conversions appear solid.You’re preserving conditional operators (
Eq,Ne,Gt, etc.) and replicating theAnd/Orlogic thoroughly. No issues spotted.
101-144: String filtering logic is accurately mirrored.All string-based filters (
Like,Ilike, etc.) and logical operators are mapped properly. This generated code is consistent with the pointer check pattern in the other filters.
145-180: Time filter conversion is consistent with other filter mappings.The code correctly sets up the time-based operators (
Gt,Gte, etc.) and recursively handlesAnd/Orwith no evident logic flaws. Looks good.
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (18)
pkg/filter/filter.go (2)
193-227: Code duplication with FilterString implementationThe implementation for
FilterIntegerfollows the exact same pattern asFilterString. Consider extracting this logic to a helper function to reduce duplication, although this would require some Go generics or interface manipulation.
296-330: Duplicated validation logicSimilar to the previous comment, this implementation for
FilterFloatrepeats the same pattern as the other filter types. While it works correctly, consider refactoring to reduce duplication in future iterations.pkg/filter/filter_test.go (1)
1010-1509: Consider parameterized tests to reduce duplicationThe test functions for different filter types follow very similar patterns. Consider using a parameterized testing approach or table-driven tests that could handle multiple filter types, reducing the amount of repeated test code.
openmeter/streaming/testutils/streaming.go (1)
77-79: Empty mock implementation for ListEventsV2The mock implementation currently returns an empty array with no error. For testing purposes, it would be beneficial to make this mock more useful by implementing test data generation.
Consider enhancing the mock to:
- Track calls to this method
- Return configurable test data
- Support test-specific behaviors
func (m *MockStreamingConnector) ListEventsV2(ctx context.Context, params meterevent.ListEventsV2Params) ([]streaming.RawEvent, error) { - return []streaming.RawEvent{}, nil + // Create a basic mock event for testing + mockEvent := streaming.RawEvent{ + Namespace: params.Namespace, + ID: "mock-event-id", + Type: "mock-event-type", + Source: "mock-source", + Subject: "mock-subject", + Time: time.Now(), + Data: "{\"value\": 1}", + IngestedAt: time.Now(), + StoredAt: time.Now(), + } + + // Return a slice with at least one event for testing + return []streaming.RawEvent{mockEvent}, nil }api/codegen.yaml (2)
1-1: YAML Schema Reference: Consider Pinning a Stable Version
The schema reference on line 1 currently points to the HEAD version of the configuration schema. While this ensures you always have the latest updates, it could lead to unexpected issues if the schema changes. Pinning to a specific version might provide more consistent validation results.
13-14: Output Options: Skipping Prune Added
The newoutput-optionswithskip-prune: trueappears to be intentionally configured to bypass the pruning step in code generation. Please confirm that this behavior is expected and that the impact of retaining non-pruned content in the generated files has been evaluated.api/openapi.yaml (2)
8989-9020: AppPaginatedResponse Schema – Descriptions Review
The updatedAppPaginatedResponseschema includes pagination properties; however, the descriptions for properties liketotalCount,page, andpageSizecurrently read “The items in the current page.” These descriptions can be misleading. For clarity:
• Use “totalCount: The total number of available items.”
• Use “page: The current page number.”
• Use “pageSize: The number of items returned per page.”
Consider revising these to improve clarity in your API documentation.
14161-14192: MarketplaceListingPaginatedResponse Schema – Descriptions Improvement
Similar to the earlier pagination schema, theMarketplaceListingPaginatedResponsehas properties (totalCount,page,pageSize, anditems) with descriptions that currently all read “The items in the current page.” It would improve clarity to adjust these as follows:
•totalCount: “The total number of available marketplace listings.”
•page: “The current page number.”
•pageSize: “Number of listings per page.”
•items: “An array of marketplace listings for the current page.”
This refinement will improve the overall clarity and usability of the API documentation.api/spec/src/query.tsp (1)
130-167: Consider including a hasMore field in the cursor pagination responseWhile the nextCursor field works for pagination flow, many cursor-based pagination implementations also include a boolean
hasMorefield to help clients determine if there are more results to fetch without having to interpret an empty/null cursor.@friendlyName("{name}CursorPaginatedResponse", T) model CursorPaginatedResponse<T> { /** * The items in the response. */ @pageItems @maxItems(1000) items: T[]; /** * The cursor of the last item in the list. */ nextCursor: string; + + /** + * Indicates if there are more items that can be fetched. + */ + hasMore: boolean; }api/openapi.cloud.yaml (1)
9047-9072: Review of AppPaginatedResponse Schema Definition
The introduction of theAppPaginatedResponseschema is solid; however, the property descriptions require improvement for clarity:
- totalCount: Currently described as "The items in the current page." It would be clearer as "Total number of available items."
- page: Should indicate "Current page number" rather than "The items in the current page."
- pageSize: Update to "Number of items per page" for clarity.
Consider applying the following diff:
- totalCount: - type: integer - description: The items in the current page. - example: 500 + totalCount: + type: integer + description: Total number of available items. + example: 500- page: - type: integer - description: The items in the current page. - example: 1 + page: + type: integer + description: Current page number. + example: 1- pageSize: - type: integer - description: The items in the current page. - example: 100 + pageSize: + type: integer + description: Number of items per page. + example: 100api/spec/src/events.tsp (1)
157-174: Consider adding documentation for the filter parameters.While the implementation is solid, the filter parameters (
id,source,subject,type,time,ingestedAt) would benefit from more detailed documentation explaining their usage and format, especially since they're using deep object notation with the new filter types.@query(#{ explode: true, style: "deepObject" }) +/** + * Filter events by ID. + * Supports operations like equals, contains, startsWith, etc. + */ id?: OpenMeter.FilterString,pkg/pagination/v2/cursor_test.go (1)
190-239: Well-structured validation tests for Cursor.The added test function
TestCursorValidationprovides good coverage for the basic validation scenarios of theCursorstruct, including tests for valid cursors, cursors with zero time, and cursors with empty IDs.Consider adding more edge cases to strengthen the test suite:
func TestCursorValidation(t *testing.T) { tests := []struct { name string cursor Cursor wantErr bool errContains string }{ // Existing test cases... + { + name: "Very old time", + cursor: Cursor{ + Time: time.Date(1000, 1, 1, 0, 0, 0, 0, time.UTC), + ID: "test-id", + }, + wantErr: false, + }, + { + name: "ID with special characters", + cursor: Cursor{ + Time: time.Now().UTC(), + ID: "id|with!special@chars#$%^&*()", + }, + wantErr: false, + }, } // Rest of function remains the same }openmeter/meterevent/adapter/event.go (1)
65-115: Good implementation, but consider refactoring duplicated logicThe implementation of
ListEventsV2follows a similar pattern to the existingListEventsmethod, which is good for consistency. However, there's significant code duplication between the two methods. Consider extracting the common logic into a helper function to improve maintainability.+func (a *adapter) validateEvents(ctx context.Context, events []meterevent.Event, namespace string) ([]meterevent.Event, error) { + // Get all meters + meters, err := meter.ListAll(ctx, a.meterService, meter.ListMetersParams{ + Namespace: namespace, + }) + if err != nil { + return nil, fmt.Errorf("get meters: %w", err) + } + + // Validate events against meters + validatedEvents := make([]meterevent.Event, len(events)) + for idx, event := range events { + validatedEvent := meterevent.Event{ + ID: event.ID, + Type: event.Type, + Source: event.Source, + Subject: event.Subject, + Time: event.Time, + Data: event.Data, + IngestedAt: event.IngestedAt, + StoredAt: event.StoredAt, + ValidationErrors: make([]error, 0), + } + + for _, m := range meters { + if event.Type == m.EventType { + _, err = meter.ParseEventString(m, event.Data) + if err != nil { + validatedEvent.ValidationErrors = append(validatedEvent.ValidationErrors, err) + } + } + } + + validatedEvents[idx] = validatedEvent + } + + return validatedEvents, nil +}Then you could simplify both methods to use this helper.
openmeter/streaming/clickhouse/event_query_v2_test.go (1)
70-96: Recommend including negative test cases for cursor boundaries.When using cursor-based pagination (
cursorTime,cursorID), it might be beneficial to include negative test cases (e.g., whencursorTimeis earlier than the earliest event, orcursorIDis smaller than any event’s ID). This can validate proper handling of corner cases and ensure the code gracefully manages out-of-range cursors.openmeter/meterevent/httphandler/mapping.go (3)
1-29: Additional unit tests for conversion logic.
convertListEventsV2Paramsis a key function but isn’t directly tested here. Consider adding unit tests (or a test file) to verify it handles different filter combinations, edge cases (like empty params), and error conditions correctly. This ensures any future changes to the conversion logic remain stable.
36-69: Consider more descriptive error contexts.When JSON unmarshalling fails or setting cloudevents data fails, the error messages are correct but somewhat generic. If possible, enrich them with additional context around the failing event or the data field to help with debugging.
70-84: Sanity-check for large result sets.
convertListEventsV2Responsetransforms potentially large lists of events. For extremely big result sets, be mindful of memory usage during the conversion. Consider streaming responses or imposing upper limits on the requested page size to avoid excessive memory consumption.openmeter/streaming/clickhouse/event_query_v2.go (1)
101-137: Document the omission of some filters in count queries.The
toCountRowSQL()method excludes certain filters to optimize performance. This design choice is valid, but it might surprise users. Add documentation clarifying which filters are skipped and why, so maintainers and users understand the performance vs. accuracy trade-off.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (36)
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)go.mod(2 hunks)openmeter/apiconverter/cursor.go(1 hunks)openmeter/apiconverter/filter.gen.go(1 hunks)openmeter/apiconverter/filter.go(1 hunks)openmeter/app/httpdriver/app.go(1 hunks)openmeter/app/httpdriver/marketplace.go(1 hunks)openmeter/meterevent/adapter/event.go(2 hunks)openmeter/meterevent/httphandler/event.go(1 hunks)openmeter/meterevent/httphandler/event_v2.go(1 hunks)openmeter/meterevent/httphandler/handler.go(1 hunks)openmeter/meterevent/httphandler/mapping.gen.go(1 hunks)openmeter/meterevent/httphandler/mapping.go(3 hunks)openmeter/meterevent/service.go(4 hunks)openmeter/server/router/event.go(1 hunks)openmeter/server/server_test.go(1 hunks)openmeter/streaming/clickhouse/connector.go(2 hunks)openmeter/streaming/clickhouse/event_query_v2.go(1 hunks)openmeter/streaming/clickhouse/event_query_v2_test.go(1 hunks)openmeter/streaming/connector.go(1 hunks)openmeter/streaming/testutils/streaming.go(1 hunks)pkg/filter/filter.go(6 hunks)pkg/filter/filter_test.go(1 hunks)pkg/pagination/v2/cursor.go(1 hunks)pkg/pagination/v2/cursor_test.go(1 hunks)tools.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 (17)
pkg/pagination/v2/cursor.go (1)
openmeter/streaming/clickhouse/connector.go (14) (14)
c(40-62)c(81-96)c(98-113)c(115-118)c(120-123)c(125-128)c(130-163)c(165-187)c(189-191)c(193-196)c(198-213)c(215-243)c(245-257)c(260-279)
openmeter/server/router/event.go (3)
openmeter/meterevent/adapter/event.go (2) (2)
a(13-63)a(65-115)openmeter/server/router/router.go (1) (1)
Router(171-191)api/api.gen.go (17) (17)
params(10044-10044)params(10204-10204)params(10345-10345)params(10422-10422)params(10591-10591)params(10776-10776)params(10891-10891)params(10943-10943)params(11065-11065)params(11185-11185)params(11221-11221)params(11345-11345)params(11469-11469)params(11566-11566)params(11713-11713)params(11868-11868)ListEventsV2Params(6509-6525)
openmeter/app/httpdriver/app.go (3)
api/api.gen.go (1) (1)
AppPaginatedResponse(813-825)api/client/go/client.gen.go (1) (1)
AppPaginatedResponse(712-724)api/client/javascript/src/client/schemas.ts (1) (1)
AppPaginatedResponse(8066-8066)
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(8328-8329)
openmeter/streaming/testutils/streaming.go (3)
openmeter/meterevent/service.go (1) (1)
ListEventsV2Params(121-142)openmeter/meterevent/httphandler/event_v2.go (1) (1)
ListEventsV2Params(15-15)openmeter/streaming/connector.go (1) (1)
RawEvent(25-35)
openmeter/server/server_test.go (2)
openmeter/meterevent/service.go (1) (1)
ListEventsV2Params(121-142)openmeter/streaming/connector.go (1) (1)
RawEvent(25-35)
openmeter/apiconverter/cursor.go (1)
pkg/pagination/v2/cursor.go (2) (2)
Cursor(10-13)DecodeCursor(39-48)
openmeter/streaming/clickhouse/event_query_v2.go (3)
openmeter/meterevent/httphandler/event_v2.go (1) (1)
ListEventsV2Params(15-15)openmeter/meterevent/service.go (1) (1)
ListEventsV2Params(121-142)pkg/pagination/v2/cursor.go (1) (1)
Cursor(10-13)
openmeter/meterevent/httphandler/event_v2.go (3)
openmeter/meterevent/service.go (2) (2)
ListEventsV2Params(121-142)p(145-199)openmeter/meterevent/httphandler/handler.go (2) (2)
h(30-37)handler(24-28)openmeter/meterevent/httphandler/mapping.go (2) (2)
convertListEventsV2Params(28-28)convertListEventsV2Response(70-84)
openmeter/streaming/clickhouse/event_query_v2_test.go (4)
openmeter/streaming/clickhouse/event_query_v2.go (1) (1)
queryEventsTableV2(12-16)openmeter/meterevent/service.go (1) (1)
ListEventsV2Params(121-142)pkg/filter/filter.go (7) (7)
FilterString(30-45)FilterTime(361-368)_(22-22)_(23-23)_(24-24)_(25-25)_(26-26)pkg/pagination/v2/cursor.go (1) (1)
Cursor(10-13)
openmeter/streaming/clickhouse/connector.go (3)
openmeter/streaming/connector.go (2) (2)
Connector(37-50)RawEvent(25-35)openmeter/meterevent/service.go (1) (1)
ListEventsV2Params(121-142)openmeter/streaming/clickhouse/event_query_v2.go (1) (1)
queryEventsTableV2(12-16)
pkg/filter/filter.go (1)
api/api.gen.go (5) (5)
FilterString(2423-2465)FilterInteger(2396-2420)FilterFloat(2369-2393)FilterTime(2468-2486)FilterBoolean(2363-2366)
openmeter/meterevent/adapter/event.go (1)
openmeter/meterevent/service.go (2) (2)
ListEventsV2Params(121-142)Event(46-65)
openmeter/apiconverter/filter.gen.go (2)
openmeter/apiconverter/filter.go (10) (10)
ConvertBoolean(21-21)ConvertBooleanPtr(22-22)ConvertFloat(17-17)ConvertFloatPtr(18-18)ConvertInt(15-15)ConvertIntPtr(16-16)ConvertString(13-13)ConvertStringPtr(14-14)ConvertTime(19-19)ConvertTimePtr(20-20)pkg/filter/filter.go (5) (5)
FilterBoolean(459-461)FilterFloat(258-267)FilterInteger(154-163)FilterString(30-45)FilterTime(361-368)
pkg/filter/filter_test.go (1)
pkg/filter/filter.go (5) (5)
FilterString(30-45)FilterInteger(154-163)FilterFloat(258-267)FilterBoolean(459-461)FilterTime(361-368)
openmeter/apiconverter/filter.go (1)
pkg/filter/filter.go (5) (5)
FilterString(30-45)FilterInteger(154-163)FilterFloat(258-267)FilterTime(361-368)FilterBoolean(459-461)
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 (62)
pkg/filter/filter.go (4)
15-16: Well-documented interface extensionThe addition of
ValidateWithComplexityto theFilterinterface is clear and well-documented. It enhances the API by allowing clients to specify maximum filter nesting depth.
77-111: Solid implementation of filter complexity validationThis implementation for
FilterStringfollows a good pattern:
- It first checks basic validity through the existing
Validate()method- Then ensures that nested filters (
And/Or) don't exceed maximum depth- Recursively validates all child filters with decremented depth
The approach correctly prevents deeply nested filter structures that could potentially cause performance issues or stack overflows.
398-432: Consistent implementation for FilterTimeThe implementation for
FilterTimecorrectly follows the same pattern as the other filter types. This consistency is good for maintainability.
468-472: Simpler implementation for FilterBooleanThe
FilterBooleanimplementation is appropriately simplified since it has no logical operators and can't contain nested filters. This demonstrates good understanding of the different requirements for each filter type.pkg/filter/filter_test.go (5)
1010-1152: Comprehensive test coverage for FilterString complexity validationThe test cases are thorough and well-structured:
- Covering simple and nested filters
- Testing both success and failure scenarios
- Validating depth limit enforcement
- Checking interaction with basic validation
The test naming is clear, and each case has a descriptive name explaining the scenario being tested.
1154-1276: Well-structured tests for FilterIntegerThe tests for
FilterInteger.ValidateWithComplexityfollow the same comprehensive pattern as theFilterStringtests, ensuring consistent testing across filter types.
1278-1374: Complete test coverage for FilterFloatThese tests properly validate the complexity enforcement for float filters, with good coverage of edge cases and error conditions.
1376-1409: Simplified tests for FilterBooleanThe tests for
FilterBooleanare appropriately simplified since boolean filters don't support nesting. Good attention to the specific requirements of each filter type.
1411-1509: Thorough testing for FilterTime complexity validationThe test cases for
FilterTimeare comprehensive and include proper time value handling. The test structure is consistent with the other filter types.tools.go (1)
8-8: New dependency for code generation added.The addition of the
govertertool as a dependency is consistent with the changes ingo.mod. This tool is used for generating converter code, which aligns with the PR objectives of enhancing API pagination and filtering capabilities.pkg/pagination/v2/cursor.go (1)
26-36: Well-implemented validation method for cursor integrity.The
Validatemethod follows good Go practices by performing essential checks on the cursor's fields and returning appropriate error messages. This validation is crucial for ensuring cursor-based pagination works reliably.The implementation checks both critical fields:
- Ensures the timestamp is not zero, which would create invalid pagination anchors
- Verifies the ID is not empty, preventing cursor manipulation issues
This follows the same validation pattern used in other parts of the codebase, as seen in the provided snippets of
Validatemethods from the ClickHouse connector.openmeter/meterevent/httphandler/handler.go (1)
1-1: Package renamed for better clarity and consistency.The package name change from
httpdrivertohttphandlerbetter reflects the purpose of this module, which handles HTTP requests for meter events. According to the summary, this is part of a consistent renaming across multiple files.go.mod (2)
40-40: Added goverter dependency for code generation.The addition of
github.com/jmattheis/goverteraligns with the new import intools.go. This dependency enables efficient generation of type conversion code, which is valuable for the pagination and filtering enhancements being implemented.
117-117: Added indirect dependency for code generation.The
jenniferpackage is likely pulled in as a transitive dependency of goverter, as it's a popular code generation library for Go. Its indirect status is properly marked.openmeter/meterevent/httphandler/event.go (1)
1-1: Package name change fromhttpdrivertohttphandlerThis change aligns with modern Go naming conventions and better represents the role of this package (handling HTTP requests rather than "driving" them). It's a good naming improvement that increases code clarity.
openmeter/streaming/connector.go (1)
42-42: New V2 interface method for enhanced event listingThe addition of
ListEventsV2to the Connector interface is part of introducing the V2 API with improved pagination and filtering capabilities. This is a good approach to versioning API methods while maintaining backward compatibility.Make sure all implementations of this interface (including mocks and tests) are updated to implement this new method.
api/client/javascript/src/client/schemas.ts (1)
8066-8066: Good implementation of paginated response types.The addition of paginated response types and their exports is well-structured and consistent with the Go implementation shown in the context. The response schemas are properly referenced in the endpoint definitions.
Also applies to: 8328-8329, 8672-8672, 13487-13487
api/openapi.yaml (8)
42-56: Pagination Parameters and Response Schema Update for List Apps Endpoint
The addition of the pagination parameters (Pagination.pageandPagination.pageSize) and the updated response schema reference (AppPaginatedResponse) appears to standardize the endpoint with the new pagination model. Please verify that the definitions for these parameters (in the components section) match exactly what is expected for all paginated endpoints.
2063-2070: Standardizing Pagination in List Customers Endpoint
The inclusion of the pagination parameters in addition to ordering and filtering parameters is consistent with the approach seen elsewhere in the API. Confirm that the consumers of this endpoint are aware of the expected pagination mechanism.
2137-2144: Adding Pagination to Query Parameters
The new pagination parameter references in this hunk (lines 2140–2141) complement the existing query parameters. Ensure that the ordering is consistent across endpoints and that the documentation clearly specifies the roles of these pagination parameters.
2442-2449: Ensuring Consistent Pagination in ID-based Endpoint
The insertion ofPagination.pageandPagination.pageSizeafter the ID parameter definition helps standardize the API behavior. Please double-check that the parameter ordering does not inadvertently change the expected client behavior.
3742-3756: Marketplace Listing Pagination Consistency
The changes for the "List available apps" endpoint correctly add the pagination parameters and update the response schema toMarketplaceListingPaginatedResponse. As with other endpoints, verify that the definitions in the components section align with this use case.
8067-8174: New /api/v2/events Endpoint Implementation
The new endpoint, defined under/api/v2/events, is comprehensive—with advanced filtering (via deepObject style), cursor pagination (usingCursorPagination.cursorandCursorPagination.limit), and additional query parameters. The use of detailed filtering schemas (e.g.,FilterStringandFilterTime) and the clear segregation of authentication error responses are well aligned with the API’s design improvement goals. Please confirm that the deepObject style is consistently handled in the server-side implementation.
8277-8301: Defining Cursor Pagination Parameters
The definitions forCursorPagination.cursorandCursorPagination.limitare now added. The constraints on the limit (minimum, maximum, default) look appropriate. Ensure these definitions are referenced by all endpoints leveraging cursor-based pagination.
12378-12397: IngestedEventCursorPaginatedResponse Schema Addition
The addition of theIngestedEventCursorPaginatedResponseschema properly supports the new cursor-pagination mechanism for events. Verify that the constraints (such asmaxItems: 1000on the items array) are correctly documented and enforced. The overall structure is clear and aligns with the new API design.openmeter/app/httpdriver/marketplace.go (1)
23-23: Response type updated to support pagination.The type alias for
ListMarketplaceListingsResponsehas been updated fromapi.MarketplaceListingListtoapi.MarketplaceListingPaginatedResponse. This change aligns with the overall enhancement of pagination support in the API, providing additional metadata like page, pageSize, and totalCount information in responses.openmeter/app/httpdriver/app.go (1)
20-20: Response type updated to support pagination.The type alias for
ListAppsResponsehas been updated fromapi.AppListtoapi.AppPaginatedResponse. This change is consistent with the overall enhancement of pagination in the API, providing additional metadata like page, pageSize, and totalCount in the responses.openmeter/server/server_test.go (1)
602-616: New V2 API method added for mock implementation.This new
ListEventsV2method in theMockStreamingConnectorstruct supports testing of the V2 version of the events API with cursor-based pagination. The implementation correctly returns a mock event, consistent with the existingListEventsmethod.Make sure all tests that rely on this mock implementation are updated to handle the new pagination parameters in
meterevent.ListEventsV2Paramsstruct.openmeter/apiconverter/cursor.go (3)
1-7: New package for API conversion utilities.This new package provides conversion utilities between API types and internal types, which is a good architectural practice to keep these concerns separated.
8-10: Clean implementation of cursor conversion.The
ConvertCursorfunction provides a clean wrapper aroundpagination.DecodeCursorfor converting API cursor strings to internal cursor objects. This promotes reuse and centralization of conversion logic.
12-18: Robust nil-handling in cursor conversion.The
ConvertCursorPtrfunction properly handles nil input by returning nil, nil (a common Go idiom for optional parameters), and otherwise delegates toConvertCursor. This defensive coding prevents potential nil pointer exceptions.openmeter/meterevent/httphandler/mapping.gen.go (3)
1-3: Standard auto-generated code header looks goodThe file correctly includes the auto-generation notice and build constraint to exclude it from builds when using the
govertertool.
6-10: Imports look appropriate for the functionalityThe imports include the necessary packages for API conversion, including the main API package and the specific converter utilities.
12-34: Conversion function looks correct and handles error cases properlyThe conversion function correctly maps fields between
api.ListEventsV2Paramsandmeterevent.ListEventsV2Params. It properly:
- Handles the namespace context parameter
- Performs cursor conversion with error handling
- Conditionally sets the limit only when provided
- Uses appropriate converter helper functions for different field types
Good job handling the error case for cursor conversion by returning early.
api/spec/src/query.tsp (2)
130-149: Well-designed cursor pagination query parametersThe
QueryCursorPaginationmodel provides a clean implementation of cursor-based pagination:
- The cursor parameter is optional, allowing for initial page requests
- The limit is properly constrained between 1-1000 with a sensible default of 100
- Good documentation explaining the purpose of each field
This aligns with best practices for cursor-based pagination.
150-167: Clear and complete cursor pagination response modelThe
CursorPaginatedResponsemodel properly defines:
- A typed array of items with appropriate annotations
- A maximum limit on the number of returned items (1000)
- A nextCursor field for continuing pagination
The model uses TypeSpec features effectively with
@pageItemsand@maxItemsannotations.api/openapi.cloud.yaml (5)
43-57: Pagination Update for List Apps Endpoint Looks Correct
The new pagination parameters (Pagination.pageandPagination.pageSize) and updated response schema (AppPaginatedResponse) have been integrated well. Please ensure that the referenced components are defined consistently in your OpenAPI components.
2064-2071: Consistent Pagination Parameters for List Customers
The replacement of legacy pagination parameters with the newPagination.pageandPagination.pageSizereferences is correct and improves consistency. Verify that any client code using these endpoints is updated to match the new parameter naming conventions.
2138-2145: Addition of Pagination Parameters in Customer List Filter
Adding the pagination parameters after the identifier pattern is properly done. It’s important that the order of parameters in the specification aligns with your expected validation and documentation behavior.
2443-2450: Pagination Parameters for This Endpoint Are Appropriately Included
IntegratingPagination.pageandPagination.pageSizehere along with the rest of the response definitions is consistent with the new pagination strategy. Ensure that the client consuming this API is aware of the new pagination metadata.
3743-3757: Updated Marketplace Listing Endpoint with New Pagination
The changes correctly incorporate the new pagination parameters and update the response schema toMarketplaceListingPaginatedResponse. This ensures consistency with your updated API design.api/spec/src/events.tsp (1)
134-175: Good implementation of a new Events V2 API with cursor pagination and advanced filtering.The
EventsV2interface implements a more sophisticated event listing endpoint with cursor-based pagination and comprehensive filtering capabilities. This is a significant improvement over the V1 API.A few observations:
- The endpoint is marked as internal (
@extension("x-internal", true))- It properly uses deep object style for filter parameters
- The cursor pagination approach is more efficient for handling large datasets
api/spec/src/app/marketplace.tsp (1)
27-29:✅ Verification successful
Updated pagination for marketplace listings.
The pagination model has been updated from using
PaginatedQueryto the newQueryPaginationmodel, with the return type now beingPaginatedResponseinstead ofPaginated. This is consistent with the overall pagination improvements being made across the API.
🏁 Script executed:
#!/bin/bash # Check if QueryPagination is consistently used across the codebase # and verify its implementation # Find all TypeSpec files that might use pagination echo "Searching for QueryPagination usage in the codebase..." rg -A 3 "QueryPagination" --type=tsLength of output: 154
I suspect the initial search didn’t return any results because TypeSpec files use the “.tsp” extension rather than the default TypeScript “.ts” extension. Let’s run a more targeted search for “QueryPagination” in all “.tsp” files to verify its usage across the codebase.
🏁 Script executed:
#!/bin/bash echo "Searching QueryPagination usage in .tsp files..." rg "QueryPagination" -g "*.tsp"Length of output: 1149
Action: Confirm Updated Pagination Implementation
The pagination update in
api/spec/src/app/marketplace.tspis consistent with the overall changes across the codebase. The change fromPaginatedQuerytoQueryPagination, along with updating the return type toPaginatedResponse, is correctly applied—as verified by the consistent usage ofQueryPaginationin other.tspfiles.api/spec/src/app/app.tsp (2)
31-31: Updated return type to use the new pagination response model.The return type has been updated from
Paginated<App>toPaginatedResponse<App>, which aligns with the new pagination strategy in the API.
71-71: Updated pagination model for ListAppsRequest.The
ListAppsRequestmodel has been updated to use the newQueryPaginationmodel instead of the previousPaginatedQuery. This change is part of the broader pagination refactoring across the API.api/spec/src/customer.tsp (3)
115-115: Updated pagination mechanism correctly implementedThe update from
PaginatedQuerytoQueryPaginationensures consistency with the new pagination strategy across the API. This change is consistent with similar changes in other API files.
137-137: Pagination update completed consistentlyThe replacement of
PaginatedQuerywithQueryPaginationin theListCustomersParamsmodel maintains consistency with the pagination approach used throughout the API.
195-195: Pagination update properly applied to all modelsThe consistent application of
QueryPaginationto theListCustomerAppDataParamsmodel completes the pagination standardization across the customer API models.openmeter/meterevent/service.go (5)
9-10: Proper imports for the new pagination and filter functionalityThe addition of the filter and pagination v2 packages is necessary to support the enhanced filtering and pagination capabilities in the new API.
20-20: Interface update correctly exposes the new V2 methodAdding the
ListEventsV2method to theServiceinterface ensures the V2 functionality is properly accessible via the service layer.
67-73: Well-implemented cursor method for Event structThe
Cursormethod is a clean addition that enables cursor-based pagination for events, properly returning both ID and Time in the cursor structure.
120-142: Well-structured parameter object with comprehensive filtering capabilitiesThe
ListEventsV2Paramsstruct is well-designed with:
- Clear field documentation
- Pointer types for optional parameters
- Support for complex filtering on multiple fields
- Separate filter types for string and time fields
This provides significant improvements over the V1 parameters by enabling more sophisticated querying.
144-199: Thorough validation with appropriate complexity checksThe validation method for
ListEventsV2Paramsis comprehensive, checking:
- Required fields
- Cursor validity
- Filter complexity
- Conflicting filters (Time and IngestedAt)
The use of
ValidateWithComplexitywith a complexity level of 1 is appropriate to prevent overly complex queries that could impact performance.openmeter/streaming/clickhouse/connector.go (2)
98-113: Properly implemented ListEventsV2 method with parameter validationThe method correctly validates input parameters before processing and handles potential namespace errors appropriately. Error messages provide good context for debugging.
351-361: Progress tracking implementation maintains consistency with existing methodsThe progress tracking implementation correctly reuses the same pattern as other methods, providing a consistent approach to tracking long-running queries.
openmeter/streaming/clickhouse/event_query_v2_test.go (2)
14-36: Validate edge cases for missing or invalid fields.While these tests verify common scenarios (namespace-only queries, ID filter, etc.), consider adding tests to cover edge cases such as empty namespace, invalid filter structures, or concurrent filters set (e.g., both
TimeandIngestedAt). This ensures robustness and helps detect unexpected behavior early.
126-137: Enhance the verification for count queries.The
TestQueryEventsTableV2_ToCountRowSQLtests confirm basic filtering logic. However, consider verifying performance optimizations or ignoring certain filters in count queries, ensuring that the logic meets user expectations. Confirm if ignoring specific filters (likeID) yields the desired performance versus correctness balance.openmeter/streaming/clickhouse/event_query_v2.go (1)
19-99: Address potential sub-second precision loss.
cursor.Time.Unix()truncates to seconds, potentially causing ordering issues if events arrive within the same second but different nanoseconds. Confirm if second-level granularity is acceptable. If not, consider using higher-resolution timestamps (e.g., UnixNano) or real timestamps in the query to avoid collisions.openmeter/apiconverter/filter.go (1)
1-23: Well-structured converter declarations for filter type conversionsThis file effectively sets up the conversion function declarations between API filter types and internal filter types, with proper goverter directives for code generation. The approach of having separate converter functions for both value and pointer types is good practice for flexibility.
The
skipCopySameTypedirective will help optimize the generated code by avoiding unnecessary conversions when types are identical.openmeter/apiconverter/filter.gen.go (3)
1-3: Appropriate build constraints for auto-generated codeThe build tag
//go:build !goverterensures this file is excluded during goverter's code generation phase, which is the correct approach for auto-generated files.
11-181: Well-implemented filter conversion functions with proper nil handlingThe conversion implementations are thorough and handle all edge cases appropriately:
- Each converter correctly maps all fields from source to destination
- Proper nil checks for pointer fields prevent nil pointer dereferences
- For recursive types (with AND/OR operators), the code correctly converts each element in the slice
- The slice allocation is optimized by pre-allocating with the known size
The pattern of having separate value and pointer converters is well-implemented throughout all filter types.
34-42: Thorough handling of nested filter collectionsThe implementation for the AND/OR filter collections is comprehensive:
- Properly checks if the source collection pointer is nil
- Checks if the actual slice is nil
- Efficiently pre-allocates the destination slice
- Correctly applies the conversion recursively to each element
- Uses different variable names for AND and OR collections to avoid conflicts
This approach ensures that complex nested filter structures are converted correctly.
Also applies to: 44-52, 71-80, 81-90, 115-124, 125-134, 151-160, 161-170
| // ListEventsV2 returns a handler for listing events. | ||
| func (h *handler) ListEventsV2() ListEventsV2Handler { | ||
| return httptransport.NewHandlerWithArgs( | ||
| func(ctx context.Context, r *http.Request, params ListEventsV2Params) (ListEventsV2Request, error) { | ||
| ns, err := h.resolveNamespace(ctx) | ||
| if err != nil { | ||
| return ListEventsV2Request{}, err | ||
| } | ||
|
|
||
| p, err := convertListEventsV2Params(params, ns) | ||
| if err != nil { | ||
| return ListEventsV2Request{}, models.NewGenericValidationError(err) | ||
| } | ||
|
|
||
| return p, nil | ||
| }, | ||
| func(ctx context.Context, params ListEventsV2Request) (ListEventsV2Response, error) { | ||
| events, err := h.metereventService.ListEventsV2(ctx, params) | ||
| if err != nil { | ||
| return ListEventsV2Response{}, err | ||
| } | ||
|
|
||
| return convertListEventsV2Response(events) | ||
| }, | ||
| commonhttp.JSONResponseEncoderWithStatus[ListEventsV2Response](http.StatusOK), | ||
| httptransport.AppendOptions( | ||
| h.options, | ||
| httptransport.WithOperationName("listEventsV2"), | ||
| )..., | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate namespace resolution error handling.
Currently, if resolveNamespace fails, the code returns an internal server error (HTTP 500). Consider returning a 400 (Bad Request) or a more specific status if the user-provided context or namespace is invalid. This can improve client feedback and clarity on errors originating from missing/invalid namespaces.
- return "", commonhttp.NewHTTPError(http.StatusInternalServerError, errors.New("internal server error"))
+ return "", commonhttp.NewHTTPError(http.StatusBadRequest, errors.New("invalid or missing namespace"))Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (15)
api/openapi.yaml (6)
42-56: Consistent Pagination Integration in List Apps EndpointThe addition of the pagination parameters (
Pagination.pageandPagination.pageSize) and the update of the response schema toAppPaginatedResponsebrings this endpoint in line with the new pagination strategy. Please double-check that the referenced components are properly defined elsewhere in the spec.
2137-2144: Enhanced Filtering with Pagination for Customer QueriesAdding pagination parameters alongside the existing customer filter (for the type parameter) is a good move. Verify that the order and grouping of these parameters conform to our established query parameter conventions to aid client developers.
3742-3756: Paginated Response for Available AppsUpdating the "List available apps" endpoint to include pagination parameters and switching to
MarketplaceListingPaginatedResponseis a positive move. However, consider revising the property descriptions in the paginated response model for better clarity, as the current descriptions seem somewhat repetitive.
8064-8174: NewlistEventsV2Endpoint ImplementationThis new endpoint introduces advanced filtering and cursor-based pagination, which is a significant enhancement. The query parameters (such as
clientId,id,source, etc.) use a mix of standard and deepObject styles. Ensure that client tools and libraries can correctly handle these deepObject parameters. Adding example values for these new query parameters might further improve the clarity of the documentation.
8277-8301: Definition of Cursor Pagination ParametersThe new definitions for
CursorPagination.cursorandCursorPagination.limitare clear and follow our design pattern. For enhanced clarity, consider adding example values or further descriptive text in these parameter definitions.
12378-12397: Cursor-Paginated Response for Ingested EventsThe newly added
IngestedEventCursorPaginatedResponseclearly outlines the required fields (itemsandnextCursor) for a cursor-based pagination response. For clarity and better API consumption, consider adding example values to these properties.api/spec/src/query.tsp (1)
151-167: Good cursor-based response model implementation.The
CursorPaginatedResponse<T>model follows TypeSpec best practices with proper annotations and type parameters. ThemaxItems(1000)constraint on the items array provides important boundaries for API consumers.Consider adding an
@exampleannotation for thenextCursorfield to improve documentation, similar to what's provided for other fields in the file.openmeter/server/server_test.go (1)
602-616: Mock implementation for ListEventsV2 looks good.The implementation correctly mirrors the existing
ListEventsmethod while adapting to the new parameter type. This maintains consistency in your test mocks while supporting the new V2 API functionality.Since you've added this mock method, consider updating the test cases in
TestRoutesto verify the new V2 endpoint behavior.api/codegen.yaml (1)
13-14: Configuration updated for output options.Adding
skip-prune: trueis likely necessary for the expanded API functionality. However, the purpose of this setting isn't immediately clear to future developers.Consider adding a comment explaining why
skip-prune: trueis needed, such as:output-options: # Prevents removal of unused components which might be referenced by dynamic code skip-prune: trueopenmeter/apiconverter/cursor.go (2)
8-10: Clean utility function for cursor conversion.The function properly delegates to
pagination.DecodeCursorto convert from API to internal cursor types.Consider adding a brief documentation comment explaining the purpose of this function:
// ConvertCursor converts an API cursor to an internal pagination cursor. func ConvertCursor(s api.CursorPaginationCursor) (*pagination.Cursor, error) { return pagination.DecodeCursor(s) }
12-18: Good handling of nil pointer case for cursor conversion.The function properly checks for nil pointers before attempting conversion, which prevents potential nil dereference issues. This pattern is idiomatic Go code for handling optional parameters.
Consider adding a documentation comment explaining the purpose and nil behavior:
// ConvertCursorPtr safely converts an optional API cursor pointer to an internal pagination cursor. // Returns nil, nil if the input pointer is nil. func ConvertCursorPtr(s *api.CursorPaginationCursor) (*pagination.Cursor, error) { // Rest of the function... }api/openapi.cloud.yaml (2)
9044-9072: Refine Property Descriptions in AppPaginatedResponse SchemaThe new
AppPaginatedResponseschema includes the required pagination properties, but the property descriptions fortotalCount,page, andpageSizeare inaccurately labeled as "The items in the current page." Consider updating these descriptions to better reflect their intended meanings. For example:@@ Definition for totalCount - description: The items in the current page. + description: Total number of items available. @@ Definition for page - description: The items in the current page. + description: Current page number. @@ Definition for pageSize - description: The items in the current page. + description: Number of items per page.This improvement will make the API documentation clearer for consumers.
13849-13880: Improve Descriptions in MarketplaceListingPaginatedResponse SchemaSimilar to the previous schema, the
MarketplaceListingPaginatedResponseis structured correctly but the descriptions fortotalCount,page, andpageSizeare misleading. Consider adjusting them as follows:@@ Definition for totalCount - description: The items in the current page. + description: Total number of marketplace listings available. @@ Definition for page - description: The items in the current page. + description: Current page number. @@ Definition for pageSize - description: The items in the current page. + description: Number of listings per page.These refinements will enhance the clarity and usefulness of the generated API documentation.
openmeter/meterevent/httphandler/mapping.go (1)
31-34: Consider ifconvertNamespaceis necessaryThe
convertNamespacefunction simply returns its input. While this might be required by the converter framework, consider adding a comment explaining why this pass-through function is necessary for clarity.// goverter:context namespace +// convertNamespace is a passthrough function required by the goverter framework func convertNamespace(namespace string) string { return namespace }pkg/filter/filter.go (1)
85-108: Consider extracting common validation logicThe validation logic for logical operators is nearly identical across all filter types. Consider extracting this common logic into a helper function to reduce code duplication and improve maintainability.
You could implement a generic helper function:
// validateLogicalOperatorComplexity is a helper function that validates the complexity // of logical operators (AND/OR) and their nested filters func validateLogicalOperatorComplexity[T Filter](maxDepth int, andFilters, orFilters *[]T) error { if maxDepth <= 0 { return errors.New("filter complexity exceeds maximum allowed depth") } // Validate AND filters if andFilters != nil { errs := lo.Map(*andFilters, func(f T, _ int) error { return f.ValidateWithComplexity(maxDepth - 1) }) if err := errors.Join(errs...); err != nil { return err } } // Validate OR filters if orFilters != nil { errs := lo.Map(*orFilters, func(f T, _ int) error { return f.ValidateWithComplexity(maxDepth - 1) }) if err := errors.Join(errs...); err != nil { return err } } return nil }Then use it in each filter type's
ValidateWithComplexitymethod.Also applies to: 201-224, 304-327, 406-429
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (36)
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)go.mod(2 hunks)openmeter/apiconverter/cursor.go(1 hunks)openmeter/apiconverter/filter.gen.go(1 hunks)openmeter/apiconverter/filter.go(1 hunks)openmeter/app/httpdriver/app.go(1 hunks)openmeter/app/httpdriver/marketplace.go(1 hunks)openmeter/meterevent/adapter/event.go(2 hunks)openmeter/meterevent/httphandler/event.go(1 hunks)openmeter/meterevent/httphandler/event_v2.go(1 hunks)openmeter/meterevent/httphandler/handler.go(1 hunks)openmeter/meterevent/httphandler/mapping.gen.go(1 hunks)openmeter/meterevent/httphandler/mapping.go(3 hunks)openmeter/meterevent/service.go(4 hunks)openmeter/server/router/event.go(1 hunks)openmeter/server/server_test.go(1 hunks)openmeter/streaming/clickhouse/connector.go(2 hunks)openmeter/streaming/clickhouse/event_query_v2.go(1 hunks)openmeter/streaming/clickhouse/event_query_v2_test.go(1 hunks)openmeter/streaming/connector.go(1 hunks)openmeter/streaming/testutils/streaming.go(1 hunks)pkg/filter/filter.go(6 hunks)pkg/filter/filter_test.go(1 hunks)pkg/pagination/v2/cursor.go(1 hunks)pkg/pagination/v2/cursor_test.go(1 hunks)tools.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 (17)
pkg/pagination/v2/cursor.go (1)
openmeter/streaming/clickhouse/connector.go (16) (16)
c(40-62)c(81-96)c(98-113)c(115-118)c(120-123)c(125-128)c(130-163)c(165-187)c(189-191)c(193-196)c(198-213)c(215-243)c(245-257)c(260-279)c(281-338)c(341-390)
openmeter/server/router/event.go (3)
openmeter/meterevent/adapter/event.go (2) (2)
a(13-63)a(65-115)openmeter/server/router/router.go (1) (1)
Router(171-191)api/api.gen.go (17) (17)
params(10044-10044)params(10204-10204)params(10345-10345)params(10422-10422)params(10591-10591)params(10776-10776)params(10891-10891)params(10943-10943)params(11065-11065)params(11185-11185)params(11221-11221)params(11345-11345)params(11469-11469)params(11566-11566)params(11713-11713)params(11868-11868)ListEventsV2Params(6509-6525)
openmeter/app/httpdriver/app.go (3)
api/api.gen.go (1) (1)
AppPaginatedResponse(813-825)api/client/go/client.gen.go (1) (1)
AppPaginatedResponse(712-724)api/client/javascript/src/client/schemas.ts (1) (1)
AppPaginatedResponse(8066-8066)
openmeter/apiconverter/cursor.go (1)
pkg/pagination/v2/cursor.go (2) (2)
Cursor(10-13)DecodeCursor(39-48)
pkg/pagination/v2/cursor_test.go (1)
pkg/pagination/v2/cursor.go (2) (2)
cursor(40-40)Cursor(10-13)
openmeter/meterevent/httphandler/mapping.gen.go (4)
openmeter/meterevent/httphandler/event_v2.go (1) (1)
ListEventsV2Params(15-15)openmeter/meterevent/service.go (1) (1)
ListEventsV2Params(121-142)openmeter/apiconverter/cursor.go (1) (1)
ConvertCursorPtr(12-18)openmeter/apiconverter/filter.go (2) (2)
ConvertStringPtr(14-14)ConvertTimePtr(20-20)
openmeter/server/server_test.go (2)
openmeter/meterevent/service.go (1) (1)
ListEventsV2Params(121-142)openmeter/streaming/connector.go (1) (1)
RawEvent(25-35)
openmeter/streaming/clickhouse/connector.go (4)
openmeter/streaming/testutils/streaming.go (3) (3)
c(61-63)c(65-67)_(17-17)openmeter/streaming/connector.go (2) (2)
Connector(37-50)RawEvent(25-35)openmeter/meterevent/service.go (1) (1)
ListEventsV2Params(121-142)openmeter/streaming/clickhouse/event_query_v2.go (1) (1)
queryEventsTableV2(12-16)
openmeter/meterevent/httphandler/mapping.go (1)
openmeter/meterevent/service.go (4) (4)
ListEventsV2Params(121-142)Event(46-65)i(76-118)e(68-73)
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(8328-8329)
pkg/filter/filter.go (1)
api/api.gen.go (5) (5)
FilterString(2423-2465)FilterInteger(2396-2420)FilterFloat(2369-2393)FilterTime(2468-2486)FilterBoolean(2363-2366)
openmeter/meterevent/adapter/event.go (1)
openmeter/meterevent/service.go (2) (2)
ListEventsV2Params(121-142)Event(46-65)
pkg/filter/filter_test.go (1)
pkg/filter/filter.go (5) (5)
FilterString(30-45)FilterInteger(154-163)FilterFloat(258-267)FilterBoolean(459-461)FilterTime(361-368)
openmeter/streaming/clickhouse/event_query_v2_test.go (4)
openmeter/streaming/clickhouse/event_query_v2.go (1) (1)
queryEventsTableV2(12-16)openmeter/meterevent/service.go (1) (1)
ListEventsV2Params(121-142)pkg/filter/filter.go (7) (7)
FilterString(30-45)FilterTime(361-368)_(22-22)_(23-23)_(24-24)_(25-25)_(26-26)pkg/pagination/v2/cursor.go (1) (1)
Cursor(10-13)
openmeter/apiconverter/filter.go (1)
pkg/filter/filter.go (5) (5)
FilterString(30-45)FilterInteger(154-163)FilterFloat(258-267)FilterTime(361-368)FilterBoolean(459-461)
openmeter/apiconverter/filter.gen.go (1)
openmeter/apiconverter/filter.go (10) (10)
ConvertBoolean(21-21)ConvertBooleanPtr(22-22)ConvertFloat(17-17)ConvertFloatPtr(18-18)ConvertInt(15-15)ConvertIntPtr(16-16)ConvertString(13-13)ConvertStringPtr(14-14)ConvertTime(19-19)ConvertTimePtr(20-20)
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 (72)
tools.go (1)
8-8: Consistent tool dependency addition.The addition of
github.com/jmattheis/goverter/cmd/goverteras a blank import is appropriate for making the tool available within the Go build system. This aligns with the new dependency added to go.mod.openmeter/meterevent/httphandler/handler.go (1)
1-1:✅ Verification successful
Package rename from httpdriver to httphandler.
This package name change appears to be part of a broader standardization effort across the codebase. The package name better reflects its role in handling HTTP requests.
🏁 Script executed:
#!/bin/bash # Check if this package rename affects imports in other files echo "Checking for files that import the old package name..." rg "github.com/openmeterio/openmeter/openmeter/meterevent/httpdriver" --type go echo "Checking for other package renames from httpdriver to httphandler..." rg -l "package httpdriver" --type go | xargs grep -l "package" | sort rg -l "package httphandler" --type go | xargs grep -l "package" | sortLength of output: 2909
HTTP Package Rename Verified:
The package inopenmeter/meterevent/httphandler/handler.gohas been successfully updated tohttphandler, which aligns with the intended naming standardization. The verification script confirmed that no files within the meterevent module are still referencing the old package name (httpdriver). Note that several other modules (e.g., underopenmeter/appandopenometer/customer) continue to usehttpdriver, which appears intentional and likely subject to a separate refactoring plan if needed.
- Updated File(s):
openmeter/meterevent/httphandler/handler.go(and related files in this module)go.mod (2)
40-40: New dependency for code generation.The addition of
github.com/jmattheis/goverter v1.8.0is consistent with the import in tools.go. This package is a code generator for type-safe converters, which will likely be used for the pagination improvements mentioned in the PR summary.
117-117: Indirect dependency added for code generation.The addition of
github.com/dave/jennifer v1.6.1as an indirect dependency is appropriate as it's likely a requirement for goverter. Jennifer is a code generation library that helps with generating Go source code.api/openapi.yaml (2)
2063-2070: Pagination in List Customers EndpointThe inclusion of
Pagination.pageandPagination.pageSizeparameters in the "List customers" endpoint aligns well with our overall pagination improvements. Ensure that the descriptions for these parameters in the components section are clear and consistent across the API.
2442-2449: Pagination Improvement for String Identifier EndpointThe integration of pagination parameters here reinforces consistency. Just ensure that the preceding string pattern and example remain contextually valid after these additions.
openmeter/meterevent/httphandler/event.go (1)
1-1:❓ Verification inconclusive
Package name updated from httpdriver to httphandler
The package name has been changed from
httpdrivertohttphandler. This is part of a broader API restructuring effort related to the introduction of the Events V2 API.Ensure that all imports referencing this package have been updated throughout the codebase to prevent compilation errors.
🏁 Script executed:
#!/bin/bash # Check for any stale imports of the old package name rg "\"github.com/openmeterio/openmeter/openmeter/meterevent/httpdriver\"" --type goLength of output: 83
Action Required: Manual Verification for Import Updates
The package rename from
httpdrivertohttphandlerinopenmeter/meterevent/httphandler/event.goappears correctly implemented. However, since the automated search (using the providedrgcommand) produced no output, the verification of stale import references is inconclusive. Please manually verify that all import statements across the codebase have been updated from:"github.com/openmeterio/openmeter/openmeter/meterevent/httpdriver"to
"github.com/openmeterio/openmeter/openmeter/meterevent/httphandler"to ensure there are no compilation issues.
openmeter/server/router/event.go (1)
19-22:❓ Verification inconclusive
Implementation missing for ListEventsV2 endpoint
The new V2 endpoint has been added but it's currently calling
unimplemented.ListEventsV2(). This suggests this is a placeholder that needs to be replaced with the actual implementation.There appears to be a missing import for the
unimplementedpackage, and the actual implementation exists in the adapter layer as shown in the relevant code snippets.Consider updating this to use the existing implementation:
func (a *Router) ListEventsV2(w http.ResponseWriter, r *http.Request, params api.ListEventsV2Params) { - unimplemented.ListEventsV2(w, r, params) + a.meterEventHandler.ListEventsV2().With(params).ServeHTTP(w, r) }Also, verify that the
unimplementedpackage is imported if you intend to keep this as a placeholder.
🏁 Script executed:
#!/bin/bash # Check if the unimplemented package exists and is imported rg "import.*\".*unimplemented\"" openmeter/server/router/event.go rg -A 5 "package unimplemented" --type goLength of output: 109
Action: Replace the placeholder implementation for ListEventsV2
The current endpoint in
openmeter/server/router/event.gostill calls the placeholderunimplemented.ListEventsV2(w, r, params). This not only leaves the endpoint unimplemented but also appears to be missing the corresponding import—verify whether theunimplementedpackage exists or is needed. If you want a functional endpoint, please update it by replacing the placeholder with the adapter layer implementation. For example:func (a *Router) ListEventsV2(w http.ResponseWriter, r *http.Request, params api.ListEventsV2Params) { - unimplemented.ListEventsV2(w, r, params) + a.meterEventHandler.ListEventsV2().With(params).ServeHTTP(w, r) }If you intend to keep the placeholder temporarily, ensure that the
unimplementedpackage is correctly imported and available.api/client/javascript/src/client/schemas.ts (5)
8066-8066: Export of AppPaginatedResponse type looks goodThe export of the AppPaginatedResponse type is correctly implemented.
8328-8329: Export of MarketplaceListingPaginatedResponse type looks goodThe export of the MarketplaceListingPaginatedResponse type is correctly implemented and follows the same pattern as other exports.
8672-8672: Response content type update looks goodThe response content type for listApps has been correctly updated to use the new AppPaginatedResponse.
13487-13487: Response content type update looks goodThe response content type for listMarketplaceListings has been correctly updated to use the new MarketplaceListingPaginatedResponse.
1855-13487: Consistent pagination implementation across endpointsI like that you've implemented pagination consistently across multiple endpoints using the same pattern. This makes the API more predictable and easier to use. The structure aligns well with the Go implementation seen in the relevant code snippets.
pkg/pagination/v2/cursor.go (1)
26-36: Good addition of validation logicThe new
Validatemethod adds important validation for cursor objects, ensuring that neither theTimefield is zero nor theIDfield is empty. This matches the validation pattern used elsewhere in the codebase and prevents invalid cursor objects from being processed further.openmeter/streaming/testutils/streaming.go (1)
77-79: LGTM: Mock implementation for new interface methodThis implementation correctly adds a mock version of the
ListEventsV2method to maintain compatibility with the updatedConnectorinterface. The implementation follows the same pattern as other mock methods in this file.openmeter/app/httpdriver/app.go (1)
20-20: LGTM: Updated response type for pagination supportThe change from
api.AppListtoapi.AppPaginatedResponsealigns with the existing implementation which already constructs a response with pagination fields (page, pageSize, totalCount, items).openmeter/streaming/connector.go (1)
42-42: LGTM: Added V2 interface method for enhanced event listingThe new
ListEventsV2method enhances theConnectorinterface with a more consolidated parameter approach, taking a singlemeterevent.ListEventsV2Paramsobject instead of separate namespace and params parameters. This is well-positioned in the interface definition after the original method.api/spec/src/query.tsp (1)
131-149: Well-structured cursor pagination model with appropriate validation.The
QueryCursorPaginationmodel provides a clean implementation of cursor-based pagination parameters with proper validation constraints and defaults. The cursor-based approach is generally more efficient than offset-based pagination for large datasets.api/codegen.yaml (1)
1-1: Good addition of schema reference.Adding the schema reference improves editor support with validation and autocompletion for the YAML configuration.
openmeter/app/httpdriver/marketplace.go (1)
23-23: Type alias update for pagination support.The updated type alias aligns with the paginated response structure. The
ListMarketplaceListingsResponsenow maps toapi.MarketplaceListingPaginatedResponsewhich includes the pagination metadata fields (Page, PageSize, TotalCount).openmeter/meterevent/httphandler/mapping.gen.go (1)
1-34: Parameter conversion function for ListEventsV2 is well-structured.This generated code properly maps API parameters to internal service parameters, including cursor handling and filter conversions. The error handling for cursor conversion is appropriate.
api/spec/src/events.tsp (1)
134-175:❓ Verification inconclusive
New EventsV2 interface adds cursor-based pagination and advanced filtering capabilities.
The implementation provides a proper evolution path by:
- Keeping the original Events interface intact
- Adding a more sophisticated filtering mechanism using deepObject query parameters
- Using CursorPaginatedResponse for better pagination handling
Note that this interface is marked as internal with
@extension("x-internal", true). Is this intentionally designed as an internal API, or should it be publicly available?
🏁 Script executed:
#!/bin/bash # Check if there are any references to this API in public documentation or client code rg -i "api/v2/events" --type mdLength of output: 31
Review Feedback: Verify API Exposure for EventsV2
- The new
EventsV2interface correctly implements cursor-based pagination and advanced filtering using deepObject query parameters.- The usage of
@extension("x-internal", true)clearly marks this API as internal. However, our search of Markdown documentation yielded no references to/api/v2/events, which might indicate that it’s indeed intended for internal use.- Please confirm if the internal designation is deliberate or if the endpoint should be publicly available.
pkg/pagination/v2/cursor_test.go (1)
190-239: Comprehensive validation tests for Cursor struct.The test cases properly validate that:
- A cursor with valid time and ID passes validation
- A cursor with zero time fails validation with appropriate error message
- A cursor with empty ID fails validation with appropriate error message
This testing approach ensures the Cursor validation logic works as expected.
api/spec/src/app/app.tsp (2)
31-31: Update return type to use the newer pagination structureThe return type has been updated from
Paginated<App>toPaginatedResponse<App>, which is part of a broader effort to standardize pagination handling across the API. This change ensures the Apps interface follows the new pagination pattern.
71-71: Update request model to use the newer pagination query parametersThe ListAppsRequest model now extends from
QueryPaginationinstead ofPaginatedQuery, aligning with the updated pagination approach. This ensures consistency with the updated return type and the pagination system used throughout the API.api/spec/src/app/marketplace.tsp (1)
27-29: Update Marketplace list method to use the new pagination structureThe method signature has been updated to use
QueryPaginationinstead ofPaginatedQueryand returnPaginatedResponse<MarketplaceListing>instead ofPaginated<MarketplaceListing>. This change is consistent with the pagination updates throughout the API.openmeter/streaming/clickhouse/event_query_v2_test.go (4)
14-124: Well-structured test cases for events query SQL generationThe test cases for
ToSQLmethod cover a comprehensive range of scenarios including basic queries, various filters (ID, subject, time), and cursor-based pagination. This ensures the SQL generation logic handles all expected use cases correctly.I particularly like how you've structured each test case with clear definitions of inputs and expected outputs, making it easy to understand the intent of each test.
126-201: Comprehensive count query tests for different filtering scenariosThe test cases for
ToCountRowSQLthoroughly validate the SQL generation for counting events with various filters. This ensures accurate count operations which is critical for pagination functionality.
97-98: Verify cursor-based pagination SQL query logicThe SQL query for cursor-based pagination uses a complex condition:
time <= ? AND (time < ? OR id < ?). This ensures proper pagination when multiple events have the same timestamp by using the ID as a secondary sorting key. This approach is correct for cursor-based pagination with composite keys.
203-206: Simple helper function to improve test readabilityThe
stringPtrhelper function improves readability by simplifying the creation of string pointers used in filter conditions throughout the tests.api/openapi.cloud.yaml (5)
43-54: Ensure Consistency in List Apps Endpoint PaginationThe update correctly replaces the previous pagination fields with the new
Pagination.pageandPagination.pageSizeparameters and updates the response schema toAppPaginatedResponse. Verify that the new schema is completely defined in the components section and that these changes properly integrate with client implementations.
2064-2071: Update List Customers Endpoint with New Pagination ReferencesThe changes add the new pagination parameters (
Pagination.pageandPagination.pageSize) to the list customers endpoint. This aligns with the updated API design for pagination consistency. Confirm that these component references are defined and that other endpoints follow the same convention for uniform behavior.
2138-2145: Append Pagination Parameters in Customer-Specific EndpointThe insertion of the
Pagination.pageandPagination.pageSizereferences after the identifier parameter is consistent with the new pagination design. Ensure that the order of parameters is intentional and that it does not inadvertently affect the behavior or documentation generated from this OpenAPI spec.
2443-2450: Consistent Pagination References for the Affected EndpointHere, the addition of the pagination parameters maintains consistency with updates elsewhere in the spec. The response section remains unchanged, but confirm that the new pagination parameters are supported by the endpoint implementation.
3743-3757: Refactor List Available Apps Endpoint to Use New Pagination SchemaBy replacing the previous pagination parameters with
Pagination.pageandPagination.pageSizeand switching the response schema toMarketplaceListingPaginatedResponse, this endpoint now aligns with the improved pattern. Double-check that the new paginated response schema is fully accurate and documented.openmeter/meterevent/adapter/event.go (2)
10-10: Import updated to use v2 pagination package.This change aligns with the migration to v2 pagination across the codebase.
65-115: ListEventsV2 method implements the updated pagination approach.The implementation follows the same pattern as the existing
ListEventsmethod but properly returns apagination.Result[meterevent.Event]instead of a slice. The error handling is consistent, and the method correctly validates events against meters.Note that you're following a similar validation pattern from the original method, creating explicit copies of the event data in a new structure rather than simply passing through the original events. This ensures consistency with the existing pattern.
openmeter/streaming/clickhouse/connector.go (1)
98-113: ListEventsV2 implementation looks correct.This method properly validates input parameters and processes errors consistent with other methods in the connector.
api/spec/src/customer.tsp (3)
115-115: Updated pagination model to QueryPagination.The parameter has been updated from
PaginatedQuerytoQueryPaginationas part of the codebase-wide pagination standardization.
137-137: Updated ListCustomersParams to use QueryPagination.Consistent with other pagination model changes across the codebase.
195-195: Updated ListCustomerAppDataParams to use QueryPagination.Consistent with other pagination model changes across the codebase.
openmeter/meterevent/httphandler/event_v2.go (1)
15-19: Well-defined type aliases for the HTTP handler.Clean organization of the request/response/params types, making the handler implementation more readable.
openmeter/meterevent/httphandler/mapping.go (4)
1-2: Code generation directive and package name change looks goodThe change from
httpdrivertohttphandleraligns with the functionality of this file, and thego:generatedirective is correctly configured for the goverter tool.
14-14: Import for the new pagination package is correctly addedThe import for the v2 pagination package is properly included to support the new functionality.
17-29: Goverter configuration looks appropriateThe goverter directives are well-structured with proper configuration for output file and extensions. The declaration of
convertListEventsV2Paramswith namespace context handling is correctly defined.
70-84: Properly implemented response conversion functionThe
convertListEventsV2Responsefunction correctly converts the paginated events and handles potential errors during conversion. The code properly maps over the event items and constructs the API response with the next cursor.openmeter/meterevent/service.go (5)
9-10: New imports are correctly addedThe imports for the filter and pagination packages are correctly added to support the new V2 functionality.
20-20: Interface extension is appropriateAdding the
ListEventsV2method to theServiceinterface is a clean way to introduce new functionality while maintaining backward compatibility.
67-73: Well-implemented cursor methodThe
Cursormethod on theEventstruct correctly creates a pagination cursor with ID and time fields, which aligns with the pagination implementation.
120-142: Comprehensive parameter struct for V2 APIThe
ListEventsV2Paramsstruct is well-designed with proper field documentation and appropriate use of pointers for optional fields. The structure aligns with the V2 API requirements for filtering and pagination.
144-199: Thorough validation implementationThe
Validatemethod forListEventsV2Paramsis comprehensive, checking required fields and validating all filters with proper complexity limits. The error messages are clear and include the field names for better diagnostics.openmeter/streaming/clickhouse/event_query_v2.go (4)
11-16: Well-defined query structThe
queryEventsTableV2struct is clearly defined with appropriate fields for database, table name, and params.
18-99: Comprehensive SQL query generationThe
toSQLmethod is well-implemented with:
- Proper field selection
- Base namespace filtering
- Conditional application of all available filters
- Cursor-based pagination logic
- Stable ordering by time and ID
- Appropriate limit handling
The code properly handles the case where either time or ingested_at is used for pagination with the timeColumn variable.
76-86: Cursor-based pagination logic is well-implementedThe cursor-based pagination logic correctly handles:
- Filtering by time less than or equal to cursor time
- For events with the same time, using ID to ensure stable ordering
- The OR condition is properly structured to maintain pagination consistency
This implementation ensures reliable pagination results.
101-137: Optimized count query implementationThe
toCountRowSQLmethod intelligently optimizes the count query by:
- Including only the base namespace filter and essential filters
- Skipping less selective filters for performance
- Including a clear comment explaining the performance optimization
This approach helps maintain good performance for count operations which is important for pagination metadata.
pkg/filter/filter.go (4)
15-16: Interface extension with ValidateWithComplexity methodAdding the
ValidateWithComplexitymethod to theFilterinterface is a good approach to control filter complexity and prevent overly complex queries that could impact performance.
77-111: Well-implemented complexity validation for string filtersThe
ValidateWithComplexityimplementation forFilterString:
- First calls the existing
Validatemethod- Properly checks for logical operators
- Enforces maximum depth for nested filters
- Recursively validates nested filters with decremented depth
- Returns appropriate error messages
This implementation helps prevent filter abuse while allowing reasonable complexity.
193-227: Consistent implementation across filter typesThe
ValidateWithComplexityimplementations forFilterInteger,FilterFloat, andFilterTimefollow the same pattern and are consistent with theFilterStringimplementation. This consistency is good for maintainability and ensures uniform behavior across filter types.Also applies to: 296-330, 398-432
468-472: Simplified implementation for boolean filtersThe
ValidateWithComplexityimplementation forFilterBooleanis appropriately simplified since boolean filters don't support logical operators. This is a good optimization.pkg/filter/filter_test.go (5)
1010-1152: Well-structured comprehensive test for FilterString complexity validation.The test cases effectively cover all important scenarios for the
ValidateWithComplexitymethod, including nil filters, simple filters, nested filters at various depths, and validation errors. I appreciate the clearly defined error messages and descriptive test names that make the purpose of each test case obvious.
1154-1276: Good test coverage for FilterInteger complexity validation.The test implementation for integer filters follows the same pattern as the string filters, ensuring consistent validation logic across different filter types. All important cases are covered including deeply nested filters that exceed the depth limit.
1278-1374: Comprehensive test cases for FilterFloat complexity validation.The tests properly verify both valid cases within the depth limit and invalid cases that exceed it. The error checking is thorough and consistent with other filter type tests.
1376-1409: Simple but effective tests for FilterBoolean complexity validation.Since FilterBoolean is simpler (only has Eq field and no nested structures), the tests appropriately focus on the core validation scenarios without unnecessary complexity.
1411-1509: Thorough test coverage for FilterTime complexity validation.The tests cover all relevant scenarios including nil filters, simple filters, nested structures, and filters that exceed the depth limit. Good use of time.Now() to create realistic test data.
openmeter/apiconverter/filter.go (2)
1-7: Good use of code generation with appropriate imports.The go:generate directive correctly specifies the goverter tool for generating the conversion functions. The package and imports are properly structured.
9-23: Well-structured variable declarations for filter conversions.The conversion function declarations are clear and follow a consistent pattern for each filter type. The goverter directives appropriately configure the code generation options, including the output file and skipping same type copying for efficiency.
openmeter/apiconverter/filter.gen.go (6)
1-3: Standard boilerplate for generated code.The code includes appropriate build tags and warning comments to prevent manual edits to the generated file.
11-24: Proper implementation of Boolean filter conversion.The conversion functions for Boolean filters correctly map the Eq field and handle nil pointers appropriately.
25-62: Thorough Float filter conversion with proper handling of nested structures.The Float conversion properly maps all fields (Eq, Ne, Gt, Gte, Lt, Lte) and correctly handles the recursive conversion of And/Or arrays. The pointer conversion function appropriately checks for nil input.
63-100: Solid implementation of Integer filter conversion.The Integer filter conversion follows the same pattern as the Float conversion, with proper field mapping and recursive handling of nested filters.
101-144: Comprehensive String filter conversion with all fields properly mapped.The String conversion handles all fields including string-specific operators like Like, Ilike, In, etc. The nested filter conversion is implemented correctly.
145-181: Well-implemented Time filter conversion.The Time filter conversion properly handles time-specific fields and nested structures. The pointer handling is consistent with the other conversion functions.
| // queryEventsTableV2 is similar to queryEventsTable but with advanced filtering options. | ||
| func (c *Connector) queryEventsTableV2(ctx context.Context, params meterevent.ListEventsV2Params) ([]streaming.RawEvent, error) { | ||
| var err error | ||
|
|
||
| // Create query struct | ||
| queryBuilder := queryEventsTableV2{ | ||
| Database: c.config.Database, | ||
| EventsTableName: c.config.EventsTableName, | ||
| Params: params, | ||
| } | ||
|
|
||
| // If a client ID is provided, track progress | ||
| if params.ClientID != nil { | ||
| // Build SQL query to count the total number of rows | ||
| countSQL, countArgs := queryBuilder.toCountRowSQL() | ||
|
|
||
| ctx, err = c.withProgressContext(ctx, params.Namespace, *params.ClientID, countSQL, countArgs) | ||
| // Log error but don't return it | ||
| if err != nil { | ||
| c.config.Logger.Error("failed track progress", "error", err, "clientId", *params.ClientID) | ||
| } | ||
| } | ||
|
|
||
| sql, args := queryBuilder.toSQL() | ||
|
|
||
| rows, err := c.config.ClickHouse.Query(ctx, sql, args...) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("query events table query: %w", err) | ||
| } | ||
|
|
||
| defer rows.Close() | ||
|
|
||
| events := []streaming.RawEvent{} | ||
|
|
||
| for rows.Next() { | ||
| var event streaming.RawEvent | ||
|
|
||
| err = rows.ScanStruct(&event) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("scan raw event: %w", err) | ||
| } | ||
|
|
||
| events = append(events, event) | ||
| } | ||
|
|
||
| if err = rows.Err(); err != nil { | ||
| return nil, fmt.Errorf("rows error: %w", err) | ||
| } | ||
|
|
||
| return events, nil | ||
| } |
There was a problem hiding this comment.
Missing namespace not found error check in queryEventsTableV2.
The error handling in this method is missing the check for "code: 60" to return a NamespaceNotFoundError which is present in other similar methods (e.g., line 314 in queryEventsTable).
Consider adding the namespace check similar to other methods:
func (c *Connector) queryEventsTableV2(ctx context.Context, params meterevent.ListEventsV2Params) ([]streaming.RawEvent, error) {
// ...existing code...
rows, err := c.config.ClickHouse.Query(ctx, sql, args...)
if err != nil {
+ if strings.Contains(err.Error(), "code: 60") {
+ return nil, models.NewNamespaceNotFoundError(params.Namespace)
+ }
return nil, fmt.Errorf("query events table query: %w", err)
}
// ...rest of the method...
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // queryEventsTableV2 is similar to queryEventsTable but with advanced filtering options. | |
| func (c *Connector) queryEventsTableV2(ctx context.Context, params meterevent.ListEventsV2Params) ([]streaming.RawEvent, error) { | |
| var err error | |
| // Create query struct | |
| queryBuilder := queryEventsTableV2{ | |
| Database: c.config.Database, | |
| EventsTableName: c.config.EventsTableName, | |
| Params: params, | |
| } | |
| // If a client ID is provided, track progress | |
| if params.ClientID != nil { | |
| // Build SQL query to count the total number of rows | |
| countSQL, countArgs := queryBuilder.toCountRowSQL() | |
| ctx, err = c.withProgressContext(ctx, params.Namespace, *params.ClientID, countSQL, countArgs) | |
| // Log error but don't return it | |
| if err != nil { | |
| c.config.Logger.Error("failed track progress", "error", err, "clientId", *params.ClientID) | |
| } | |
| } | |
| sql, args := queryBuilder.toSQL() | |
| rows, err := c.config.ClickHouse.Query(ctx, sql, args...) | |
| if err != nil { | |
| return nil, fmt.Errorf("query events table query: %w", err) | |
| } | |
| defer rows.Close() | |
| events := []streaming.RawEvent{} | |
| for rows.Next() { | |
| var event streaming.RawEvent | |
| err = rows.ScanStruct(&event) | |
| if err != nil { | |
| return nil, fmt.Errorf("scan raw event: %w", err) | |
| } | |
| events = append(events, event) | |
| } | |
| if err = rows.Err(); err != nil { | |
| return nil, fmt.Errorf("rows error: %w", err) | |
| } | |
| return events, nil | |
| } | |
| // queryEventsTableV2 is similar to queryEventsTable but with advanced filtering options. | |
| func (c *Connector) queryEventsTableV2(ctx context.Context, params meterevent.ListEventsV2Params) ([]streaming.RawEvent, error) { | |
| var err error | |
| // Create query struct | |
| queryBuilder := queryEventsTableV2{ | |
| Database: c.config.Database, | |
| EventsTableName: c.config.EventsTableName, | |
| Params: params, | |
| } | |
| // If a client ID is provided, track progress | |
| if params.ClientID != nil { | |
| // Build SQL query to count the total number of rows | |
| countSQL, countArgs := queryBuilder.toCountRowSQL() | |
| ctx, err = c.withProgressContext(ctx, params.Namespace, *params.ClientID, countSQL, countArgs) | |
| // Log error but don't return it | |
| if err != nil { | |
| c.config.Logger.Error("failed track progress", "error", err, "clientId", *params.ClientID) | |
| } | |
| } | |
| sql, args := queryBuilder.toSQL() | |
| rows, err := c.config.ClickHouse.Query(ctx, sql, args...) | |
| if err != nil { | |
| if strings.Contains(err.Error(), "code: 60") { | |
| return nil, models.NewNamespaceNotFoundError(params.Namespace) | |
| } | |
| return nil, fmt.Errorf("query events table query: %w", err) | |
| } | |
| defer rows.Close() | |
| events := []streaming.RawEvent{} | |
| for rows.Next() { | |
| var event streaming.RawEvent | |
| err = rows.ScanStruct(&event) | |
| if err != nil { | |
| return nil, fmt.Errorf("scan raw event: %w", err) | |
| } | |
| events = append(events, event) | |
| } | |
| if err = rows.Err(); err != nil { | |
| return nil, fmt.Errorf("rows error: %w", err) | |
| } | |
| return events, nil | |
| } |
| func (h *handler) ListEventsV2() ListEventsV2Handler { | ||
| return httptransport.NewHandlerWithArgs( | ||
| func(ctx context.Context, r *http.Request, params ListEventsV2Params) (ListEventsV2Request, error) { | ||
| ns, err := h.resolveNamespace(ctx) | ||
| if err != nil { | ||
| return ListEventsV2Request{}, err | ||
| } | ||
|
|
||
| p, err := convertListEventsV2Params(params, ns) | ||
| if err != nil { | ||
| return ListEventsV2Request{}, models.NewGenericValidationError(err) | ||
| } | ||
|
|
||
| return p, nil | ||
| }, | ||
| func(ctx context.Context, params ListEventsV2Request) (ListEventsV2Response, error) { | ||
| events, err := h.metereventService.ListEventsV2(ctx, params) | ||
| if err != nil { | ||
| return ListEventsV2Response{}, err | ||
| } | ||
|
|
||
| return convertListEventsV2Response(events) | ||
| }, | ||
| commonhttp.JSONResponseEncoderWithStatus[ListEventsV2Response](http.StatusOK), | ||
| httptransport.AppendOptions( | ||
| h.options, | ||
| httptransport.WithOperationName("listEventsV2"), | ||
| )..., | ||
| ) | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
ListEventsV2 handler is well-structured but references undefined functions.
The handler follows the established patterns in the codebase, properly handles error cases, and includes appropriate HTTP status codes and operation naming.
However, the handler calls helper functions convertListEventsV2Params and convertListEventsV2Response that are not defined in this file.
Please verify that these conversion functions are defined elsewhere in the codebase:
🏁 Script executed:
#!/bin/bash
# Search for convertListEventsV2Params and convertListEventsV2Response function definitions
echo "Searching for convertListEventsV2Params function..."
rg -A 5 "func convertListEventsV2Params"
echo "Searching for convertListEventsV2Response function..."
rg -A 5 "func convertListEventsV2Response"Length of output: 904
Action Required: Missing Conversion Function Implementation
The ListEventsV2 handler is well-structured and adheres to the codebase’s established patterns. Verification shows that the helper function convertListEventsV2Response is implemented (in openmeter/meterevent/httphandler/mapping.go); however, the function convertListEventsV2Params remains undefined in the repository.
- Please verify whether
convertListEventsV2Paramsshould be implemented (or imported) to convert the request parameters as intended. - If its implementation exists elsewhere under a different name, update the handler accordingly.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (23)
openmeter/streaming/testutils/streaming.go (1)
77-79: Consider adding documentation to the new ListEventsV2 methodThe mock implementation correctly matches the new interface method, but adding documentation comments would improve clarity about the purpose of this V2 method and how it differs from the original ListEvents method.
+// ListEventsV2 mocks the V2 implementation of event listing which supports pagination and enhanced filtering. func (m *MockStreamingConnector) ListEventsV2(ctx context.Context, params meterevent.ListEventsV2Params) ([]streaming.RawEvent, error) { return []streaming.RawEvent{}, nil }openmeter/streaming/connector.go (1)
42-42: Add documentation for the new ListEventsV2 interface methodThe new method is added correctly to the interface, but lacks documentation explaining its purpose, expected behavior, and how it differs from the original ListEvents method. Since interfaces define contracts that implementations must follow, clear documentation is particularly important.
ListEvents(ctx context.Context, namespace string, params meterevent.ListEventsParams) ([]RawEvent, error) + // ListEventsV2 retrieves events with pagination and enhanced filtering capabilities. + // Unlike ListEvents, this method uses the V2 parameters which support cursor-based pagination + // and more advanced filtering options. ListEventsV2(ctx context.Context, params meterevent.ListEventsV2Params) ([]RawEvent, error)api/client/javascript/src/client/schemas.ts (7)
1855-1873: Documentation inconsistency in AppPaginatedResponse schema.The field descriptions for
totalCount,page, andpageSizeall incorrectly state "The items in the current page." Each field should have a unique description that accurately reflects its purpose./** @description Paginated response */ AppPaginatedResponse: { /** - * @description The items in the current page. + * @description The total number of items available across all pages. * @example 500 */ totalCount: number /** - * @description The items in the current page. + * @description The current page number. * @example 1 */ page: number /** - * @description The items in the current page. + * @description The number of items per page. * @example 100 */ pageSize: number /** @description The items in the current page. */ items: components['schemas']['App'][] }
5234-5252: Documentation inconsistency in MarketplaceListingPaginatedResponse schema.Similar to the AppPaginatedResponse, the field descriptions for
totalCount,page, andpageSizeall incorrectly state "The items in the current page." Each field should have a unique description that accurately reflects its purpose./** @description Paginated response */ MarketplaceListingPaginatedResponse: { /** - * @description The items in the current page. + * @description The total number of items available across all pages. * @example 500 */ totalCount: number /** - * @description The items in the current page. + * @description The current page number. * @example 1 */ page: number /** - * @description The items in the current page. + * @description The number of items per page. * @example 100 */ pageSize: number /** @description The items in the current page. */ items: components['schemas']['MarketplaceListing'][] }
8651-8658: Incorrect parameter descriptions for pagination in listApps.The descriptions for
pageandpageSizeparameters incorrectly reference date-time formats, which is not appropriate for pagination parameters.-/** @description Start date-time in RFC 3339 format. +/** @description Page number to retrieve. * -* Inclusive. */ +* Starts at 1. */ page?: components['parameters']['Pagination.page'] -/** @description Number of items per page. +/** @description Maximum number of items to return per page. * * Default is 100. */ pageSize?: components['parameters']['Pagination.pageSize']
11217-11224: Incorrect parameter descriptions for pagination in listCustomers.The descriptions for
pageandpageSizeparameters incorrectly reference date-time formats instead of describing pagination parameters.-/** @description Start date-time in RFC 3339 format. +/** @description Page number to retrieve. * -* Inclusive. */ +* Starts at 1. */ page?: components['parameters']['Pagination.page'] -/** @description Number of items per page. +/** @description Maximum number of items to return per page. * * Default is 100. */ pageSize?: components['parameters']['Pagination.pageSize']
11416-11423: Incorrect parameter descriptions for pagination in listCustomerAppData.The descriptions for
pageandpageSizeparameters incorrectly reference date-time formats instead of describing pagination parameters.-/** @description Start date-time in RFC 3339 format. +/** @description Page number to retrieve. * -* Inclusive. */ +* Starts at 1. */ page?: components['parameters']['Pagination.page'] -/** @description Number of items per page. +/** @description Maximum number of items to return per page. * * Default is 100. */ pageSize?: components['parameters']['Pagination.pageSize']
11809-11816: Incorrect parameter descriptions for pagination in listCustomerSubscriptions.The descriptions for
pageandpageSizeparameters incorrectly reference date-time formats instead of describing pagination parameters.-/** @description Start date-time in RFC 3339 format. +/** @description Page number to retrieve. * -* Inclusive. */ +* Starts at 1. */ page?: components['parameters']['Pagination.page'] -/** @description Number of items per page. +/** @description Maximum number of items to return per page. * * Default is 100. */ pageSize?: components['parameters']['Pagination.pageSize']
13466-13473: Incorrect parameter descriptions for pagination in listMarketplaceListings.The descriptions for
pageandpageSizeparameters incorrectly reference date-time formats instead of describing pagination parameters.-/** @description Start date-time in RFC 3339 format. +/** @description Page number to retrieve. * -* Inclusive. */ +* Starts at 1. */ page?: components['parameters']['Pagination.page'] -/** @description Number of items per page. +/** @description Maximum number of items to return per page. * * Default is 100. */ pageSize?: components['parameters']['Pagination.pageSize']pkg/pagination/v2/cursor.go (3)
26-36: Add documentation for the Validate methodThe newly added
Validatemethod is public (exported) but lacks documentation. Consider adding a doc comment that explains its purpose, validation rules, and possible error returns - similar to how other methods in this file are documented.+// Validate checks if the cursor has valid values. +// It returns an error if the Time field is zero or if the ID field is empty. func (c Cursor) Validate() error { if c.Time.IsZero() { return fmt.Errorf("time is zero") } if c.ID == "" { return fmt.Errorf("id is empty") } return nil }
38-48: Consider validating cursors after decodingThe
DecodeCursorfunction returns a cursor pointer without validating it. Consider calling the newly addedValidatemethod after decoding to ensure only valid cursors are returned by this function.func DecodeCursor(s string) (*Cursor, error) { var cursor Cursor err := cursor.UnmarshalText([]byte(s)) if err != nil { return nil, err } + if err := cursor.Validate(); err != nil { + return nil, fmt.Errorf("invalid cursor: %w", err) + } + return &cursor, nil }
67-99: Add validation in UnmarshalTextSince the
UnmarshalTextmethod is responsible for populating the cursor from external data, it would be a good place to validate the cursor before returning. This would ensure that cursor validation happens as early as possible in the data flow.func (c *Cursor) UnmarshalText(text []byte) error { if len(text) == 0 { return fmt.Errorf("text is empty") } b, err := base64.StdEncoding.DecodeString(string(text)) if err != nil { return fmt.Errorf("decode cursor: %w", err) } parts := strings.SplitN(string(b), cursorDelimiter, 2) if len(parts) != 2 { return fmt.Errorf("cursor is invalid: no delimiter found") } // Parse the time timestamp, err := time.Parse(time.RFC3339, parts[0]) if err != nil { return fmt.Errorf("parse cursor timestamp: %w", err) } id := parts[1] *c = Cursor{ Time: timestamp, ID: id, } + // Validate the cursor after populating it + if err := c.Validate(); err != nil { + return fmt.Errorf("invalid cursor: %w", err) + } + return nil }api/openapi.yaml (3)
8992-9017: AppPaginatedResponse Schema Details
The updatedAppPaginatedResponseschema now includes keys fortotalCount,page,pageSize, anditems. However, the descriptions fortotalCount,page, andpageSizeall read as "The items in the current page." It is suggested to revise these descriptions so that they clearly differentiate each field’s purpose (for example,totalCountshould indicate the overall number of items, whilepagerepresents the current page number).
12381-12396: IngestedEventCursorPaginatedResponse Schema
The new schema for cursor-based pagination of ingested events is well-structured, requiring bothitemsandnextCursor. Consider clarifying the description ofnextCursorto indicate that it should be used to fetch the next page (or note if it represents the cursor from the last item). Overall, this schema meets the enhanced pagination design.
14164-14164: MarketplaceListingPaginatedResponse Schema Revision
The schema forMarketplaceListingPaginatedResponsereiterates pagination fields similar toAppPaginatedResponse. As with the previous pagination schema, the descriptions fortotalCount,page, andpageSizeare generic. It is recommended to refine these descriptions to accurately express that:
totalCountrepresents the total number of available listings,pagestands for the current page number, andpageSizedenotes the maximum number of listings per page.
This clarification will improve API documentation clarity.Also applies to: 14167-14188
api/spec/src/query.tsp (1)
130-149: Consider adding example values for better developer experience.The implementation of
QueryCursorPaginationlooks good with appropriate constraints and documentation. For consistency with other models in this file (likeQueryPaginationandQueryLimitOffset), consider adding@exampledecorators to thecursorandlimitparameters to improve developer experience.@query + @example("eyJpZCI6IjEyMzQifQ==") cursor?: string; @query @minValue(1) @maxValue(1000) + @example(100) limit?: integer = 100;api/openapi.cloud.yaml (2)
9044-9073: Improve Descriptions in AppPaginatedResponse SchemaThe addition of
AppPaginatedResponseis well configured with required fields and types. However, the descriptions for some fields are not as descriptive as they could be. Consider refining them as follows:
- totalCount: "Total number of items available."
- page: "Current page number."
- pageSize: "Number of items per page."
- items: "List of App objects on the current page."
These improvements will help consumers better understand the pagination metadata.
13849-13880: Clarify Descriptions in MarketplaceListingPaginatedResponse SchemaThe new
MarketplaceListingPaginatedResponseis properly defined; however, the field descriptions (fortotalCount,page, andpageSize) currently state "The items in the current page," which seems copied from theitemsdescription. Consider updating them to:
- totalCount: "Total number of listings available."
- page: "Current page number."
- pageSize: "Number of listings per page."
This will lead to clearer API documentation.pkg/pagination/v2/cursor_test.go (1)
190-239: Comprehensive validation test coverageGood addition of the
TestCursorValidationfunction that thoroughly tests both success and failure scenarios for theCursorvalidation logic. The table-driven test approach is well-implemented, covering zero time values, empty IDs, and valid cursors.One suggestion would be to add one more test case that checks for both zero time and empty ID simultaneously to ensure validation properly handles multiple errors.
tests := []struct { name string cursor Cursor wantErr bool errContains string }{ // existing test cases... + { + name: "Both zero time and empty ID", + cursor: Cursor{ + Time: time.Time{}, + ID: "", + }, + wantErr: true, + errContains: "time is zero", // would check the first error encountered + }, }api/spec/src/events.tsp (2)
134-144: Well-structured new API version with additional documentationThe new
EventsV2interface is well-defined with proper route, tag, and operation metadata. The@extension("x-internal", true)annotation suggests this API might be for internal use or in preview.Consider adding more context in the comments about why this V2 version was created and how it differs from V1 to help developers understand the evolution of the API.
156-173: Advanced filtering with consistent deepObject styleThe implementation of deep object style filtering for various event attributes follows a consistent pattern, which is good for API usability. However, there's no documentation explaining the available filter operations or syntax.
Consider adding documentation that explains the filter syntax and available operations:
+ /** + * Filter events by ID. + * Supports operations: eq, neq, like, notLike + * Example: id[eq]=event-123 + */ @query(#{ explode: true, style: "deepObject" }) id?: OpenMeter.FilterString,Apply similar documentation to the other filter parameters.
openmeter/apiconverter/filter.gen.go (1)
34-42: Potential optimization for nested filter arraysThe code for handling nested
AndandOrarrays is duplicated across all filter types with similar patterns. While this is auto-generated code, consider simplifying the template to avoid the nested nil checks that appear redundant:if (*source.And) != nil { filterFilterTimeList = make([]filter.FilterTime, len((*source.And))) // ... }The outer check
if source.And != nilalready ensures the pointer is not nil, so the inner check on the dereferenced pointer may be unnecessary.Also applies to: 44-52, 72-80, 82-90, 116-124, 126-134, 152-160, 162-170
openmeter/apiconverter/filter.go (1)
4-7: Consider adding package documentation commentThe package lacks a package-level documentation comment explaining its purpose and use case. Adding a comment would improve developer understanding of this converter package's role in the API architecture.
//go:generate go run github.com/jmattheis/goverter/cmd/goverter gen ./ +// Package apiconverter provides type conversion utilities between API models and internal domain models. +// It uses goverter to auto-generate conversion code for complex filter structures. package apiconverter
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (36)
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)go.mod(2 hunks)openmeter/apiconverter/cursor.go(1 hunks)openmeter/apiconverter/filter.gen.go(1 hunks)openmeter/apiconverter/filter.go(1 hunks)openmeter/app/httpdriver/app.go(1 hunks)openmeter/app/httpdriver/marketplace.go(1 hunks)openmeter/meterevent/adapter/event.go(2 hunks)openmeter/meterevent/httphandler/event.go(1 hunks)openmeter/meterevent/httphandler/event_v2.go(1 hunks)openmeter/meterevent/httphandler/handler.go(1 hunks)openmeter/meterevent/httphandler/mapping.gen.go(1 hunks)openmeter/meterevent/httphandler/mapping.go(3 hunks)openmeter/meterevent/service.go(4 hunks)openmeter/server/router/event.go(1 hunks)openmeter/server/server_test.go(1 hunks)openmeter/streaming/clickhouse/connector.go(2 hunks)openmeter/streaming/clickhouse/event_query_v2.go(1 hunks)openmeter/streaming/clickhouse/event_query_v2_test.go(1 hunks)openmeter/streaming/connector.go(1 hunks)openmeter/streaming/testutils/streaming.go(1 hunks)pkg/filter/filter.go(6 hunks)pkg/filter/filter_test.go(1 hunks)pkg/pagination/v2/cursor.go(1 hunks)pkg/pagination/v2/cursor_test.go(1 hunks)tools.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 (16)
openmeter/server/server_test.go (3)
openmeter/meterevent/service.go (1) (1)
ListEventsV2Params(121-142)openmeter/meterevent/httphandler/event_v2.go (1) (1)
ListEventsV2Params(15-15)openmeter/streaming/connector.go (1) (1)
RawEvent(25-35)
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(8328-8329)
pkg/pagination/v2/cursor_test.go (1)
pkg/pagination/v2/cursor.go (2) (2)
cursor(40-40)Cursor(10-13)
openmeter/meterevent/adapter/event.go (2)
openmeter/meterevent/service.go (2) (2)
ListEventsV2Params(121-142)Event(46-65)openmeter/streaming/testutils/streaming.go (15) (15)
m(37-40)m(42-48)m(50-55)m(57-59)m(69-71)m(73-75)m(77-79)m(81-83)m(85-87)m(89-91)m(95-114)m(116-118)m(120-122)m(125-196)m(198-200)
openmeter/apiconverter/cursor.go (1)
pkg/pagination/v2/cursor.go (2) (2)
Cursor(10-13)DecodeCursor(39-48)
openmeter/meterevent/httphandler/mapping.go (2)
openmeter/meterevent/service.go (4) (4)
ListEventsV2Params(121-142)Event(46-65)i(76-118)e(68-73)openmeter/meterevent/httphandler/event_v2.go (1) (1)
ListEventsV2Params(15-15)
pkg/filter/filter_test.go (1)
pkg/filter/filter.go (5) (5)
FilterString(30-45)FilterInteger(154-163)FilterFloat(258-267)FilterBoolean(459-461)FilterTime(361-368)
openmeter/streaming/clickhouse/event_query_v2.go (3)
openmeter/meterevent/httphandler/event_v2.go (1) (1)
ListEventsV2Params(15-15)openmeter/meterevent/service.go (1) (1)
ListEventsV2Params(121-142)pkg/pagination/v2/cursor.go (1) (1)
Cursor(10-13)
openmeter/streaming/clickhouse/connector.go (4)
openmeter/streaming/testutils/streaming.go (3) (3)
c(61-63)c(65-67)_(17-17)openmeter/streaming/connector.go (2) (2)
Connector(37-50)RawEvent(25-35)openmeter/meterevent/service.go (1) (1)
ListEventsV2Params(121-142)openmeter/streaming/clickhouse/event_query_v2.go (1) (1)
queryEventsTableV2(12-16)
openmeter/apiconverter/filter.go (1)
pkg/filter/filter.go (5) (5)
FilterString(30-45)FilterInteger(154-163)FilterFloat(258-267)FilterTime(361-368)FilterBoolean(459-461)
openmeter/server/router/event.go (3)
openmeter/meterevent/adapter/event.go (2) (2)
a(13-63)a(65-115)openmeter/server/router/router.go (1) (1)
Router(171-191)api/api.gen.go (17) (17)
params(10044-10044)params(10204-10204)params(10345-10345)params(10422-10422)params(10591-10591)params(10776-10776)params(10891-10891)params(10943-10943)params(11065-11065)params(11185-11185)params(11221-11221)params(11345-11345)params(11469-11469)params(11566-11566)params(11713-11713)params(11868-11868)ListEventsV2Params(6509-6525)
pkg/filter/filter.go (1)
api/api.gen.go (5) (5)
FilterString(2423-2465)FilterInteger(2396-2420)FilterFloat(2369-2393)FilterTime(2468-2486)FilterBoolean(2363-2366)
openmeter/streaming/clickhouse/event_query_v2_test.go (4)
openmeter/streaming/clickhouse/event_query_v2.go (1) (1)
queryEventsTableV2(12-16)openmeter/meterevent/service.go (1) (1)
ListEventsV2Params(121-142)pkg/filter/filter.go (7) (7)
FilterString(30-45)FilterTime(361-368)_(22-22)_(23-23)_(24-24)_(25-25)_(26-26)pkg/pagination/v2/cursor.go (1) (1)
Cursor(10-13)
openmeter/apiconverter/filter.gen.go (2)
openmeter/apiconverter/filter.go (10) (10)
ConvertBoolean(21-21)ConvertBooleanPtr(22-22)ConvertFloat(17-17)ConvertFloatPtr(18-18)ConvertInt(15-15)ConvertIntPtr(16-16)ConvertString(13-13)ConvertStringPtr(14-14)ConvertTime(19-19)ConvertTimePtr(20-20)pkg/filter/filter.go (5) (5)
FilterBoolean(459-461)FilterFloat(258-267)FilterInteger(154-163)FilterString(30-45)FilterTime(361-368)
openmeter/meterevent/service.go (2)
openmeter/meterevent/httphandler/event_v2.go (1) (1)
ListEventsV2Params(15-15)pkg/filter/filter.go (2) (2)
FilterString(30-45)FilterTime(361-368)
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 (77)
tools.go (1)
8-8: Adding goverter tool for automatic type conversions.The inclusion of goverter is a good addition as it will help generate code for automatic type conversions between structs. This will be particularly useful in the pagination-related refactoring that's being implemented across the API.
go.mod (2)
40-40: Added goverter library for code generation.This dependency addition aligns with the import in tools.go. Goverter is a useful tool for generating conversion code between different struct types, which should help reduce boilerplate code in the codebase.
117-117: Added jennifer as an indirect dependency.Jennifer is an indirect dependency used by goverter for code generation. This addition is appropriate.
openmeter/meterevent/httphandler/event.go (1)
1-1: Package name standardization from httpdriver to httphandler.This package renaming better reflects the responsibility of these components (handling HTTP requests rather than "driving" HTTP). This aligns with modern Go naming conventions and improves code readability.
openmeter/meterevent/httphandler/handler.go (1)
1-1: Package name standardization from httpdriver to httphandler.This package renaming is consistent with the change in event.go and reflects a systematic approach to standardizing naming conventions throughout the codebase.
openmeter/streaming/connector.go (1)
42-42:✅ Verification successful
Verify namespace handling in ListEventsV2
Unlike the original ListEvents method which takes a namespace parameter, ListEventsV2 doesn't have an explicit namespace parameter. Ensure that the namespace is properly handled within the ListEventsV2Params structure to maintain consistent security boundaries.
🏁 Script executed:
#!/bin/bash # Check how namespace is handled in the ListEventsV2Params structure rg -A 5 "type ListEventsV2Params struct" --type goLength of output: 752
Namespace Handling Verified in ListEventsV2
The verification confirms that the
ListEventsV2Paramsdefined in openmeter/meterevent/service.go includes an explicitNamespacefield (as shown in the grep output), which preserves the security boundary originally provided by the explicit namespace parameter inListEvents. Note that a similarly named struct in api/api.gen.go does not contain this field. Please ensure that theListEventsV2method in openmeter/streaming/connector.go is indeed using the service-layer version (with the namespace) and that any conversions between API and service parameters correctly propagate the namespace.
- Action Items:
- Confirm that all calls to
ListEventsV2supply the appropriateNamespaceviameterevent.ListEventsV2Params.- Verify that any mappings or conversions between the generated API struct and the service struct preserve the namespace.
api/client/javascript/src/client/schemas.ts (1)
8066-8066: The pagination implementation across endpoints looks good.The pagination structure is consistent across all endpoints with appropriate type definitions and response schemas. The schema design with
totalCount,page,pageSize, anditemsfields follows standard pagination patterns and matches the Go implementation in the provided code snippets.Also applies to: 8328-8329, 8672-8672, 13487-13487
api/openapi.yaml (7)
45-46: Pagination Parameters in "List apps" Endpoint
The addition of pagination parameters (Pagination.pageandPagination.pageSize) and the update of the response schema to referenceAppPaginatedResponseimprove consistency. Please verify that theAppPaginatedResponseschema truly provides all needed pagination metadata (e.g. total count, current page, page size, and items list).Also applies to: 53-53
2066-2067: Pagination Parameters in "List customers" Endpoint
The inclusion ofPagination.pageandPagination.pageSizeparameters in this endpoint is a good move for standardizing pagination across endpoints. Ensure these parameters work harmoniously with the additional filtering parameters already in use.
2140-2141: Pagination Addition in Customer Filtering Section
Pagination parameters have been added alongside thequeryCustomerList.typeparameter. This is consistent with the other endpoints. Please confirm that their ordering and usage integrate well with the existing filtering logic.
2445-2446: Pagination in Endpoint with Pattern Validation
The new addition ofPagination.pageandPagination.pageSizeparameters here looks appropriate. Consider verifying that the validation (especially for the page identifier pattern) remains valid and does not conflict with the pagination logic.
3745-3746: Marketplace "List Available Apps" Pagination Update
Introducing pagination parameters together with the updated response schema (MarketplaceListingPaginatedResponse) for available apps is a positive step. Just ensure that the schema definition fully reflects standard pagination fields and that the example values match expected responses.Also applies to: 3753-3753
8067-8174: New/api/v2/eventsEndpoint with Cursor Pagination
A new endpoint for listing ingested events with advanced filtering and cursor pagination has been added. The query parameters—ranging from pagination (CursorPagination.cursorandCursorPagination.limit) to various filter options (e.g.,clientId,id,source, etc.)—are comprehensively defined. Please verify that all parameter constraints (e.g. string length, deepObject style) and error response schemas (400, 401, 403, etc.) are correctly implemented and consistent with the API’s overall design.
8280-8298: Definition of Cursor-Based Pagination Parameters
The newly defined parameters for cursor-based pagination (cursorandlimit) include appropriate validations such as minimum, maximum, and default values. Confirm that these definitions are referenced correctly in endpoints that implement cursor-based pagination.openmeter/app/httpdriver/app.go (1)
20-20: Update response type to support paginationThe response type for
ListAppsHandlerhas been updated fromapi.AppListtoapi.AppPaginatedResponseto support standardized pagination across the API. This change ensures consistent handling of paginated responses and aligns with the broader effort to implement pagination throughout the application.openmeter/server/server_test.go (1)
602-616: New ListEventsV2 mock method for testing the V2 events APIThis implementation adds the required mock method to support the new V2 events API with pagination capabilities. The method correctly returns a mock event with the same structure as the existing
ListEventsmethod but accepts the new V2 parameter type that supports cursor-based pagination.The implementation is consistent with the structure of other mock methods in this file and properly initializes all required fields in the returned events.
openmeter/app/httpdriver/marketplace.go (1)
23-23: Update marketplace listings response type to support paginationThe response type for
ListMarketplaceListingsHandlerhas been updated fromapi.MarketplaceListingListtoapi.MarketplaceListingPaginatedResponseto support standardized pagination across the API. This change ensures consistent handling of paginated marketplace listing responses.This change is part of the broader pagination standardization effort throughout the application and aligns with similar changes in other API responses.
api/codegen.yaml (2)
1-1: Add YAML schema reference for better editor supportAdding a schema reference for the YAML language server improves developer experience by enabling validation and autocompletion for this configuration file.
13-14: Configure OpenAPI code generator to retain all elementsThe new
output-optionssection withskip-prune: trueprevents the code generator from removing unused schema components. This is important when implementing pagination and related features across the API, as it ensures all schema components needed for consistent type definitions are retained in the generated code.This option helps maintain the integrity of the API client models, particularly for the newly introduced pagination structures.
openmeter/apiconverter/cursor.go (2)
8-10: Good implementation of cursor conversion functionThe function correctly uses the
pagination.DecodeCursormethod to convert an API cursor string to the internal pagination cursor type.
12-18: Well-structured pointer handling implementationThe pointer conversion function properly handles nil case and delegates to the non-pointer function. This is a clean pattern that follows Go best practices for nullable value handling.
openmeter/meterevent/httphandler/mapping.gen.go (1)
12-34: Auto-generated converter looks goodThis generated code correctly maps API parameters to internal data structures with proper error handling, especially around the cursor conversion. The implementation handles nil values appropriately and propagates errors when they occur.
Note: Since this is an auto-generated file (as indicated by the comment on line 1), any modifications should be made to the generator source rather than directly to this file.
api/spec/src/app/marketplace.tsp (1)
27-29:❓ Verification inconclusive
API contract update for pagination
The list method signature has been updated to use the new pagination model, changing both the input parameter type from
PaginatedQuerytoQueryPaginationand the return type fromPaginated<MarketplaceListing>toPaginatedResponse<MarketplaceListing>.Make sure all consumers of this API have been updated to handle the new pagination approach. This is a breaking change that could affect clients if not properly communicated.
🏁 Script executed:
#!/bin/bash # Find any remaining usages of the old pagination types in TypeSpec files echo "Checking for any remaining usages of old pagination types:" rg -t tsp "Paginated<|PaginatedQuery" ./apiLength of output: 200
I'll run a new shell script that adds the
.tspfile type torgto properly scan for any remaining usages of the old pagination types. This will help verify if there are any fragile references to the removed types.Please execute this script and share the results so we can finalize the review comment accordingly.
🏁 Script executed:
#!/bin/bash echo "Searching for any usages of old pagination types in .tsp files using updated type definition:" rg --type-add="tsp:*.tsp" -t tsp "Paginated<|PaginatedQuery" ./apiLength of output: 265
Pagination API Change Verified
The
listmethod inapi/spec/src/app/marketplace.tsphas been updated to use the new pagination model. Specifically:
- The input parameter type has changed from
PaginatedQuerytoQueryPagination.- The return type has changed from
Paginated<MarketplaceListing>toPaginatedResponse<MarketplaceListing>.An updated search for any remaining references to the deprecated pagination types (using
rgwith an added file type for.tspfiles) has returned no matches. However, because this is a breaking change, please manually verify that all consumers of this API have been updated accordingly.api/spec/src/app/app.tsp (2)
31-31: Updated return type for list endpointThe return type has been changed from
Paginated<App>toPaginatedResponse<App>, which aligns with the pagination model updates throughout the API.
71-71: Model inheritance updated for paginationThe
ListAppsRequestmodel now extendsQueryPaginationinstead ofPaginatedQuery, ensuring consistency with the new pagination approach.api/openapi.cloud.yaml (5)
43-57: Confirm Updated Pagination Parameters & Response Schema for List AppsThe changes correctly update the endpoint by replacing legacy pagination references with
Pagination.pageandPagination.pageSizeand by switching the response schema toAppPaginatedResponse. Ensure that these modifications are consistent with both server-side implementations and client-side expectations.
2064-2071: Verify Pagination and Ordering for List Customers EndpointThe pagination parameters (
Pagination.pageandPagination.pageSize) now reflect the updated API specification. The inclusion of ordering and additional filtering parameters appears intact. Confirm that these changes align with the updated behavior in the service.
2138-2145: Validate Pagination Parameters in Customer App Data EndpointNew pagination references have been added to the parameter list. Please double-check that their position within the list maintains proper ordering as expected by the API documentation generators and validators.
2443-2450: Confirm Pagination Setup in Customer-Related EndpointThe addition of
Pagination.pageandPagination.pageSizehere is consistent with other endpoints. Verify that the parameter definitions and ordering fulfill the API validation requirements.
3743-3757: Review Marketplace Listing Pagination IntegrationThe endpoint now uses the updated pagination parameters and references the new schema
MarketplaceListingPaginatedResponsefor its successful response. Ensure that the schema’s structure and the pagination metadata are consistent across the API.api/spec/src/customer.tsp (3)
115-115: Consistent migration to QueryPaginationThis change from
...PaginatedQueryto...QueryPaginationin thelistCustomerSubscriptionsmethod signature aligns with the broader API pagination refactoring effort.
137-137: Consistent pagination model updateThe change from
...PaginatedQueryto...QueryPaginationin theListCustomersParamsmodel maintains consistency with the overall pagination refactoring.
195-195: Pagination model standardizationThe update from
...PaginatedQueryto...QueryPaginationin theListCustomerAppDataParamsmodel completes the standardization of pagination interfaces throughout the customer API.api/spec/src/events.tsp (2)
145-155: Proper pagination and client ID parameter handlingThe method correctly incorporates cursor pagination and preserves the client ID parameter from the V1 API. The documentation for the client ID parameter is clear and helpful.
174-175: Response type uses cursor paginationThe return type
CursorPaginatedResponse<IngestedEvent>correctly reflects the cursor-based pagination mechanism rather than using the previous pagination approach.openmeter/streaming/clickhouse/connector.go (2)
98-113: Well-structured implementation of ListEventsV2The
ListEventsV2method implements proper parameter validation and error handling, following the same pattern as existing methods. This maintains consistency in the codebase.
340-361: Comprehensive progress tracking for queriesThe implementation of the
queryEventsTableV2method includes proper setup for tracking query progress, which is important for long-running queries. The error handling in the progress tracking section correctly logs errors without interrupting the main flow.openmeter/meterevent/adapter/event.go (2)
10-10: Package import for the new pagination looks good.The new import supports the pagination functionality being added to the ListEventsV2 method.
65-115: ListEventsV2 implementation follows good design patterns.This new method follows the same structure as the existing ListEvents method but returns a paginated result. The code properly handles:
- Input validation
- Error handling with detailed error messages
- Meter retrieval and validation
- Conversion to pagination result
A few observations:
- The method properly propagates errors using the models.NewGenericValidationError
- Event validation against meters follows the same pattern as in ListEvents
- Error handling is consistent throughout the method
openmeter/meterevent/httphandler/mapping.go (4)
1-2: Package name change and code generation setup.The package name has been changed from
httpdrivertohttphandler, which better reflects its purpose. The added go:generate directive enables automatic code generation for conversion functions.
17-29: Clean implementation of goverter directives for conversion functions.The directives are well-structured and clearly document how the conversion functions should be generated. The use of a code generator here will help maintain consistency between API and internal models.
31-34: Simple namespace conversion function.This is used as a mapping function for the goverter framework. It's a straightforward implementation that passes through the namespace value.
70-84: Well-implemented conversion function for paginated responses.The function correctly:
- Iterates through each event in the result
- Converts each event using the existing convertEvent function
- Properly handles errors during conversion
- Constructs the paginated response with items and next cursor
- Uses lo.FromPtr to safely handle optional cursor
openmeter/streaming/clickhouse/event_query_v2.go (3)
11-16: Clearly structured query parameters struct.The
queryEventsTableV2struct is well-designed with appropriate fields for database, table name, and query parameters.
19-99: Comprehensive SQL query generation with cursor-based pagination.The
toSQLmethod is well-implemented with:
- Proper base filters for namespace
- Dynamic filter application based on provided parameters
- Smart handling of time column selection
- Effective cursor-based pagination implementation
- Stable ordering by time and ID
- Configurable result limiting
The cursor-based pagination implementation (lines 76-86) is particularly well done, handling the case where events share the same timestamp by using ID as a secondary sort key.
101-137: Performance-optimized count query implementation.The
toCountRowSQLmethod intelligently:
- Applies only the most significant filters for better performance
- Skips pagination and ordering which aren't needed for counts
- Maintains the essential namespace filter
- Includes only filters likely to significantly affect the count
The comment on line 113 explains the performance consideration, which is good documentation.
openmeter/meterevent/service.go (5)
9-10: New imports for filter and pagination packages.These imports support the new filtering and pagination functionality being added to the service.
20-20: Service interface extension with ListEventsV2 method.This cleanly adds the new V2 method to the service interface while maintaining backward compatibility.
67-73: Well-implemented Cursor method for Event struct.This method creates a pagination cursor from an event's ID and time, enabling efficient cursor-based pagination. The struct follows the definition in pagination.Cursor with the appropriate fields.
120-142: Comprehensive parameter structure for V2 events listing.The
ListEventsV2Paramsstruct is well-designed with:
- Clear field naming and documentation
- Support for cursor-based pagination
- Optional limit parameter
- Comprehensive filtering options using the filter package
- Support for different types of filters (string and time)
144-199: Thorough validation logic for ListEventsV2Params.The validation method:
- Enforces required fields like namespace
- Validates cursor if provided
- Checks filter complexity to prevent excessive query complexity
- Ensures time and ingested_at are not both set, preventing conflicting filters
- Properly formats and joins error messages
- Uses appropriate error wrapping for context
openmeter/meterevent/httphandler/event_v2.go (3)
1-12: Good import organizationThe imports are well organized with standard library imports first, followed by project-specific imports. This follows Go best practices.
14-19: Well-structured type definitionsThe type aliases are clearly defined and provide a good abstraction layer between the API, handler, and service logic. This approach makes it easier to maintain and evolve the API independently of the internal implementations.
21-51: Clean handler implementation with proper error handlingThe ListEventsV2 handler follows good practices:
- It properly resolves the namespace context
- It handles parameter conversion and validation errors
- It returns appropriate error responses
- It uses the httptransport framework consistently
- It adds operation name for traceability
This implementation will help maintain consistency across the API and supports the cursor-based pagination approach.
pkg/filter/filter_test.go (5)
1010-1152: Comprehensive test cases for FilterString complexity validationThe test covers multiple important scenarios for filter complexity validation:
- Simple filters
- Nested filters within allowed depth
- Excessively deep filters exceeding the limit
- Mixed logical operators (AND/OR)
- Filters with other validation errors
The test structure is clear and the assertions properly check both valid cases and expected error messages.
1154-1276: Well-structured test cases for FilterInteger complexity validationSimilar to the FilterString tests, these test cases provide good coverage for integer filter validation with various depth scenarios. The pattern of testing is consistent with the other filter types which helps with maintainability.
1278-1374: Thorough test coverage for FilterFloat complexity validationThe tests follow the same pattern as the other filter types, ensuring consistency across all filter implementations. This consistency helps with maintaining the code and understanding the expected behavior.
1376-1409: Straightforward tests for FilterBoolean complexity validationThese tests are appropriately simpler since the FilterBoolean type doesn't support logical operators (AND/OR) and thus doesn't have nested structures. Good to see that the tests match the implementation complexity.
1411-1509: Complete test cases for FilterTime complexity validationThe FilterTime complexity validation tests cover all necessary scenarios including time-specific values. The test for deep nesting of time filters is particularly important as complex time-based queries could have significant performance implications.
openmeter/streaming/clickhouse/event_query_v2_test.go (3)
14-124: Comprehensive SQL generation tests for event queriesThe test cases cover a good range of scenarios:
- Basic namespace-only queries
- Various filter types (ID, subject, time)
- Cursor-based pagination with custom limits
- Time-based filtering
Each test verifies both the generated SQL string and the prepared statement arguments, which is crucial for catching parameter binding issues.
126-201: Well-structured tests for count queriesThese tests ensure that the count query SQL is generated correctly for various filter combinations. The tests appropriately focus on the WHERE clause variations rather than redundantly testing the ORDER BY and LIMIT clauses that don't apply to count queries.
203-206: Useful helper functionThe stringPtr helper function improves readability by reducing boilerplate. This is a good pattern for test code where you need to create many pointer values.
pkg/filter/filter.go (6)
15-16: Good interface extensionAdding the ValidateWithComplexity method to the Filter interface is a clean way to enforce complexity limits across all filter implementations. This helps prevent potential DoS vectors from overly complex queries.
77-111: Well-implemented complexity validation for FilterStringThe implementation:
- Reuses the existing Validate method to ensure basic validity
- Checks depth limits only when logical operators are present
- Recursively validates nested filters with proper depth decrementing
- Aggregates errors from nested validations
The approach is efficient since it avoids unnecessary recursion for simple filters.
193-227: Consistent implementation for FilterIntegerThe FilterInteger complexity validation follows the same pattern as FilterString, ensuring consistency across filter implementations. This helps with maintainability and makes the code easier to reason about.
296-330: Properly implemented FilterFloat complexity validationThe implementation maintains the consistent pattern established with the other filter types. The code duplication is acceptable here as it keeps each filter type implementation self-contained and independently verifiable.
398-432: Well-structured FilterTime complexity validationThe implementation correctly handles time-specific filters while maintaining the same validation pattern as the other filter types. This consistency across different data types is a sign of good design.
468-472: Simplified FilterBoolean implementationThe FilterBoolean implementation is appropriately simpler since it doesn't support logical operators and therefore can't have nested filters. This implementation correctly just delegates to the basic Validate method.
openmeter/apiconverter/filter.gen.go (6)
1-3: Build constraint ensures proper code generation workflowThe build constraint
//go:build !govertercorrectly ensures this generated file is excluded duringgovertertool execution, preventing circular references during the generation process.
11-24: Well-structured initialization of converter functionsThe
initfunction properly initializes all the converter functions declared in the source file. This approach allows for clean separation of interface declaration and implementation while enabling runtime function assignment.
25-62: Comprehensive Float filter conversion with proper null handlingThe
ConvertFloatandConvertFloatPtrfunctions correctly handle all fields from the source filter type, including nestedAndandOroperations. The implementation properly handles nil checks for pointer types and correctly performs recursive conversion for nested filters.
63-100: Integer filter conversion with proper recursive handlingThe implementation correctly maps all integer filter fields and handles recursive conversion of nested
AndandOrconditions. The pointer version appropriately handles nil cases to prevent nil pointer dereferences.
101-144: String filter conversion handles all comparison operatorsThe
ConvertStringfunction properly maps all string comparison operators, including equality, inequality, pattern matching, and range comparisons. The implementation also correctly handles recursive conversion of compound conditions.
145-181: Time filter conversion with appropriate recursionThe time filter conversion functions properly handle temporal range comparisons and compound conditions. The implementation correctly preserves the structure and semantics of the filter expressions.
openmeter/apiconverter/filter.go (3)
1-2: Well-configured code generation directiveThe
go:generatedirective is properly configured to run thegovertertool to generate the conversion code.
9-11: Clear goverter directives for code generationThe directives provide clear instructions to
goverter:
variablesindicates that variable functions should be generatedskipCopySameTypeis an optimization to avoid unnecessary conversionsoutput:filespecifies the output file pathThese settings ensure proper code generation with optimized performance.
12-23: Complete set of filter type convertersThe variable declarations provide a complete set of conversion functions for all filter types (String, Int, Float, Time, Boolean), each with both value and pointer variants. These converters are essential for the newly implemented v2 API endpoints to transform filter types between the API and internal representations.
tothandras
force-pushed
the
feat/eventsv2
branch
from
24523a8 to
e4a5ce1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
api/openapi.cloud.yaml (1)
8809-8822: Definition of Pagination.page Parameter
ThePagination.pageparameter is defined with a multi-line description combining "Page index" and "Default is 1." While functionally correct, consider merging the description into a single clear sentence to improve clarity in documentation.api/openapi.yaml (3)
8677-8690: Pagination.page parameter definition review.
The parameter definition for Pagination.page includes a multi-line description that indicates the page index and its default value. Consider rewording the description to “The current page number (default is 1)” for enhanced clarity.
8695-8701: Pagination.pageSize parameter definition review.
The description concisely states “The maximum number of items per page. Default is 100.” A slight rephrasing to “Number of items per page (default is 100)” might improve readability.
8989-9017: AppPaginatedResponse schema definition assessment.
The schema correctly requires totalCount, page, pageSize, and items. Descriptions and examples are provided, although standardizing the terminology (e.g., “Current page number” rather than “page index”) across schemas could be considered for uniformity.openmeter/meterevent/httphandler/mapping.go (1)
70-84: Clean implementation of pagination response conversionThe
convertListEventsV2Responsefunction is well implemented with proper error handling. It correctly converts each event and constructs the paginated response with the next cursor.Consider adding unit tests for this conversion function to ensure it works correctly with various pagination scenarios, including:
- Empty result sets
- Results with no next cursor
- Results with various types of events
func TestConvertListEventsV2Response(t *testing.T) { // Test case 1: Empty result emptyResult := pagination.Result[meterevent.Event]{} resp, err := convertListEventsV2Response(emptyResult) assert.NoError(t, err) assert.Empty(t, resp.Items) assert.Nil(t, resp.NextCursor) // Test case 2: Result with items but no next cursor events := []meterevent.Event{ // Add test events here } resultWithoutCursor := pagination.Result[meterevent.Event]{ Items: events, } // Add assertions // Test case 3: Result with items and next cursor // Add test case }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
api/client/javascript/src/client/schemas.ts(35 hunks)api/openapi.cloud.yaml(22 hunks)api/openapi.yaml(26 hunks)api/spec/src/query.tsp(4 hunks)openmeter/meterevent/httphandler/handler.go(2 hunks)openmeter/meterevent/httphandler/mapping.go(3 hunks)openmeter/server/router/event.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- openmeter/server/router/event.go
- api/client/javascript/src/client/schemas.ts
🧰 Additional context used
🧬 Code Definitions (2)
openmeter/meterevent/httphandler/handler.go (1)
openmeter/meterevent/httphandler/event_v2.go (1) (1)
ListEventsV2Handler(18-18)
openmeter/meterevent/httphandler/mapping.go (4)
api/api.gen.go (30) (30)
ListEventsV2Params(6509-6525)Event(2269-2269)IngestedEventCursorPaginatedResponse(2611-2617)IngestedEvent(2596-2608)e(13885-13887)e(13889-13891)e(13898-13900)e(13902-13904)e(13910-13912)e(13919-13921)e(13923-13925)e(13932-13934)e(13936-13938)e(13945-13947)err(10041-10041)err(10076-10076)err(10101-10101)err(10126-10126)err(10151-10151)err(10176-10176)err(10201-10201)err(10308-10308)err(10333-10333)err(10369-10369)err(10394-10394)err(10419-10419)err(10554-10554)err(10579-10579)err(10623-10623)err(10648-10648)openmeter/meterevent/httphandler/event_v2.go (1) (1)
ListEventsV2Params(15-15)api/client/go/client.gen.go (2) (2)
Event(2099-2099)IngestedEvent(2296-2308)openmeter/meter/parse.go (4) (4)
e(21-28)e(30-32)e(44-51)e(53-55)
🔇 Additional comments (55)
api/openapi.cloud.yaml (21)
43-57: Pagination Update for List Apps Endpoint
The parameters for pagination have been updated to usePagination.pageandPagination.pageSize, and the response schema now correctly points toAppPaginatedResponse. Please verify that downstream clients and documentation reflect these new references.
2064-2071: Pagination Parameter Integration for List Customers
The "List customers" endpoint now uses the new pagination parameters correctly while preserving the customer-specific order and filter parameters. No issues detected here.
2138-2145: Pagination in Customer App Data Endpoint
The insertion ofPagination.pageandPagination.pageSizeinto the parameter list is consistent with the new pagination approach. Ensure the ordering and filtering parameters still meet client expectations.
2443-2450: Pagination Reference Addition
The endpoint now adds the pagination references (Pagination.pageandPagination.pageSize) after the identifier field. This update aligns with the new pagination model—please double-check that the order of parameters is as intended.
3743-3757: Marketplace Listing: Paginated Response Update
The "List available apps" endpoint is updated to include the new pagination parameters and now returns aMarketplaceListingPaginatedResponse. Verify that the response schema’s descriptions (especially for pagination fields) meet the documentation guidelines and that indentation is correct.
8827-8833: Definition of Pagination.pageSize Parameter
The parameter for the maximum number of items per page is set with an appropriate multi-line description. Ensure that the API docs render "The maximum number of items per page. Default is 100." clearly for end users.
9044-9073: AppPaginatedResponse Schema Update
TheAppPaginatedResponseschema now requirestotalCount,page,pageSize, anditemswith clear descriptions and examples. This update correctly reflects the new pagination metadata.
9566-9580: Consistent Pagination Properties in Schema
The properties for pagination in this schema (i.e.totalCount,page, andpageSize) include clear descriptions and examples. Ensure that these remain consistent across all paginated responses.
9605-9619: Uniform Pagination Schema Definitions
The schema block maintains uniform definitions for pagination properties, mirroring previous segments. No further action is needed.
10448-10462: Replicated Pagination Definitions
The properties here are well defined with appropriate types, descriptions, and examples. Consistency with other parts of the API schema is maintained.
10557-10571: Consistent Pagination Field Descriptions
This segment reiterates the standard pagination properties with clear and concise descriptions. Everything appears consistent.
11409-11423: Uniform Pagination Property Descriptions
The pagination fields defined in this section are consistent with earlier definitions. The clear examples and descriptions support better clarity in API docs.
11865-11879: Consistent Pagination Schema Definitions
The updated pagination properties are accurately defined. The descriptions and examples match the overall API pagination design.
12015-12029: Pagination Attributes Consistency
This hunk consistently defines pagination properties, ensuring thattotalCount,page, andpageSizeare all clearly described and exampled.
12948-12962: Standardized Pagination Fields
The pagination properties here continue to align with the standardized schema across the API. The implementation is clear and concise.
13849-13880: MarketplaceListingPaginatedResponse & MeasureUsageFrom Definitions
The newMarketplaceListingPaginatedResponseschema is introduced with required pagination fields, and its properties have clear descriptions and examples. Additionally, theMeasureUsageFromcomponent is defined immediately afterward, ensuring it is not accidentally nested within the Marketplace schema. Please verify the YAML indentation to avoid schema misinterpretation.
14322-14336: Pagination Schema Properties Validation
The definitions fortotalCount,page, andpageSizein this block follow the established pattern with correct descriptions and examples.
14655-14669: Pagination Block Consistency
This section maintains the standard pagination properties with clear documentation. It is consistent with the rest of the API schema.
14874-14888: Standardized Pagination Schema
The pagination properties are uniformly defined and follow the prescribed schema structure with appropriate descriptions and examples.
15206-15220: Pagination Properties Consistency Check
The pagination properties in this segment are correctly defined, ensuring consistency across all related responses.
16777-16791: Final Pagination Schema Check
The final pagination schema properties remain consistent with previous definitions. Clear descriptions and examples help maintain overall documentation quality.api/spec/src/query.tsp (7)
16-25: Documentation improvement for page field is clear and accurate.The updated comment for the
pagefield now correctly describes it as a "Page index" rather than a date-time format, which aligns better with its actual purpose in pagination. The documentation also clarifies that the default value is 1.
27-37: Clear documentation update for pageSize field.The comment now explicitly states this is "The maximum number of items per page" rather than just "Number of items per page", which better conveys the constraint nature of this parameter.
106-110: Documentation correction for totalCount field.The updated comment now correctly describes the purpose of
totalCountas "The total number of items" rather than incorrectly describing it as "The items in the current page."
112-116: Fixed documentation for page field in response.The comment now properly describes this field as "The page index" rather than incorrectly describing it as "The items in the current page."
118-122: Documentation improvement for pageSize in response.The comment now correctly specifies this as "The maximum number of items per page" rather than incorrectly describing it as "The items in the current page."
130-149: Well-structured cursor pagination query parameters.The new
QueryCursorPaginationmodel is a good addition to support cursor-based pagination, which is more efficient than offset-based pagination for large datasets. The constraints on thelimitparameter (min 1, max 1000) help prevent excessive resource usage.
151-167: Consider enhancing the nextCursor documentation for clarity.The
CursorPaginatedResponsemodel looks good. ThenextCursorfield is correctly implemented as optional, which addresses the previous review comment about pagination termination. However, the documentation could be clearer about how to interpret when this field is missing./** * The cursor of the last item in the list. + * When null or empty, there are no more items to paginate. */ nextCursor?: string;api/openapi.yaml (23)
42-56: Pagination parameters and response schema in the "List apps" endpoint look correct.
The hunk properly adds the pagination query parameters (Pagination.page and Pagination.pageSize) and updates the response to reference the new AppPaginatedResponse schema. This ensures that the endpoint returns pagination metadata consistently.
2063-2070: Pagination added to "List customers" endpoint.
The inclusion of Pagination.page and Pagination.pageSize in the parameters ensures that customer listings are paginated in line with the new API standards.
2137-2143: Pagination parameters integrated in customer filtering schema.
Adding the Pagination.page and Pagination.pageSize references alongside the existing filter (queryCustomerList.type) helps maintain consistent pagination behavior.
2442-2449: Pagination parameters appended in the filter block.
The addition of Pagination.page and Pagination.pageSize here aligns this schema with the updated pagination strategy. Ensure that the pattern and example for the preceding field continue to match expected formats.
3742-3756: Marketplace listing endpoint updated for pagination.
The endpoint "List available apps" now includes the pagination properties and correctly references the MarketplaceListingPaginatedResponse schema. This update provides clear pagination support for marketplace listings.
8064-8173: New/api/v2/eventsendpoint implementation is comprehensive.
This hunk introduces the listEventsV2 endpoint with advanced filtering and cursor-based pagination. All expected parameters (cursor, limit, clientId, and various filter fields) are defined, and the responses properly reference the IngestedEventCursorPaginatedResponse. The internal flag (x-internal: true) is appropriately set.
8277-8301: Cursor pagination parameter definitions updated.
Both CursorPagination.cursor and CursorPagination.limit are clearly defined with proper types, constraints, and descriptions. This will enable robust cursor-based pagination across endpoints.
9519-9533: Paginated response schema properties are consistent.
The definitions for totalCount, page, and pageSize here mirror those in other parts of the API. Maintain this uniformity to aid both documentation and client-side integration.
9593-9607: Pagination schema properties consistency check.
The fields totalCount, page, and pageSize are defined with clear descriptions and examples, ensuring uniformity across paginated responses.
10449-10463: Paginated response schema properties are accurate.
The consistent use of descriptions and examples across these schema definitions reinforces a clear and coherent pagination model.
10558-10572: Pagination schema structure confirmed.
All required fields (totalCount, page, pageSize, items) are defined and described appropriately. This provides a standard response structure for paginated endpoints.
11484-11498: Repeated pagination schema properties are consistent.
The properties remain well-defined and consistent with the overall pagination strategy used throughout the API specification.
11940-11954: Pagination model properties verification.
The schema maintains consistency in defining totalCount, page, and pageSize with clear minimum values and examples.
12288-12302: Pagination schema integrity check.
The definitions for totalCount, page, and pageSize are uniform with previous models, ensuring a consistent design across documents.
12378-12396: New IngestedEventCursorPaginatedResponse schema defined.
This schema introduces cursor-based pagination for ingested events with fields for items (with a maxItems constraint) and nextCursor. It appropriately supports the new events endpoint. Verify that the maxItems value aligns with your overall API design considerations.
13236-13250: Pagination model properties verified.
Once again, the schema for paginated responses is in line with the accepted structure across the API.
14069-14089: Query parameters for ListEntitlementsResult updated.
The definitions for both 'page' and 'pageSize' include minimum values, maximum constraints, defaults, and examples. This detailed specification aids the client in correctly using the pagination functionality.
14160-14189: MarketplaceListingPaginatedResponse schema consistency.
The schema for marketplace listings now clearly defines required fields and includes proper examples and descriptions for pagination properties. The additional MeasureUsageFrom definition is noted and appears appropriately defined as a compositional schema.
14633-14647: Uniform pagination schema properties.
The properties defined here for totalCount, page, and pageSize are consistent with other pagination models throughout the spec.
14966-14980: Pagination schema consistency continued.
The definitions for pagination continue to mirror the overall API standard, ensuring that all endpoints exhibit similar behavior and documentation.
15205-15219: Pagination model structure verified.
This section adheres to the consistent pattern used elsewhere in the API for pagination responses.
15537-15551: Pagination properties check.
The consistent application of totalCount, page, and pageSize with correct descriptions and examples ensures an easily understandable pagination model.
17220-17234: Final pagination schema verification.
The pagination schema properties here maintain the expected definitions and descriptions as laid out in earlier sections. No discrepancies detected.openmeter/meterevent/httphandler/handler.go (2)
1-1: Package name change improves clarityThe package name change from
httpdrivertohttphandlerbetter reflects the actual purpose of this package, which is to handle HTTP requests rather than drive HTTP operations.
19-21:❓ Verification inconclusive
Interface extension done correctly
Good job adding the new
ListEventsV2()method to theEventHandlerinterface in a backward-compatible way. This extends the interface without modifying existing functionality, which minimizes the risk of breaking changes.
🏁 Script executed:
#!/bin/bash # Verify that all implementations of EventHandler implement the new ListEventsV2() method echo "Checking implementations of EventHandler interface..." rg -l "type \w+ struct" | grep -v "_test.go" | xargs rg --no-heading -A 5 "func \(\w+ \*\w+\) ListEventsV2\(\)"Length of output: 228
Interface Extension Approved – Manual Verification Required
The updatedEventHandlerinterface now includes the newListEventsV2()method correctly without affecting existing functionality. However, our initial automated script did not return any output for method implementations. Please manually verify (or run a more targeted search) that all concrete implementations of theEventHandlerinterface correctly implementListEventsV2().
- Ensure that implementations in all relevant Go files contain a
ListEventsV2()method in addition to the existing methods.- If no implementations are found by an automated search, double-check whether these implementations use a different pattern (e.g., via embedding or alternative receiver definitions) that might not match the initial regex search.
openmeter/meterevent/httphandler/mapping.go (2)
1-2: Package name change and code generation setupThe package renaming is consistent with the handler.go file, and the go:generate directive is properly set up to use goverter for automatic conversion between types.
17-29: Well-structured type conversion configurationThe goverter directives are well organized to configure the automatic generation of conversion functions. The variable declaration for
convertListEventsV2Paramsproperly indicates its purpose with appropriate mapping and context directives.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
api/openapi.yaml (2)
8677-8690: Clarification for Pagination.page Parameter
ThePagination.pageparameter is described as "Page index" and notes a default of 1 in the description. Consider specifying the default value explicitly within the schema (e.g.,default: 1) to enforce this at the validation level.
8695-8701: Enhancing Pagination.pageSize Parameter Definition
ThepageSizeparameter clearly explains its role and default value. As withpage, it may be beneficial to include an explicitdefault: 100property in the schema rather than only in the description.api/openapi.cloud.yaml (2)
8809-8822: Pagination.page Parameter Definition Update
The definition forPagination.pageis updated to provide a descriptive multi-line explanation. It would be beneficial to add an explicitdefault: 1property under the schema for clarity and potential client code generation.
8827-8833: Pagination.pageSize Parameter Definition
ThePagination.pageSizeparameter includes a clear description and notes a default of 100. Consider adding adefault: 100property within its schema to enforce the default value explicitly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
api/client/javascript/src/client/schemas.ts(35 hunks)api/openapi.cloud.yaml(22 hunks)api/openapi.yaml(26 hunks)api/spec/src/query.tsp(4 hunks)openmeter/meterevent/httphandler/handler.go(2 hunks)openmeter/meterevent/httphandler/mapping.go(3 hunks)openmeter/server/router/event.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- openmeter/meterevent/httphandler/handler.go
- openmeter/server/router/event.go
- openmeter/meterevent/httphandler/mapping.go
- api/spec/src/query.tsp
- api/client/javascript/src/client/schemas.ts
🔇 Additional comments (44)
api/openapi.yaml (24)
42-53: Pagination Parameters in List Apps Endpoint
The use of references for pagination parameters (Pagination.pageandPagination.pageSize) and the linkage to theAppPaginatedResponseschema is well implemented. This promotes consistency across endpoints.
2063-2070: Pagination Integration in List Customers Endpoint
Adding pagination parameters here supports consistent client-side pagination. The inclusion of ordering parameters further clarifies the expected behavior for customer listings.
2137-2143: Appending Pagination References in Filtered Query
Incorporating the pagination references immediately after the pattern definition for theidproperty is appropriate. Please verify that the underlying definitions for these parameters specify the correct integer type and descriptive text.
2442-2449: Appending Pagination to Pattern-Based Definition
The addition of$reffor bothPagination.pageandPagination.pageSizefollowing the pattern and example definitions is correct. This enhancement maintains clarity in how pagination is applied to endpoints with custom validations.
3742-3756: Marketplace Listings: Consistent Pagination Adoption
The integration of pagination parameters along with the accurate reference toMarketplaceListingPaginatedResponseensures a uniform design for listing endpoints.
8064-8174: New /api/v2/events Endpoint with Cursor-Based Pagination
This hunk introduces thelistEventsV2endpoint. The use of cursor-based parameters (CursorPagination.cursorandCursorPagination.limit) together with inline query parameters (e.g.,clientId) is well structured. Ensure that the differing styles (deepObject vs. basic query) are intentional and documented in the API specification.
8277-8301: Defining Cursor Pagination Parameters
The new definitions forCursorPagination.cursorandCursorPagination.limitare clear—with proper validations on limit (minimum, maximum, and default). This addition will support robust cursor-based pagination.
8989-9017: AppPaginatedResponse Schema Consistency
The schema forAppPaginatedResponseis defined with clear, consistent pagination properties—totalCount,page,pageSize, anditems. Example values further clarify the expected data.
9519-9533: Consistent Pagination Schema Properties
The pagination fields in this schema mirror those defined elsewhere (with clear descriptions and examples). This consistency across models aids in predictable client interactions.
9593-9606: Standardizing Pagination Definitions
The defined properties (totalCount, page, pageSize, items) maintain uniformity with other response schemas. Keeping identical descriptions helps consumers understand the pagination model.
10449-10463: Uniform Pagination Model Definition
The structured definition here—with required pagination properties and consistent examples—continues the best practices for paginated responses across the API.
10558-10572: Consistent Pagination Fields in Response Schema
The pagination properties continue to be defined uniformly, ensuring that all API consumers receive the same metadata irrespective of the endpoint.
11484-11498: Reinforcing Uniform Pagination Descriptions
These pagination fields (totalCount,page,pageSize, anditems) are adequately described and standardized. Consistency across different schema definitions is maintained.
11940-11954: Standardized Pagination Properties
This schema reaffirms the consistent application of pagination properties with clear descriptions and examples, enhancing API usability.
12288-12302: Reiterated Consistency in Pagination Definitions
Maintaining the same pattern for pagination fields ensures a uniform structure across all response models within the API.
12378-12396: IngestedEventCursorPaginatedResponse Schema Definition
The schema forIngestedEventCursorPaginatedResponseis clearly defined with a requireditemsarray and an optionalnextCursorfor subsequent pagination. The descriptions are concise and informative, supporting cursor-based navigation.
13236-13249: Additional Schema: Consistent Pagination
The pagination fields here are in line with the standardized models used elsewhere, ensuring predictability in response formats.
14069-14089: Enhanced Query Parameter Validation for Pagination
The query parameters for pagination now include explicitminimum,maximum, anddefaultvalues. This reduces client errors by enforcing valid input ranges.
14160-14189: MarketplaceListingPaginatedResponse Schema Update
The schema updates forMarketplaceListingPaginatedResponsenow clearly delineate pagination fields with accurate descriptions and examples. This standardization improves clarity for API users.
14633-14647: Standard Pagination in Additional Models
The consistent use of the pagination properties (totalCount,page,pageSize, anditems) across different schema definitions is commendable. It ensures a uniform experience for API consumers.
14966-14980: Reinforced Pagination Property Definitions
This hunk further confirms the uniformity of pagination field definitions. Maintaining identical descriptions and examples helps prevent client-side misinterpretations.
15205-15219: Consistent Pagination Format in Response Schema
The pagination fields here adhere to the established standard. Consistent examples and clear descriptions make for an intuitive API design.
15537-15551: Uniform Pagination Configuration Confirmed
The pagination configuration is uniformly implemented with descriptive properties and example values, reinforcing the API’s consistent design language.
17220-17234: Standardized Pagination Fields for Final Schema
The final schema section continues the established pattern for pagination properties—this uniformity greatly aids developers in understanding and integrating with the API.api/openapi.cloud.yaml (20)
43-57: Update to List Apps Endpoint with Pagination Parameters
The changes correctly update the endpoint by replacing legacy pagination parameters withPagination.pageandPagination.pageSizeand switching the response schema toAppPaginatedResponse. Please ensure that the referenced components are defined consistently in your components section.
2064-2071: List Customers Endpoint – Pagination Parameter Refactor
The update replaces the old pagination parameters with the newPagination.pageandPagination.pageSize. This looks consistent with the overall pagination refactor. Verify that other related endpoints have been similarly updated.
2138-2145: Adding Pagination Parameters in Customer Query
The addition of$refentries forPagination.pageandPagination.pageSizein this parameter block ensures that pagination is applied uniformly. Please double-check that the definitions of these parameters (type, defaults, etc.) match across the API specification.
2443-2450: Pagination Parameters Added for Identifier-Based Endpoint
The inclusion ofPagination.pageandPagination.pageSizealongside the existing string identifier is well implemented. Ensure that the example and pattern provided remain appropriate given these changes.
3743-3757: List Available Apps Endpoint – Updated Pagination Schema
The endpoint now uses the new pagination parameters and updates its response schema toMarketplaceListingPaginatedResponse. This change is consistent with the API-wide pagination improvements.
9044-9075: Definition of AppPaginatedResponse Schema
TheAppPaginatedResponseschema properly lists the required pagination fields (totalCount,page,pageSize, anditems) with clear descriptions and examples. Verify that these descriptions align with your documentation standards—especially if any further clarification (e.g., stating "available items") is desired.
9566-9580: Paginated Schema Properties Consistency
The properties fortotalCount,page, andpageSizeare updated consistently with accurate examples. This uniformity helps API consumers understand the pagination response format.
9605-9619: Consistent Pagination Schema for Endpoint
The pagination fields are consistently defined here. It is advisable to ensure that the same descriptions and examples are used across all similar schemas throughout the document.
10448-10462: Uniform Pagination Property Definitions
The pagination properties maintain a consistent structure and description format. This aids clarity for consumers of the API.
10557-10571: Pagination Schema Fields Consistency
The updated pagination fields across this schema are clearly and uniformly described. This consistency helps reinforce the new pagination model.
11409-11423: Standardized Pagination Schema
The schema properties continue to follow the established pattern for pagination. No issues observed; the descriptions and examples are adequate.
11865-11879: Consistent Definition for Pagination in Schema
This segment maintains uniform pagination property definitions with clear descriptions. Verify that these definitions match across all endpoints.
12015-12029: Pagination Schema Consistency Check
The updated pagination properties here are consistent with those in other parts of the API spec. Great job on maintaining the consistency.
12948-12962: Uniform Pagination Fields Definition
The pagination fields in this section are defined in a uniform manner. This consistency is key for API consumers.
13849-13880: MarketplaceListingPaginatedResponse Schema Update
TheMarketplaceListingPaginatedResponseschema is updated properly with the required pagination fields and an items array referencingMarketplaceListing. Additionally, the separation ofMeasureUsageFromas a separate component appears correct. Confirm thatMeasureUsageFromis defined as expected elsewhere in your components.
14322-14336: Standardized Pagination Properties in Response Schema
The pagination properties in this segment are consistent with the updates across the API. The clarity in descriptions and examples is maintained.
14655-14669: Consistent Pagination Schema in Additional Endpoint
This update reinforces the consistent structure for pagination responses across the API. All property descriptions and examples align well.
14874-14888: Pagination Schema Defined Consistently
The changes continue to follow the established pagination schema, with clear and consistent definitions for each property.
15206-15220: Maintaining Uniform Pagination Schema
The pagination fields in this hunk adhere to the overall structural changes. No issues observed in the description or example values.
16777-16791: Final Pagination Schema Consistency Check
The pagination schema defined here is consistent with all previous updates. This uniformity across the API specification should greatly improve clarity for API consumers.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (23)
pkg/pagination/v2/cursor.go (1)
27-39: Add documentation comment for the Validate methodThe code correctly implements validation for the Cursor struct, but it's missing a documentation comment. All other methods in this file have descriptive comments explaining their purpose and behavior. Consider adding a documentation comment to maintain consistency and improve code clarity.
+// Validate checks if the Cursor has valid Time and ID fields. +// It returns an error if either field is invalid. func (c Cursor) Validate() error { var errs []errorapi/openapi.yaml (13)
8067-8191: New Events V2 Endpoint Implementation
The new/api/v2/eventsendpoint introduces advanced filtering and cursor-based pagination. The extensive list of query parameters (including cursor, limit, and various filters) is well structured. Please verify that each parameter’s description and style (e.g., usage of deepObject for filters) meets the API consumer’s needs and that thex-internal: trueflag is intended for internal use only.
9010-9034: AppPaginatedResponse Schema Definition
TheAppPaginatedResponseschema is now defined with the necessary pagination properties and example values. For improved clarity, consider refining each description (for example, using "Total number of available items" fortotalCount, "Current page number" forpage, and "Maximum items per page" forpageSize).
9537-9551: Review of Pagination Properties in Schema
The pagination properties (totalCount,page,pageSize) in this schema are defined consistently with previous ones. As a best practice, you might differentiate the descriptions further to reduce any ambiguity for API consumers.
9611-9625: Ensuring Consistent Pagination Descriptions
The pagination property definitions here maintain the established pattern. Consider standardizing the descriptive text (or even referencing a common component) across the API to enhance clarity and reduce maintenance overhead.
10467-10481: Pagination Property Consistency Check
The schema segment reiterates the pagination properties with inline examples. While consistency is good, consider unifying these descriptions with previous definitions for a more uniform API documentation.
10576-10590: Verification of Pagination Schema Components
The pagination-related fields here (totalCount, page, pageSize, items) are consistent with established patterns. Revisiting the descriptions to provide unique, clarifying details for each property might further improve the documentation.
11502-11516: Reinforcing Standardization of Pagination Schema
This segment’s pagination definitions are consistent with other parts of the API. Consistency is key, so consider refining some descriptions for better clarity, as suggested in earlier comments.
11958-11972: Consistency in Pagination Attributes
The pagination definitions remain uniform with the rest of the API. If possible, consolidating these repeated structures into a common reusable component could enhance maintainability.
12306-12320: Standardization of Pagination Properties
The repeated pagination structure here is consistent with other sections. As a future improvement, consider creating a shared reference for these properties to avoid duplicated definitions.
12399-12413: New IngestedEventCursorPaginatedResponse Schema
This new schema effectively encapsulates the cursor-based pagination concept for ingested events. You may wish to review the naming ofnextCursor(versus simplycursor) for consistency with other pagination strategies in the API.
13254-13268: Pagination Schema Verification
The pagination properties here confirm the uniform pattern established throughout the specification. Maintaining this consistency is key; however, think about consolidating these repeating definitions.
14181-14206: MarketplaceListingPaginatedResponse Schema Update
The updated definition forMarketplaceListingPaginatedResponsealigns with the established pagination pattern. As with other schemas, consider harmonizing descriptive texts across similar objects to aid API consumers.
14651-14665: Consistent Pagination Schema Check
This segment reiterates the standard pagination properties. Watch for opportunities to consolidate these definitions into a single reusable component to avoid repetition in the OpenAPI spec.api/openapi.cloud.yaml (2)
9710-9724: Consistent Pagination Properties in Schema
The pagination fields are defined consistently, with clear descriptions and examples fortotalCount,page, andpageSize. Consider adding an explicit description for theitemsarray for improved clarity if not inherited elsewhere.
9749-9763: Uniform Pagination Schema Across Endpoints
This schema fragment repeats the standard pagination properties. A brief note on the contents or purpose of theitemsarray could further enhance understanding for API consumers.api/spec/src/events.tsp (1)
157-165: Consider adding more comprehensive filter examplesWhile the examples for each filter parameter are good, they only demonstrate the
$eqoperator for string filters. Consider adding more complex examples showing multiple filter operators (like$in,$regex, etc.) to better illustrate the capabilities of the filtering system.For example, for the
idfilter:@example(#{ $eq: "my-event-id", + // Or alternatively: + // $in: ["id-1", "id-2", "id-3"], + // $regex: "^my-event-.*$", })Also applies to: 167-175, 177-185, 187-195
api/client/javascript/src/client/schemas.ts (4)
4195-4201: Optional clarification fornextCursor
TheIngestedEventCursorPaginatedResponseincludesnextCursor. If there's no next page, consider documenting whether this field isnull, empty, or omitted.
8007-8010: Consider documenting default or max limit
For'CursorPagination.limit', it might help to note any default values or maximum limits to guide client usage, similar to how page-based endpoints mention “Default is 100.”
8638-8641: Consider clarifying cursor usage
CursorPagination.cursorandlimitmight benefit from additional doc, e.g., default limit or usage examples.
19401-19509: Clarify defaults and format inlistEventsV2docs
All filtering fields (id,source,subject,type,time,ingestedAt) look comprehensive. A few suggestions:
• Specify whetherlimithas a default or maximum.
• Confirm if date-time filters (time,ingestedAt) must be in RFC 3339.
• Consider explaining hownextCursoris handled if there are no further pages.openmeter/streaming/clickhouse/event_query_v2.go (2)
74-77: Consider handling default time column more explicitlyThe code determines which time column to use for ordering based on whether
IngestedAtfilter is provided. This logic might not be immediately obvious to future developers.Consider adding a more explicit comment about the time column selection logic:
timeColumn := "time" if q.Params.IngestedAt != nil { + // If ingested_at filter is specified, use it for pagination ordering instead of event time timeColumn = "ingested_at" }
108-110: Consider documenting why namespace filter is always appliedThe namespace filter is applied to both the main query and count query, suggesting it's a critical filter for data segregation.
Consider adding a brief comment explaining why namespace is always required:
// Base filter for namespace +// Namespace is a mandatory filter for security/isolation query.Where(query.Equal("namespace", q.Params.Namespace))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
api/client/javascript/src/client/schemas.ts(42 hunks)api/openapi.cloud.yaml(25 hunks)api/openapi.yaml(26 hunks)api/spec/src/cloud/main.tsp(1 hunks)api/spec/src/events.tsp(1 hunks)api/spec/src/query.tsp(4 hunks)openmeter/meterevent/service.go(4 hunks)openmeter/streaming/clickhouse/event_query_v2.go(1 hunks)pkg/pagination/v2/cursor.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- api/spec/src/query.tsp
- openmeter/meterevent/service.go
🧰 Additional context used
🧬 Code Definitions (2)
openmeter/streaming/clickhouse/event_query_v2.go (2)
openmeter/meterevent/service.go (1) (1)
ListEventsV2Params(121-142)pkg/pagination/v2/cursor.go (1) (1)
Cursor(11-14)
api/client/javascript/src/client/schemas.ts (2)
api/client/go/client.gen.go (4) (4)
AppPaginatedResponse(712-724)FilterString(2193-2235)FilterTime(2238-2256)IngestedEventCursorPaginatedResponse(2377-2383)api/api.gen.go (4) (4)
AppPaginatedResponse(813-825)FilterString(2423-2465)FilterTime(2468-2486)IngestedEventCursorPaginatedResponse(2611-2617)
🔇 Additional comments (86)
pkg/pagination/v2/cursor.go (4)
5-5: LGTM! Added error package import for errors.Join.The import of the errors package is required for the new Validate method.
30-32: LGTM! Valid time check.Checking if the time is zero is an appropriate validation for a cursor.
34-36: LGTM! Valid ID check.Checking if the ID is empty is an appropriate validation for a cursor.
38-38: LGTM! Error joining implementation.The implementation correctly uses errors.Join to combine multiple validation errors, as suggested in the previous review comment.
api/openapi.yaml (9)
45-46: Pagination Parameter References Verification
The endpoint now references the pagination parameters (Pagination.pageandPagination.pageSize) and uses theAppPaginatedResponseschema. Please verify that the referenced components have clear and consistent descriptions aligned with the API’s pagination strategy.Also applies to: 53-53
2066-2067: Consistent Pagination for Customers Endpoint
Adding pagination parameters to the customers endpoint enhances consistency with other list endpoints. Ensure that the definitions forPagination.pageandPagination.pageSizein the components section include accurate descriptions.
2140-2141: Addition of Pagination Parameters
The integration of pagination parameters in this segment standardizes the interface. Please confirm that the referenced parameter definitions (forPagination.pageandPagination.pageSize) provide users with clear guidance on usage.
2445-2446: Standardizing Pagination in Resource Identifier Endpoint
Incorporating pagination references here aligns this endpoint with the overall API design. Verify that the descriptions and constraints in the referenced parameters are consistent across endpoints.
3745-3746: Marketplace Listing Pagination Integration
The “List available apps” endpoint now properly includes pagination parameters and references theMarketplaceListingPaginatedResponseschema. Ensure that the schema’s properties accurately convey the intended values (e.g., current page, maximum number of items per page).Also applies to: 3753-3753
8298-8316: Definition of Cursor-based Pagination Parameters
The additions forCursorPagination.cursorandCursorPagination.limitare clearly defined with proper validation (e.g., minimum, maximum, default). Confirm that these constraints meet the system requirements for cursor pagination.
8698-8705: Clarification for Pagination Page Parameter
ThePagination.pageparameter now includes a multi-line description indicating the page index and its default value. Ensure that this description is consistent with similar parameters across the API.
8713-8719: Pagination Page Size Description Enhancement
The updated description for the page size parameter clearly explains its purpose and default value. Please double-check that the associated schema constraints match this description across all endpoints.
14087-14107: Enhanced Pagination Constraints for Installed Apps
The explicit constraints forpage(minimum of 1) andpageSize(minimum of 1, maximum of 1000, default 100) are clearly defined. Verify that the increased maximum forpageSizeis intentional and consistent with overall performance expectations across the API.api/spec/src/cloud/main.tsp (1)
109-111: LGTM: Clean implementation of Events V2 API endpointThe new
EventsV2interface is correctly implemented following the established API patterns in the codebase. It properly extends the base interface from OpenMeter namespace and is appropriately tagged. This approach allows adding new functionality without breaking existing V1 clients.api/openapi.cloud.yaml (24)
43-57: Pagination Update for List Apps Endpoint
The new pagination parameters (Pagination.pageandPagination.pageSize) and the updated schema reference (AppPaginatedResponse) are correctly integrated into this endpoint. Ensure that the descriptions of the pagination fields in the schema match the API documentation standards.
2064-2071: Pagination Parameters for List Customers
The update adds the new pagination parameters to the "List customers" endpoint. The integration looks correct; please verify that the overall order of parameters (including the ordering parameters) aligns with client expectations.
2138-2145: Adding Pagination to Customer Query
The insertion ofPagination.pageandPagination.pageSizereferences in this endpoint ensures consistency with the new pagination model. Double-check that any client-side validation or usage reflects this change.
2443-2450: Pagination Parameters in Customer‐Related Endpoint
The addition of the pagination parameters here is consistent with similar endpoints. Confirm that the schema and validation for the customer identifier remain unaffected by these changes.
3743-3757: Marketplace Listing Endpoint Pagination Update
The endpoint now correctly references thePagination.pageandPagination.pageSizeparameters and uses the newMarketplaceListingPaginatedResponseschema. Ensure that integration tests validate that the response metadata (totalCount, page, pageSize) is correctly populated.
8319-8449: Enhanced Cursor-Based Pagination for ListEventsV2
The/api/v2/eventsGET operation now incorporates cursor-based pagination (withCursorPagination.cursorandCursorPagination.limit) and detailed filtering options. The use ofdeepObjectfor complex query parameters is well implemented. Please ensure that the documentation clearly explains the expected formats and possible edge cases for these filters.
8553-8577: Definition of CursorPagination Parameters
The new parameters for cursor-based pagination are detailed with appropriate constraints (e.g., minimum value, maximum value, and default). Verify that these constraints are enforced in the implementation and are clearly documented.
8953-8966: Pagination.page Parameter Definition
ThePagination.pageparameter is now defined with a clear description indicating the default value and minimal constraints. For consistency, consider a similar formatting style for all pagination parameters used across endpoints.
8971-8977: Pagination.pageSize Parameter Description
The description correctly states that this parameter defines the maximum number of items per page, with a default of 100. Please confirm that a complete schema (including type and constraints) is provided later in the definition.
9188-9219: AppPaginatedResponse Schema Update
The updatedAppPaginatedResponseschema now includes all relevant pagination metadata (totalCount,page,pageSize, anditems) with clear descriptions and examples. Ensure this format is uniformly applied to all similar paginated responses.
10592-10606: Paginated Response Properties Verification
The schema correctly details all required pagination properties. If theitemsfield is not further documented in its own schema, consider adding a short description to help API users.
10701-10715: Uniform Pagination Schema Confirmation
The pagination definitions remain consistent with previous schemas. Consistent naming and examples aid in client-side integration.
11553-11567: Pagination Properties in Response Schema
The fieldstotalCount,page, andpageSizeshow consistent definitions and examples. If possible, documenting the purpose of theitemsarray in every instance would contribute to better clarity.
12009-12027: Paginated Response for Feature List
The response schema for the features list correctly includes pagination metadata along with a properly defineditemsarray. Good use of examples here.
12028-12132: New Filter Schema Definitions (FilterString & FilterTime)
The newFilterStringandFilterTimeschemas support a comprehensive set of operators, enhancing filter flexibility. Ensure that these definitions are well documented so that consumers can understand how to construct complex queries.
12263-12277: Standard Pagination Properties
This fragment reinforces consistent pagination property definitions. Verify that any endpoint using these properties validates data against these constraints.
12353-12371: Cursor-Based Pagination for Ingested Events
TheIngestedEventCursorPaginatedResponseschema is clearly defined with an array of ingested events and a next cursor for pagination. Confirm that themaxItemsvalue here aligns with the limits enforced via query parameters.
13211-13225: Pagination Property Standards Maintained
The pagination properties here are defined properly and mirror those used in other response schemas. Double-check that theitemsarray has adequate documentation in its respective schema.
14112-14143: MarketplaceListingPaginatedResponse & MeasureUsageFrom Component
The updatedMarketplaceListingPaginatedResponsenow cleanly organizes pagination metadata with clear examples, and the newMeasureUsageFromcomponent is defined using aoneOfconstruct referencingMeasureUsageFromPreset. Please ensure that the referenced preset schema covers all necessary cases.
14585-14599: Pagination Fields Consistency Check
This fragment reiterates the standard pagination property definitions. Minor suggestion: consider adding a description for theitemsarray if it isn’t sufficiently documented elsewhere.
14918-14932: Page Metadata Definition Validated
The consistent application oftotalCount,page, andpageSizewith clear examples ensures predictable API responses. Confirm that these definitions are used throughout the API responses.
15137-15151: Uniform Pagination Schema Application
The pagination fields are standardized as expected. If theitemsarray is meant to be self-explanatory through its schema, this is acceptable; otherwise, consider an inline description for clarity.
15469-15483: Consistent Pagination Definitions
The response schema clearly defines all necessary pagination fields in a uniform manner. No issues detected.
17040-17054: Final Verification of Pagination Structure
The pagination properties (totalCount,page,pageSize, anditems) are uniformly defined, ensuring consistency across all responses. This contributes to a predictable and reliable API behavior.api/spec/src/events.tsp (4)
134-223: Good implementation of the Events V2 API with enhanced filtering capabilitiesThe new EventsV2 interface introduces robust filtering capabilities and cursor-based pagination, which represents a significant improvement over the v1 API. The implementation allows for more flexible querying through operators like $eq, $gte, $lte, and $and, enabling users to construct complex queries.
I particularly like how examples have been added for all filter parameters, which addresses the previous review feedback from "hekike".
143-143: Confirm if the internal API designation is intentionalThe endpoint is marked as internal with
@extension("x-internal", true). Is this intentional, or is this API meant to be publicly available?
145-145: Verify QueryCursorPagination implementationThe code references
...QueryCursorPaginationbut doesn't define it in this file. Ensure that this type is properly defined elsewhere in the codebase and that it includes all necessary pagination parameters likelimit(which was explicitly defined in the v1 API).
222-222: Verify CursorPaginatedResponse implementationThe response type
CursorPaginatedResponse<IngestedEvent>is referenced but not defined in this file. Ensure this type is properly defined elsewhere in the codebase and includes all necessary pagination metadata fields (likenextCursor,items, etc.).api/client/javascript/src/client/schemas.ts (38)
1762-1781: Potential conflict in query parameter definitions
Here,query?: neversuggests no query parameters are supported. However, in a later chunk (lines 19401–19509) forlistEventsV2, we see multiple query parameters defined (e.g.,cursor,limit, etc.). This discrepancy could lead to confusion or incorrectly generated clients.
1875-1893: No issues with paginated response docs
The fields forAppPaginatedResponse(totalCount,page,pageSize,items) look consistent with a standard pagination model.
2201-2214: No issues with paginated response docs
The fields (totalCount,page,pageSize) are correct and consistently documented.
2231-2244: No issues with paginated response docs
This chunk is consistent with the established pagination fields.
2847-2860: No issues with paginated response docs
TheBillingProfileCustomerOverrideWithDetailsPaginatedResponsepagination fields look good.
2931-2944: No issues with paginated response docs
TheCustomerPaginatedResponsefields are clear and correctly reference pagination.
3590-3603: No issues with paginated response docs
TheEntitlementPaginatedResponsestructure follows the same pagination format.
3953-4027: Confirm omission of$eq/$nein FilterTime
TheFilterStringsupports$eq/$ne, butFilterTimeonly supports$gt,$gte,$lt,$lte. Please verify if equality checks for time are deliberately omitted, or if they should be included. Also consider clarifying in the doc that date/time strings must follow an RFC 3339 or ISO 8601 format for consistency.
4117-4130: No issues with paginated response docs
TheGrantPaginatedResponsefields are consistent.
4740-4753: No issues with paginated response docs
TheInvoicePaginatedResponsedefinition is in line with other paginated schemas.
5319-5338: No issues with paginated response docs
TheMarketplaceListingPaginatedResponsefor pagination fields looks correct.
5713-5726: No issues with paginated response docs
TheNotificationChannelPaginatedResponseis consistent.
5967-5980: No issues with paginated response docs
All pagination fields inNotificationEventPaginatedResponseappear fine.
6125-6138: No issues with paginated response docs
NotificationRulePaginatedResponsemaintains the established pagination pattern.
6365-6378: No issues with paginated response docs
PlanPaginatedResponseadheres to the same page/size/totalCount structure.
7541-7554: No issues with paginated response docs
SubscriptionPaginatedResponseis consistent with other pagination schemas.
8113-8120: No issues with pagination param docs
ThePagination.pageandPagination.pageSizedefinitions stating defaults are consistent.
8155-8155: No issues with export type
AppPaginatedResponseexport is straightforward.
8319-8320: No issues with new filter exports
FilterStringandFilterTimeexports look fine.
8337-8338: No issues with new cursor paginated response export
IngestedEventCursorPaginatedResponseexport is properly linked.
8421-8422: No issues with new marketplace listing paginated response export
MarketplaceListingPaginatedResponseexport aligns with the schema.
8748-8755: No issues withlistAppspagination
pageandpageSizedocs are consistent and mention defaults.
8769-8769: No issues with schema reference
'application/json': components['schemas']['AppPaginatedResponse']is straightforward.
9331-9338: No issues with billing profile overrides pagination
Page parameters are documented with defaults.
9812-9819: No issues with invoice pagination
pageandpageSizelook well documented.
10845-10852: No issues with billing profile pagination
All fields align with the standard page-based approach.
11314-11321: No issues withlistCustomerspagination
The doc forpageandpageSizereferencing defaults is consistent.
11513-11520: No issues withlistCustomerAppDatapagination
Parameters look fine.
11906-11913: No issues withlistCustomerSubscriptionspagination
Pagination remains uniform with other endpoints.
12390-12397: No issues with entitlements pagination
The fields are consistent with the rest.
12792-12799: No issues with meter-based pagination
page/pageSizeappear well-documented.
13179-13186: No issues with archived filtering pagination
page/pageSizeremain consistent.
13563-13570: No issues with marketplace listings pagination params
All default references align well.
13584-13584: No issues with schema reference
'application/json': components['schemas']['MarketplaceListingPaginatedResponse']is consistent.
14675-14682: No issues with plan-based pagination
Fields and docs align with page-based defaults.
15164-15171: No issues with channel-based pagination
page/pageSizeusage is correct.
15370-15377: No issues with param definitions
The pagination docs remain coherent.
16037-16044: No issues with plan currency filters
Pagination fields complemented by relevant filters.openmeter/streaming/clickhouse/event_query_v2.go (6)
12-12: Good job defining the default limit as a constantDefining the default limit as a constant makes the code more maintainable and is in line with the previous review comment requesting to "move 100 to constant."
14-19: LGTM: Clear and well-structured struct definitionThe struct is well-defined with descriptive field names and appropriate types. The comment explains its purpose clearly.
21-98: The SQL query generation logic is solid with proper cursor pagination handlingThe
toSQLmethod constructs a well-structured ClickHouse query with:
- Proper column selection
- Namespace filtering as a base requirement
- Comprehensive application of optional filters
- Cursor-based pagination with time and ID ordering for stability
- Consistent application of limit with proper default value
The cursor pagination implementation correctly handles the case where two events share the same timestamp by using ID as a secondary ordering criterion.
79-89: Cursor-based pagination implementation looks goodThe cursor-based pagination implementation correctly handles the stable ordering by:
- Filtering events by time less than or equal to the cursor time
- Using a compound condition to handle cases where events share the same timestamp
100-136: Good optimization in count query by applying only selective filtersThe
toCountRowSQLmethod intelligently limits the filters applied to the count query for better performance, focusing on filters that are most likely to significantly affect the count (namespace, type, subject, time).Not including all filters (like ID, source) is a reasonable performance optimization for count queries, as mentioned in your comment on lines 111-112.
22-98:❓ Verification inconclusive
Potential for query optimization through selective string interpolation
Both
toSQLandtoCountRowSQLmethods callgetTableName()which likely performs string interpolation to create the full table name. Since this is used in every query, it might be worth considering whether this could be optimized.Also applies to: 101-136
🏁 Script executed:
#!/bin/bash # Check implementation of getTableName function to understand potential optimizations fd -t f -e go "getTableName" | xargs grep -l "func getTableName" | xargs catLength of output: 76
Performance Review: Verify
getTableNameImplementationIt appears that both the
toSQLandtoCountRowSQLmethods rely ongetTableNamefor constructing the table name, and since this is invoked on every query, there is potential for unnecessary overhead if it employs expensive string interpolation. Our initial automated search for thegetTableNamedefinition did not return any results. This could mean that its implementation is located in an external package or wasn't captured by our search.Next Steps:
- Manually verify the implementation of
getTableNameto ensure it doesn’t perform redundant or heavy operations, especially since it’s used frequently.- If the function is found to incur unnecessary performance costs (e.g., due solely to string interpolation), consider optimizing it by caching the table name or refactoring its logic.
| properties: | ||
| totalCount: | ||
| type: integer | ||
| description: The items in the current page. | ||
| description: The total number of items. | ||
| example: 500 | ||
| page: | ||
| type: integer | ||
| description: The items in the current page. | ||
| description: The page index. | ||
| example: 1 | ||
| pageSize: | ||
| type: integer | ||
| description: The items in the current page. | ||
| description: The maximum number of items per page. | ||
| example: 100 | ||
| items: | ||
| type: array |
There was a problem hiding this comment.
🛠️ Refactor suggestion
DRY Improvement for Pagination Definitions
The recurring pagination definitions here could be consolidated into a reusable component. This would reduce redundancy and simplify future maintenance of the API documentation.
| properties: | ||
| totalCount: | ||
| type: integer | ||
| description: The items in the current page. | ||
| description: The total number of items. | ||
| example: 500 | ||
| page: | ||
| type: integer | ||
| description: The items in the current page. | ||
| description: The page index. | ||
| example: 1 | ||
| pageSize: | ||
| type: integer | ||
| description: The items in the current page. | ||
| description: The maximum number of items per page. | ||
| example: 100 | ||
| items: | ||
| type: array |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Redundant Pagination Schema Detection
The pagination properties are defined repeatedly across multiple schemas. Consolidating these into a shared reference would improve maintainability and adherence to the DRY principle.
| properties: | ||
| totalCount: | ||
| type: integer | ||
| description: The items in the current page. | ||
| description: The total number of items. | ||
| example: 500 | ||
| page: | ||
| type: integer | ||
| description: The items in the current page. | ||
| description: The page index. | ||
| example: 1 | ||
| pageSize: | ||
| type: integer | ||
| description: The items in the current page. | ||
| description: The maximum number of items per page. | ||
| example: 100 | ||
| items: | ||
| type: array |
There was a problem hiding this comment.
🛠️ Refactor suggestion
DRY Consideration for Pagination Definitions
The same pagination properties appear again. Refactoring them into a common schema or component could reduce duplication and ease long-term management of the API spec.
| properties: | ||
| totalCount: | ||
| type: integer | ||
| description: The items in the current page. | ||
| description: The total number of items. | ||
| example: 500 | ||
| page: | ||
| type: integer | ||
| description: The items in the current page. | ||
| description: The page index. | ||
| example: 1 | ||
| pageSize: | ||
| type: integer | ||
| description: The items in the current page. | ||
| description: The maximum number of items per page. | ||
| example: 100 | ||
| items: | ||
| type: array |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Repeated Pagination Structure Review
The final segment continues the pattern of pagination property definitions. Consolidating these recurring blocks into a shared component is recommended for improved clarity and maintainability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (17)
api/openapi.cloud.yaml (15)
8953-8966: ClarifyPagination.pageDefault Handling.
ThePagination.pageparameter is defined as an integer with a minimum of 1, and the default value is mentioned in the description. For clarity and to support automated tools, consider moving the default value into the parameter’s schema (using thedefaultproperty).
8971-8977: ImprovePagination.pageSizeDefault Declaration.
Similar to the page parameter, the default of 100 for the page size is stated in the description. It would be clearer to explicitly set this default within the schema.
9710-9725: Paginated Schema: MissingitemsDescription.
While thetotalCount,page, andpageSizeproperties include descriptions, theitemsproperty is defined only by its type. Adding a description (e.g., “The items in the current page”) would improve the schema’s clarity and consistency.
9749-9764: Enhance Paginated Response Schema Consistency.
This schema section repeats the pagination properties; however, theitemsproperty again lacks a descriptive text. For uniform documentation, consider adding a description for theitemsfield.
10592-10606: Include Description foritemsArray.
The paginated schema correctly details the properties fortotalCount,page, andpageSize, but theitemsarray is missing a descriptive comment. A brief description would help API consumers understand its purpose.
10701-10715: Add Description foritemsProperty.
The schema’s consistency would benefit from an added description for theitemsproperty, similar to those used in previous paginated responses.
11553-11567: Paginated Schema: Add Description foritems.
The common pattern in these paginated definitions shows the need for a dedicated description for theitemsarray. Consider adding such a description to enhance documentation consistency.
12009-12132: Extended Schema Definitions for Pagination and Filtering.
The combined paginated response and filter object definitions (FilterString and FilterTime) are comprehensive. To further improve clarity, consider including concrete examples for each filter operator.
12263-12277: Consistent Pagination Schema —itemsDescription.
The properties fortotalCount,page, andpageSizeare well defined; however, similar to earlier cases, adding a description for theitemsarray would ensure consistency.
13211-13225: Pagination Schema Detail Consistency.
The schema properties are correctly assigned, though theitemsproperty could benefit from an added description to mirror the pattern used in other paginated responses.
14585-14599: Consistent Pagination Template.
The paginated response schema adheres to the established template. As a nitpick, consider including a description for theitemsarray to align with other similar schemas.
14918-14932: Pagination Schema: DescribeitemsArray.
The properties are consistent with earlier schemas. For greater clarity, it would be beneficial to add a description for theitemsproperty.
15137-15151: Review Pagination Schema Consistency.
The schema is uniformly defined; however, adding a description for theitemsfield will improve documentation and assist consumers in understanding its purpose.
15469-15483: Enhance Pagination Schema Documentation.
While the numeric properties are thoroughly documented, the definition for theitemsarray would benefit from an accompanying description for clarity and consistency.
17040-17054: Final Check on Pagination Schema.
The schema consistently defines the paginated response. As noted in previous instances, consider adding a short description for theitemsproperty to complete the documentation.api/openapi.yaml (1)
9537-9551: Consistent Pagination Structure in Response Model:
The pagination properties in this schema are consistent with the overall design. If similar definitions occur multiple times, consider centralizing them to reduce duplication.api/spec/src/events.tsp (1)
143-143: Consider documenting the internal flag usageThe
@extension("x-internal", true)decorator suggests this endpoint is intended for internal use only. Consider adding a comment explaining why this API is marked as internal and whether there are any usage restrictions that consumers should be aware of.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
api/client/javascript/src/client/schemas.ts(42 hunks)api/openapi.cloud.yaml(25 hunks)api/openapi.yaml(26 hunks)api/spec/src/cloud/main.tsp(1 hunks)api/spec/src/events.tsp(1 hunks)api/spec/src/query.tsp(4 hunks)openmeter/meterevent/service.go(4 hunks)openmeter/streaming/clickhouse/event_query_v2.go(1 hunks)pkg/pagination/v2/cursor.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/pagination/v2/cursor.go
- api/spec/src/query.tsp
- openmeter/meterevent/service.go
🧰 Additional context used
🧬 Code Definitions (2)
openmeter/streaming/clickhouse/event_query_v2.go (2)
openmeter/meterevent/service.go (1) (1)
ListEventsV2Params(121-142)pkg/pagination/v2/cursor.go (1) (1)
Cursor(11-14)
api/client/javascript/src/client/schemas.ts (1)
api/client/go/client.gen.go (4) (4)
AppPaginatedResponse(712-724)FilterString(2193-2235)FilterTime(2238-2256)IngestedEventCursorPaginatedResponse(2377-2383)
🔇 Additional comments (56)
openmeter/streaming/clickhouse/event_query_v2.go (8)
1-2: Package declaration looks good.
No issues with naming or structure.
3-13: Imports and constant definition appear appropriate.
Nice job extracting the limit value intoeventQueryV2DefaultLimitto adhere to the previous feedback about using constants.
14-19: Consider whether theClientIDparameter should be handled.
ListEventsV2Paramsincludes aClientIDfield (per the relevant code snippets), but this struct and subsequent SQL generation logic do not appear to filter by it. If it's intentional to excludeClientIDat this stage, please confirm. Otherwise, consider adding a filter for consistency.
21-73: V2 filter logic for ID, Source, Subject, Type, Time, and IngestedAt is consistent.
All filters follow the same pattern and use parameterized expressions, reducing the risk of SQL injection. The code is well-structured, with each filter neatly encapsulated.
74-78: Verify logic fortimeColumnselection.
SettingtimeColumnto"ingested_at"wheneverParams.IngestedAt != nilmay overshadowtimecolumn usage if both filters are provided. Confirm that this fallback toingested_atis the intended behavior whenTimeandIngestedAtare both applicable.
79-89: Confirm descending pagination condition.
The cursor condition ensures(timeColumn <= cursor.Time) AND (timeColumn < cursor.Time OR id < cursor.ID). This is logical for descending order, but please verify it aligns with your row ordering and use cases. For example, rows at the exact same timestamp rely on ID for ordering. If that mirrors your intended chronological descending page navigation, it looks good.
91-95: Ordering and limit usage are consistent.
Descending order by time first, then ID, combined withlo.FromPtrOr(q.Params.Limit, eventQueryV2DefaultLimit)is coherent for stable pagination.
100-136: Some filters are omitted intoCountRowSQL().
For performance reasons, ID, Source, IngestedAt, and cursor-based filters are not applied here, which can yield mismatched counts compared to the fetched rows. Confirm that this discrepancy is acceptable. If an exact count matching all filters is needed, consider extending filter coverage or providing a note for end-users about the approximate nature of this count.api/openapi.cloud.yaml (10)
43-57: Ensure Consistency in Pagination References.
The endpoint for listing apps now uses the new pagination parameters (Pagination.pageandPagination.pageSize) and theAppPaginatedResponseschema. Verify that these references match the updated definitions across all related endpoints.
2064-2071: Pagination Update for Customer Listing.
The customer listing endpoint now includes the updated pagination parameters along with ordering and deletion parameters. Make sure the ordering and filtering behavior is documented and consistent with the rest of the API.
2138-2145: Pagination in Filter Parameters.
The addition ofPagination.pageandPagination.pageSizeto the parameters (following the pattern property definition) is consistent. Confirm that their placement and behavior align with other endpoints’ filtering logic.
2443-2450: Proper Integration of Pagination.
The inclusion of pagination parameters in this endpoint is in line with the new model. Ensure that the parameter order and naming are consistent with similar endpoints to avoid any consumer confusion.
3743-3757: Marketplace Listing Endpoint Pagination Updated.
The endpoint listing available apps now properly referencesPagination.pageandPagination.pageSizealong with theMarketplaceListingPaginatedResponseschema. Verify that the response schema’s property descriptions provide clear guidance to API consumers.
8319-8449: New Events V2 Endpoint Implementation.
The/api/v2/eventsendpoint introduces cursor-based pagination parameters (CursorPagination.cursorandCursorPagination.limit) and extensive filtering options using deepObject style. Confirm that the descriptions, parameter styles, and overall documentation meet your internal standards and that thex-internal: trueflag is intentional.
8553-8577: CursorPagination Parameters Definition.
The definitions forCursorPagination.cursorandCursorPagination.limitinclude clear boundaries and a default limit of 100—which is excellent. Please ensure that these definitions are harmonized with the overall API documentation and that any consumers of the API are aware of these limits.
9188-9219: AppPaginatedResponse Schema Definition.
TheAppPaginatedResponseschema is well defined with required properties such astotalCount,page,pageSize, anditems. Ensure that property descriptions (especially foritems) remain consistent with other paginated responses throughout the API.
12353-12371: IngestedEventCursorPaginatedResponse Schema.
The new cursor-based pagination response for ingested events is well structured with anitemsarray and anextCursor. Double-check that themaxItems: 100constraint is aligned with theCursorPagination.limitsettings defined elsewhere in the API.
14112-14142: MarketplaceListingPaginatedResponse and MeasureUsageFrom Definitions.
TheMarketplaceListingPaginatedResponseschema is defined clearly, including all necessary pagination fields, and theMeasureUsageFromcomponent is declared using a oneOf reference. Please ensure thatMeasureUsageFromis intended to be defined as a separate component rather than nested within the pagination schema.api/openapi.yaml (25)
42-56: Pagination for List Apps Endpoint:
The addition of pagination parameters using$ref: '#/components/parameters/Pagination.page'and$ref: '#/components/parameters/Pagination.pageSize'along with the response schema reference toAppPaginatedResponseis well structured. Please verify that the referenced components contain clear, accurate, and consistent descriptions.
2063-2070: Pagination for List Customers Endpoint:
The endpoint now properly includes pagination parameters forpageandpageSizealongside the ordering and other filtering parameters. Ensure that the referenced pagination definitions align with the API documentation standards.
2137-2143: Enhancing Query Parameters with Pagination:
Adding the pagination parameters in the parameter list here appears appropriate. Please confirm that this insertion—between the property with the specified pattern and the additional query filter for customer type—maintains the intended logical ordering.
2442-2449: Consistent Pagination in Endpoint Parameter Definitions:
The addition of pagination parameters here is consistent with other endpoints. Double-check that the referenced$refdefinitions for Pagination provide the intended constraints and descriptions.
3742-3756: Marketplace Listing Endpoint Pagination:
The endpoint for listing available apps now leverages the pagination parameters and uses the newMarketplaceListingPaginatedResponseschema. This update is in line with the new pagination approach; please ensure the definitions in the referenced components are correct.
8064-8192: New Events V2 Endpoint with Cursor-Based Pagination:
This new/api/v2/eventsendpoint introduces advanced filtering and cursor-based pagination while using deepObject style for complex query parameters. The structure is comprehensive and clear. Please verify that all referenced components (including filters and error responses) are defined as expected and that client implementations can correctly process deepObject parameters.
8295-8318: Definition of CursorPagination Parameters:
The new definitions forCursorPagination.cursorandCursorPagination.limitare clear and include appropriate constraints (such as a default and maximum for the limit). Confirm that a maximum limit of 100 aligns with client expectations and that these definitions are harmonized with global pagination behavior.
8695-8706: Updating Pagination.page Parameter:
The new definition for thePagination.pageparameter—with a type of integer, a minimum of 1, and a default value of 1—is well defined. Please ensure that no legacy definitions conflict with this updated parameter.
8713-8719: Pagination.pageSize Parameter Description:
The description for thePagination.pageSizeparameter indicates it sets the maximum number of items per page. Please double-check that the complete schema (including type and any constraints) is provided immediately after this description so that the parameter is fully defined.
9007-9037: AppPaginatedResponse Schema Definition:
TheAppPaginatedResponseschema is comprehensively defined with required properties such astotalCount,page,pageSize, anditems. The use of clear examples and descriptions should enhance client-side integration.
9611-9625: Verification of Pagination Schema Consistency:
The pagination definition remains consistent and clear here. Consistency across all pagination responses is beneficial.
10467-10481: Maintaining Uniformity in Pagination Details:
This response model continues to exhibit consistent pagination definitions. Ensure that related business logic and tests reflect these fields accurately.
10576-10590: Consistent Pagination Fields in Schema Definitions:
The definition of pagination fields—includingtotalCount,page,pageSize, anditems—is clear and consistent. Confirm that these align with the client’s model expectations.
11502-11516: Review of Pagination Property Consistency:
The specified properties for pagination are straightforward and in line with the refactoring goals. There are no issues noted here.
11958-11972: Repeated Pagination Schema Validation:
This section uses the same pagination structure as seen in previous models. Consistent use of these definitions is excellent for the overall API design.
12306-12320: Uniform Pagination Schema:
The pagination properties remain consistent, indicating that the refactor is being applied uniformly across different schemas.
12396-12414: IngestedEventCursorPaginatedResponse Schema:
The newly introducedIngestedEventCursorPaginatedResponseis well defined with an array ofIngestedEventobjects and anextCursorfor pagination. This design should support efficient cursor-based pagination. In future iterations, consider whether additional metadata (such as total count) might be beneficial.
13254-13268: Consistent Pagination Properties in Response Schema:
The response schema here follows the standard pagination structure with clear descriptions and examples.
14087-14107: Query Parameters for Listing Installed Apps:
The updated definitions forpageandpageSizeinclude explicit validation (minimum, maximum, default values). Note that here thepageSizemaximum is set to 1000, which differs from other endpoints (where it is typically 100). Please verify that this discrepancy is intentional and documented.
14178-14208: MarketplaceListingPaginatedResponse Schema Update:
TheMarketplaceListingPaginatedResponseschema is now consistent with the overall pagination approach. Ensure that the descriptions fortotalCount,page, andpageSizeaccurately reflect their roles to aid client developers.
14651-14664: Standardized Pagination Schema:
The properties for pagination in this segment are defined clearly and consistently with earlier schemas.
14984-14998: Consistency Check for Pagination Fields:
The pagination fields adhere to the standardized pattern observed throughout the API spec. No issues are identified here.
15223-15237: Uniform Pagination Structure in Schema:
The pagination properties remain uniformly defined with clear descriptions and examples.
15555-15569: Review of Pagination Property Definitions:
The response model in this segment is consistent with other pagination definitions, which is beneficial for API consistency.
17238-17252: Final Pagination Schema Consistency Check:
The last pagination schema confirms that the same structure is maintained across the entire spec. Consistent definitions simplify both maintenance and client integration.api/spec/src/cloud/main.tsp (1)
109-111: LGTM: New EventsV2 interface follows established patternsThe addition of the
EventsV2interface is consistent with the existing codebase organization. It properly extends the base interface and maintains the versioning convention in the route path.api/spec/src/events.tsp (2)
134-223: Well-implemented EventsV2 interface with cursor-based paginationThe new interface provides several improvements over the v1 API:
- Uses modern cursor-based pagination instead of limit-based pagination
- Implements sophisticated filtering capabilities via FilterString and FilterTime types
- Provides comprehensive examples for each filter parameter
- Returns data in a structured CursorPaginatedResponse format
The examples for the filter parameters satisfy the previous review request.
161-165: Comprehensive filter examples added as requestedThanks for adding the examples for filters as requested in the previous review. The examples are clear and demonstrate the proper usage of the filter parameters.
api/client/javascript/src/client/schemas.ts (10)
1762-1781: New endpoint added for listing ingested events with advanced filteringThis new
/api/v2/eventsendpoint implementation looks good. It introduces advanced filtering and cursor pagination capabilities for event listing, which aligns well with modern API design practices.
1875-1894: Well-defined paginated response schemas with clear field descriptionsThe new paginated response schemas (
AppPaginatedResponseandMarketplaceListingPaginatedResponse) are well structured with accurate field descriptions:
totalCount: The total number of itemspage: The page indexpageSize: The maximum number of items per pageitems: The actual items in the current pageThis is a good improvement over previous schemas and aligns with the Go client implementation.
Also applies to: 5319-5338
3970-4027: Comprehensive filter schemas for advanced queryingThe new
FilterStringandFilterTimeschemas provide robust filtering capabilities:
- String operations: equality, inequality, pattern matching, range comparisons
- Time operations: range comparisons
- Logical operations: AND, OR combinations
This implementation enables powerful querying capabilities for clients and matches the Go implementation seen in the relevant code snippets.
4195-4201: Cursor-based pagination for efficient event listingThe
IngestedEventCursorPaginatedResponseschema adds cursor-based pagination, which is more efficient than offset pagination for large datasets like event logs. The schema correctly includes:
items: The data in the current pagenextCursor: Optional cursor for fetching the next page of resultsThis matches the Go implementation and follows best practices for paginating large, time-series data.
8007-8010: Consistent pagination parameter definitionsThe pagination parameters are well defined with clear descriptions:
CursorPagination.cursor: The cursor for starting paginationCursorPagination.limit: The maximum number of items to returnPagination.page: Page index (default: 1)Pagination.pageSize: Maximum items per page (default: 100)These definitions are consistent with best practices and with the Go client implementation.
Also applies to: 8113-8120
19401-19509: Well-implemented Events V2 API with robust filtering optionsThe
listEventsV2operation is thoroughly implemented with:
- Comprehensive cursor pagination parameters
- Rich filtering capabilities for event properties (id, source, subject, type, time)
- Client ID tracking for query progress
- Proper error response handling
The API design enables efficient querying of event data with specific filters and follows RESTful principles with appropriate status codes and response formats.
8745-8755: Consistent pagination parameter documentation across all endpointsThe pagination parameter descriptions have been updated consistently across all endpoints with:
- Clear descriptions indicating their purpose
- Information about default values
- Consistent formatting
This addresses the issues raised in previous reviews about incorrect JSDoc descriptions for pagination parameters.
Also applies to: 9331-9338, 9812-9819, 10845-10852, 11314-11321, 11513-11520, 11906-11913, 12390-12397, 12792-12799, 13179-13186, 13563-13570, 14675-14682, 15164-15171, 15370-15377, 16037-16044
2201-2241: Improved documentation for pagination fields across all response schemasThe descriptions for pagination fields have been consistently improved across all response schemas:
totalCount: Now correctly described as "The total number of items."page: Now correctly described as "The page index."pageSize: Now correctly described as "The maximum number of items per page."This addresses the issues raised in previous reviews about incorrect JSDoc descriptions.
Also applies to: 2847-2860, 2931-2944, 3590-3603, 3953-3966, 4117-4130, 4740-4753, 5322-5335, 5713-5726, 5967-5980, 6125-6138, 6365-6378, 7541-7554
8319-8320: Type exports for new schemasThe export declarations for the new types are well-structured and consistent with the existing codebase style:
FilterStringandFilterTimefor filteringIngestedEventCursorPaginatedResponsefor cursor paginationAppPaginatedResponseandMarketplaceListingPaginatedResponsefor standard pagination- Parameters for cursor-based pagination
This ensures the new types are properly accessible to consumers of this module.
Also applies to: 8337-8338, 8421-8422, 8638-8641, 8155-8155
8769-8769: Updated response content types for paginated responsesThe response content types have been correctly updated to use the new paginated response schemas:
AppPaginatedResponsefor app listingMarketplaceListingPaginatedResponsefor marketplace listingThis ensures consistency between the endpoint definitions and the response schemas.
Also applies to: 13584-13584
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (12)
pkg/pagination/v2/cursor.go (1)
27-39: Add documentation for the Validate method.The implementation of the
Validatemethod correctly checks for zero time and empty ID, collecting and joining errors as needed. However, adding method documentation would improve code clarity and help future developers understand the validation's purpose.Consider adding a documentation comment for the method:
+// Validate checks if the cursor has valid Time and ID fields. +// It returns an error if any field is invalid, joining multiple validation errors if present. func (c Cursor) Validate() error { var errs []error if c.Time.IsZero() { errs = append(errs, fmt.Errorf("cursor time is zero")) } if c.ID == "" { errs = append(errs, fmt.Errorf("cursor id is empty")) } return errors.Join(errs...) }api/openapi.yaml (5)
8695-8708: Improve Default Value Handling for Pagination.page
ThePagination.pageparameter now describes its default ("Default is 1") in plain text; however, consider moving this default into the schema so that clients can automatically detect it. For example:- description: |- - Page index. - Default is 1. - schema: - type: integer - minimum: 1 + description: "Page index (default is 1)." + schema: + type: integer + minimum: 1 + default: 1
8713-8719: Establish Default for Pagination.pageSize Parameter
Similar toPagination.page, the description forPagination.pageSizementions a default value without binding it to the schema. Consider applying the default within the schema block as shown below:- description: |- - The maximum number of items per page. - Default is 100. - schema: + description: "The maximum number of items per page (default is 100)." + schema: + type: integer + minimum: 1 + default: 100
9007-9036: AppPaginatedResponse Schema Consistency
TheAppPaginatedResponseschema is defined with all the necessary properties (totalCount,page,pageSize, anditems). For additional clarity, you might consider renaming the description ofpagefrom "The page index" to "The current page number" to avoid ambiguity.
12306-12320: Maintain Consistent Pagination Descriptions
The pagination properties are defined similarly to other responses. For clarity, you might consider unifying the description ofpageacross all instances to "Current page number."
14178-14208: MarketplaceListingPaginatedResponse and Related Models
TheMarketplaceListingPaginatedResponseis clearly defined with standardized pagination properties. For improved clarity, consider revising thepageproperty description to "Current page number" rather than "page index."api/openapi.cloud.yaml (3)
3743-3757: Marketplace Listing Endpoint Update:
Switching to theMarketplaceListingPaginatedResponseschema and incorporatingPagination.pageandPagination.pageSizein this endpoint is in line with the broader pagination updates. Consider enhancing the parameter descriptions (for example, clarifying if cursor pagination might also apply) to further aid API consumers.
9188-9215: AppPaginatedResponse Schema Definition:
The updated schema forAppPaginatedResponseproperly includestotalCount,page,pageSize, anditemswith clear descriptions and examples. For enhanced clarity, you might consider more descriptive labels (e.g., “Current page number” forpageand “Number of items per page” forpageSize).
12009-12132: Expanded Schema: Feature Pagination and Filter Definitions:
This hunk extends the pagination schema for Feature items and introduces detailed definitions forFilterStringandFilterTime. The wide range of supported filter operators improves API filtering flexibility. Consider reviewing for potential reuse of common filtering constructs across your schemas.api/spec/src/events.tsp (1)
210-221: Consider adding more diverse filter examples.While the current examples are good, consider adding more diverse examples for
ingestedAtto showcase different filter operations (e.g.,$orconditions or single comparisons like just$gte). This would help API users understand the full range of filtering capabilities.openmeter/streaming/clickhouse/event_query_v2.go (1)
22-98: Consider refactoring repetitive filter code patterns.There's a repetitive pattern when applying filters (lines 32-72) where you check if a filter exists, get the expression, check if the expression is empty, then apply it. Consider refactoring this into a helper function to improve code maintainability.
+ // applyFilterIfPresent applies a filter to the query if the filter exists and generates a non-empty expression + func applyFilterIfPresent(query *sqlbuilder.SelectBuilder, filter interface{}, column string, exprFunc func(string, *sqlbuilder.SelectBuilder) string) { + if filter == nil { + return + } + + expr := exprFunc(column, query) + if expr != "" { + query.Where(expr) + } + } func (q queryEventsTableV2) toSQL() (string, []interface{}) { // ... - if q.Params.ID != nil { - expr := q.Params.ID.SelectWhereExpr("id", query) - if expr != "" { - query.Where(expr) - } - } + applyFilterIfPresent(query, q.Params.ID, "id", func(col string, qb *sqlbuilder.SelectBuilder) string { + return q.Params.ID.SelectWhereExpr(col, qb) + }) // Apply similar changes to other filter applicationsopenmeter/meterevent/service.go (1)
144-207: Consider clarifying complexity limit for filters.The validation uses
ValidateWithComplexity(1)for all filters. Consider adding a comment explaining what this complexity level means, or making it a named constant with a descriptive name to improve code readability.+ // MaxFilterComplexity defines the maximum allowed complexity for filters + // Complexity = 1 allows for simple expressions but prevents deeply nested conditions + const MaxFilterComplexity = 1 func (p ListEventsV2Params) Validate() error { // ... if p.ID != nil { - if err := p.ID.ValidateWithComplexity(1); err != nil { + if err := p.ID.ValidateWithComplexity(MaxFilterComplexity); err != nil { errs = append(errs, fmt.Errorf("id: %w", err)) } } // Apply similar changes to other filter validations
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
api/client/javascript/src/client/schemas.ts(42 hunks)api/openapi.cloud.yaml(25 hunks)api/openapi.yaml(26 hunks)api/spec/src/cloud/main.tsp(1 hunks)api/spec/src/events.tsp(1 hunks)api/spec/src/query.tsp(4 hunks)openmeter/meterevent/service.go(4 hunks)openmeter/streaming/clickhouse/event_query_v2.go(1 hunks)pkg/pagination/v2/cursor.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/spec/src/query.tsp
🧰 Additional context used
🧬 Code Definitions (3)
openmeter/streaming/clickhouse/event_query_v2.go (2)
openmeter/meterevent/service.go (1) (1)
ListEventsV2Params(121-142)pkg/pagination/v2/cursor.go (1) (1)
Cursor(11-14)
openmeter/meterevent/service.go (3)
api/client/go/client.gen.go (5) (5)
ListEventsV2Params(6127-6161)Event(2099-2099)FilterString(2193-2235)Subject(4615-4632)FilterTime(2238-2256)openmeter/meterevent/httphandler/event_v2.go (1) (1)
ListEventsV2Params(15-15)pkg/filter/filter.go (2) (2)
FilterString(30-45)FilterTime(361-368)
api/client/javascript/src/client/schemas.ts (2)
api/client/go/client.gen.go (4) (4)
AppPaginatedResponse(712-724)FilterString(2193-2235)FilterTime(2238-2256)IngestedEventCursorPaginatedResponse(2377-2383)api/api.gen.go (4) (4)
AppPaginatedResponse(813-825)FilterString(2423-2465)FilterTime(2468-2486)IngestedEventCursorPaginatedResponse(2611-2617)
🔇 Additional comments (63)
api/openapi.yaml (16)
42-56: Pagination Parameter References for Listing Apps
The endpoint correctly reuses thePagination.pageandPagination.pageSizereferences, and the response schema is linked toAppPaginatedResponse. Please double-check that these referenced components are updated to match the latest pagination standards across the API.
2063-2070: Consistent Use of Pagination in the Customers Endpoint
The customer listing endpoint now reuses the pagination parameters consistently. Ensure the ordering parameters (CustomerOrderByOrdering.orderandorderBy) remain correctly integrated with these pagination parameters.
2137-2144: Integrating Pagination with Filtered Queries
After the parameter definition (with its type, pattern, and example), the inclusion of pagination references is done seamlessly. Verify that this addition does not interfere with the intended filtering logic for customer queries.
2442-2449: Enhance Filtering with Pagination
The addition ofPagination.pageandPagination.pageSizeimmediately following the schema attributes (type, pattern, example) in this fragment is consistent. Just ensure that the original filter schema is not unintentionally overridden by these new entries.
3742-3756: Listing Available Apps with Market-specific Response
This endpoint for listing available apps now uses the pagination references and returns aMarketplaceListingPaginatedResponse. The structure appears solid; just confirm that the response details (like the items array) conform to the intended market listing format.
8064-8192: New Events V2 Endpoint Implementation
The newly introduced/api/v2/eventsendpoint is well structured with an operationId oflistEventsV2and detailed filtering options using deepObject style. The parameters—including cursor pagination (CursorPagination.cursorandCursorPagination.limit) and additional filters (clientId, id, source, subject, type, time, ingestedAt)—are clearly defined. Please ensure that the API documentation covers all these advanced filtering details.
8295-8318: Definition of Cursor-based Pagination Parameters
The new definitions forCursorPagination.cursor(a string) andCursorPagination.limit(an integer with boundaries and default) are clear and concise. Verify that the chosen limits (e.g., maximum 100) align with overall performance expectations for the API.
9537-9551: Uniform Paginated Response Schema
This paginated response definition is consistent with similar definitions elsewhere in the API. No issues are found.
10467-10481: Standardized Pagination Schema
The pagination properties and examples are consistent with other parts of the specification. Good job maintaining uniformity.
10576-10590: Consistent Pagination Definition
This section repeats the pagination property definitions in a manner consistent with previous sections. No further issues are identified.
11958-11972: Pagination Properties Consistency
The definitions fortotalCount,page, andpageSizehere maintain consistency with earlier schemas. No changes are required.
12396-12414: IngestedEventCursorPaginatedResponse Model
The new model for cursor-based pagination is well defined withitems(capped at a maximum of 100) and anextCursorproperty. It might prove beneficial to mention in the documentation thatnextCursorwill be omitted when there are no further pages.
14087-14107: ListEntitlementsResult Pagination Configuration
The query parameters for listing installed apps now explicitly define defaults and constraints—thepageparameter has a default of 1, andpageSizeallows a maximum of 1000. This maximum differs from the standard 100 in other schemas. Please confirm that the higher upper limit forpageSizeis intentional and, if so, ensure it is documented accordingly.
14651-14664: Consistent Pagination Field Definitions
This fragment continues the established pattern for defining pagination fields. Everything appears in order.
15223-15237: Standardized Pagination Properties
The pagination-related properties are defined as expected and in line with previous sections. No modifications are necessary.
17238-17252: Final Pagination Schema Consistency
The final pagination schema block aligns perfectly with prior definitions. No issues were identified.api/spec/src/cloud/main.tsp (1)
109-111: Well-designed API versioning approach!The addition of the
EventsV2interface follows good API versioning practices by creating a new endpoint (/api/v2/events) while preserving backward compatibility with the existing v1 endpoint. This aligns perfectly with the pull request's objective of implementing the events v2 API with enhanced features like cursor pagination and advanced filtering.The implementation is clean and consistent with the established pattern of other interfaces in this file. This approach ensures a smooth transition for clients who can adopt the new API version at their own pace.
api/openapi.cloud.yaml (22)
43-57: Pagination Parameters Update in 'List apps':
The addition ofPagination.pageandPagination.pageSizereferences, along with updating the response schema toAppPaginatedResponse, is consistent with the new pagination model. Ensure that these component references are defined properly elsewhere in the document and that any legacy pagination constructs have been fully removed.
2064-2071: Pagination Update for 'List customers':
The inclusion ofPagination.pageandPagination.pageSizein the parameters list aligns with the new pagination strategy. Verify that these definitions match across endpoints and that any outdated parameters have been deprecated accordingly.
2138-2145: Additional Pagination Parameters for Customer Filter:
AddingPagination.pageandPagination.pageSizeafter the ID pattern example is appropriate. Ensure that when combined with the other parameter (queryCustomerList.type), the overall behavior remains predictable and well documented.
2443-2450: Consistent Pagination in Endpoint Definition:
IncludingPagination.pageandPagination.pageSizereinforces uniform pagination across endpoints. Please double-check that the schema reference for the identification field remains correct and that there isn’t any conflict with previous definitions.
8319-8449: New 'List ingested events' Endpoint with Cursor Pagination:
The new/api/v2/eventsendpoint is structured well with advanced filtering and cursor pagination. Each query parameter (e.g.,clientId,id,source,subject,type,time,ingestedAt) includes a clear description and deepObject style is used appropriately. Additionally, the use of thex-internalflag and the security schemes (CloudTokenAuthandCloudCookieAuth) is consistent with internal endpoints. Please verify these settings against your access control requirements.
8553-8577: CursorPagination Parameter Definitions:
The newly added definitions forCursorPagination.cursorandCursorPagination.limitare clear and include proper constraints (such asminimum,maximum, anddefaultvalues). Ensure these components are referenced correctly in all cursor-based endpoints.
8953-8966: Pagination.page Definition Enhancement:
ThePagination.pageparameter now includes a detailed description ("Page index") and specifies a default value. Confirm if any additional numeric constraints (e.g., a minimum value) should be enforced in the schema.
8971-8977: Pagination.pageSize Description Update:
The description now clearly states that the parameter represents the maximum number of items per page. Make sure that the default value is consistently applied between the schema definition and the implementation logic.
9710-9724: Consistent Pagination Schema Properties:
The propertiestotalCount,page, andpageSizeare described consistently here. This uniformity across pagination schemas enhances maintainability.
9749-9763: Additional Pagination Schema Update:
The schema maintains consistency with previous pagination definitions. Please ensure that no conflicting definitions exist and that all similar response objects follow this standard.
10592-10606: Further Pagination Schema Consistency:
The pagination properties are again defined clearly and consistently. This uniform documentation benefits API clarity and consumer expectations.
10701-10715: Consistent Pagination Field Descriptions:
The schema continues to adhere to a uniform structure for pagination. Maintaining this consistency will help in reducing confusion across different endpoints.
11553-11567: Uniform Pagination Schema Update:
Clear and concise descriptions fortotalCount,page, andpageSizeare provided. Such consistency across components is excellent for internal documentation and external API usage.
12263-12277: Consistent Pagination Schema Verification:
The properties fortotalCount,page,pageSize, anditemsremain consistent with previous definitions. Uniformity in these fields across components is maintained.
12353-12371: IngestedEventCursorPaginatedResponse Schema:
The new schema forIngestedEventCursorPaginatedResponseincludes anitemsarray and anextCursorfor cursor-based pagination, which is well structured. Please ensure themaxItems: 100constraint aligns with the intended limits set byCursorPagination.limit.
13211-13225: Updated Pagination Component:
This section’s pagination properties are clearly documented and consistent with the rest of the API specifications.
14112-14141: MarketplaceListingPaginatedResponse and MeasureUsageFrom Definitions:
TheMarketplaceListingPaginatedResponsecomponent is updated with the standard pagination fields, and the additionalMeasureUsageFromcomponent appears correctly defined. Double-check the indentation to ensureMeasureUsageFromis at the correct hierarchy level and not erroneously nested, which could lead to misinterpretation by YAML parsers.
14585-14598: Consistent Pagination Schema in Component:
The defined properties and their descriptions in this section adhere perfectly to the established pagination format.
14918-14932: Uniform Pagination Definition:
The update tototalCount,page, andpageSizeconsistently follows the new schema design.
15137-15151: Pagination Schema Consistency Continued:
The properties here maintain the uniform description style observed in previous pagination definitions. Excellent consistency across the board.
15469-15483: Continued Consistent Pagination Schema Update:
This hunk reinforces the updated pagination structure with no discrepancies found.
17040-17054: Final Pagination Schema Update:
The pagination properties (totalCount,page,pageSize, anditems) in this section align with earlier definitions, ensuring an overall uniform API response format.api/spec/src/events.tsp (4)
143-143: Is the x-internal extension intentional?The endpoint is marked with
@extension("x-internal", true). Please confirm if this endpoint is intended to be internal-only or if it should be publicly accessible as part of your API.
161-165: Examples for filters added as requested.The examples for the ID filter have been added as requested in the previous review. The implementation looks good.
197-208: Good implementation of complex time filter example.The time filter example effectively demonstrates how to use complex conditions with
$andoperators, providing users with a clear pattern to follow for date range queries.
145-146: Good use of spread operator for pagination.Using the spread operator with
QueryCursorPaginationis a clean way to include the cursor pagination parameters in the method signature.openmeter/streaming/clickhouse/event_query_v2.go (4)
12-12: Good: Magic number moved to constant.The limit value is now properly defined as a constant, addressing the previous review comment.
79-89: Solid cursor pagination implementation.The cursor pagination implementation correctly handles the case where events share the same timestamp by using ID as a secondary sorting key. The approach ensures stable and consistent pagination results.
100-136: Good performance optimization for count query.The
toCountRowSQLmethod intelligently limits the number of filters applied to improve performance for count queries, which is a good optimization. The comment clearly explains the reasoning behind this decision.
74-77:✅ Verification successful
Verify time column selection logic.
The code chooses between
timeandingested_atas the time column based solely on whetherq.Params.IngestedAtis non-nil. Make sure this logic is correct - if a user provides both time and ingestedAt filters, the ingestedAt would take precedence for ordering.
🏁 Script executed:
#!/bin/bash # Check if there's any validation preventing both time and ingestedAt filters grep -A 10 "time and ingested_at cannot both be set" openmeter/meterevent/service.goLength of output: 397
Time Column Selection Logic Verified
The conditional setup in
event_query_v2.go—using"ingested_at"whenq.Params.IngestedAtis non-nil and defaulting to"time"otherwise—is correct. The validation inopenmeter/meterevent/service.goexplicitly prevents both filters from being set at the same time, so there's no risk of conflict where both values are provided.openmeter/meterevent/service.go (5)
67-73: Good implementation of Cursor method.The
Cursormethod on theEventstruct provides a clean way to generate pagination cursors from events, which will make implementing cursor-based pagination more straightforward.
120-142: Well-structured parameter object with comprehensive filter options.The
ListEventsV2Paramsstruct is well-designed with clear documentation and appropriate types. Using pointer types for optional parameters is a good practice.
182-184: Good validation to prevent conflicting filters.Checking that
TimeandIngestedAtcannot both be set is important for preventing ambiguous queries where it's unclear which time dimension should be prioritized.
148-156: Good basic validation for required fields and cursor.The validation correctly checks for required fields and validates the cursor if present. This ensures the API will receive valid parameters before attempting to query the database.
20-20: Interface updated with backwards compatibility maintained.Adding the new
ListEventsV2method to theServiceinterface while keeping the originalListEventsmethod is a good approach for maintaining backward compatibility while introducing new functionality.api/client/javascript/src/client/schemas.ts (11)
1762-1781: New Events V2 API endpoint addedThe addition of the
/api/v2/eventsendpoint with thelistEventsV2operation introduces the ability to list ingested events with advanced filtering and pagination capabilities. This API path is properly structured following REST standards.
1875-1894: Improved pagination response structure with clear field descriptionsThe
AppPaginatedResponseschema has been properly documented with clear and accurate field descriptions, correcting previous issues noted in past reviews.
2201-2201: Consistent pagination documentation across all response typesDocumentation for pagination-related fields has been standardized across all paginated response types, improving developer experience and API consistency.
Also applies to: 2206-2206, 2211-2211, 2231-2231, 2236-2236, 2241-2241, 2847-2847, 2852-2852, 2857-2857, 2931-2931, 2936-2936, 2941-2941, 3590-3590, 3595-3595, 3600-3600, 3953-3953, 3958-3958, 3963-3963, 4117-4117, 4122-4122, 4127-4127, 4740-4740, 4745-4745, 4750-4750, 5322-5322, 5327-5327, 5332-5332, 5713-5713, 5718-5718, 5723-5723, 5967-5967, 5972-5972, 5977-5977, 6125-6125, 6130-6130, 6135-6135, 6365-6365, 6370-6370, 6375-6375, 7541-7541, 7546-7546, 7551-7551
3970-4000: Well-structured string filtering capabilitiesThe
FilterStringschema provides comprehensive string filtering options with a complete set of comparison operators and logical combiners. This implementation follows best practices for API filtering:
- Equal/not equal operators (
$eq,$ne)- List inclusion/exclusion (
$in,$nin)- Pattern matching with and without case sensitivity (
$like,$nlike,$ilike,$nilike)- Lexical ordering (
$gt,$gte,$lt,$lte)- Logical combiners (
$and,$or)The nullable types ensure proper handling of empty filter conditions.
4001-4027: Comprehensive time filtering capabilitiesThe
FilterTimeschema enables advanced time-based filtering with appropriate operators for date-time comparisons and logical combiners.
4195-4201: Proper cursor pagination response structureThe
IngestedEventCursorPaginatedResponseschema follows best practices for cursor-based pagination by including both the items and an optional cursor for subsequent requests.
8007-8010: Parameter definitions for cursor paginationThe cursor pagination parameters are properly defined with clear descriptions of their purpose:
cursor: The cursor after which to start the paginationlimit: The pagination limit (number of items to return)
8113-8120: Improved pagination parameter documentationThe descriptions for pagination parameters have been updated to be accurate and consistent:
page: Correctly described as page index with default of 1pageSize: Correctly described as maximum items per page with default of 100This addresses the issues that were previously identified in past review comments.
8155-8155: Type exports for new pagination and filtering componentsAll new types have been properly exported, ensuring they're available for use throughout the codebase.
Also applies to: 8319-8320, 8337-8338, 8421-8422, 8638-8641
8748-8755: Consistent pagination parameter documentation across all endpointsThe pagination parameter descriptions have been standardized across all endpoints that support pagination, improving the developer experience and API consistency.
Also applies to: 8769-8769, 9331-9338, 9812-9819, 10845-10852, 11314-11321, 11513-11520, 11906-11913, 12390-12397, 12792-12799, 13179-13186, 13563-13570, 13584-13584, 14675-14682, 15164-15171, 15370-15377, 16037-16044
19401-19508: New listEventsV2 operation with comprehensive filtering and paginationThe new operation provides:
- Cursor-based pagination with
cursorandlimitparameters- Client ID tracking for query progress
- Rich filtering options for all relevant event fields (id, source, subject, type, time, ingestedAt)
- Complete error response handling for all potential status codes
The implementation is thorough and follows REST API best practices.
| properties: | ||
| totalCount: | ||
| type: integer | ||
| description: The items in the current page. | ||
| description: The total number of items. | ||
| example: 500 | ||
| page: | ||
| type: integer | ||
| description: The items in the current page. | ||
| description: The page index. | ||
| example: 1 | ||
| pageSize: | ||
| type: integer | ||
| description: The items in the current page. | ||
| description: The maximum number of items per page. | ||
| example: 100 | ||
| items: | ||
| type: array |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Repeated Paginated Schema Definition
The block defining pagination properties (totalCount, page, pageSize, items) mirrors earlier fragments. Consider centralizing this definition into one common component to reduce redundancy and maintenance overhead.
| properties: | ||
| totalCount: | ||
| type: integer | ||
| description: The items in the current page. | ||
| description: The total number of items. | ||
| example: 500 | ||
| page: | ||
| type: integer | ||
| description: The items in the current page. | ||
| description: The page index. | ||
| example: 1 | ||
| pageSize: | ||
| type: integer | ||
| description: The items in the current page. | ||
| description: The maximum number of items per page. | ||
| example: 100 | ||
| items: | ||
| type: array |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Identical Paginated Response Schema Across Endpoints
The repeated use of pagination fields (totalCount, page, pageSize, and items) suggests an opportunity to abstract these into a shared component. This could simplify future updates if the schema needs to change.
| properties: | ||
| totalCount: | ||
| type: integer | ||
| description: The items in the current page. | ||
| description: The total number of items. | ||
| example: 500 | ||
| page: | ||
| type: integer | ||
| description: The items in the current page. | ||
| description: The page index. | ||
| example: 1 | ||
| pageSize: | ||
| type: integer | ||
| description: The items in the current page. | ||
| description: The maximum number of items per page. | ||
| example: 100 | ||
| items: | ||
| type: array |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reiterated Pagination Schema
This set of pagination properties matches previous definitions. Consider centralizing these common definitions to simplify maintenance.
| properties: | ||
| totalCount: | ||
| type: integer | ||
| description: The items in the current page. | ||
| description: The total number of items. | ||
| example: 500 | ||
| page: | ||
| type: integer | ||
| description: The items in the current page. | ||
| description: The page index. | ||
| example: 1 | ||
| pageSize: | ||
| type: integer | ||
| description: The items in the current page. | ||
| description: The maximum number of items per page. | ||
| example: 100 | ||
| items: | ||
| type: array |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Uniform Paginated Response Structure
The pagination fields remain uniform across the specification. As these definitions recur frequently, reducing duplication by centralizing them could be beneficial.
| properties: | ||
| totalCount: | ||
| type: integer | ||
| description: The items in the current page. | ||
| description: The total number of items. | ||
| example: 500 | ||
| page: | ||
| type: integer | ||
| description: The items in the current page. | ||
| description: The page index. | ||
| example: 1 | ||
| pageSize: | ||
| type: integer | ||
| description: The items in the current page. | ||
| description: The maximum number of items per page. | ||
| example: 100 | ||
| items: | ||
| type: array |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Repetitive Pagination Schemas
The recurring pattern of pagination property definitions suggests an opportunity for abstraction into a shared component, which would streamline future modifications and improve maintainability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
api/spec/src/events.tsp (1)
134-215: Consider adding migration guidance for API consumers.The v2 API introduces more advanced filtering capabilities compared to the v1 API. Consider adding documentation or a migration guide explaining how users of the v1 API can transition to v2, particularly how the old parameters like
ingestedAtFrom,ingestedAtTo,from,to, andlimitmap to the new filter syntax.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
api/spec/src/events.tsp(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Artifacts / Container image
- GitHub Check: Artifacts / Benthos Collector Container image
- GitHub Check: Test
- GitHub Check: Quickstart
- GitHub Check: Migration Checks
- GitHub Check: Lint
- GitHub Check: E2E
- GitHub Check: Developer environment
- GitHub Check: CI
- GitHub Check: Commit hooks
- GitHub Check: Build
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
api/spec/src/events.tsp (5)
134-144: Good implementation of a new versioned API endpoint.The introduction of a new v2 endpoint for events is well-structured, with clear documentation. The
@extension("x-internal", true)is a good practice for a new API version that might not be fully stable yet.
157-163: Thank you for adding filter examples as requested.The examples for filters have been added as requested in the previous review. The format is clear and helpful for understanding how to use the API.
189-200: Well-documented complex time filtering.The time filter documentation and example effectively demonstrate how to use the
$andoperator to combine multiple conditions, which is particularly helpful for time range queries.
214-214:✅ Verification successful
Verify that
CursorPaginatedResponseis properly defined.The return type uses
CursorPaginatedResponse<IngestedEvent>, but I don't see where this is defined in this file. Ensure it's properly defined elsewhere in the codebase and its documentation is accessible to API consumers.
🏁 Script executed:
#!/bin/bash # Find where CursorPaginatedResponse is defined rg "model CursorPaginatedResponse" -A 10 # Find where it might be exported/imported rg "CursorPaginatedResponse" --type=typescriptLength of output: 559
Verification:
CursorPaginatedResponseis correctly definedOur search confirms that
CursorPaginatedResponse<T>is defined inapi/spec/src/query.tspand is being used correctly (withIngestedEventas its type parameter) in the events API file. Please ensure that the related documentation in your project references this definition so that API consumers can easily access its details.
145-145:✅ Verification successful
Make sure
QueryCursorPaginationis properly defined and documented.The code uses
...QueryCursorPaginationto include pagination parameters, but I don't see where this is defined in this file. Ensure it's properly defined elsewhere in the codebase and its documentation is accessible to API consumers.
🏁 Script executed:
#!/bin/bash # Find where QueryCursorPagination is defined rg "model QueryCursorPagination" -A 10 # Find where it might be exported/imported rg "QueryCursorPagination" --type=typescriptLength of output: 552
Action: Confirm that the pagination model is defined and referenced as intended.
The verification shows that
QueryCursorPaginationis indeed defined—with clear documentation for itscursorandlimitproperties—inapi/spec/src/query.tsp. Given that the spread operator (...QueryCursorPagination) is used inapi/spec/src/events.tsp, please double-check that this model is correctly imported or available in that context (especially since file type detection for.tspfiles may behave differently from standard TypeScript files).If the integration between these files is as expected and the API documentation tooling includes the comments from
QueryCursorPagination, no further changes are necessary.
Includes #2485
Summary by CodeRabbit