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

✨ feat(aci): invoke the correct legacy registry from the notification action #84245

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

iamrajjoshi
Copy link
Member

@iamrajjoshi iamrajjoshi commented Jan 29, 2025

we need the notification action to invoke the existing registries (issue alert or metric alert). we can do this by look at the detector type, since it is mapped to the grouptype slug

# maps to registry (sentry.issues.grouptype.registry) entries for GroupType.slug in sentry.issues.grouptype.GroupType
type = models.CharField(max_length=200)

so what we do is register each of the individual functions invoke_metric_alert_registry and invoke_issue_alert_registry to grouptype, so we can look at the detector type to invoke the correct function.

might be easier to review via https://github.com/getsentry/sentry/blob/raj/noa/invoke-correct-legacy-registry/src/sentry/workflow_engine/handlers/action/notification.py

https://www.notion.so/sentry/Alerts-Create-Issues-Notification-Action-NOA-1268b10e4b5d805b967ed2655c962db6#1268b10e4b5d812aaa59e13c96a04abd

@iamrajjoshi iamrajjoshi self-assigned this Jan 29, 2025
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jan 29, 2025
Copy link

codecov bot commented Jan 29, 2025

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
23707 1 23706 293
View the top 1 failed tests by shortest run time
tests.sentry.uptime.endpoints.test_organization_uptime_stats.OrganizationUptimeCheckIndexEndpointTest::test_too_many_periods
Stack Traces | 3.4s run time
#x1B[1m#x1B[.../uptime/endpoints/test_organization_uptime_stats.py#x1B[0m:99: in test_too_many_periods
    assert response.status_code == 400
#x1B[1m#x1B[31mE   assert 200 == 400#x1B[0m
#x1B[1m#x1B[31mE    +  where 200 = <Response status_code=200, "application/json">.status_code#x1B[0m

To view more test analytics, go to the Test Analytics Dashboard
📢 Thoughts on this report? Let us know!

from tests.sentry.workflow_engine.test_base import BaseWorkflowTest


class TestNotificationActionHandler(BaseWorkflowTest):
Copy link
Member Author

Choose a reason for hiding this comment

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

don't need test to ensure group_type exists since

def enforce_config_schema(sender, instance: Detector, **kwargs):
takes care of this

@iamrajjoshi iamrajjoshi marked this pull request as ready for review January 29, 2025 21:17
@iamrajjoshi iamrajjoshi requested a review from a team as a code owner January 29, 2025 21:17
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