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

🔧 chore(noa): invoke all ticketing actions via the issue alert handlers #87372

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

Conversation

iamrajjoshi
Copy link
Member

we need all of ticketing actions to flow to the issue alert handler.

this pr defines a new TicketingActionHandler ABC to handle invoking the issue alert handlers directly and updates the corresponding implementing classes to use the new ABC.

the changes are covered by this, so we should be good

@iamrajjoshi iamrajjoshi self-assigned this Mar 19, 2025
@iamrajjoshi iamrajjoshi requested a review from a team as a code owner March 19, 2025 01:33
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Mar 19, 2025
Copy link

codecov bot commented Mar 19, 2025

Codecov Report

Attention: Patch coverage is 90.47619% with 2 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...low_engine/handlers/action/notification/handler.py 84.61% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #87372      +/-   ##
==========================================
+ Coverage   85.15%   87.76%   +2.60%     
==========================================
  Files        9875     9891      +16     
  Lines      559911   561363    +1452     
  Branches    22081    22081              
==========================================
+ Hits       476809   492694   +15885     
+ Misses      82697    68264   -14433     
  Partials      405      405              

@iamrajjoshi iamrajjoshi requested a review from Copilot March 19, 2025 05:25
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors ticketing actions by funneling them through the issue alert handler and introduces a new TicketingActionHandler abstract base class.

  • Implements execute_via_metric_alert_handler to centralize notification handling for ticketing actions.
  • Introduces TicketingActionHandler with a dedicated configuration schema for ticketing actions.
  • Updates GitHub, GitHub Enterprise, Jira, Jira Server, and Azure Devops action handlers to extend TicketingActionHandler.
Comments suppressed due to low confidence (2)

src/sentry/workflow_engine/handlers/action/notification/handler.py:102

  • [nitpick] The configuration schema for 'target_identifier' is defined as type ["null"]. Verify if this is intentional or if a more flexible type (e.g., ["string", "null"]) is expected.
"type": ["null"],

src/sentry/workflow_engine/handlers/action/notification/handler.py:109

  • Ensure that ActionTarget is an iterable since using the splat operator (*) requires it to be unpackable.
"enum": [*ActionTarget],

Base automatically changed from raj/aci/action-group to master March 19, 2025 18:10
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.

lgtm - but i think we should figure out a way to make execute_via_metric_alert_handler calling the issue alert registry a little less confusing.

},
}

data_schema = {}
Copy link
Contributor

Choose a reason for hiding this comment

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

is there any data that should be included to ticketing actions by default?

Copy link
Member Author

Choose a reason for hiding this comment

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

oop i forgot to add this, just did.

Comment on lines 57 to 62
def execute_via_metric_alert_handler(job: WorkflowJob, action: Action, detector: Detector) -> None:
# TODO(iamrajjoshi): Implement this, it should be used for the ticketing actions
pass
"""
This exists so that all ticketing actions can use the same handler as issue alerts since that's the only way we can
ensure that the same thread is used for the notification action.
"""
IssueAlertRegistryInvoker.handle_workflow_action(job, action, detector)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit confusing -- metric_alert_handler calling into the IssueAlertRegistry 🤔 should this be via the issue_alert_handler?

Copy link
Member Author

Choose a reason for hiding this comment

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

ope i used the wrong function name, it should be execute_via_issue_alert_handler

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.

None yet

2 participants