feat(metrics): Add equation support to tracemetrics#112609
Merged
Conversation
Member
wmak
commented
Apr 9, 2026
wmak
commented
- This adds equation support to the tracemetrics dataset
- I'm really not sure what the extra_conditions are doing here, they add a top level condition on the metric details, but we're already adding them per-aggregate anyways? I'm just going to leave this alone (but add equation support) for now and move on
- This adds equation support to the tracemetrics dataset - I'm really not sure what the extra_conditions are doing here, they add a top level condition on the metric details, but we're already adding them per-aggregate anyways? I'm just going to leave this alone (but add equation support) for now and move on
github-actions
bot
added
the
Scope: Backend
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Comment on lines
+57
to
+60
| for equation in equations: | ||
| _, _, terms = parse_arithmetic(equation) | ||
| for term in terms: | ||
| columns.add(term) |
Contributor
There was a problem hiding this comment.
Bug: The parse_arithmetic function is called with a hardcoded allowlist that omits trace-metric-specific functions like per_second, causing validation to fail for valid equations.
Severity: HIGH
Suggested Fix
When calling parse_arithmetic in src/sentry/search/eap/trace_metrics/config.py, provide an extended function_allowlist that includes the trace-metric-specific functions (per_second, per_minute, per_second_if, per_minute_if) in addition to the default ones.
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/config.py#L57-L60
Potential issue: The code for handling equations in trace metrics calls
`parse_arithmetic` to validate and extract function names. However, the underlying
`ArithmeticVisitor` uses a hardcoded `function_allowlist` that does not include
trace-metric-specific functions like `per_second`, `per_minute`, `per_second_if`, and
`per_minute_if`. As a result, any attempt to use these functions in an equation will
cause an `ArithmeticValidationError`, preventing the feature from working as intended
for these specific, but essential, functions. The provided tests do not cover these
functions, only using `count_if` which is already in the allowlist.
Did we get this right? 👍 / 👎 to inform future reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.