fix: alert threshold fixes #8752

oasisk · 2025-10-07T09:00:34Z

PR Type

Bug fix, Enhancement

Description

Add null-safety in PromQL data utils
Improve axis show checks for empty results
Pass executed query and Y-axis column to alerts
Apply thresholds via HAVING/PromQL condition

Diagram Walkthrough

flowchart LR
  Panel["PanelSchemaRenderer: derive executedQuery + yAxis"] -- "pass via router" --> AddAlert["AddAlert: build alert payload"]
  AddAlert -- "SQL: apply HAVING; PromQL: promql_condition" --> Trigger["Trigger condition: threshold=1 operator>="]
  Utils1["convertPromQLData: null checks"] -- "limit, xAxis, axis show" --> Chart["Chart config"]
  Utils2["colorPalette: null-safe min/max"] -- "process metric values" --> Chart

File Walkthrough

Relevant files

Bug fix

colorPalette.ts `Null-safe metric flattening for min/max` web/src/utils/dashboard/colorPalette.ts Filter out null metrics before flattening Preserve NaN filtering logic	+1/-0
convertPromQLData.ts `Null-safe PromQL series handling and axes` web/src/utils/dashboard/convertPromQLData.ts Guard result access with null checks Safely compute total and limited series Safely build x-axis timestamps Robust axisLine show condition	+16/-9

Enhancement

AddAlert.vue `Reliable threshold application for alerts` web/src/components/alerts/AddAlert.vue Use executed query when available Inject HAVING for SQL thresholds Add promql_condition for PromQL thresholds Set trigger threshold to 1 (exists check)	+62/-22
PanelSchemaRenderer.vue `Provide context for alert threshold creation` web/src/components/dashboards/PanelSchemaRenderer.vue Determine executedQuery from metadata Infer Y-axis column for SQL thresholds Pass yAxisColumn and executedQuery to alert Support both builder and raw SQL	+60/-0

