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(aci): add WorkflowGroupStatus model #83071

Closed
wants to merge 5 commits into from

Conversation

cathteng
Copy link
Member

@cathteng cathteng commented Jan 7, 2025

Add workflow engine equivalent of GroupRuleStatus to enforce the number of times a workflow can fire for a group in a period of time. This "period" is the frequency key in workflow.config.

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jan 7, 2025
Copy link
Contributor

github-actions bot commented Jan 7, 2025

This PR has a migration; here is the generated SQL for src/sentry/workflow_engine/migrations/0020_add_workflow_group_status_model.py ()

--
-- Create model WorkflowGroupStatus
--
CREATE TABLE "workflow_engine_workflowgroupstatus" ("id" bigint NOT NULL PRIMARY KEY GENERATED BY DEFAULT AS IDENTITY, "date_updated" timestamp with time zone NOT NULL, "date_added" timestamp with time zone NOT NULL, "status" smallint NOT NULL CHECK ("status" >= 0), "group_id" bigint NOT NULL, "workflow_id" bigint NOT NULL, CONSTRAINT "workflow_engine_uniq_workflow_group" UNIQUE ("workflow_id", "group_id"));
ALTER TABLE "workflow_engine_workflowgroupstatus" ADD CONSTRAINT "workflow_engine_work_group_id_b90bf29b_fk_sentry_gr" FOREIGN KEY ("group_id") REFERENCES "sentry_groupedmessage" ("id") DEFERRABLE INITIALLY DEFERRED NOT VALID;
ALTER TABLE "workflow_engine_workflowgroupstatus" VALIDATE CONSTRAINT "workflow_engine_work_group_id_b90bf29b_fk_sentry_gr";
ALTER TABLE "workflow_engine_workflowgroupstatus" ADD CONSTRAINT "workflow_engine_work_workflow_id_a719e7cf_fk_workflow_" FOREIGN KEY ("workflow_id") REFERENCES "workflow_engine_workflow" ("id") DEFERRABLE INITIALLY DEFERRED NOT VALID;
ALTER TABLE "workflow_engine_workflowgroupstatus" VALIDATE CONSTRAINT "workflow_engine_work_workflow_id_a719e7cf_fk_workflow_";
CREATE INDEX CONCURRENTLY "workflow_engine_workflowgroupstatus_group_id_b90bf29b" ON "workflow_engine_workflowgroupstatus" ("group_id");
CREATE INDEX CONCURRENTLY "workflow_engine_workflowgroupstatus_workflow_id_a719e7cf" ON "workflow_engine_workflowgroupstatus" ("workflow_id");

Copy link

codecov bot commented Jan 7, 2025

❌ 8 Tests Failed:

Tests completed Failed Passed Skipped
23435 8 23427 270
View the top 3 failed tests by shortest run time
tests.sentry.backup.test_exports.ScopingTests test_organization_export_scoping
Stack Traces | 22.8s run time
#x1B[1m#x1B[.../sentry/backup/test_exports.py#x1B[0m:261: in test_organization_export_scoping
    self.verify_model_inclusion(unencrypted, ExportScope.Organization)
#x1B[1m#x1B[.../sentry/backup/test_exports.py#x1B[0m:180: in verify_model_inclusion
    raise AssertionError(
#x1B[1m#x1B[31mE   AssertionError: The following models were not included in the export: ${<class 'sentry.workflow_engine.models.workflow_group_status.WorkflowGroupStatus'>}; this is despite it being included in at least one of the following relocation scopes: {<RelocationScope.User: 2>, <RelocationScope.Organization: 3>}#x1B[0m
tests.sentry.backup.test_exports.ScopingTests test_global_export_scoping
Stack Traces | 23.6s run time
#x1B[1m#x1B[.../sentry/backup/test_exports.py#x1B[0m:397: in test_global_export_scoping
    self.verify_model_inclusion(unencrypted, ExportScope.Global)
#x1B[1m#x1B[.../sentry/backup/test_exports.py#x1B[0m:180: in verify_model_inclusion
    raise AssertionError(
#x1B[1m#x1B[31mE   AssertionError: The following models were not included in the export: ${<class 'sentry.workflow_engine.models.workflow_group_status.WorkflowGroupStatus'>}; this is despite it being included in at least one of the following relocation scopes: {<RelocationScope.User: 2>, <RelocationScope.Global: 5>, <RelocationScope.Organization: 3>, <RelocationScope.Config: 4>}#x1B[0m
tests.sentry.backup.test_exports.ScopingTests::test_global_export_scoping
Stack Traces | 24.7s run time
#x1B[1m#x1B[.../sentry/backup/test_exports.py#x1B[0m:397: in test_global_export_scoping
    self.verify_model_inclusion(unencrypted, ExportScope.Global)
#x1B[1m#x1B[.../sentry/backup/test_exports.py#x1B[0m:180: in verify_model_inclusion
    raise AssertionError(
#x1B[1m#x1B[31mE   AssertionError: The following models were not included in the export: ${<class 'sentry.workflow_engine.models.workflow_group_status.WorkflowGroupStatus'>}; this is despite it being included in at least one of the following relocation scopes: {<RelocationScope.Organization: 3>, <RelocationScope.User: 2>, <RelocationScope.Global: 5>, <RelocationScope.Config: 4>}#x1B[0m

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

@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Jan 8, 2025
Copy link
Contributor

github-actions bot commented Jan 8, 2025

🚨 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.

@cathteng
Copy link
Member Author

cathteng commented Jan 8, 2025

i think i'm missing an updated test_clean_pks.pysnap but it doesn't get generated when i run SENTRY_SNAPSHOTS_WRITEBACK=1 pytest tests/sentry/backup/test_sanitize.py

@cathteng cathteng marked this pull request as ready for review January 8, 2025 18:19
@cathteng cathteng requested review from a team as code owners January 8, 2025 18:19


@region_silo_model
class WorkflowGroupStatus(DefaultFieldsModel):
Copy link
Member

Choose a reason for hiding this comment

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

Is the plan here to use date_updated as the equivalent of last_active?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, is that possible/wise? i believe you can manually update it with update()

Copy link
Member

Choose a reason for hiding this comment

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

I think it's ok... we'd only really be updating it in one case. Maybe just test locally (write a throwaway test?) to see if you can do the equivalent of

        updated = (
            GroupRuleStatus.objects.filter(id=status.id)
            .exclude(last_active__gt=freq_offset)
            .update(last_active=now)
        )

Copy link
Member

@wedamija wedamija left a comment

Choose a reason for hiding this comment

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

@saponifi3d We discussed possibly needing to keep the status by action instead of workflow - just wanted to make sure that didn't get missed

@saponifi3d
Copy link
Contributor

@saponifi3d We discussed possibly needing to keep the status by action instead of workflow - just wanted to make sure that didn't get missed

@wedamija - nah, didn't miss it, we're just kicking the can down the road on tracking the history of each action call. (we'll still emit a metric for SLOs etc though). In the end, we were able to support everything in the current design through open periods, but need this table to support the the frequency check.

@cathteng
Copy link
Member Author

cathteng commented Jan 8, 2025

moving to #83125 because making a new migration and generating snapshots is a headache in the same PR

@cathteng cathteng closed this Jan 8, 2025
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.

3 participants