Skip to content

feat(alerts): Allow arithmetic in alert validation#113105

Merged
wmak merged 1 commit intomasterfrom
wmak/feat/alert-validation-allow-mertics
Apr 15, 2026
Merged

feat(alerts): Allow arithmetic in alert validation#113105
wmak merged 1 commit intomasterfrom
wmak/feat/alert-validation-allow-mertics

Conversation

@wmak
Copy link
Copy Markdown
Member

@wmak wmak commented Apr 15, 2026

  • This updates the validators to allow arithmetic
  • Unsure how to test the actual arithmetic works but this no longer errors haha

- This updates the validators to allow arithmetic
- Unsure how to test the actual arithmetic works but this no longer
  errors haha
@wmak wmak requested review from a team as code owners April 15, 2026 20:20
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Apr 15, 2026
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
Copy link
Copy Markdown
Contributor

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_arithmetic function (line 28) can raise ArithmeticParseError, ArithmeticValidationError, or MaxOperatorError when parsing malformed equations. At line 1909, parse_arithmetic(strip_equation(aggregate)) is called without handling these exceptions. The caller in snuba_query_validator.py only catches InvalidSearchQuery, so arithmetic parse failures will propagate as 500 internal server errors instead of 400 validation errors. A user submitting equation|5 + + 5 or equation|5 / 0 would 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-6
  • src/sentry/search/eap/trace_metrics/validator.py:63-63

Identified by Warden sentry-backend-bugs · 7ZJ-VR5

or allow_eap
)
if is_equation(aggregate):
_, _, terms = parse_arithmetic(strip_equation(aggregate))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Equation validation skips field validation by only checking functions

The parse_arithmetic function returns (result, fields, functions) but the code only unpacks the third element (functions) into terms, completely ignoring the second element (fields). This means equations containing raw fields (e.g., equation|transaction.duration / 1000) will have an empty terms list and return True without validation. Similarly, mixed equations like equation|count(project) + transaction.duration will only validate the function, not the field.

Verification

Read 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
_, _, terms = parse_arithmetic(strip_equation(aggregate))
_, fields, functions = parse_arithmetic(strip_equation(aggregate))
terms = fields + functions

Identified by Warden sentry-backend-bugs · KKV-XLH

Comment on lines +1908 to +1909
if is_equation(aggregate):
_, _, terms = parse_arithmetic(strip_equation(aggregate))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: parse_arithmetic can raise ArithmeticParseError/ArithmeticValidationError which are not InvalidSearchQuery, causing uncaught exceptions in callers.
Severity: MEDIUM

Suggested Fix

Wrap the parse_arithmetic call in a try/except that catches ArithmeticError (or its subclasses) and either returns False or raises InvalidSearchQuery so the caller's existing error handling can process it. For example: try: _, _, terms = parse_arithmetic(strip_equation(aggregate)) except ArithmeticError: return False.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: src/sentry/incidents/logic.py#L1908-L1909

Potential issue: When `check_aggregate_column_support` receives a malformed equation
(e.g., `equation|+++`), `parse_arithmetic` raises `ArithmeticParseError` (a subclass of
`ArithmeticError`). The caller in `snuba_query_validator.py` (`_validate_aggregate`)
only catches `InvalidSearchQuery`, not `ArithmeticError`. Since `ArithmeticError` and
`InvalidSearchQuery` are unrelated exception hierarchies, the error propagates uncaught,
resulting in a 500 server error instead of a proper validation error response. A user
submitting a malformed equation string in an alert rule creation/update request would
trigger this crash.

Did we get this right? 👍 / 👎 to inform future reviews.

Comment on lines +62 to +63
if is_equation(aggregate):
_, _, terms = parse_arithmetic(strip_equation(aggregate))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: parse_arithmetic can raise ArithmeticError outside the try/except InvalidSearchQuery block, causing unhandled 500 errors.
Severity: MEDIUM

Suggested Fix

Wrap the parse_arithmetic call in a try/except that catches ArithmeticError and converts it to a serializers.ValidationError. For example: try: _, _, terms = parse_arithmetic(strip_equation(aggregate)) except ArithmeticError as e: raise serializers.ValidationError({"aggregate": f"Invalid equation: {e}"}).

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: src/sentry/search/eap/trace_metrics/validator.py#L62-L63

Potential issue: In `validate_trace_metrics_aggregate`, the `parse_arithmetic` call at
line 63 is outside the `try/except InvalidSearchQuery` block (which starts at line 68).
If a user submits a malformed equation string, `parse_arithmetic` raises
`ArithmeticParseError` or `ArithmeticValidationError`, both subclasses of
`ArithmeticError` (not `InvalidSearchQuery`). This exception propagates uncaught,
resulting in a 500 server error instead of a proper `serializers.ValidationError`
response. This is a realistic scenario when users input invalid arithmetic expressions
in trace metrics alert configurations.

Did we get this right? 👍 / 👎 to inform future reviews.

Copy link
Copy Markdown
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 3 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit c7050ec. Configure here.

or allow_eap
)
if is_equation(aggregate):
_, _, terms = parse_arithmetic(strip_equation(aggregate))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unhandled ArithmeticError causes 500 on malformed equations

High Severity

parse_arithmetic can raise ArithmeticParseError, ArithmeticValidationError, or MaxOperatorError (all inheriting from ArithmeticError(Exception)). Neither check_aggregate_column_support nor validate_trace_metrics_aggregate catches these. The caller in snuba_query_validator.py only catches InvalidSearchQuery, which is a completely separate exception hierarchy. A malformed equation string would result in an unhandled 500 error instead of a proper validation error.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit c7050ec. Configure here.

if is_equation(aggregate):
_, _, terms = parse_arithmetic(strip_equation(aggregate))
else:
terms = [aggregate]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Equation validation skips non-function terms entirely

Medium Severity

parse_arithmetic returns (result, fields, functions). The unpacking _, _, terms only captures the functions list (third element), discarding the fields list. Equations containing only field-based terms or purely numeric expressions (e.g., equation|5 * 2) produce an empty terms list, causing the validation loop to be skipped entirely and allowing invalid aggregates to pass.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit c7050ec. Configure here.

if is_equation(aggregate):
_, _, terms = parse_arithmetic(strip_equation(aggregate))
else:
terms = [aggregate]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Equation strings crash in translate_aggregate_field after passing validation

High Severity

check_aggregate_column_support now accepts equation strings in the non-trace-metrics path, but the immediately subsequent call to translate_aggregate_field in the validator was not updated to handle them. translate_aggregate_field passes the raw equation string to get_column_from_aggregate, which calls resolve_field on it — that raises InvalidSearchQuery because the equation string contains invalid field characters. This exception is thrown outside the try/except InvalidSearchQuery block in _validate_aggregate, resulting in an unhandled crash.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit c7050ec. Configure here.

@wmak wmak merged commit e641e79 into master Apr 15, 2026
61 checks passed
@wmak wmak deleted the wmak/feat/alert-validation-allow-mertics branch April 15, 2026 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants