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

Add output to workflow run #7276

Merged
merged 4 commits into from
Sep 30, 2024
Merged

Add output to workflow run #7276

merged 4 commits into from
Sep 30, 2024

Conversation

thomtrp
Copy link
Contributor

@thomtrp thomtrp commented Sep 27, 2024

Example of output stored for following workflow:

Capture d’écran 2024-09-27 à 11 18 06

Output:

{"steps": [
  {"type": "CODE", "result": {"email": "[email protected]"}}, 
  {"type": "SEND_EMAIL", "result": {"success": true}}
]}

@thomtrp thomtrp marked this pull request as ready for review September 27, 2024 09:20
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This pull request implements output functionality for workflow runs, enhancing tracking and analysis capabilities for workflow executions.

  • Added 'output' field to WorkflowRunWorkspaceEntity in workflow-run.workspace-entity.ts to store execution results
  • Modified WorkflowExecutorWorkspaceService in workflow-executor.workspace-service.ts to track step-by-step execution output
  • Updated RunWorkflowJob in run-workflow.job.ts to handle and store execution output for both successful and failed runs
  • Enhanced WorkflowRunWorkspaceService in workflow-run.workspace-service.ts to persist detailed execution output when ending a workflow run
  • Added 'output' field ID to WORKFLOW_RUN_STANDARD_FIELD_IDS in standard-field-ids.ts for consistent field identification

5 file(s) reviewed, 4 comment(s)
Edit PR Review Bot Settings

Copy link
Contributor

@martmull martmull left a comment

Choose a reason for hiding this comment

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

LGTM, small comment though

Copy link
Contributor

@Devessier Devessier left a comment

Choose a reason for hiding this comment

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

Awesome job, @thomtrp, as usual! 😄

I wrote a few comments about some edge cases I think we should consider. I would prefer to log everything happening, it will help people debug their failing workflows.

Comment on lines 12 to 19
export type WorkflowExecutorOutput = {
steps: {
type: WorkflowActionType;
result: object | undefined;
error: object | undefined;
}[];
status: WorkflowRunStatus;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be great to store two more things per step:

  • The step id; it will be easier to connect the output with the step node on the front-end
  • The attemptCount; it would make it possible to store the output of all the attempts. Two attempts can fail for different reasons, and I think storing all of them would be worth it.

@@ -75,13 +102,11 @@ export class WorkflowExecutorWorkspaceService {
currentStepIndex,
steps,
payload,
output,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be interesting to also store the output of the failed attempts. See my comment where I suggest adding two more fields to WorkflowExecutorOutput.

const stepOutput = {
type: step.type,
result: result.result,
error: result.error,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should be careful when storing the raw error. Indeed, here, we store the error with the stack trace, showing the internal file system of the worker running the jobs. These errors will be displayed to the user, and it's usually considered unsafe to expose stack traces publicly.

@Devessier Devessier self-requested a review September 30, 2024 16:33
Copy link
Contributor

@Devessier Devessier left a comment

Choose a reason for hiding this comment

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

Awesome, @thomtrp!

As I told you in DM, I'm worried we lose the stack trace definitely, but I'm sure we'll find a way to bring it back securely later.

We can merge!

@thomtrp thomtrp merged commit ca027d6 into main Sep 30, 2024
5 of 6 checks passed
@thomtrp thomtrp deleted the tt-add-output-to-workflows branch September 30, 2024 16:45
harshit078 pushed a commit to harshit078/twenty that referenced this pull request Oct 14, 2024
Example of output stored for following workflow:

<img width="244" alt="Capture d’écran 2024-09-27 à 11 18 06"
src="https://github.com/user-attachments/assets/722bfa96-2dd1-41f7-ab87-d39584ac9efc">

Output:

```
{"steps": [
  {"type": "CODE", "result": {"email": "[email protected]"}}, 
  {"type": "SEND_EMAIL", "result": {"success": true}}
]}
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants