-
Notifications
You must be signed in to change notification settings - Fork 14.7k
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
Link state icons in dashboard to list page with instances filtered by the state for the given duration. #46968
base: main
Are you sure you want to change the base?
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.
Great idea! I've been wanting to make the dashboard link to more parts of the UI
A few comments.
We could probably move all the task instance filters into a separate component. And move TaskInstances
into its own folder.
And if we refactor that. We should probably fix the Search Tasks button to not have the same hotkey as search dags since they conflict in the Dag Details page
const { | ||
END_DATE: END_DATE_PARAM, | ||
START_DATE: START_DATE_PARAM, | ||
STATE: STATE_PARAM, | ||
}: SearchParamsKeysType = SearchParamsKeys; |
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.
We can declare this outside of the component so we don't redeclare it every time the component mounts.
@@ -154,7 +162,7 @@ export const DagRuns = () => { | |||
}); | |||
setSearchParams(searchParams); | |||
}, | |||
[pagination, searchParams, setSearchParams, setTableURLState, sorting], | |||
[pagination, searchParams, setSearchParams, setTableURLState, sorting, STATE_PARAM], |
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.
If we do the above comment then we don't need STATE_PARAM here
<StateBadge fontSize="md" state={state}> | ||
{runs} | ||
</StateBadge> | ||
<Link href={`/webapp/${kind}?state=${state}&start_date=${startDate}&end_date=${endDate}`}> |
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.
Let's use the Link from react-router-dom so that we don't have to hardcode /webapp/
If we want the link to be styled like chakra, we just wrap the RouterLink with the chakra one and pass the param asChild
(look at our TaskInstances and DagRuns pages for examples`
const { | ||
END_DATE: END_DATE_PARAM, | ||
NAME_PATTERN: NAME_PATTERN_PARAM, | ||
START_DATE: START_DATE_PARAM, | ||
STATE: STATE_PARAM, | ||
}: SearchParamsKeysType = SearchParamsKeys; |
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.
Again, let's move this outside of the component itself
On visiting the dashboard home page user could see many failed task instances in the last 24 hours. Quickly identifying the task instances involves user going to the dags page, task instances tab and then selecting failed state to sort by start_date/end_date to get recent failures. With this PR by clicking on the state icon in metrics section the respective list page in this case task instance is rendered and the state is filtered by "failed" and start_date/end_date passed as query parameters to get the task instances failed in last 24 hours.
It seems
airflow/ui/src/pages/TaskInstances.tsx
has become longer than 250 lines. I tried refactoring by moving some query parameter fields to constants but it's still at 253. I have disabled the eslint rule for now to this page. Any suggestions welcome.