github-actions · 2025-10-07T09:01:54Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 Security concerns SQL injection: Directly concatenating threshold, operator, and yAxis column into the SQL HAVING clause can allow injection if any component is user-influenced (e.g., alias/column from UI or metadata). Sanitize/whitelist operators, validate/quote identifiers (yAxisColumn), ensure threshold is a number, and prefer parameterized queries where possible.
⚡ Recommended focus areas for review Possible Issue SQL HAVING injection/placement logic may corrupt queries: naive string replace of 'having' and appending HAVING without ensuring presence of GROUP BY can yield invalid SQL or duplicate/incorrect clauses. Validate for GROUP BY, handle existing HAVING by appending 'AND ' after existing clause rather than replacing the keyword, and consider parameterization/quoting for identifiers and numbers. const sourceQuery = panelData.executedQuery \|\| query.query; if (sourceQuery) { let sqlQuery = sourceQuery; // If threshold is provided and we have a SQL query with GROUP BY, // add a HAVING clause to filter the aggregated column if (panelData.threshold !== undefined && panelData.condition && panelData.yAxisColumn) { const threshold = panelData.threshold; const operator = panelData.condition === 'above' ? '>=' : '<='; const yAxisColumn = panelData.yAxisColumn; // Add HAVING clause to the SQL const sqlLower = sqlQuery.toLowerCase(); if (sqlLower.includes('having')) { // Already has HAVING clause, add our condition with AND sqlQuery = sqlQuery.replace(/having\s+/i, `HAVING ${yAxisColumn} ${operator} ${threshold} AND `); } else { // No HAVING clause yet, add it at the end sqlQuery = `${sqlQuery.trim()} HAVING ${yAxisColumn} ${operator} ${threshold}`; } Fragile Parsing Regex-based extraction of yAxis column from SQL may fail for complex queries (CTEs, nested selects, quoted identifiers, multiple aggregations). Consider safer derivation from query builder metadata or a lightweight SQL parser; at minimum, guard for undefined results and multiple matches. const aliasOrColumn = yField.alias \|\| yField.column; // Extract from SQL to get the exact case (including quotes if present) if (sqlQuery) { // Look for pattern: aggregation_func(...) as "alias" or aggregation_func(...) as alias const escapedAlias = aliasOrColumn.replace(/[.+?^${}()\|[\]\\]/g, '\\$&'); const regex = new RegExp(`\\s+as\\s+(["']?${escapedAlias}["']?)(?:\\s\|,\|\\)\|$)`, 'i'); const match = sqlQuery.match(regex); if (match && match[1]) { // Use the alias with quotes if they exist in SQL yAxisColumn = match[1]; } else { yAxisColumn = aliasOrColumn; } } else { yAxisColumn = aliasOrColumn; } } else if (clickedSeriesName && sqlQuery) { // Fallback: try to match the clicked series name in the SQL // First try exact match with the series name const regex = new RegExp(`\\s+as\\s+["']?(${clickedSeriesName.replace(/[.+?^${}()\|[\]\\]/g, '\\$&')})["']?(?:\\s\|,\|\\)\|$)`, 'i'); const match = sqlQuery.match(regex); if (match && match[1]) { yAxisColumn = match[1]; } else { // Last resort: extract any aggregation column from SQL (first one found) // Pattern: count(...) as alias, avg(...) as alias, etc. const aggRegex = /(?:count\|sum\|avg\|min\|max\|median)\s$[^)]+$\s+as\s+["']?([^"',\s)]+)["']?/i; const aggMatch = sqlQuery.match(aggRegex); if (aggMatch && aggMatch[1]) { yAxisColumn = aggMatch[1]; } else { yAxisColumn = clickedSeriesName; } Null Checks Incomplete* Iteration over result.values assumes it's always an array; earlier guards check only queryData.result. Add null-safety for result.values to avoid runtime errors when values is missing or null. if (queryData && queryData.result) { queryData.result.forEach((result: any) => result.values.forEach((value: any) => xAxisData.add(value[0])), ); } });

greptile-apps

Greptile Overview

Summary

This pull request fixes issues with alert threshold handling when creating alerts from dashboard panels. The changes implement comprehensive defensive programming practices and enhance the integration between dashboard visualizations and alerting functionality.

The core improvements focus on three main areas:

Null/Undefined Safety: Added robust null checks across data processing functions (getMetricMinMaxValue in colorPalette.ts and convertPromQLData function) to prevent runtime errors when processing metric data with missing or malformed results. These functions are critical for alert threshold calculations as they process query results that may be incomplete or timeout.
Dashboard-to-Alert Integration: Enhanced PanelSchemaRenderer.vue with new context menu functionality that allows users to right-click on dashboard panels to create alerts. The implementation includes sophisticated SQL query parsing to extract Y-axis column names for accurate threshold comparisons, supporting both query builder and raw SQL formats with regex-based column extraction.
Threshold Logic Implementation: Modified AddAlert.vue to properly handle threshold conditions when creating alerts from dashboard panels. The solution dynamically injects HAVING clauses into SQL queries for aggregation-based thresholds and configures PromQL conditions appropriately. The alert trigger system is configured to fire when any row is returned (threshold=1) since the actual value filtering happens within the query itself.

These changes work together to create a seamless workflow where users can visualize data in dashboards and create meaningful alerts based on the displayed metrics. The defensive programming approach ensures the system remains stable even when dealing with incomplete or failed query results, which is essential for production alert systems.

Important Files Changed

Changed Files

Filename	Score	Overview
web/src/utils/dashboard/colorPalette.ts	5/5	Added defensive filter to prevent null/undefined metrics from causing runtime errors in getMetricMinMaxValue function
web/src/utils/dashboard/convertPromQLData.ts	4/5	Added comprehensive null/undefined safety checks throughout convertPromQLData function to prevent runtime errors
web/src/components/dashboards/PanelSchemaRenderer.vue	4/5	Implemented alert creation functionality with context menu system and SQL query column extraction logic
web/src/components/alerts/AddAlert.vue	4/5	Enhanced threshold-based alert creation with dynamic SQL HAVING clause insertion and PromQL condition handling

Confidence score: 4/5

This PR addresses real production issues with alert threshold handling and includes necessary defensive programming practices
Score reflects the complexity of SQL query manipulation and potential edge cases in different query formats that may not be fully covered
Pay close attention to the SQL query modification logic in AddAlert.vue, particularly the HAVING clause insertion patterns

Sequence Diagram

sequenceDiagram
    participant User
    participant PanelSchemaRenderer
    participant AlertContextMenu
    participant AddAlert
    participant AlertsService
    participant ScheduledAlert
    participant ValidationUtils
    participant PayloadUtils

    User->>PanelSchemaRenderer: "Right-click on chart data point"
    PanelSchemaRenderer->>PanelSchemaRenderer: "onChartContextMenu(event)"
    PanelSchemaRenderer->>AlertContextMenu: "Show context menu with threshold options"
    
    User->>AlertContextMenu: "Select condition (above/below) and threshold value"
    AlertContextMenu->>PanelSchemaRenderer: "handleCreateAlert(selection)"
    
    PanelSchemaRenderer->>PanelSchemaRenderer: "Prepare panel data with threshold info"
    Note over PanelSchemaRenderer: "Extract query, queryType, yAxisColumn, executedQuery"
    
    PanelSchemaRenderer->>AddAlert: "Navigate to alert creation with panelData"
    
    AddAlert->>AddAlert: "loadPanelDataIfPresent()"
    Note over AddAlert: "Parse panelData from URL query"
    
    alt SQL Query with Aggregation
        AddAlert->>AddAlert: "Set aggregation.having clause with threshold"
        Note over AddAlert: "having.value = threshold, having.operator = condition"
    else PromQL Query  
        AddAlert->>AddAlert: "Set promql_condition with threshold"
        Note over AddAlert: "promql_condition.value = threshold"
    end
    
    AddAlert->>AddAlert: "Set trigger_condition.threshold = 1"
    Note over AddAlert: "Fire alert when ANY row is returned"
    
    User->>AddAlert: "Fill additional alert details"
    User->>AddAlert: "Submit alert"
    
    AddAlert->>AddAlert: "onSubmit()"
    AddAlert->>AddAlert: "validateFormAndNavigateToErrorField()"
    
    alt Scheduled Alert
        AddAlert->>ScheduledAlert: "Validate scheduled alert fields"
        ScheduledAlert-->>AddAlert: "Validation result"
    end
    
    AddAlert->>ValidationUtils: "validateInputs(payload)"
    ValidationUtils-->>AddAlert: "Validation result"
    
    AddAlert->>PayloadUtils: "getAlertPayload()"
    PayloadUtils-->>AddAlert: "Formatted payload"
    
    AddAlert->>AddAlert: "transformFEToBE(conditions)"
    Note over AddAlert: "Transform frontend format to backend format"
    
    alt Update Alert
        AddAlert->>AlertsService: "update_by_alert_id(payload)"
    else Create Alert
        AddAlert->>AlertsService: "create_by_alert_id(payload)"
    end
    
    AlertsService-->>AddAlert: "Success/Error response"
    AddAlert->>User: "Show success/error notification"

_{4 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps · 2025-10-07T09:02:07Z

web/src/components/alerts/AddAlert.vue

+ if (sqlLower.includes('having')) {
+ // Already has HAVING clause, add our condition with AND
+ sqlQuery = sqlQuery.replace(/having\s+/i, `HAVING ${yAxisColumn} ${operator} ${threshold} AND `);
+                  } else {
+ // No HAVING clause yet, add it at the end
+ sqlQuery = `${sqlQuery.trim()} HAVING ${yAxisColumn} ${operator} ${threshold}`;
+                  }


logic: String interpolation in SQL could be vulnerable to injection. Consider using parameterized queries or proper escaping for yAxisColumn, operator, and threshold values.

Context Used: Context from dashboard - Avoid directly interpolating values into SQL queries; instead, generate parameter placeholders and b... (source)

Prompt To Fix With AI

This is a comment left during a code review. Path: web/src/components/alerts/AddAlert.vue Line: 1118:1124 Comment: **logic:** String interpolation in SQL could be vulnerable to injection. Consider using parameterized queries or proper escaping for `yAxisColumn`, `operator`, and `threshold` values. **Context Used:** Context from `dashboard` - Avoid directly interpolating values into SQL queries; instead, generate parameter placeholders and b... ([source](https://app.greptile.com/review/custom-context?memory=5078ddce-9cb0-4860-a7aa-e4a273809929)) How can I resolve this? If you propose a fix, please make it concise.

greptile-apps · 2025-10-07T09:02:08Z

web/src/components/alerts/AddAlert.vue

+ const sqlLower = sqlQuery.toLowerCase();
+ if (sqlLower.includes('having')) {
+ // Already has HAVING clause, add our condition with AND
+ sqlQuery = sqlQuery.replace(/having\s+/i, `HAVING ${yAxisColumn} ${operator} ${threshold} AND `);


style: The regex replacement could be fragile - consider using a proper SQL parser for more robust HAVING clause insertion.

Prompt To Fix With AI

This is a comment left during a code review. Path: web/src/components/alerts/AddAlert.vue Line: 1120:1120 Comment: **style:** The regex replacement could be fragile - consider using a proper SQL parser for more robust HAVING clause insertion. How can I resolve this? If you propose a fix, please make it concise.

github-actions · 2025-10-07T09:02:32Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Avoid mutable limit side-effects Mutating `limitSeries` within map causes later entries to be truncated based on prior items, which may lead to negative limits and inconsistent results. Compute remaining capacity once and stop slicing when capacity reaches zero to avoid unexpected behavior. web/src/utils/dashboard/convertPromQLData.ts [110-115] +let remaining = limitSeries; const limitedSearchQueryData = searchQueryData.map((queryData: any) => { - if (!queryData \|\| !queryData.result) { - return queryData; + if (!queryData \|\| !queryData.result \|\| remaining <= 0) { + return { ...queryData, result: queryData?.result ? [] : queryData?.result }; } - const remainingSeries = queryData.result.slice(0, limitSeries); - limitSeries = limitSeries - remainingSeries.length; - return { - ...queryData, - result: remainingSeries, + const sliceCount = Math.max(0, Math.min(remaining, queryData.result.length)); + const remainingSeries = queryData.result.slice(0, sliceCount); + remaining -= remainingSeries.length; + return { ...queryData, result: remainingSeries }; +}); Suggestion importance[1-10]: 8 __ Why: Highlights a real bug from mutating `limitSeries` inside `map`, proposing a clear fix that prevents negative/erroneous limits and ensures deterministic truncation across queries.	Medium
Possible issue	Safely append HAVING condition The current replace injects the condition immediately after the HAVING token, potentially corrupting existing HAVING expressions and losing their content. Instead, append with "AND" after the existing HAVING clause, and ensure a preceding GROUP BY exists to avoid invalid SQL. This prevents malformed queries when a HAVING already exists. web/src/components/alerts/AddAlert.vue [1118-1124] -if (sqlLower.includes('having')) { - // Already has HAVING clause, add our condition with AND - sqlQuery = sqlQuery.replace(/having\s+/i, `HAVING ${yAxisColumn} ${operator} ${threshold} AND `); -} else { - // No HAVING clause yet, add it at the end - sqlQuery = `${sqlQuery.trim()} HAVING ${yAxisColumn} ${operator} ${threshold}`; -} +const hasGroupBy = /\bgroup\s+by\b/i.test(sqlQuery); +if (sqlLower.includes('having') && hasGroupBy) { + // Append condition after existing HAVING expression + sqlQuery = sqlQuery.replace(/(\bhaving\b\s)(.+)$/i, (_m, prefix, clause) => { + const trimmed = clause.trim().replace(/;$/, ''); + return `${prefix}${trimmed} AND ${yAxisColumn} ${operator} ${threshold}`; + }); +} else if (hasGroupBy) { + sqlQuery = `${sqlQuery.replace(/;+\s$/,'').trim()} HAVING ${yAxisColumn} ${operator} ${threshold}`; +} // else: skip injecting HAVING if no GROUP BY Suggestion importance[1-10]: 7 __ Why: Correctly notes that replacing "having " with a prefixed condition can corrupt existing clauses and proposes a safer append preserving prior conditions and trimming semicolons, though skipping injection without GROUP BY may alter intended behavior in edge cases.	Medium
Security	Validate numeric threshold input Validate that `threshold` is a finite number before embedding it into SQL to avoid SQL errors or injection via stringy values. Coerce to Number and bail if invalid; also quote-identify `yAxisColumn` only when already quoted to prevent breaking identifiers unintentionally. web/src/components/alerts/AddAlert.vue [1114-1117] if (panelData.threshold !== undefined && panelData.condition && panelData.yAxisColumn) { - const threshold = panelData.threshold; - const operator = panelData.condition === 'above' ? '>=' : '<='; - const yAxisColumn = panelData.yAxisColumn; - ... + const thresholdNum = Number(panelData.threshold); + if (Number.isFinite(thresholdNum)) { + const operator = panelData.condition === 'above' ? '>=' : '<='; + // Respect existing quotes; otherwise use as-is + const yAxisColumn = /^["'`].*["'`]$/.test(panelData.yAxisColumn) ? panelData.yAxisColumn : panelData.yAxisColumn; + ... + // use thresholdNum below when building SQL + } } Suggestion importance[1-10]: 6 __ Why: Validating `threshold` as a finite number prevents malformed SQL and runtime issues; however, the identifier handling is effectively a no-op and does not improve quoting, slightly reducing impact.	Low

web/src/utils/alerts/alertSqlUtils.spec.ts

oasisk requested a review from ktx-kirtan October 7, 2025 09:00

github-actions bot added ☢️ Bug Something isn't working Review effort 3/5 labels Oct 7, 2025

greptile-apps bot reviewed Oct 7, 2025

View reviewed changes

oasisk force-pushed the fix-alert-threashold-fixes branch from 560cafd to cb30bef Compare October 7, 2025 09:50

github-advanced-security bot found potential problems Oct 7, 2025

View reviewed changes

web/src/utils/alerts/alertSqlUtils.spec.ts Fixed Show fixed Hide fixed

oasisk force-pushed the fix-alert-threashold-fixes branch 2 times, most recently from 4c45014 to bb3f041 Compare October 8, 2025 03:27

oasisk added 4 commits October 8, 2025 17:31

fix: alert threshold fixes

7589958

fix for complex queries

7227a34

codeql issue fix

bf8fad4

CTE

c0604a7

oasisk force-pushed the fix-alert-threashold-fixes branch from bb3f041 to c0604a7 Compare October 8, 2025 12:01

ktx-abhay approved these changes Oct 8, 2025

View reviewed changes

ktx-abhay merged commit e3f8cb8 into main Oct 8, 2025
31 checks passed

ktx-abhay deleted the fix-alert-threashold-fixes branch October 8, 2025 12:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: alert threshold fixes #8752

fix: alert threshold fixes #8752

Uh oh!

oasisk commented Oct 7, 2025 •

edited by github-actions bot

Loading

Uh oh!

github-actions bot commented Oct 7, 2025

Uh oh!

greptile-apps bot left a comment

Uh oh!

greptile-apps bot Oct 7, 2025

Uh oh!

greptile-apps bot Oct 7, 2025

Uh oh!

github-actions bot commented Oct 7, 2025

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fix: alert threshold fixes #8752

fix: alert threshold fixes #8752

Uh oh!

Conversation

oasisk commented Oct 7, 2025 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Type

Description

Diagram Walkthrough

File Walkthrough

Uh oh!

github-actions bot commented Oct 7, 2025

PR Reviewer Guide 🔍

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Greptile Overview

Summary

Important Files Changed

Confidence score: 4/5

Sequence Diagram

Uh oh!

greptile-apps bot Oct 7, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Oct 7, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Oct 7, 2025

PR Code Suggestions ✨

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

oasisk commented Oct 7, 2025 •

edited by github-actions bot

Loading