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): Generate Workflow names based on actions #87256

Merged
merged 7 commits into from
Mar 19, 2025
Merged

Conversation

ceorourke
Copy link
Member

We want to name workflows something meaningful and descriptive that isn't the same thing as the detector name. This PR builds the workflow name based on the action text so the name will be like "Email colleen@sentry.io".

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Mar 18, 2025
from sentry.models.team import Team
from sentry.users.services.user import RpcUser

MAX_CHARS = 248 # (256 minus space for '...(+3_)')
Copy link
Member Author

Choose a reason for hiding this comment

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

do we have a limit for Actions on a Workflow? if it's possible to be higher than 99 actions I'll have to change this

Copy link
Member

Choose a reason for hiding this comment

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

you could do something like (99+) if there are more?

Copy link
Member Author

Choose a reason for hiding this comment

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

oh I like that idea 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

There's no limit on the number of actions / workflows. I'd honestly just cap it to the first 3 actions and append ... though, beyond that and it won't be super useful.

MAX_CHARS = 248 # (256 minus space for '...(+3_)')


def get_action_description(action: AlertRuleTriggerAction) -> str:
Copy link
Member Author

Choose a reason for hiding this comment

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

I based this off of https://github.com/getsentry/sentry/blob/master/src/sentry/incidents/endpoints/serializers/alert_rule_trigger_action.py#L9-L9 didn't seem worth it to refactor to use both since a) we'll be getting rid of the serializer and b) I changed all the text

Copy link
Member Author

Choose a reason for hiding this comment

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

Some of these read a little strangely like "PagerDuty to #my-team" but I was shortening it from "Send a {priority} PagerDuty notification to {target_display}", very open to other ideas for these! "Slack #michelle" rolls off the tongue a lot nicer than some of these

Copy link
Member

Choose a reason for hiding this comment

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

Notify {target_display} via {notification_type}? for something super generic you can keep reusing

"""
name = ""
triggers = AlertRuleTrigger.objects.filter(alert_rule_id=alert_rule.id).order_by("label")
include_label = False if triggers.count() == 1 else True
Copy link
Member Author

Choose a reason for hiding this comment

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

This makes it so that if there is only one trigger we don't bother with adding the prefix like "Critical - Slack #hb-bets" since there is only one trigger it feels a bit redundant

@ceorourke ceorourke marked this pull request as ready for review March 18, 2025 19:16
@ceorourke ceorourke requested a review from a team as a code owner March 18, 2025 19:16
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 🎉

Copy link

codecov bot commented Mar 19, 2025

❌ 3 Tests Failed:

Tests completed Failed Passed Skipped
24423 3 24420 299
View the top 3 failed test(s) by shortest run time
tests.sentry.api.endpoints.test_organization_spans_fields_stats.OrganizationSpansFieldsStatsEndpointTest::test_filter_query
Stack Traces | 3.92s run time
#x1B[1m#x1B[.../api/endpoints/test_organization_spans_fields_stats.py#x1B[0m:157: in test_filter_query
    assert distributions[0]["attributeName"] == "broswer"
#x1B[1m#x1B[31mE   AssertionError: assert 'sentry.segment_id' == 'broswer'#x1B[0m
#x1B[1m#x1B[31mE     #x1B[0m
#x1B[1m#x1B[31mE     - broswer#x1B[0m
#x1B[1m#x1B[31mE     + sentry.segment_id#x1B[0m
tests.sentry.api.endpoints.test_organization_spans_fields_stats.OrganizationSpansFieldsStatsEndpointTest::test_max_buckets
Stack Traces | 4.4s run time
#x1B[1m#x1B[.../api/endpoints/test_organization_spans_fields_stats.py#x1B[0m:117: in test_max_buckets
    assert len(distributions) == max_buckets - 1
#x1B[1m#x1B[31mE   AssertionError: assert 1 == (3 - 1)#x1B[0m
#x1B[1m#x1B[31mE    +  where 1 = len([{'label': 'foo', 'value': 3.0}])#x1B[0m
tests.sentry.api.endpoints.test_organization_spans_fields_stats.OrganizationSpansFieldsStatsEndpointTest::test_distribution_values
Stack Traces | 5.43s run time
#x1B[1m#x1B[.../api/endpoints/test_organization_spans_fields_stats.py#x1B[0m:134: in test_distribution_values
    assert distributions[0]["attributeName"] == "broswer"
#x1B[1m#x1B[31mE   AssertionError: assert 'sentry.transaction' == 'broswer'#x1B[0m
#x1B[1m#x1B[31mE     #x1B[0m
#x1B[1m#x1B[31mE     - broswer#x1B[0m
#x1B[1m#x1B[31mE     + sentry.transaction#x1B[0m

To view more test analytics, go to the Test Analytics Dashboard
📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

@ceorourke ceorourke merged commit eca4183 into master Mar 19, 2025
48 checks passed
@ceorourke ceorourke deleted the ceorourke/ACI-209 branch March 19, 2025 17:34
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

3 participants