Skip to content

chore(ACI): Add warning banner to old alerts#111344

Merged
ceorourke merged 6 commits intomasterfrom
ceorourke/warning-rule-banner
Mar 26, 2026
Merged

chore(ACI): Add warning banner to old alerts#111344
ceorourke merged 6 commits intomasterfrom
ceorourke/warning-rule-banner

Conversation

@ceorourke
Copy link
Member

@ceorourke ceorourke commented Mar 23, 2026

Add a warning banner to the old issue alerts pages if the workflow engine serializers flag is enabled letting users know there could be parts of the alert we cannot render in the old UI if they've created or edited alerts or monitors in the new UI. For issue alerts this is filtered down to alerts where there are unsupported conditions or the new alert uses multiple if/then conditions which can't be shown in the old UI.

Screenshot 2026-03-25 at 2 22 33 PM Screenshot 2026-03-25 at 2 22 57 PM

@ceorourke ceorourke requested a review from a team as a code owner March 23, 2026 20:37
@github-actions github-actions bot added Scope: Frontend Automatically applied to PRs that change frontend components Scope: Backend Automatically applied to PRs that change backend components labels Mar 23, 2026
@github-actions
Copy link
Contributor

🚨 Warning: This pull request contains Frontend and Backend changes!

It's discouraged to make changes to Sentry's Frontend and Backend in a single pull request. The Frontend and Backend are not atomically deployed. If the changes are interdependent of each other, they must be separated into two pull requests and be made forward or backwards compatible, such that the Backend or Frontend can be safely deployed independently.

Have questions? Please ask in the #discuss-dev-infra channel.

