Skip to content

feat(tracemetrics): Bypass metric field validation for equations#112479

Merged
narsaynorath merged 5 commits intomasterfrom
nar/feat/tracemetrics-bypass-metric-field-validation-for-equations
Apr 9, 2026
Merged

feat(tracemetrics): Bypass metric field validation for equations#112479
narsaynorath merged 5 commits intomasterfrom
nar/feat/tracemetrics-bypass-metric-field-validation-for-equations

Conversation

@narsaynorath
Copy link
Copy Markdown
Member

We enforce that there's a metric payload for saved queries, but when a user is plotting an equation, there can be multiple metrics involved, so this field no longer makes sense.

This PR relaxes the constraint so metric can be skipped if the user is querying an equation (i.e. we check if there's the equation| prefix)

@narsaynorath narsaynorath requested a review from a team as a code owner April 8, 2026 16:26
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Apr 8, 2026
@narsaynorath
Copy link
Copy Markdown
Member Author

narsaynorath commented Apr 8, 2026

There's a chance that, since we encode the metric name in the aggregate function now, this validation can go away, but for now I do not want to make that change because frontend code may be reliant on this field being filled out.

Post-GA we can circle back and see if this field is still necessary, but for now we do not need to do large changes to account for this.

Comment thread src/sentry/explore/endpoints/serializers.py Outdated
Comment thread tests/sentry/explore/endpoints/test_explore_saved_queries.py
Comment thread src/sentry/explore/endpoints/serializers.py Outdated
@github-actions

This comment was marked as outdated.

re-worded an error message
Comment thread src/sentry/explore/endpoints/serializers.py Outdated
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 1 potential issue.

Fix All in Cursor

Bugbot Autofix prepared a fix for the issue found in the latest run.

  • ✅ Fixed: all() on empty iterable bypasses metric validation
    • Fixed by collecting y_axes into a list and checking both that the list is non-empty AND that all items are equations, ensuring has_equations is only True when actual equation y-axes are present.

Create PR

Or push these changes by commenting:

@cursor push 1300763d88
Preview (1300763d88)
diff --git a/src/sentry/explore/endpoints/serializers.py b/src/sentry/explore/endpoints/serializers.py
--- a/src/sentry/explore/endpoints/serializers.py
+++ b/src/sentry/explore/endpoints/serializers.py
@@ -225,11 +225,12 @@
                     )
 
                 # the metrics field is only required for non-equation queries
-                has_equations = all(
-                    is_equation(y_axis)
+                y_axes = [
+                    y_axis
                     for aggregate_field in q.get("aggregateField") or []
                     for y_axis in aggregate_field.get("yAxes") or []
-                )
+                ]
+                has_equations = len(y_axes) > 0 and all(is_equation(y_axis) for y_axis in y_axes)
                 if data["dataset"] == "metrics" and not has_equations and "metric" not in q:
                     raise serializers.ValidationError(
                         "Metric field is required for non-equation queries on the metrics dataset"

This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 2156b77. Configure here.

Comment thread src/sentry/explore/endpoints/serializers.py Outdated
Comment thread src/sentry/explore/endpoints/serializers.py Outdated
@github-actions

This comment was marked as outdated.

raise serializers.ValidationError(
"Metric field is only allowed for metrics dataset"
)
if data["dataset"] == "metrics" and "metric" not in q:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we actually can get rid of metric constraint, have to check redash first to see if it'll break any old saved queries, but this is left over before we switched function syntax to count(..., metric tuple) since we had to discern count() meant it was for a particular metric.

We should update the test to ensure it's properly validating the function count() since it should be confirming all functions contain the metrics tuple if it's properly using search resolver

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Checked, the first non-internal saved query was avg(value,<metric name>, <type>,-) so we're fine.

Copy link
Copy Markdown
Member

@k-fish k-fish left a comment

Choose a reason for hiding this comment

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

Agreed we can defer this, make a ticket so we don't forget

@narsaynorath narsaynorath merged commit 94fe6cb into master Apr 9, 2026
56 checks passed
@narsaynorath narsaynorath deleted the nar/feat/tracemetrics-bypass-metric-field-validation-for-equations branch April 9, 2026 16:14
george-sentry pushed a commit that referenced this pull request Apr 9, 2026
…2479)

We enforce that there's a `metric` payload for saved queries, but when a
user is plotting an equation, there can be multiple `metric`s involved,
so this field no longer makes sense.

This PR relaxes the constraint so `metric` can be skipped if the user is
querying an equation (i.e. we check if there's the `equation|` prefix)
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