feat(streaming): clickhouse query optimization #2974
Conversation
📝 WalkthroughWalkthroughThis update introduces support for custom meter query settings in the ClickHouse streaming connector by extending configuration structs and propagating these settings through the query execution logic. The ClickHouse server image versions are updated across deployment files. Query construction logic is enhanced for optimized filtering and settings injection, with corresponding updates to related tests. Changes
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 ✨ 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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
hekike
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
openmeter/streaming/clickhouse/meter_query.go (1)
230-283: 🛠️ Refactor suggestion
⚠️ Potential issueThe PREWHERE implementation uses fragile string manipulation.
While the optimization approach is correct, the string manipulation method (splitting SQL and replacing keywords) is fragile and could break if the SQL builder changes its output format.
Consider using a more robust approach:
- Option 1: Extend the SQL builder to support PREWHERE natively
- Option 2: Build the PREWHERE and WHERE clauses separately:
// Build PREWHERE conditions prewhereBuilder := sqlbuilder.ClickHouse.NewSelectBuilder() prewhereBuilder.Select(selectColumns...) prewhereBuilder.From(tableName) prewhereConditions := []string{ fmt.Sprintf("%s = ?", getColumn("namespace")), fmt.Sprintf("%s = ?", getColumn("type")), } // Build WHERE conditions for JSON filters whereConditions := []string{} if len(d.FilterGroupBy) > 0 { // Add JSON_VALUE conditions to whereConditions } // Combine into final SQL sql = fmt.Sprintf("SELECT ... FROM ... PREWHERE %s WHERE %s", strings.Join(prewhereConditions, " AND "), strings.Join(whereConditions, " AND "))
- Option 3: Add a comment explaining the string manipulation assumptions and add tests to catch any SQL builder format changes.
🧹 Nitpick comments (1)
openmeter/streaming/clickhouse/connector.go (1)
39-39: Consider adding validation for MeterQuerySettings values.The
MeterQuerySettingsfield is correctly added to propagate custom query settings. However, invalid settings could cause query failures at runtime.Consider adding validation in the
Validate()method to ensure the settings are valid ClickHouse query settings:func (c Config) Validate() error { if c.Logger == nil { return fmt.Errorf("logger is required") } + + // Validate MeterQuerySettings if provided + if c.MeterQuerySettings != nil { + // Example: validate known settings or check format + for key, value := range c.MeterQuerySettings { + // Ensure values don't contain SQL injection risks + if strings.ContainsAny(value, ";'\"") { + return fmt.Errorf("invalid character in meter query setting value for key %s", key) + } + } + } if c.ClickHouse == nil { return fmt.Errorf("clickhouse connection is required") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.dagger/versions_pinned.go(1 hunks)app/common/streaming.go(1 hunks)app/config/aggregation.go(1 hunks)deploy/charts/openmeter/templates/clickhouse.yaml(1 hunks)docker-compose.yaml(1 hunks)examples/collectors/database/docker-compose.yaml(1 hunks)openmeter/streaming/clickhouse/connector.go(2 hunks)openmeter/streaming/clickhouse/meter_query.go(6 hunks)openmeter/streaming/clickhouse/meter_query_test.go(14 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: CI
- GitHub Check: Analyze (go)
🔇 Additional comments (9)
.dagger/versions_pinned.go (1)
3-7: Confirm ClickHouse version bump.The
clickhouseVersionconstant has been updated from"24.5.5.78"to"24.10", matching the other deployment targets. Verify that this version matches your CI/CD expectations and is compatible with the features used in the streaming connector.deploy/charts/openmeter/templates/clickhouse.yaml (1)
23-27: Align Kubernetes image with pinned version.The ClickHouse image tag has been updated to
clickhouse/clickhouse-server:24.10, which aligns with the pinned Go constant. Confirm that the non-alpine (glibc) variant is the intended runtime for your Kubernetes clusters.examples/collectors/database/docker-compose.yaml (1)
49-53: Verify example ClickHouse Alpine tag.The example uses
clickhouse/clickhouse-server:24.0-alpine, diverging from the pinned24.10and main Compose24.10-alpine. Please confirm if the example should be updated to24.10-alpinefor consistency or if this version difference is intentional.docker-compose.yaml (1)
29-33: Approve Alpine image update.The ClickHouse service has been bumped to
clickhouse/clickhouse-server:24.10-alpine, aligning with the new version. Ensure any client defaults (e.g., default database name) continue to work with this image.openmeter/streaming/clickhouse/connector.go (1)
209-209: LGTM!The meter query settings are correctly propagated to the query execution.
app/common/streaming.go (1)
38-38: LGTM!The meter query settings are correctly passed from the aggregation configuration to the ClickHouse connector.
openmeter/streaming/clickhouse/meter_query_test.go (1)
46-47: Test updates correctly reflect the query optimization changes.The test expectations have been properly updated to verify:
- The new time window calculation using
toStartOfMinutefor non-windowed queries- The addition of optimization SETTINGS clause to all queries
- The PREWHERE clause implementation for JSON filtering
Also applies to: 65-66, 83-84, 102-103, 122-123, 143-144, 165-166, 188-189, 209-210, 230-231, 251-252, 271-272, 291-292, 311-312
openmeter/streaming/clickhouse/meter_query.go (2)
30-30: LGTM!The QuerySettings field is correctly added to support custom query optimization settings.
145-149: Clarify the TODO comment timeline.The TODO comment mentions removing the minute rounding but doesn't specify when this should happen. This could lead to the technical debt being forgotten.
Could you provide more context about:
- Why is the minute rounding needed currently?
- What conditions need to be met before it can be removed?
- Should this be tracked in an issue?
prewhereto pre-filter rows before group by filterSummary by CodeRabbit
New Features
Bug Fixes
Chores