-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
chore(aci): manually add spans for delayed workflow processing #91908
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generally lgtm 🎉
project = fetch_project(project_id) | ||
if not project: | ||
return | ||
|
||
workflow_event_dcg_data = fetch_group_to_event_data(project_id, Workflow, batch_key) | ||
workflow_event_dcg_data = fetch_group_to_event_data(project_id, Workflow, batch_key) | ||
|
||
# Get mappings from DataConditionGroups to other info | ||
dcg_to_groups, trigger_group_to_dcg_model = get_dcg_group_workflow_detector_data( | ||
workflow_event_dcg_data | ||
) | ||
dcg_to_workflow = trigger_group_to_dcg_model[DataConditionHandler.Group.WORKFLOW_TRIGGER].copy() | ||
dcg_to_workflow.update(trigger_group_to_dcg_model[DataConditionHandler.Group.ACTION_FILTER]) | ||
# Get mappings from DataConditionGroups to other info | ||
dcg_to_groups, trigger_group_to_dcg_model = get_dcg_group_workflow_detector_data( | ||
workflow_event_dcg_data | ||
) | ||
dcg_to_workflow = trigger_group_to_dcg_model[ | ||
DataConditionHandler.Group.WORKFLOW_TRIGGER | ||
].copy() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: should this be moved into a method to make it a little easier to read? perhaps naming it prepare_data
? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm yeah i can
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
err well there's the early return if there's no project
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the return type starts looking a little crazy if i put it all into a single function, so just going to keep it separate for now
Codecov ReportAttention: Patch coverage is
|
Files with missing lines | Patch % | Lines |
---|---|---|
...try/workflow_engine/processors/delayed_workflow.py | 4.16% | 23 Missing |
Additional details and impacted files
@@ Coverage Diff @@
## master #91908 +/- ##
==========================================
- Coverage 87.63% 87.62% -0.01%
==========================================
Files 10356 10356
Lines 587133 587141 +8
Branches 22585 22585
==========================================
+ Hits 514506 514510 +4
- Misses 72199 72203 +4
Partials 428 428
* master: (58 commits) link: cleanup link (#91687) ref: create project_id index for organizationonboardingtask (#91918) storybook: smaller last edited (#91875) issues: fix chonk stacktrace alignment (#91891) alert: drop custom alert (#91892) insights: fix bar height (#91895) ref(span-buffer): Move max-memory-percentage to right CLI (#91924) ref(js): Factor button functionality (#91763) tests(resolve_groups): Clean up the tests (#91779) ref(span-buffer): Add backpressure (#91707) fix(nextjs-insights): project id is not passed to explore link (#91920) fix(crons): Floor seconds / microsecond on recorded dateClock (#91890) fix(uptime): Fix bug with the uptime_checks dataset in the events endpoint (#91824) ref: add state-only migration to reflect existing indexes in prod (#91901) ref: remove unnecssary metaclass (#91906) fix(stats): use data category title name (#91913) feat(issues): Add success messages to some actions (#91899) test(taskworker): Lower exec time (#91907) chore(aci): manually add spans for delayed workflow processing (#91908) chore(aci): remove uses of WorkflowFireHistory rollout columns (#91904) ...
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Like for delayed rule processing #91764, we should track the performance of delayed workflow processing. We need to manually add spans though.