Skip to content

feat(aci): Allow equations in span metric monitors behind a flag - #130742

Open
sentry-junior[bot] wants to merge 9 commits into
masterfrom
feat/aci/span-monitor-equations
Open

sentry-junior[bot] wants to merge 9 commits into
masterfrom
feat/aci/span-monitor-equations

Conversation

@sentry-junior

@sentry-junior sentry-junior Bot commented Nov 11, 2026 •

Copy link
Copy Markdown
Contributor

Creating a span metric monitor with an equation aggregate fails with a 400:

{"dataSources":{"aggregate":["Invalid Metric: equation|sum(a) / sum(b) is not a valid function"]}}

Spans Explore has supported equations since mid-2025, and Application Metrics monitors already accept them (#113105). For span monitors, _validate_aggregate sends the whole equation|… string to translate_aggregate_field, which can only handle a single function. So ratio monitors such as sum(gen_ai.usage.cache_read.input_tokens) / sum(gen_ai.usage.input_tokens) can't be created.

With the new organizations:spans-monitor-equations flag (FlagPole, api_expose=True), span equations get their own validation path:

  • The equation is parsed against SPAN_DEFINITIONS. It must contain aggregate functions and no bare fields, because the subscription only sends aggregate expressions to Snuba.
  • Every term has to be a function span monitors already accept (EAP_MONITOR_FUNCTIONS["spans"]). For example, count_sample() and count_if(...) are still rejected.
  • The whole equation is resolved with the span SearchResolver, which catches bad argument types such as sum(span.op).
  • The aggregate is stored unchanged in SnubaQuery.aggregate, the same equation|<expr> format Application Metrics uses.

Without the flag, span equations still fail, but with Span monitors do not support equations. instead of the "not a valid function" message.

Nothing downstream needed changing. PerformanceSpansEAPRpcEntitySubscription.build_rpc_request already turns a span equation into one formula expression, with an extrapolation mode on each aggregation. That is the shape Snuba's subscription validation requires. A new entity subscription test checks this, and the detector create test confirms the Snuba subscription gets created.

Out of scope:

  • Monitor form editor for span equations. See feat(aci): Equation editor for span metric monitors behind a flag #130741
  • Logs monitors, which still accept single functions only.
  • Empty windows. get_aggregation_value_helper still turns an empty result into 0, so a "below X" ratio monitor fires on a window with no traffic. Ratio equations probably want to skip those updates; that's left as a follow-up.

via @mrduncan.

--

View Session [Sentry]

Span metric monitors rejected equation aggregates with
`equation|... is not a valid function` because the validator ran the
whole equation through translate_aggregate_field. With the
organizations:spans-monitor-equations flag, span equations are now
parsed against the span definitions, every term must be a function that
span monitors already accept, and the equation must resolve. The
subscription path already sends an equation to Snuba as one formula
expression.

Without the flag the request still fails, now with a clear message.

Co-Authored-By: Matt Duncan <14761+mrduncan@users.noreply.github.com>
@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label Nov 11, 2026
Span equations are still behind a feature flag, so the public API docs should not advertise them yet.
Span equations are still behind a feature flag, so the public API schema should not accept them yet.
The span equation check imported its allowlist from the API schema module, which only documents monitor aggregates. Read EAP_FUNCTIONS from incidents.logic instead, so validation no longer depends on a docs module.
Assert exact messages for equations without functions, mixed functions and fields, and unparseable equations, and check that logs equations are still rejected.
Match the trace metrics layout by moving the span equation checks to search/eap/spans/validator.py. The feature flag check stays in SnubaQueryValidator because it needs the organization from the validator context.
Bad number arguments such as apdex(span.duration,foo) raised ValueError,
and opportunity_score raised because resolving it runs a query that needs
an organization. Both surfaced as 500s. Catch ValueError and pass the
organization into the resolver.
@mrduncan
mrduncan marked this pull request as ready for review November 12, 2026 00:37
@mrduncan
mrduncan requested review from a team as code owners November 12, 2026 00:37

@cursor cursor Bot left a comment

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.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

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 57ba0b5. Configure here.

…ject

Resolving formulas such as opportunity_score runs a Snuba query. Pass the
monitor's project instead of an empty project list so that query is
scoped correctly.

@saponifi3d saponifi3d left a comment

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.

lgtm! might be some cleanup for the tests if you wanted (paramterizing them might make the diff a little smaller).

Fold the accepted and rejected span equation cases into subTest tables,
as suggested in review. Same cases, less repetition.

Co-Authored-By: Matt Duncan <14761+mrduncan@users.noreply.github.com>
@sentry-junior

sentry-junior Bot commented Nov 12, 2026

Copy link
Copy Markdown
Contributor Author

@saponifi3d Thanks for the suggestion. I folded the span equation validator cases into subTest tables in 4b4b5f9 (TestCase classes can't use pytest.mark.parametrize). The cases are the same, and the PR's test diff is 61 lines smaller (227 → 166).

This branch has not been deployed

No deployments
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