feat(streaming): clickhouse query optimize #3030
Conversation
📝 WalkthroughWalkthroughThe changes introduce support for custom query settings for meter queries in the ClickHouse connector by extending configuration structures and updating SQL query construction. Additionally, ClickHouse image versions are updated across deployment and development configurations. Unit tests are adjusted to reflect the revised SQL generation logic, including prewhere clause optimization and new settings. 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 (
|
![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
🧹 Nitpick comments (2)
app/config/aggregation.go (1)
31-34: Add default for newMeterQuerySettings.You introduced
MeterQuerySettingsin the struct but didn't add a default inConfigureAggregation. Consider adding:func ConfigureAggregation(v *viper.Viper) { // existing defaults... v.SetDefault("aggregation.clickhouse.blockBufferSize", 10) + // Default settings for meter queries + v.SetDefault("aggregation.meterQuerySettings", map[string]string{}) }openmeter/streaming/clickhouse/connector.go (1)
173-173: Consider validating MeterQuerySettings.The QuerySettings are passed directly without validation. Invalid ClickHouse settings could cause query failures at runtime.
Consider adding validation for the query settings to catch configuration errors early. You could validate against a whitelist of known ClickHouse settings or at least ensure the values don't contain SQL injection risks.
📜 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 (11)
- GitHub Check: Artifacts / Container image
- GitHub Check: Commit hooks
- GitHub Check: E2E
- GitHub Check: Quickstart
- GitHub Check: CI
- GitHub Check: Developer environment
- GitHub Check: Test
- GitHub Check: Build
- GitHub Check: Migration Checks
- GitHub Check: Lint
- GitHub Check: Analyze (go)
🔇 Additional comments (9)
.dagger/versions_pinned.go (1)
5-6: Bump ClickHouse version to align with other configurations.The ClickHouse version constant has been updated from "24.5.5.78" to "24.10", matching the image tags in deployment and Docker Compose files.
deploy/charts/openmeter/templates/clickhouse.yaml (1)
24-24: Confirm use of non-alpine ClickHouse image.The chart now pins
clickhouse-server:24.10, whereas Docker Compose uses the alpine variant. This is acceptable if deliberate, but please verify that the standard image is intended here (vs.24.10-alpine).examples/collectors/database/docker-compose.yaml (1)
51-51: Inconsistent ClickHouse image version compared to main Docker Compose.This example uses
24.0-alpine, while the maindocker-compose.yamland other configurations use24.10-alpine. Please confirm whether this discrepancy is intentional or if it should be aligned.docker-compose.yaml (1)
30-30: Align ClickHouse image tag with pinned version.The service now uses
clickhouse-server:24.10-alpine, matching the pinned version constant. Ensure this matches your production requirements.app/common/streaming.go (1)
38-38: LGTM!The MeterQuerySettings field is correctly propagated from the aggregation configuration to the ClickHouse connector configuration.
openmeter/streaming/clickhouse/connector.go (1)
37-37: LGTM!The MeterQuerySettings field is correctly added to the Config struct to support custom ClickHouse query settings.
openmeter/streaming/clickhouse/meter_query_test.go (1)
46-46: Test updates correctly reflect the new query generation logic.All test cases have been properly updated to expect:
- The new SETTINGS clause with PREWHERE optimization flags
- Updated window calculations using
toStartOfMinutefor non-windowed queries- PREWHERE clauses for filtered queries
Also applies to: 65-65, 83-83, 102-102, 122-122, 143-143, 165-165, 188-188, 209-209, 230-230, 251-251, 271-271, 291-291, 311-311
openmeter/streaming/clickhouse/meter_query.go (2)
8-8: LGTM!The addition of the strings import and QuerySettings field enables custom ClickHouse query settings.
Also applies to: 30-30
155-155: Verify the impact of window calculation change.The window calculation for non-windowed queries has changed from using
tumbleStart/tumbleEndtotoStartOfMinute(min/max). This is a breaking change that affects the window boundaries returned by queries.Please ensure that:
- This change is intentional and documented
- API consumers are aware of this behavioral change
- The minute-level rounding is acceptable for all use cases
| // Only add prewhere if there are filters on JSON data | ||
| if sqlPreWhere != "" { | ||
| sqlParts := strings.Split(sql, sqlPreWhere) | ||
| sqlAfter := sqlParts[1] | ||
|
|
||
| if strings.HasPrefix(sqlAfter, " AND") { | ||
| sqlAfter = strings.Replace(sqlAfter, "AND", "WHERE", 1) | ||
| } | ||
|
|
||
| sqlPreWhere = strings.Replace(sqlPreWhere, "WHERE", "PREWHERE", 1) | ||
| sql = fmt.Sprintf("%s%s", sqlPreWhere, sqlAfter) | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor the fragile string manipulation for PREWHERE clause.
The current approach using string splitting and replacement is error-prone and could break if the query structure changes. This makes the code difficult to maintain.
Consider using a more robust approach:
- Build PREWHERE and WHERE clauses separately using the query builder
- Use a custom SQL builder that supports PREWHERE natively
- Or at minimum, use more robust string manipulation with proper parsing
Example approach:
// Build PREWHERE conditions separately
prewhereBuilder := sqlbuilder.ClickHouse.NewSelectBuilder()
prewhereConditions := []string{
prewhereBuilder.Equal(getColumn("namespace"), d.Namespace),
prewhereBuilder.Equal(getColumn("type"), d.Meter.EventType),
}
// Build WHERE conditions for filters
whereConditions := []string{}
// ... add filter conditions to whereConditions
// Construct the final query with both PREWHERE and WHERE🤖 Prompt for AI Agents
In openmeter/streaming/clickhouse/meter_query.go around lines 279 to 291, the
current string manipulation to insert the PREWHERE clause by splitting and
replacing parts of the SQL query is fragile and error-prone. To fix this,
refactor the code to build PREWHERE and WHERE clauses separately using a query
builder that supports PREWHERE natively or at least construct the conditions as
separate lists before combining them. Avoid direct string splitting and
replacements; instead, assemble the final query by explicitly adding PREWHERE
conditions first and then WHERE conditions to ensure correctness and
maintainability.
| for key, value := range d.QuerySettings { | ||
| settings = append(settings, fmt.Sprintf("%s = %s", key, value)) | ||
| } | ||
|
|
||
| sql = sql + fmt.Sprintf(" SETTINGS %s", strings.Join(settings, ", ")) | ||
|
|
There was a problem hiding this comment.
Add validation for QuerySettings to prevent SQL injection.
The QuerySettings map values are directly interpolated into the SQL query without any validation or escaping, which could lead to SQL injection vulnerabilities if these settings come from untrusted sources.
Apply this diff to add basic validation:
+// Validate query settings to prevent SQL injection
+for key, value := range d.QuerySettings {
+ // Validate key contains only alphanumeric and underscore
+ if !regexp.MustCompile(`^[a-zA-Z0-9_]+$`).MatchString(key) {
+ return "", nil, fmt.Errorf("invalid query setting key: %s", key)
+ }
+ // Validate value contains only safe characters
+ if !regexp.MustCompile(`^[a-zA-Z0-9_\s,=.]+$`).MatchString(value) {
+ return "", nil, fmt.Errorf("invalid query setting value for %s: %s", key, value)
+ }
+}
// Add settings
settings := []string{
"optimize_move_to_prewhere = 1",
"allow_reorder_prewhere_conditions = 1",
}
for key, value := range d.QuerySettings {
settings = append(settings, fmt.Sprintf("%s = %s", key, value))
}Alternatively, consider using a whitelist of allowed ClickHouse settings.
📝 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.
| for key, value := range d.QuerySettings { | |
| settings = append(settings, fmt.Sprintf("%s = %s", key, value)) | |
| } | |
| sql = sql + fmt.Sprintf(" SETTINGS %s", strings.Join(settings, ", ")) | |
| // Validate query settings to prevent SQL injection | |
| for key, value := range d.QuerySettings { | |
| // Validate key contains only alphanumeric and underscore | |
| if !regexp.MustCompile(`^[a-zA-Z0-9_]+$`).MatchString(key) { | |
| return "", nil, fmt.Errorf("invalid query setting key: %s", key) | |
| } | |
| // Validate value contains only safe characters | |
| if !regexp.MustCompile(`^[a-zA-Z0-9_\s,=.]+$`).MatchString(value) { | |
| return "", nil, fmt.Errorf("invalid query setting value for %s: %s", key, value) | |
| } | |
| } | |
| for key, value := range d.QuerySettings { | |
| settings = append(settings, fmt.Sprintf("%s = %s", key, value)) | |
| } | |
| sql = sql + fmt.Sprintf(" SETTINGS %s", strings.Join(settings, ", ")) |
🤖 Prompt for AI Agents
In openmeter/streaming/clickhouse/meter_query.go around lines 297 to 302, the
QuerySettings map values are directly inserted into the SQL query without
validation, risking SQL injection. To fix this, add validation to ensure each
key and value conforms to expected patterns or use a whitelist of allowed
settings before appending them to the query. Reject or sanitize any entries that
do not meet these criteria to prevent injection vulnerabilities.
2 participants
Rebased #2974
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests