ref(workflow_engine): Add logging for process_workflows #103190
Conversation
github-actions
bot
added
the
Scope: Backend
|
|
||
| # This dictionary stores the data for WorkflowNotProcessable | ||
| # and is eventually unpacked to pass into the frozen dataclass | ||
| # TODO -- should this be a workflow event context data instead? |
There was a problem hiding this comment.
Curious if anyone has any thoughts on this; should this extra_data dict just be the WorkflowEventContextData?
There was a problem hiding this comment.
I think this is all evaluation context that makes sense for this method primarily, and arguably for all outcomes.
My inclination is to have a WorkflowEvaluationContext (..Result?) dataclass with the fields we want, populate that, and return it at the end.
Like the dict, just a bit clearer and more type-y.
To the extent we want a different return for firing and not-firing, I think we could do that with a variant within WER.. I don't think we currently have data differences between the two cases, so it may just be a bool to make "we actually triggered" explicit and not require knowledge of what action counts mean.
There was a problem hiding this comment.
ah, i was more referring to the already existing data type WorkflowEventContextData -- but i don't think we're using that anywhere other than to set it. i'll go through and remove all that code in another PR, i'm hoping that this and a couple other changes will remove any need for it.
for this PR, i'll update this extra_data dict to be WorkflowEvaluationData or something along those lines so it's a little clearer what's up.
| TODO - further refactor the `process_actions` portion, to improve handling / logging. | ||
| """ | ||
| from sentry.workflow_engine.processors.workflow import process_workflows |
There was a problem hiding this comment.
this import is probably unnecessary.
There was a problem hiding this comment.
😅 those pesky file refactors. thanks for the catch!
|
|
||
| # This dictionary stores the data for WorkflowNotProcessable | ||
| # and is eventually unpacked to pass into the frozen dataclass | ||
| # TODO -- should this be a workflow event context data instead? |
There was a problem hiding this comment.
I think this is all evaluation context that makes sense for this method primarily, and arguably for all outcomes.
My inclination is to have a WorkflowEvaluationContext (..Result?) dataclass with the fields we want, populate that, and return it at the end.
Like the dict, just a bit clearer and more type-y.
To the extent we want a different return for firing and not-firing, I think we could do that with a variant within WER.. I don't think we currently have data differences between the two cases, so it may just be a bool to make "we actually triggered" explicit and not require knowledge of what action counts mean.
| """ | ||
| from sentry.workflow_engine.processors.workflow import process_workflows | ||
|
|
||
| evaluation = process_workflows(batch_client, event_data, event_start_time, detector) |
There was a problem hiding this comment.
if we define a def log_to(logger: Logger, event: str) on the return type, this function can be replaced with a one-line addition to the task code.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #103190 +/- ##
===========================================
- Coverage 80.66% 80.66% -0.01%
===========================================
Files 9241 9242 +1
Lines 393680 393855 +175
Branches 25055 25055
===========================================
+ Hits 317578 317707 +129
- Misses 75640 75686 +46
Partials 462 462 |
…low us to have a deterministic result from the evaluation, and simplified logging outside of it.
… TODOs where action refactoring can happen in the near future.
saponifi3d
force-pushed
the
jcallender/aci/debugging-log
branch
from
697cecd to
be27816
Compare
| workflow_env: Environment | None = None | ||
|
|
||
|
|
||
| @dataclass |
There was a problem hiding this comment.
🤔 should we freeze these classes for safety / remake them or is it better for us to just modify during processing... (ui person in me tells me that mutating state is bad 🤣)
…ently being used, rather than optimistically will be used. and use the 'msg' field
)" This reverts commit 49b7c13.
|
PR reverted: 238b51a |
)" This reverts commit 49b7c13. Co-authored-by: saponifi3d <[email protected]>
Description
Started the work to improve logging and discoverability in the stack for investigating issues. Created the
WorkflowNotProcessabledataclass as a way to store / structure the result and have an improved consistent return value from the method. This way, we'll know that the processing stopped at one point, why, and all the information gathered so far about the evaluation.Out of scope for now:
process_actionstuff i left comments on; i'd like to makeprocess_workflowsto be side-effect free in the future; this is the first step in getting there and covers the requirements to debug the customer cases.