Additional metric alert clickhouse performance improvements#8201
Additional metric alert clickhouse performance improvements#8201jonathanawesome wants to merge 20 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.github.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request optimizes metric alert evaluations for long-window rules (>= 7 days) by introducing a daily ClickHouse rollup table (operations_by_target_daily) and materialized view. It updates the workflows evaluator to query this rollup, skip previous window scans for absolute-only rules, avoid reading unused duration columns, and run evaluations on a tiered interval. It also adds Prometheus metrics, Grafana dashboards, and API validation for these windows. Feedback is provided regarding a ClickHouse query issue where grouping by the alias timestamp instead of toStartOfDay(timestamp) will cause ClickHouse to resolve to the high-resolution source column, preventing proper daily aggregation.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
🐋 This PR was built and pushed to the following Docker images: Targets: Platforms: Image Tags: |
| ? requireColumn(row.average, 'duration_avg', rule) | ||
| : requireColumn(row.percentiles, 'duration_quantiles', rule)[ | ||
| percentileIndex(rule.metric) | ||
| ]; |
There was a problem hiding this comment.
Minor: I think what this is doing is clear and no need for the long comments.
For requireColumn -- is this if for example, there's no data for this window? Or is it for if there's an issue in the code and this column wasnt fetched for some reason?
Could we get away with a null check and return null if it's missing?
| evaluationTime: Date, | ||
| ): boolean { | ||
| if (rule.state === 'PENDING' || rule.state === 'RECOVERING') return true; | ||
| if (!rule.lastEvaluatedAt) return true; |
There was a problem hiding this comment.
can combine this condition with the others to return true.
If you want them separate for clarity on what they are checking, then you could assign a new variable, i.e. const isInPriorityState = rule.state === 'PENDING' || rule.state == 'RECOVERING';
| // All rules in a group share a saved filter (it's part of the group key), so the | ||
| // filter is built once from the representative. A malformed filter yields no | ||
| // conditions (evaluates unfiltered) and is logged, isolating the failure here. | ||
| export function deriveGroupNeeds(groupRules: MetricAlertRuleRow[], logger: Logger): GroupNeeds { |
There was a problem hiding this comment.
This function was unexpected. I anticipated more concrete query building depending on the rule.
E.g. buildLatencyAlertQuery(rule: MetricAlertRuleRow)
I think generic can be good, but as we add more rules i worry this will become spaghetti.
| filteredRules += group.length; | ||
| } | ||
| } | ||
| metricAlertRuleGroups.set({ filtered: 'true' }, filteredGroups); |
There was a problem hiding this comment.
I'm not following this, but it's late. I'll try to take another look tomorrow.
|
This PR combines quite a few pretty much independent changes. Could we split work like this into smaller, focused PRs? Reviewing all of these changes together makes it difficult to understand each decision and verify performance. |
|
There are known things that can improve performance, that we can verify in production quickly:
|
3 participants
This PR flattens the climbing ClickHouse query duration and total run time of the
evaluateMetricAlertRulescron by cutting how often long-window rules run, how many buckets each query scans, and how much work each query does. It also adds Grafana dash panels to keep watch on the number of rules with filters attached (expensive).last_evaluated_at, keeping full 1-min resolution during PENDING/RECOVERING so confirmation dwell is unaffected.operations_by_target_dailyClickHouse rollup; windows ≥7 days read a daily table (~60 buckets for a 30-day query instead of ~1,440), fixing both per-query cost and the 30-day hourly-retention cliff for long windows.nullprevious value consistently regardless of group-mates so the UI responds accordingly.