-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
feat(alerts): Allow arithmetic in alert validation #113105
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -6,6 +6,7 @@ | |||||||
| from copy import deepcopy | ||||||||
| from dataclasses import dataclass, replace | ||||||||
| from datetime import datetime, timedelta, timezone | ||||||||
| from re import Match | ||||||||
| from typing import Any, TypedDict | ||||||||
| from uuid import UUID, uuid4 | ||||||||
|
|
||||||||
|
|
@@ -24,6 +25,7 @@ | |||||||
| from sentry.db.models import Model | ||||||||
| from sentry.db.models.manager.base_query_set import BaseQuerySet | ||||||||
| from sentry.deletions.models.scheduleddeletion import CellScheduledDeletion | ||||||||
| from sentry.discover.arithmetic import is_equation, parse_arithmetic, strip_equation | ||||||||
|
Check warning on line 28 in src/sentry/incidents/logic.py
|
||||||||
| from sentry.incidents import tasks | ||||||||
| from sentry.incidents.events import IncidentCreatedEvent, IncidentStatusUpdatedEvent | ||||||||
| from sentry.incidents.models.alert_rule import ( | ||||||||
|
|
@@ -1848,12 +1850,15 @@ | |||||||
|
|
||||||||
|
|
||||||||
| def get_column_from_aggregate( | ||||||||
| aggregate: str, allow_mri: bool, allow_eap: bool = False | ||||||||
| aggregate: str, | ||||||||
| allow_mri: bool, | ||||||||
| allow_eap: bool = False, | ||||||||
| match: Match[str] | None = None, | ||||||||
| ) -> str | None: | ||||||||
| # These functions exist as SnQLFunction definitions and are not supported in the older | ||||||||
| # logic for resolving functions. We parse these using `fields.is_function`, otherwise | ||||||||
| # they will fail using the old resolve_field logic. | ||||||||
| match = is_function(aggregate) | ||||||||
| match = is_function(aggregate) if match is None else match | ||||||||
| if match and ( | ||||||||
| match.group("function") in SPANS_METRICS_FUNCTIONS | ||||||||
| or match.group("function") in METRICS_LAYER_UNSUPPORTED_TRANSACTION_METRICS_FUNCTIONS | ||||||||
|
|
@@ -1900,21 +1905,29 @@ | |||||||
| aggregate: str, allow_mri: bool = False, allow_eap: bool = False | ||||||||
| ) -> bool: | ||||||||
| # TODO(ddm): remove `allow_mri` once the experimental feature flag is removed. | ||||||||
| column = get_column_from_aggregate(aggregate, allow_mri, allow_eap) | ||||||||
| match = is_function(aggregate) | ||||||||
| function = match.group("function") if match else None | ||||||||
| return ( | ||||||||
| column is None | ||||||||
| or is_measurement(column) | ||||||||
| or column in SUPPORTED_COLUMNS | ||||||||
| or column in TRANSLATABLE_COLUMNS | ||||||||
| or (is_mri(column) and allow_mri) | ||||||||
| or ( | ||||||||
| isinstance(function, str) | ||||||||
| and column in INSIGHTS_FUNCTION_VALID_ARGS_MAP.get(function, []) | ||||||||
| ) | ||||||||
| or allow_eap | ||||||||
| ) | ||||||||
| if is_equation(aggregate): | ||||||||
| _, _, terms = parse_arithmetic(strip_equation(aggregate)) | ||||||||
|
Check warning on line 1909 in src/sentry/incidents/logic.py
|
||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Equation validation skips field validation by only checking functions The VerificationRead src/sentry/discover/arithmetic.py to confirm parse_arithmetic returns tuple[Operation|float|str, list[str], list[str]] where second element is fields and third is functions (lines 338-363). Verified that ArithmeticVisitor tracks fields and functions separately (visit_field_value adds to self.fields, visit_function_value adds to self.functions). Confirmed field_allowlist includes raw fields like 'transaction.duration' that can be used directly in equations. Suggested fix: Unpack both fields and functions from parse_arithmetic and iterate over both lists, or combine them into a single terms list.
Suggested change
Identified by Warden
Comment on lines
+1908
to
+1909
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Suggested FixWrap the Prompt for AI AgentDid we get this right? 👍 / 👎 to inform future reviews.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unhandled ArithmeticError causes 500 on malformed equationsHigh Severity
Additional Locations (1)Reviewed by Cursor Bugbot for commit c7050ec. Configure here. |
||||||||
| else: | ||||||||
| terms = [aggregate] | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Equation validation skips non-function terms entirelyMedium Severity
Additional Locations (1)Reviewed by Cursor Bugbot for commit c7050ec. Configure here.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Equation strings crash in
|
||||||||
|
|
||||||||
| for term in terms: | ||||||||
| match = is_function(term) | ||||||||
| column = get_column_from_aggregate(term, allow_mri, allow_eap, match) | ||||||||
| function = match.group("function") if match else None | ||||||||
| if not ( | ||||||||
| column is None | ||||||||
| or is_measurement(column) | ||||||||
| or column in SUPPORTED_COLUMNS | ||||||||
| or column in TRANSLATABLE_COLUMNS | ||||||||
| or (is_mri(column) and allow_mri) | ||||||||
| or ( | ||||||||
| isinstance(function, str) | ||||||||
| and column in INSIGHTS_FUNCTION_VALID_ARGS_MAP.get(function, []) | ||||||||
| ) | ||||||||
| or allow_eap | ||||||||
| ): | ||||||||
| return False | ||||||||
| return True | ||||||||
|
|
||||||||
|
|
||||||||
| def translate_aggregate_field( | ||||||||
|
|
||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ | |
|
|
||
| from rest_framework import serializers | ||
|
|
||
| from sentry.discover.arithmetic import is_equation, parse_arithmetic, strip_equation | ||
|
Check warning on line 6 in src/sentry/search/eap/trace_metrics/validator.py
|
||
| from sentry.exceptions import InvalidSearchQuery | ||
| from sentry.search.eap.resolver import SearchResolver | ||
| from sentry.search.eap.trace_metrics.config import TraceMetricsSearchResolverConfig | ||
|
|
@@ -58,14 +59,20 @@ | |
| Raises: | ||
| serializers.ValidationError: If the aggregate is invalid | ||
| """ | ||
| try: | ||
| trace_metric = extract_trace_metric_from_aggregate(aggregate) | ||
| if trace_metric is None: | ||
| raise InvalidSearchQuery( | ||
| f"Trace metrics aggregate {aggregate} must specify metric name, type, and unit" | ||
| if is_equation(aggregate): | ||
| _, _, terms = parse_arithmetic(strip_equation(aggregate)) | ||
|
Check warning on line 63 in src/sentry/search/eap/trace_metrics/validator.py
|
||
|
Comment on lines
+62
to
+63
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Suggested FixWrap the Prompt for AI AgentDid we get this right? 👍 / 👎 to inform future reviews. |
||
| else: | ||
| terms = [aggregate] | ||
|
|
||
| for term in terms: | ||
| try: | ||
| trace_metric = extract_trace_metric_from_aggregate(term) | ||
| if trace_metric is None: | ||
| raise InvalidSearchQuery( | ||
| f"Trace metrics aggregate {term} must specify metric name, type, and unit" | ||
| ) | ||
| except InvalidSearchQuery as e: | ||
| logger.exception(f"Invalid trace metrics aggregate: {term} {e}") | ||
| raise serializers.ValidationError( | ||
| {"aggregate": f"Invalid trace metrics aggregate: {term}"} | ||
| ) | ||
| except InvalidSearchQuery as e: | ||
| logger.exception("Invalid trace metrics aggregate: %s %s", aggregate, e) | ||
| raise serializers.ValidationError( | ||
| {"aggregate": f"Invalid trace metrics aggregate: {aggregate}"} | ||
| ) | ||


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ArithmeticError exceptions from parse_arithmetic not caught in alert validation
The newly imported
parse_arithmeticfunction (line 28) can raiseArithmeticParseError,ArithmeticValidationError, orMaxOperatorErrorwhen parsing malformed equations. At line 1909,parse_arithmetic(strip_equation(aggregate))is called without handling these exceptions. The caller insnuba_query_validator.pyonly catchesInvalidSearchQuery, so arithmetic parse failures will propagate as 500 internal server errors instead of 400 validation errors. A user submittingequation|5 + + 5orequation|5 / 0would trigger this.Verification
Read src/sentry/discover/arithmetic.py to confirm ArithmeticError hierarchy (lines 20-33). Read src/sentry/incidents/logic.py to find usage at line 1909. Read src/sentry/snuba/snuba_query_validator.py lines 285-295 to confirm only InvalidSearchQuery is caught. ArithmeticError is not a subclass of InvalidSearchQuery.
Also found at 2 additional locations
src/sentry/search/eap/trace_metrics/validator.py:6-6src/sentry/search/eap/trace_metrics/validator.py:63-63Identified by Warden
sentry-backend-bugs·7ZJ-VR5