Skip to content

Conversation

@tirkarthi
Copy link
Contributor

This adds support to display all events under browse page. This also adds support to display events only related to the dag under events tab in dag details which is basically filter by dag_id in the API when dag_id is present in the URL. The events per dag skips dag_id column which is redundant.

Add support to filter by when, event_log_id which needs to be replaced in the backend before querying. This was done in the legacy API connexion code and the same support is added here

to_replace = {"event_log_id": "id", "when": "dttm"}

Notes for self and review :

  1. eslint fails with below error that <Time /> cannot be constructed for when column but I have seen this pattern used elsewhere and also previously when dags list page only had timestamp for next/last dagrun as <Time />

/home/karthikeyan/stuff/python/airflow/airflow/ui/src/pages/Events/Events.tsx
56:13 error Do not define components during render. React will see a new component type on every render and destroy the entire subtree’s DOM nodes and state (https://reactjs.org/docs/reconciliation.html#elements-of-different-types). Instead, move this component definition out of the parent component “Events” and pass data as props. If you want to allow component creation in props, set allowAsProps option to true react/no-unstable-nested-component

  1. Legacy UI does sorting by when in descending manner by default to display latest events first. I have passed it to useTableURLState as default yet somehow this is not working.
  2. When there are no events the page displays No Eventss found where Events has a double s which needs to be fixed.
  3. extra column is usually a json and might need a followup PR in new UI to pretty display JSON like legacy UI.
  4. The events table under the dag details tab on scroll pushes the column of the table up and needs to be fixed. This is not observed in the code page.

Related

#43704
#43705

Screenshots

image

image

@boring-cyborg boring-cyborg bot added the area:UI Related to UI/UX. For Frontend Developers. label Nov 7, 2024
Copy link
Contributor

@bbovenzi bbovenzi left a comment

Choose a reason for hiding this comment

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

I will work on a fix for the scroll state and search params in a separate PR.

For extra you can try to use the renderSubComponent field on DataTable. But I'm not sure yet if that will be a great UX

@tirkarthi
Copy link
Contributor Author

Thanks @bbovenzi , I rebased the PR with latest main branch changes with fixes to scrolling and default state. Currently, clicking on events goes to webapp/events and does sorting by when in descending mode which is the default value. But there could be a situation where user doesn't want any sort and tries to click on when which causes the sorting to be empty but since sorting is empty the default sort supplied to useTableURLState still gets applied. User can sort by dag_id and then clicking on when will sort in asc order but once the user loads the page by default or reaches a state where no sorting is done it becomes which leads to confusion. This is slightly tricky for me to solve since default state of sort=-when is to be used when there is no sorting on page load but there could be legitimate use case where user wants no sorting at all including sort=-when .

I guess maybe the link to events page in browse menu could be changed to events?sort=-when by default but I don't see any example in createBrowserRouter and it's not expected to match with query parameter as expected as per the comment remix-run/react-router#9613 (comment)

https://reactrouter.com/en/main/routers/create-browser-router

@tirkarthi
Copy link
Contributor Author

For extra you can try to use the renderSubComponent field on DataTable. But I'm not sure yet if that will be a great UX

I will skip rendering extra column in this PR since this might have more work on design and discussion. I will create an issue to track this. Thanks.

@bbovenzi
Copy link
Contributor

bbovenzi commented Nov 8, 2024

Thanks @bbovenzi , I rebased the PR with latest main branch changes with fixes to scrolling and default state. Currently, clicking on events goes to webapp/events and does sorting by when in descending mode which is the default value. But there could be a situation where user doesn't want any sort and tries to click on when which causes the sorting to be empty but since sorting is empty the default sort supplied to useTableURLState still gets applied. User can sort by dag_id and then clicking on when will sort in asc order but once the user loads the page by default or reaches a state where no sorting is done it becomes which leads to confusion. This is slightly tricky for me to solve since default state of sort=-when is to be used when there is no sorting on page load but there could be legitimate use case where user wants no sorting at all including sort=-when .

I guess maybe the link to events page in browse menu could be changed to events?sort=-when by default but I don't see any example in createBrowserRouter and it's not expected to match with query parameter as expected as per the comment remix-run/react-router#9613 (comment)

https://reactrouter.com/en/main/routers/create-browser-router

Oh ok so that's still a bug. If its manually set to be empty then it shouldn't fall back on the default. Let me fix that.

Copy link
Contributor

@bbovenzi bbovenzi left a comment

Choose a reason for hiding this comment

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

This is a great start. We'll continue to refine this page in future PRs:

  • fixing default table sort
  • adding filters
  • adding extras

@bbovenzi bbovenzi merged commit ddbdf2e into apache:main Nov 11, 2024
ellisms pushed a commit to ellisms/airflow that referenced this pull request Nov 13, 2024
…vents for the dag. (apache#43793)

* Add global events page to browse along with support to display only events for the dag.

* Move column definition to a separate function outside of component due to eslint rule.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:UI Related to UI/UX. For Frontend Developers.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants