Implement task duration page in react. #35863
Conversation
tirkarthi
requested review from
ryanahamilton,
ashb,
bbovenzi and
pierrejeambrun
as code owners
boring-cyborg
bot
added
area:UI
tirkarthi
force-pushed
the
task-duration-react
branch
from
5a890f6
to
4841239
Compare
There was a problem hiding this comment.
Nice work!
First off, I think we want to change how task/run selection works.
Right now there are 3 states:
- nothing is selected: show general information about the DAG
- a runId is selected: show information about the DAG run
- a runId and a taskId are selected: show information about that task instance
This view really needs a fourth state, where a taskId is selected, but no runId and then we can show a task across runs.
Also, before this is fully ready. We should delete the old Task Duration page and redirect links over to this page.
Can you please add here pointers if any over where to look for selection related code? I see I also had to rewrite most of the code that was simpler to satisfy typescript compiler and linter by using a class to model storing taskinstance, queued duration and run duration. Please let me know if there are better ways to write them as I have less experience in frontend. Thanks again for the review @bbovenzi . |
|
@bbovenzi Task row selection is added as suggested. On selecting a task row only the task duration tab is displayed with duration for the task across runs. I still feel the graph can be present with 50% of the panel height and is better. Please let me know if this needs to be changed. Thanks |
tirkarthi
force-pushed
the
task-duration-react
branch
from
fbccdd1
to
7f076d8
Compare
|
Updated screenshot after PR comments to show task duration when task row is clicked.
|
Nice! Some UI feedback:
|
|
Thanks @bbovenzi for the feedback. I will try to fix them in next commit. I had few questions :
Does this mean just having x-axis to indicate that just says "run_id" like "duration(seconds)" or the actual value of run_id beneath each bar which might be make it little harder to read on large num_runs. |
X-Axis labels: No, just the formatted time of the datetime we are ordering the tasks by. RunId can stay in the tooltip I also think we could bring in some Task details to this page. Basic stuff like the operator that comes from |
Done. I had instance as x-axis which leads to formatting as [object object] in the UI. I rely on execution date as using dataIntervalEnd leads to cases where there is a scheduled run and manual run with same dataIntervalEnd that causes them to form a single bar though they are two different task instances. I feel execution date which is mostly referred as logical date is unique enough. Please share your thoughts on this and the date format.
Done. Used opacity as 0.6 as done in gantt chart
There is hand pointer on hovering over the bar in task instance. Do you want the task duration and grid view in sync? I had it in the previous version to show it on hover which needs setting task_id and run_id query params and accidental hover causes browser history to be filled with values that makes clicking on back little difficult. Since now task duration is a separate tab only with task_id in query param I have changed it to on click so that when user is interested in filtering by task instance they can click on it to move to task instance detail view. Reverting to older method means hover by itself moves to task instance detail and hides task duration.
Done Screenshot after addressing comments :
|
I can pick it up as a follow-up PR since this might involve more changes and iterations. |
|
I'm also happy to contribute to this branch if that would be helpful. |
|
@bbovenzi Please feel free to push changes. I tried to fix conflicts since recent xcom details page also had changes to tab number and panels in same file so please verify if the merge is okay. I can cherry pick changes after merge to test this in our non production environment. My availability might be limited due to holiday season PTO and will try my best to address changes. Thanks for your review on this. |
bbovenzi
force-pushed
the
task-duration-react
branch
from
199cb10
to
6273168
Compare
|
Just rebased and made some updates. Most notably, support for task groups and moving Task Duration into a general Task Details tab. 
|
|
Thanks @bbovenzi . This looks good to have it under details tab instead of a separate tab. I noticed one discrepancy when there is a running task instance between the order of grid view and the task duration chart. In the grid view the task instance is the third one but in the chart the task instance is the last one. I guess there is some sorting logic. I noticed below comment in this PR maybe there is some logic that is missing. The order remains incorrect even after the task finishes.
|
|
Sorting will now match the ordering we receive from the grid_data endpoint. Clicking on a task name will select the row. But I increased the click area to expand/collapse groups or you can double click a task name to expand/collapse. Also, I just realized that it looks quite cool if you select a ton of dag runs at once (eg 365 below) 
|
|
Thanks @bbovenzi . This looks great. As a tip echarts also provides dataZoom through which large graphs can be drilled down to plot values between two intervals. https://echarts.apache.org/examples/en/editor.html?c=bar-large While testing I am also seeing below error when num_runs count is greater than dag runs along with a new task added to the dag after few dag runs are created. |
|
echarts also provides a mechanism to mark a line with average value. Below is code and a sample with average run duration and average queued duration of the series marked.
diff --git a/airflow/www/static/js/dag/details/task/TaskDuration.tsx b/airflow/www/static/js/dag/details/task/TaskDuration.tsx
index 1f99f39e4d..7ce2e8aed9 100644
--- a/airflow/www/static/js/dag/details/task/TaskDuration.tsx
+++ b/airflow/www/static/js/dag/details/task/TaskDuration.tsx
@@ -155,6 +155,9 @@ const TaskDuration = () => {
opacity: 0.6,
},
stack: "x",
+ markLine: {
+ data: [{ type: "average", name: "Avg" }],
+ },
},
{
type: "bar",
@@ -164,6 +167,9 @@ const TaskDuration = () => {
color: (params) => stateColors[params.data.state],
},
stack: "x",
+ markLine: {
+ data: [{ type: "average", name: "Avg" }],
+ },
},
],
// @ts-ignore |
|
I fixed the issue if there are no TIs and some breadcrumb header navigation. We can leave the average values for a follow up PR |
|
@bbovenzi Is there anything I can help here to have this reviewed and merged? There is one pending item left to redirect links from old task duration page but I would like your thoughts on having this page for a release just in case if there is something missing in the new implementation and to switch links in the next release.
Thanks |
Yeah, I'd like to keep them as separate PRs. I think the most useful thing is for anyone to pull down this branch, try out the new page and give usability feedback. |
bbovenzi
force-pushed
the
task-duration-react
branch
2 times, most recently
from
93a03d5
to
7752e9b
Compare
Update icon to match existing task duration page. Update with queued duration. PR comments about variables and using moment to convert duration. Add task row selection support. Implement task duration page in react. Update icon to match existing task duration page. Add task row selection support. Sort as per DagRun order in grid and add logical date as x-axis label. Rearrange task selection tabs fix group/mapped details and select task node if no run Fix tests and remove unneeded rebase changes Fix TI ordering and add dbl click to expand/collapse groups Fix header nav and no TIs Refactor TaskName, hide null tooltips, and add fake tab link
bbovenzi
force-pushed
the
task-duration-react
branch
from
7752e9b
to
6e5b97c
Compare
|
Thanks @bbovenzi and @jedcunningham for the review and feedback 👍 |
Great job doing this PR. I'm glad we finally got it merged. Let me know if you want help removing the old task duration page. |
* Implement task duration page in react. Update icon to match existing task duration page. Update with queued duration. PR comments about variables and using moment to convert duration. Add task row selection support. Implement task duration page in react. Update icon to match existing task duration page. Add task row selection support. Sort as per DagRun order in grid and add logical date as x-axis label. Rearrange task selection tabs fix group/mapped details and select task node if no run Fix tests and remove unneeded rebase changes Fix TI ordering and add dbl click to expand/collapse groups Fix header nav and no TIs Refactor TaskName, hide null tooltips, and add fake tab link * Fix task name selection --------- Co-authored-by: Brent Bovenzi <brent.bovenzi@gmail.com>
@bbovenzi I tried testing this with multiple DAG runs, however, in graph I do not see more than 26 runs. How you were able to generate this graph for 365 runs? 
|
|
@tirkarthi @bbovenzi I was testing this out and noticed task duration for manual runs were not showing |
|
@vatsrahul1001 Please check value of ordering in object/grid_data response. It could be possible that orderingLabel is dataIntervalEnd and manual runs have same dataIntervalEnd have same value as scheduled runs. I am not sure we can have adjacent bars for same category value in echarts.
I suspect manual runs which have same value as scheduled runs could be causing this issue like the case with your question about manual runs where 4 runs were shown in grid but only 2 are shown in the graph. I would recommend new issues if any to track them since the thread is long. Thanks. |
yes @tirkarthi you are right manual runs having same dataIntervalEnd is causing this. I will create a new issue for this |
* Implement task duration page in react. Update icon to match existing task duration page. Update with queued duration. PR comments about variables and using moment to convert duration. Add task row selection support. Implement task duration page in react. Update icon to match existing task duration page. Add task row selection support. Sort as per DagRun order in grid and add logical date as x-axis label. Rearrange task selection tabs fix group/mapped details and select task node if no run Fix tests and remove unneeded rebase changes Fix TI ordering and add dbl click to expand/collapse groups Fix header nav and no TIs Refactor TaskName, hide null tooltips, and add fake tab link * Fix task name selection --------- Co-authored-by: Brent Bovenzi <brent.bovenzi@gmail.com>
closes: #30328
related: #30328