chore(aci): setup single processing flag for issue alerts in workflow engine #95178
Conversation
c41fe6c to
5b0885c
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
089a71d to
86920a9
Compare
kcons
left a comment
There was a problem hiding this comment.
This looks functionally correct as far as I can see, but I think we're probably doing it wrong. I'm not sure if we have the tools to do it right, though.
There's an argument that our flags should be shallowly behaviorally based, so or logic involving two flags is usually wrong; the flag should already encode that OR. And I guess that pushes the complexity to maintaining the YAML, which has some added benefit in that we could write tests to ensure that the YAML groups we define are coherent.
Another thought here is that if our flags are somewhat related, we should be exposing a sentry.workflow_engine.gating module or something that provides the interface we'd want (eg if alert_execution(org).enables(Processing.Workflow): or whatever) and ensures that all the correct flags are honored, so that we can bake in an test the proper combining of flags and callsites just ask the right question. That's sort of a runtime version of the other idea, but with better ergonomics and type safety, and this seems a step in that direction.
My larger point here is that the gating stuff seems one of our higher risk things, and it's already kind of confusing, so we should take every step toward more complexity as a chance to seriously consider our alternatives.
| manager.add("organizations:workflow-engine-process-metric-issue-workflows", OrganizationFeature, FeatureHandlerStrategy.FLAGPOLE, api_expose=False) | ||
| # Enable workflow engine for issue alerts | ||
| manager.add("organizations:workflow-engine-process-workflows", OrganizationFeature, FeatureHandlerStrategy.FLAGPOLE, api_expose=False) | ||
| # Enable single processing through workflow engine for issue alerts |
There was a problem hiding this comment.
I'd love a better name here, but I realize it's tricky given that consistency with existing naming is valuable.
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #95178 +/- ##
==========================================
- Coverage 76.42% 76.35% -0.08%
==========================================
Files 10509 10511 +2
Lines 606347 606410 +63
Branches 23728 23728
==========================================
- Hits 463419 463007 -412
+ Misses 142633 141509 -1124
- Partials 295 1894 +1599 |
Add a new feature flag to toggle single processing of issue alerts through workflow engine.
We currently have 2 feature flags: 1 that controls dual processing, and 1 that controls where the notifications fire from (the current system or workflow engine).
The new flag will act as an override of the two flags such that only a single system will process and notify about events. If the new flag is disabled, then we will revert to the dual processing flag logic.