feat(workflows): Make thresholds in prune_old_fire_history options#111575
feat(workflows): Make thresholds in prune_old_fire_history options#111575
Conversation
github-actions
bot
added
the
Scope: Backend
saponifi3d
left a comment
saponifi3d
left a comment
There was a problem hiding this comment.
lgtm, really like that we can ramp this as we go!
| @override_options( | ||
| { | ||
| "workflow_engine.fire_history_cleanup.batch_size": 10, | ||
| "workflow_engine.fire_history_cleanup.time_limit_seconds": 5.0, |
There was a problem hiding this comment.
we should probably also have a test or modify this to override the time_limit_seconds as well.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| mock_metrics.incr.assert_called_once() | ||
| assert mock_metrics.incr.call_args.kwargs["amount"] >= 2 | ||
|
|
||
| @patch("sentry.workflow_engine.tasks.cleanup.FIRE_HISTORY_BATCH_SIZE", 10) |
There was a problem hiding this comment.
Test missing time_limit_seconds override is fragile
Medium Severity
test_multiple_batches (and test_options_honored) only override batch_size but not time_limit_seconds. These tests rely on the default time_limit_seconds being large enough to complete all batches. Since the whole point of this PR is to make these values tweakable, if the default is ever lowered (e.g., to 0, which the doc comment says "prevents any batches from running"), these tests will silently break. The test_time_bounded_leaves_remaining_rows test correctly overrides both options — these tests need the same treatment.
Additional Locations (1)
2 participants
We plan on tweaking these values as necessary, might as well make them possible to tweak very quickly.