Skip to content
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

fix(alerts): avoid race condition in anomaly detection requests #87815

Merged
merged 3 commits into from
Mar 25, 2025

Conversation

natemoo-re
Copy link
Member

Reported bug was that some requests to Seer from the UI did not include config.sensitivity and config.expected_seasonality. Because this was an intermittent, I suspected a race condition.

After digging into the logic more, I noticed one spot where fetchAnomalies() could potentially be called before the component's state changes had been synchronized.

The fix, as in other places, is to defer fetching until the state changes have been flushed by passing () => this.fetchAnomalies() as the second callback argument to this.setState.

I'm very eager to get rid of this class component and eliminate this class of bug 😄👍

Closes getsentry/seer#2141

@natemoo-re natemoo-re requested a review from a team as a code owner March 24, 2025 22:27
@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Mar 24, 2025
Copy link
Contributor

@saponifi3d saponifi3d left a comment

Choose a reason for hiding this comment

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

it'd be nice to also include a unit test for the error to ensure we avoid this specific case in the future.

@natemoo-re
Copy link
Member Author

Just went to add a test but hit some old bugs in the accessibility of our RadioGroup component that made it impossible to query the Anomaly Detection radio option...

// TODO(epurkhiser): There should be a `name` and `label` attribute in
// the options type to allow for the aria label to work correctly. For
// now we slap a `toString` on there, but it may sometimes return
// [object Object] if the name is a react node.

Will have to fix this another time.

@natemoo-re natemoo-re enabled auto-merge (squash) March 25, 2025 17:44
@natemoo-re natemoo-re merged commit 4388081 into master Mar 25, 2025
40 checks passed
@natemoo-re natemoo-re deleted the nm/ad-race-condition branch March 25, 2025 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: Frontend Automatically applied to PRs that change frontend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(dynamic alert threshold): Request From UI Missing Required Fields
2 participants