export function WorkflowEngineAlert() {
const organization = useOrganization();

if (!organization.features.includes('workflow-engine-rule-serializers')) {
Copy link
Member Author

Choose a reason for hiding this comment

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

We've been releasing individual backwards compatible endpoints behind different flags but this is the main control one

<Alert.Container>
<Alert variant="warning">
{tct(
"Some aspects of alerts may not be visible if you've used the [link:new monitors and alerts UI].",
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'm open to wording changes here - usage of the new API could also result in this but I didn't necessarily want to push users to it in this view.

Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 we can probably assume they don't have access to the new UI here, otherwise we should be redirecting them to it.

Maybe we can just say "The legacy alerts UI does not support all features. If you've modified the alert using the new API, those settings will be used during evaluation but aren't fully reflected in the UI." -- or something along those lines, basically saying "hey - we are evaluating this the way you expected, but you're using an experimental API so it caused a mismatch."

manager.add("organizations:workflow-engine-redirect-opt-out", OrganizationFeature, FeatureHandlerStrategy.FLAGPOLE, api_expose=True)
# Use workflow engine serializers to return data for old rule / incident endpoints
manager.add("organizations:workflow-engine-rule-serializers", OrganizationFeature, FeatureHandlerStrategy.FLAGPOLE, api_expose=False)
manager.add("organizations:workflow-engine-rule-serializers", OrganizationFeature, FeatureHandlerStrategy.FLAGPOLE, api_expose=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.

I know we're supposed to decouple fe and be changes but given this flag isn't widely enabled I think it's ok.

)}
<Layout.Body>
<Layout.Main>
<WorkflowEngineAlert />
Copy link
Contributor

Choose a reason for hiding this comment

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

can we check the api results for a flag to see if we need to render the warning?

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 have unsupported conditions in the errors results of the response but there are other situations (like if they used an if/then block) that would take more effort to identify.

Copy link
Member Author

@ceorourke ceorourke Mar 23, 2026

Choose a reason for hiding this comment

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

Ok kinda went down a rabbithole here, I think for now what I'm going to do is add the if/then block condition to the errors response and conditionally render the banner if any of the known error cases are hit for issue alerts. For metric alerts we might have more proverbial fish to fry, but this is at least a starting point that covers the majority of cases in an already edge case problem set.

</Layout.Header>
<Layout.Body>
<Layout.Main>
<WorkflowEngineAlert />
Copy link
Contributor

Choose a reason for hiding this comment

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

same question about being able to read the rule value to determine if we need to render the banner or not.

Copy link
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix prepared a fix for the issue found in the latest run.

  • ✅ Fixed: Feature flag default=True enables serializers for all orgs
    • Removed the default=True override from organizations:workflow-engine-rule-serializers so orgs only use workflow serializer paths when explicitly enabled.

Create PR

Or push these changes by commenting:

@cursor push 442257f1f4
Preview (442257f1f4)
diff --git a/src/sentry/features/temporary.py b/src/sentry/features/temporary.py
--- a/src/sentry/features/temporary.py
+++ b/src/sentry/features/temporary.py
@@ -418,7 +418,7 @@
     # Disable redirects from alert rules to the new workflow_engine UI
     manager.add("organizations:workflow-engine-redirect-opt-out", OrganizationFeature, FeatureHandlerStrategy.FLAGPOLE, api_expose=True, default=True)
     # Use workflow engine serializers to return data for old rule / incident endpoints
-    manager.add("organizations:workflow-engine-rule-serializers", OrganizationFeature, FeatureHandlerStrategy.FLAGPOLE, api_expose=True, default=True)
+    manager.add("organizations:workflow-engine-rule-serializers", OrganizationFeature, FeatureHandlerStrategy.FLAGPOLE, api_expose=True)
     # Use workflow engine exclusively for ProjectRulesEndpoint.get results.
     # See src/sentry/workflow_engine/docs/legacy_backport.md for context.
     manager.add("organizations:workflow-engine-projectrulesendpoint-get", OrganizationFeature, FeatureHandlerStrategy.FLAGPOLE, api_expose=False)

This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.

@getsentry getsentry deleted a comment from github-actions bot Mar 25, 2026
@getsentry getsentry deleted a comment from github-actions bot Mar 25, 2026
ceorourke added a commit that referenced this pull request Mar 25, 2026
…workflow (#111378)

If multiple if/then blocks are used in a workflow we're only able to
render one of them in the legacy issue alerts UI. This PR adds an item
to the `errors` response so that we can render a warning on the front
end. See #111344 which will be
updated to read this error.
@ceorourke ceorourke requested a review from saponifi3d March 25, 2026 21:28
@ceorourke ceorourke force-pushed the ceorourke/warning-rule-banner branch from e206341 to 90429a7 Compare March 25, 2026 21:29
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, thanks for updating!

@github-actions
Copy link
Contributor

Backend Test Failures

Failures on 35a8556 in this run:

tests/sentry/taskworker/test_config.py::test_all_instrumented_tasks_registeredlog
tests/sentry/taskworker/test_config.py:120: in test_all_instrumented_tasks_registered
    raise AssertionError(
E   AssertionError: Found 1 module(s) with @instrumented_task that are NOT registered in TASKWORKER_IMPORTS.
E   These tasks will not be discovered by the taskworker in production!
E   
E   Missing modules:
E     - sentry.workflow_engine.tasks.cleanup
E   
E   Add these to TASKWORKER_IMPORTS in src/sentry/conf/server.py

@ceorourke ceorourke force-pushed the ceorourke/warning-rule-banner branch from 90429a7 to 292ea5d Compare March 25, 2026 22:02
@github-actions
Copy link
Contributor

Backend Test Failures

Failures on 673c700 in this run:

tests/sentry/taskworker/test_config.py::test_all_instrumented_tasks_registeredlog
tests/sentry/taskworker/test_config.py:120: in test_all_instrumented_tasks_registered
    raise AssertionError(
E   AssertionError: Found 1 module(s) with @instrumented_task that are NOT registered in TASKWORKER_IMPORTS.
E   These tasks will not be discovered by the taskworker in production!
E   
E   Missing modules:
E     - sentry.workflow_engine.tasks.cleanup
E   
E   Add these to TASKWORKER_IMPORTS in src/sentry/conf/server.py

@ceorourke ceorourke force-pushed the ceorourke/warning-rule-banner branch from 292ea5d to c99770e Compare March 25, 2026 23:05
@ceorourke ceorourke merged commit 4cf2eaf into master Mar 26, 2026
97 of 99 checks passed
@ceorourke ceorourke deleted the ceorourke/warning-rule-banner branch March 26, 2026 17:20
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 Scope: Frontend Automatically applied to PRs that change frontend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants