-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
feat(triage signals): Seer orgs on new pricing need to have Github integration to use autofix #104477
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
base: master
Are you sure you want to change the base?
Conversation
|
@roaga I'm unsure if this is the right way to stop it in the UI. lmk what you think. |
roaga
left a comment
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 we're requiring code mappings, only github integration
Okay so that is already checked in the issue details UI right? So i can leave that unchanged. And in kickoff_seer_automation should I just use |
|
Actually |
@Mihir-Mavalankar code mappings are an extra thing people can configure but are an extra piece of work on top of a plain GitHub integration. If your goal is to stop Autofix from running automation on a project without connected repos, you should check that the project has either code mappings or repos connected via seer project preferences. To stop it in the UI, you need to do frontend work as Autofix is currently allowed to run without repo connections. It will currently only show a warning at the top. |
Okay so the backend check is fine since it checks for code mapping via |
if you're trying to block autofix runs without github integration, the existing check won't do that. The current function to get code mappings is just optional extra data we send to seer |
@roaga so is the only way to check that to call the project preference api in Seer and and check if there is a list of projects? |
| from sentry.seer.autofix.utils import has_project_connected_repos | ||
|
|
||
| if not has_project_connected_repos(group.organization.id, group.project.id): | ||
| return |
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: API failure after cache lock causes missed automation
The has_project_connected_repos check is placed after cache.add(automation_dispatch_cache_key, ...) on line 1688, which atomically blocks duplicate dispatches. If the Seer API call in has_project_connected_repos raises a SeerApiError (as the test test_raises_on_api_error confirms it can), the exception propagates but the cache key remains set for 5 minutes. This blocks any subsequent automation dispatch attempts for that group, even though no automation was actually dispatched. The repo check should happen before the cache lock to avoid this scenario.
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.
The risk is acceptable IMO. Keeping outside the lock means a lot more volume to the API endpoint which we want to avoid.
If it fails because of an API error automation will be re-tried after 5 minutes when new events come.
roaga
left a comment
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.
if the user doesn't have repos connected, automation is not running. Then let's say they set up the repos. Doesn't that mean we won't run automation for a whole hour until the cache expires? Is that acceptable?
Yes. There is a chance that they add repos in that hour but yes, till the cache expires yes. Otherwise we need to find a new way to check if projects have connected repos because calling an endpoint everytime is expensive. |
0695a4d to
6be5430
Compare
PR Details