-
Notifications
You must be signed in to change notification settings - Fork 79
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
H-1323: Add /workers
page, update flow run routes
#4479
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4479 +/- ##
==========================================
- Coverage 21.52% 21.23% -0.29%
==========================================
Files 451 454 +3
Lines 14953 15156 +203
Branches 2216 2267 +51
==========================================
Hits 3218 3218
- Misses 11694 11897 +203
Partials 41 41 β View full report in Codecov by Sentry. |
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.
Thanks Ciaran, these changes look good to me π
throw new Error(errorMessage, { | ||
cause: { | ||
code: StatusCode.FailedPrecondition, | ||
message: errorMessage, | ||
contents: [{ flow }], | ||
}, | ||
}); | ||
throw new Error(errorMessage); |
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.
Maybe we can discuss this internally rather than in a thread, but these outputs have been useful for me in the past when debugging errors in flows - but it makes sense to remove them if they're leading to the flow not being failed correctly.
I suppose I can use the getFlowRuns
resolver in the meantime.
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.
I can actually restore these, I thought it was causing temporal to not fail but it's actually because I wasn't using their special ApplicationError (added in the follow up goals PR)
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.
I did this in the next PR, b211df2
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.
Gotcha, thanks!
β¦er request less data
4869b15
to
9c9cbf5
Compare
π What is the purpose of this PR?
The PR introduces a
/workers
page which lists all active worker runs. These may be 'goals' which are research-driven flows (the/goals
page to be introduced in the next PR), or other flows.It also updates the routes for viewing:
/@[namespace]/flows/[id]
/@[namespace]/workers/[id]
These routes have essentially the same UI for now so they share a component. I've also updated the context providers so that we poll for all flow runs with fewer details (that doesn't require the event history) on a slower schedule, and the single selected flow run more frequently.
π Related links
π What does this change?
As well as the main changes above:
cause
fromrun-workflow-run.ts
because the cause causes Temporal to not fail the workflow properlypersistFlow
activity to use the providedwebId
instead of always the userPre-Merge Checklist π
π’ Has this modified a publishable library?
This PR:
π Does this require a change to the docs?
The changes in this PR:
πΈοΈ Does this require a change to the Turbo Graph?
The changes in this PR:
π‘ What tests cover this?