-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
fix(aci): set failure rate y-axis range based on seriesMax and threshold #105962
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
Conversation
static/app/views/detectors/hooks/useMetricDetectorThresholdSeries.tsx
Outdated
Show resolved
Hide resolved
|
@scttcper fixed, had to pass
|
| if (outputType !== 'percentage') { | ||
| return value; | ||
| } | ||
|
|
||
| // Session operations store as percentage (5 for 5%), needs conversion to 0-1 | ||
| if (isSessionPercentageOperation(aggregate)) { | ||
| return value / 100; | ||
| } | ||
|
|
||
| // Other operations like failure_rate store as decimal (0.05 for 5%), already in 0-1 scale | ||
| return value; |
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.
Bug: The normalizePercentageThreshold function doesn't divide failure_rate() thresholds by 100, but the UI shows a '%' suffix. This causes user-entered percentages to be saved as values 100x too large.
Severity: CRITICAL
🔍 Detailed Analysis
The normalizePercentageThreshold() function in useMetricDetectorThresholdSeries.tsx incorrectly handles thresholds for failure_rate(). While the UI displays a '%' suffix, prompting users to enter whole numbers (e.g., 5 for 5%), the function does not normalize this input by dividing by 100. It only performs this normalization for session-based operations. Consequently, a user-entered value like 5 is stored as 5.0 instead of the expected 0.05. Since the backend expects failure_rate() data on a 0-1 scale, this results in an alert threshold that is 100 times larger than intended (e.g., 500% instead of 5%), making it practically impossible for the alert to ever trigger.
💡 Suggested Fix
Update the normalizePercentageThreshold() function to also divide the threshold value by 100 for failure_rate(). This can be achieved by modifying the condition that checks for percentage operations to include failure_rate(), ensuring its value is correctly scaled from a user-entered percentage to a decimal representation (0-1) before being sent to the backend.
🤖 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:
static/app/views/detectors/hooks/useMetricDetectorThresholdSeries.tsx#L105-L117
Potential issue: The `normalizePercentageThreshold()` function in
`useMetricDetectorThresholdSeries.tsx` incorrectly handles thresholds for
`failure_rate()`. While the UI displays a '%' suffix, prompting users to enter whole
numbers (e.g., `5` for 5%), the function does not normalize this input by dividing by
100. It only performs this normalization for session-based operations. Consequently, a
user-entered value like `5` is stored as `5.0` instead of the expected `0.05`. Since the
backend expects `failure_rate()` data on a 0-1 scale, this results in an alert threshold
that is 100 times larger than intended (e.g., 500% instead of 5%), making it practically
impossible for the alert to ever trigger.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 8370020
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.
Need to remove the % in the UI, will do in a separate PR
| ) | ||
| .map(condition => ({ | ||
| value: Number(condition.comparison), | ||
| value: normalizePercentageThreshold(aggregate, Number(condition.comparison)), |
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.
Percent detection thresholds incorrectly normalized for session metrics
High Severity
The normalizePercentageThreshold function is applied to ALL thresholds in extractThresholdsFromConditions, including those for percent detection type. However, percent detection thresholds represent a percentage change (e.g., "5% higher than comparison"), not an absolute percentage value. For session percentage operations like crash_free_rate, the threshold (e.g., 5) gets incorrectly divided by 100 to become 0.05. This causes createPercentThresholdSeries to calculate the wrong multiplier (1 + 0.05/100 = 1.0005 instead of 1 + 5/100 = 1.05), making threshold lines nearly invisible. The series name also displays "0.05% Higher Threshold" instead of "5% Higher Threshold".
🔬 Verification Test
Why verification test was not possible: This is a React hook that requires the full Sentry frontend environment with Jest and React testing utilities. The bug is identified through static code analysis tracing the data flow:
extractThresholdsFromConditionsnow normalizes ALL thresholds vianormalizePercentageThreshold- For session ops like
crash_free_rate, a 5% threshold becomes 0.05 - In
percentdetection type,createPercentThresholdSeriescalculates multiplier as1 + thresholdPercentage / 100 - With normalized value:
1 + 0.05/100 = 1.0005(nearly no change) - Expected without normalization:
1 + 5/100 = 1.05(5% increase)
The old code applied division by 100 only in the static detection handling, not for percent detection.
Additional Locations (1)
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.
I don't think this is valid because we don't allow percent change for crash_free_rate 🧐
fixes a bug found while fixing #105962 where `failure_rate` takes 0.05 as 5%, but the UI does not reflect this fix: <img width="629" height="530" alt="Screenshot 2026-01-12 at 12 05 38 PM" src="https://github.com/user-attachments/assets/17913aa5-8d80-4ab6-8d3a-b51c62d9d007" /> note that `crash_free_rate` and other session operations take 5 for 5%: <img width="748" height="505" alt="Screenshot 2026-01-12 at 12 05 22 PM" src="https://github.com/user-attachments/assets/4387e20b-c8f4-4efd-ba3c-1efb5c0359a1" />


we were hardcoding the y-axis max to be 100%, which makes it difficult to see small percentages. this change sets the max to the max between the seriesMax and the specified threshold.
before

after
