feat(api): query meters v2 with filters #2718
Conversation
tothandras
added
area/api
release-note/feature
|
Warning Rate limit exceeded@tothandras has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 21 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (14)
📝 WalkthroughWalkthroughThis change set introduces a new, advanced filtering mechanism for querying meter data. A JSON-encoded Changes
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 (
|
tothandras
force-pushed
the
feat/meter-query-v2
branch
2 times, most recently
from
947735f to
367308b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (2)
openmeter/meter/httphandler/mapping.go (2)
75-105:⚠️ Potential issueFrom/To and
FilterGroupByare no longer forwarded – legacy queries will break
ToQueryMeterParamsused to copyfrom,to, andfilterGroupByfrom the publicapi.QueryMeterParamsinto the internalstreaming.QueryParams, but those assignments disappeared during the refactor.
The ClickHouse connector still relies onparams.From,params.To, andparams.FilterGroupBy(seequeryMeter), so any v1 request that omits the newfilterparameter will now return all time results and ignorefilterGroupBy.func ToQueryMeterParams(m meter.Meter, apiParams api.QueryMeterParams) (streaming.QueryParams, error) { params := streaming.QueryParams{ ClientID: apiParams.ClientId, } + + // restore time & filter mappings for v1 + params.From = apiParams.From + params.To = apiParams.To + if apiParams.FilterGroupBy != nil { + params.FilterGroupBy = *apiParams.FilterGroupBy + }Without this, every existing client that still uses
from/towill get incorrect data.
118-166: 🛠️ Refactor suggestionDuplicate validation logic & missing deterministic ordering
ToQueryMeterParamsV2re-implements almost the same group-by validation that exists above. Consider extracting a shared helper to avoid divergence.In addition,
params.GroupByis left in request order.queryMeter(v1) explicitly sorted to guarantee the column order used for SQL generation and row-scanning. The V2 path omits that, but the reader inqueryMeterV2assumes the same order when mapping scan args (for i, key := range params.GroupBy { … }).
If the caller passes"b,a"instead of"a,b", the scan will mismatch columns.+ // ensure deterministic order like v1 + sort.Strings(params.GroupBy)This keeps both SQL and result parsing stable, simplifies testing, and avoids subtle data shifts.
🧹 Nitpick comments (11)
openmeter/server/server_test.go (1)
651-655: Implement conditional logic based on filter parameters.The V2 implementation always returns the subject value from
mockQueryValue, unlike the V1 implementation which conditionally setsSubjectto nil based on filtering parameters. This mock doesn't accurately reflect how filtering would work in the real implementation.Consider adding conditional logic to handle filter parameters:
func (c *MockStreamingConnector) QueryMeterV2(ctx context.Context, namespace string, m meter.Meter, params streaming.QueryParamsV2) ([]meter.MeterQueryRow, error) { value := mockQueryValue + // Apply filtering based on params.Filter + if params.Filter != nil { + // Handle subject filtering + if params.Filter.Subject == nil || len(params.Filter.Subject.In) == 0 { + value.Subject = nil + } + + // Handle other filter conditions + } return []meter.MeterQueryRow{value}, nil }api/client/javascript/src/client/schemas.ts (1)
17446-17448: Consider adding an example JSON structure for the filter parameterWhile the documentation clearly states the purpose and constraints of the filter parameter, it would be helpful to include an example of the expected JSON structure to guide API users.
/** @description The filter for the events encoded as JSON string. - Can't be used together with from, to, subject, and filterGroupBy. */ + Can't be used together with from, to, subject, and filterGroupBy. + + For example: ?filter={"groupBy":{"model":"gpt-4"},"time":{"from":"2023-01-01T00:00:00Z","to":"2023-01-02T00:00:00Z"}} */api/openapi.cloud.yaml (1)
10622-10642: Refine the JSON schema definition for thefilterparameter.
The current schema omits an explicittype: objectand includes an unnecessaryformatfield. Consider this diff to improve clarity and correctness:content: application/json: schema: - properties: + type: object + properties: groupBy: type: object additionalProperties: $ref: '#/components/schemas/FilterString' subject: $ref: '#/components/schemas/FilterString' time: $ref: '#/components/schemas/FilterTime' - format: application/jsonAdding
type: objectclarifies that the filter is a JSON object, and removingformatavoids misuse of the field.openmeter/streaming/clickhouse/meter_query_v2_test.go (2)
15-438: Comprehensive test cases fortoSQL()method, but consider splitting this large function.The test function covers multiple scenarios including different aggregation types, subject filtering, time range filtering, windowing, timezone handling, and group filtering. All test cases are well structured with clear inputs and expected outputs.
Consider splitting this large test function into smaller, more focused test functions or subtest groups for better maintainability. You could group tests by functionality (e.g., basic queries, filtering, windowing, etc.).
-func TestQueryMeterV2(t *testing.T) { +func TestQueryMeterV2_BasicQueries(t *testing.T) { // Setup common test variables... tests := []struct { // Test structure... }{ // Basic query test cases only... } // Test execution... } +func TestQueryMeterV2_GroupingAndFiltering(t *testing.T) { + // Setup common test variables... + + tests := []struct { + // Test structure... + }{ + // Group filtering test cases only... + } + + // Test execution... +}
428-436: Add test cases for error scenarios.The code handles errors from
toSQL(), but no test case is designed to trigger an error path. Consider adding test cases that exercise the error handling code.Add a test case with invalid inputs that would trigger an error:
+ { + name: "error case", + query: queryMeterTableV2{ + // Invalid input that should trigger an error + Database: "", // Empty database name + EventsTableName: "om_events", + Namespace: "my_namespace", + Meter: meter.Meter{ + Key: "meter1", + EventType: "event1", + Aggregation: "invalid_aggregation", // Invalid aggregation + }, + }, + wantError: true, + },Then update the test execution to check for expected errors:
t.Run("", func(t *testing.T) { gotSql, gotArgs, err := tt.query.toSQL() - if err != nil { - t.Error(err) - return - } + if tt.wantError { + assert.Error(t, err) + return + } else if err != nil { + t.Error(err) + return + } assert.Equal(t, tt.wantSQL, gotSql) assert.Equal(t, tt.wantArgs, gotArgs) })openmeter/streaming/clickhouse/connector.go (1)
647-657: Stale TODO & redundant validation callThe comment says “TODO: Add validation for params once we have ValidateV2”, yet
params.Validate()is already invoked.
Either delete the TODO or remove the call – having both is confusing.openmeter/meter/httphandler/query.go (2)
85-90: Validation message could be clearer & cover all legacy fieldsThe current check blocks
from,to,filterGroupBy, orsubjectwhenfilteris set, but omitswindowSizeandwindowTimeZone, which are also semantically orthogonal in V2.
Consider extending the guard and making the error actionable:if p.Filter != nil && (p.From != nil || p.To != nil || p.FilterGroupBy != nil || p.Subject != nil) { return models.NewGenericValidationError( fmt.Errorf("the top-level filter is mutually exclusive with from, to, filterGroupBy and subject parameters")) }
197-209: CSV header order may fluctuate – sort for deterministic output
groupByoriginates from the converted params slice, which preserves user order. The downstream CSV builder iterates that slice to emit headers, so two identical queries with"b,a"vs"a,b"yield different CSV layouts.For stable exports (important when users automate parsing) sort before passing:
- groupBy = params.GroupBy + groupBy = append([]string(nil), params.GroupBy...) + sort.Strings(groupBy)openmeter/streaming/clickhouse/meter_query_v2.go (3)
62-63: Align error return signatures
return "", nil, errvsreturn "", []interface{}{}, erris inconsistent.
Prefernilfor empty slices to avoid unnecessary allocations.- return "", []interface{}{}, fmt.Errorf("invalid aggregation type: %s", q.Meter.Aggregation) + return "", nil, fmt.Errorf("invalid aggregation type: %s", q.Meter.Aggregation)Also applies to: 90-90
194-195: PrefergetColumnover hard-coded names for consistencyUsing literals bypasses the column-aliasing logic encapsulated in
columnFactory, making future refactors harder and risking mismatches if
the physical column names ever change.- query.Where(query.Equal("namespace", q.Namespace)) - query.Where(query.Equal("type", q.Meter.EventType)) + query.Where(query.Equal(getColumn("namespace"), q.Namespace)) + query.Where(query.Equal(getColumn("type"), q.Meter.EventType))
23-24: Use consistent method receivers
toSQLhas a value receiver whiletoCountRowSQLuses a pointer.
Stick to one style (pointer is usual for objects with many fields) to avoid
unexpected copies.Also applies to: 184-184
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
api/client/javascript/src/client/schemas.ts(5 hunks)api/openapi.cloud.yaml(3 hunks)api/openapi.yaml(3 hunks)api/spec/src/meters.tsp(2 hunks)openmeter/meter/httphandler/mapping.go(2 hunks)openmeter/meter/httphandler/query.go(6 hunks)openmeter/server/server_test.go(1 hunks)openmeter/streaming/clickhouse/connector.go(2 hunks)openmeter/streaming/clickhouse/meter_query_test.go(1 hunks)openmeter/streaming/clickhouse/meter_query_v2.go(1 hunks)openmeter/streaming/clickhouse/meter_query_v2_test.go(1 hunks)openmeter/streaming/connector.go(1 hunks)openmeter/streaming/query_params.go(3 hunks)openmeter/streaming/testutils/streaming.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
openmeter/streaming/testutils/streaming.go (2)
openmeter/meter/meter.go (2)
Meter(152-164)MeterQueryRow(333-339)openmeter/streaming/query_params.go (1)
QueryParamsV2(23-29)
openmeter/streaming/connector.go (2)
openmeter/meter/meter.go (2)
Meter(152-164)MeterQueryRow(333-339)openmeter/streaming/query_params.go (1)
QueryParamsV2(23-29)
openmeter/server/server_test.go (3)
openmeter/streaming/testutils/streaming.go (1)
MockStreamingConnector(32-35)openmeter/meter/meter.go (2)
Meter(152-164)MeterQueryRow(333-339)openmeter/streaming/query_params.go (1)
QueryParamsV2(23-29)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: E2E
- GitHub Check: CI
- GitHub Check: Quickstart
- GitHub Check: Developer environment
- GitHub Check: Commit hooks
- GitHub Check: Test
- GitHub Check: Lint
- GitHub Check: Build
- GitHub Check: Analyze (go)
🔇 Additional comments (18)
openmeter/streaming/clickhouse/meter_query_test.go (1)
295-311: LGTM! The test case correctly verifies the EventFrom behavior.This test case properly verifies that when the meter's
EventFromtime is later than the query'sFromtime, the SQL query uses the laterEventFromtime as the effective start time filter in the WHERE clause.api/client/javascript/src/client/schemas.ts (3)
9721-9723: LGTM: The filter parameter addition is well-documentedThe new filter parameter is clearly defined with appropriate documentation that indicates its purpose and mutual exclusivity constraints with existing parameters.
10469-10470: LGTM: Proper type export for the new filter parameterThe type export for the new filter parameter follows the established pattern in the codebase and ensures type safety for API client users.
20296-20302: LGTM: Parameters correctly added to queryPortalMeter operationThe filterGroupBy and filter parameters are correctly added to the queryPortalMeter operation with consistent documentation.
api/openapi.cloud.yaml (2)
5357-5360: Ensure consistent parameter ordering for the newfilterquery parameter.
Thefilterparameter is correctly added afterfilterGroupByin this endpoint. Please confirm that this ordering matches the core OpenAPI spec (api/openapi.yaml) and that generated client code will respect this sequence.
7679-7680: Mirror the newfilterparameter in the portal endpoint.
The portal endpoint now listsfilterimmediately afterfilterGroupBy. Verify that its description and mutual-exclusivity rules are identical to the primary meter query endpoint.openmeter/streaming/clickhouse/meter_query_v2_test.go (2)
1-13: Well-structured imports with appropriate packages.The imports cover all needed functionality including testing, time handling, assertions, and OpenMeter-specific packages.
440-611: Well-structured test cases fortoCountRowSQL()method.The tests are well-organized with clear names and descriptions. Each test case has appropriate assertions and covers important scenarios like filtering, subject handling, and time constraints.
openmeter/streaming/query_params.go (5)
8-8: Appropriate import for filter package.Adding the filter package import supports the new filtering capabilities.
23-29: Well-structured QueryParamsV2 definition.The new struct provides a clear parameter structure for the advanced filtering capabilities with appropriate field types.
45-49: Clean Filter struct definition with clear purpose.The Filter struct encapsulates the filtering capabilities for group, subject, and time dimensions.
51-79: Thorough validation logic for Filter.The validation method covers all fields and properly handles nil case. The complexity validation and error accumulation approach is well-implemented.
81-100: Improved validation pattern for QueryParams.The validation method now accumulates errors rather than returning early, improving the error reporting experience. The consistent use of error joining and the NillableGenericValidationError helper is good.
api/spec/src/meters.tsp (1)
356-365: Clear definition for the new groupBy parameter.The parameter is well-documented with appropriate examples and explode behavior. The documentation clearly explains the purpose and usage.
api/openapi.yaml (2)
5356-5359: Approve addition offilterto V2 endpoint parameters
The newfilterreference is correctly positioned afterfilterGroupBy, reflecting the updated API contract for advanced JSON‐encoded filtering. This aligns with the mutual‐exclusivity rules defined in the component schema.
7678-7679: Approve addition offilterto portal endpoint parameters
Consistent with the primary meter query endpoint, the portal API now exposes thefilterparameter afterfilterGroupBy. This maintains symmetry between the two routes and supports the new V2 filtering semantics.openmeter/streaming/clickhouse/connector.go (1)
670-683: Window overwrite only honoursgte/lte, ignoresgt/lt
getTimeRangeFromFilterreturnsGte/Lteonly. When a client specifies{ "time": { "gt": … } }the post-processing silently leavesWindowStartzero, causing an inconsistent response (SQL was filtered, but the window header is wrong).Consider:
func getTimeRangeFromFilter(tf *filter.FilterTime) (*time.Time, *time.Time) { - if tf == nil { - return nil, nil - } - return tf.Gte, tf.Lte + if tf == nil { + return nil, nil + } + // prefer inclusive bounds, fall back to exclusive ones + from := lo.Coalesce(tf.Gte, tf.Gt) + to := lo.Coalesce(tf.Lte, tf.Lt) + return from, to }openmeter/streaming/clickhouse/meter_query_v2.go (1)
111-114:❌ Incorrect review comment
Nil-map lookup can panic when a
groupBykey is missing
q.Meter.GroupBy[groupByKey]assumes the key exists. If the API user
requests an arbitrary key the meter wasn’t configured for, the server will
panic.Add an existence check and surface a validation error instead.
Map lookup on missing key won’t panic in Go
Reading a non-existent key (or from a nil map) returns the zero value (
"") rather than causing a panic. If the real concern is generating an invalid JSON path or SQL snippet whengroupByKeyis absent, you can add an existence check to emit a clear validation error instead of proceeding with an empty path.• In Go,
q.Meter.GroupBy[groupByKey]returns""if the key isn’t found—no panic occurs.
• To prevent invalid SQL, explicitly verify thatgroupByKeyexists inq.Meter.GroupByand return a user-friendly error when it doesn’t.Likely an incorrect or invalid review comment.
tothandras
force-pushed
the
feat/meter-query-v2
branch
2 times, most recently
from
eeaa879 to
43de4a7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
openmeter/streaming/query_params.go (1)
38-40:⚠️ Potential issueMissing nil check for Filter before validation.
The code tries to call
p.Filter.Validate()without first checking ifp.Filteris nil, which can lead to a nil pointer dereference. This was previously flagged in a review and supposedly fixed, but the fix isn't present here.Apply this fix:
-if err := p.Filter.Validate(); err != nil { +if p.Filter != nil { + if err := p.Filter.Validate(); err != nil { errs = append(errs, err) + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
api/client/javascript/src/client/schemas.ts(5 hunks)api/openapi.cloud.yaml(3 hunks)api/openapi.yaml(3 hunks)api/spec/src/meters.tsp(2 hunks)openmeter/meter/httphandler/mapping.go(2 hunks)openmeter/meter/httphandler/query.go(6 hunks)openmeter/server/server_test.go(1 hunks)openmeter/streaming/clickhouse/connector.go(2 hunks)openmeter/streaming/clickhouse/meter_query_test.go(1 hunks)openmeter/streaming/clickhouse/meter_query_v2.go(1 hunks)openmeter/streaming/clickhouse/meter_query_v2_test.go(1 hunks)openmeter/streaming/connector.go(1 hunks)openmeter/streaming/query_params.go(3 hunks)openmeter/streaming/testutils/streaming.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- openmeter/streaming/testutils/streaming.go
- openmeter/meter/httphandler/mapping.go
- openmeter/streaming/connector.go
- openmeter/server/server_test.go
- openmeter/streaming/clickhouse/meter_query_test.go
- api/client/javascript/src/client/schemas.ts
- openmeter/streaming/clickhouse/meter_query_v2.go
- openmeter/streaming/clickhouse/meter_query_v2_test.go
- api/openapi.yaml
- openmeter/streaming/clickhouse/connector.go
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Quickstart
- GitHub Check: E2E
- GitHub Check: CI
- GitHub Check: Lint
- GitHub Check: Developer environment
- GitHub Check: Commit hooks
- GitHub Check: Test
- GitHub Check: Analyze (go)
- GitHub Check: Build
🔇 Additional comments (20)
openmeter/streaming/query_params.go (3)
51-79: Good implementation of validation with complexity limits.The validation method properly checks for nil before validating each filter component and enforces complexity limits for filters, which helps prevent overly complicated queries that could impact performance.
85-87: Good improvement in error handling.The change to append errors rather than returning immediately allows for more comprehensive validation by collecting all validation errors.
99-99: Good use of utility function for error handling.Using
models.NewNillableGenericValidationErrorwitherrors.Joincreates a clean pattern for handling multiple validation errors.api/spec/src/meters.tsp (2)
356-365: Well-designed groupBy parameter with clear documentation.The new
groupByparameter is properly implemented as an exploded array of strings with good documentation explaining its usage and defaults.
384-405:✅ Verification successful
Well-structured filter parameter with comprehensive documentation.
The filter parameter implementation includes:
- Clear documentation noting mutual exclusivity with other parameters
- Detailed examples showing the structure with operators ($eq, $gte, $lte, $and)
- Proper encoding as application/json
- Well-defined schema for groupBy, subject, and time filters
Make sure that the validation logic enforcing the mutual exclusivity constraint is implemented in the request handling code.
🏁 Script executed:
#!/bin/bash # Look for validation logic that enforces filter parameter mutual exclusivity rg -A 10 "filter.*from|from.*filter|filter.*to|to.*filter|filter.*subject|subject.*filter|filter.*filterGroupBy|filterGroupBy.*filter" --glob "*.go" | grep -i "error\\|valid\\|exclusive"Length of output: 7506
Validation in place for mutual exclusivity of
filterThe
filterparameter is well-structured and documented, with:
- Clear JSDoc noting that
filtercan’t be used withfrom,to,subject, orfilterGroupBy- A comprehensive
@exampledemonstrating operators ($eq,$gte,$lte,$and)- Proper
@encode("application/json")annotation- Precise TypeScript schema for
groupBy,subject, andtimeThe request handler in
openmeter/meter/httphandler/query.goalready enforces this constraint by returning a validation error iffilteris combined with any of the other parameters:return models.NewGenericValidationError( fmt.Errorf("filter and from, to, filter group by or subject cannot be set at the same time"), )No further changes required—approving these updates.
api/openapi.cloud.yaml (2)
5357-5360: Correctly append groupBy and filter parameters
The newMeterQuery.groupByandMeterQuery.filterreferences are added in the correct order within the cloud meter query endpoint, preserving mutual‐exclusivity semantics while enabling advanced filtering.
7679-7680: Synchronize filter parameter on portal endpoint
MeterQuery.filteris properly inserted afterfilterGroupByin the portal meter query operation, ensuring feature parity with the main cloud API.openmeter/meter/httphandler/query.go (13)
14-14: Added import for validation errorsThe added import for
modelspackage is necessary for the new validation logic that uses theNewGenericValidationErrorfunction.
27-28: Field capitalization improves consistencyThe capitalization of
NamespaceandIdOrSlugin theListSubjectsRequeststruct makes these fields exported, which ensures consistent visibility across similar structs in the codebase.
80-82: Field capitalization for consistent struct field exposureSimilar to the changes in
ListSubjectsRequest, capitalizing fields inQueryMeterRequestensures proper exportability across the codebase.
84-90: Well-implemented validation for mutually exclusive filtersGood implementation of the validation logic to ensure the new
Filterparameter is mutually exclusive with the existing filter parameters. This prevents ambiguous or conflicting query specifications.
101-103: Early validation prevents invalid requestsAdding validation early in the request processing pipeline prevents invalid requests from reaching the service layer, which is a good practice.
111-117: Error handling simplificationThe error from
GetMeterByIDOrSlugis now returned directly without wrapping it in additional context. This seems to be a deliberate simplification across the file.
119-148: Clean implementation of filter-based query path selectionThe conditional logic based on
Filterpresence is well-structured and follows a consistent pattern:
- Convert parameters based on filter type
- Validate parameters
- Call the appropriate query method
This ensures a clear separation between the standard query path and the new filtered query path.
150-150: Response formatting is consistent regardless of query pathThe same response formatting function is used regardless of which query path was taken, ensuring consistent response structure.
178-180: Consistent validation pattern in CSV handlerThe same validation pattern is applied in the CSV handler, maintaining consistency with the JSON handler implementation.
196-228: Consistent conditional logic in CSV handlerThe CSV handler follows the same pattern as the JSON handler for determining the query path based on filter presence, ensuring consistency between endpoints.
196-197: Good variable initialization before conditional logicThe variables for rows and groupBy are declared before the conditional block, which ensures they are in scope for the entire function body.
208-208: Consistent extraction of groupBy parameterThe groupBy extraction is handled consistently in both branches of the condition, making the code more maintainable.
Also applies to: 223-223
230-230: Improved CSV result constructionThe CSV result now uses the meter key from the retrieved meter object rather than from the request, which is more robust as it ensures the data source is authoritative.
tothandras
force-pushed
the
feat/meter-query-v2
branch
from
43de4a7 to
fc5de55
Compare
Summary by CodeRabbit
New Features
filterquery parameter for meter queries, supporting flexible filtering ongroupBy,subject, andtime.groupByparameter.filterparameter with mutual exclusivity to simpler filters.Bug Fixes
Tests
Documentation