-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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 dag run and task instance metrics to dashboard. #43888
Conversation
Nice start! A few other thoughts:
|
708136a
to
feb5141
Compare
Thanks @bbovenzi for the comments. Did the following changes.
Agreed, it looks like Chakra doesn't have a datepicker component. So something like https://www.npmjs.com/package/react-datepicker can be used or one of the options from the discussion chakra-ui/chakra-ui#580 . The datepicker can be a general component since it will be used across the app. Feel free to push to the branch if you have any changes directly too. Thanks. |
feb5141
to
c388a5d
Compare
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.
A few more nits.
airflow/ui/src/pages/Dashboard/HistoricalMetrics/MetricSection.tsx
Outdated
Show resolved
Hide resolved
airflow/ui/src/pages/Dashboard/HistoricalMetrics/DagRunMetrics.tsx
Outdated
Show resolved
Hide resolved
airflow/ui/src/pages/Dashboard/HistoricalMetrics/TaskInstanceMetrics.tsx
Outdated
Show resolved
Hide resolved
airflow/ui/src/pages/Dashboard/HistoricalMetrics/MetricsBadge.tsx
Outdated
Show resolved
Hide resolved
airflow/ui/src/pages/Dashboard/HistoricalMetrics/MetricsBadge.tsx
Outdated
Show resolved
Hide resolved
airflow/ui/src/pages/Dashboard/HistoricalMetrics/DagRunMetrics.tsx
Outdated
Show resolved
Hide resolved
airflow/ui/src/pages/Dashboard/HistoricalMetrics/TaskInstanceMetrics.tsx
Outdated
Show resolved
Hide resolved
c388a5d
to
0e4cd64
Compare
Applied all the PR comments locally and rebased with latest main branch. Thanks. |
Are screenshots up to date following latest changes ? Just to get a rough idea of the rendered UI. |
Looking nice. Just a minor nit, it looks like the number in the badge is not centered vertically. Maybe that's just because of the screenshot but it looks slightly off. |
Yes, in the layout tab for the badge WxH is 15.5667×20 in Firefox on Ubuntu for me. I can see the centering not correct in inspect toolbar. The padding is equally applied at the y-axis with |
It appears okay in Brave on Ubuntu. Probably a Firefox/Linux issue as many have reported it in chakra-ui/chakra-ui#983 , chakra-ui/chakra-ui#2314 |
Nice to hear, thanks for investigating. |
airflow/ui/src/pages/Dashboard/HistoricalMetrics/HistoricalMetrics.tsx
Outdated
Show resolved
Hide resolved
airflow/ui/src/pages/Dashboard/HistoricalMetrics/HistoricalMetrics.tsx
Outdated
Show resolved
Hide resolved
Thanks @bbovenzi for the feedback. Made the below changes as per latest review.
|
0e4cd64
to
273b6b8
Compare
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.
Nice work! A few small nits before we can merge.
airflow/ui/src/pages/Dashboard/HistoricalMetrics/MetricSectionSkeleton.tsx
Outdated
Show resolved
Hide resolved
273b6b8
to
26e6db1
Compare
Lgtm! We can iterate on a more detailed loading state and update |
Thanks @bbovenzi and @pierrejeambrun . |
Related #42700
Adds dag run and task instance metrics from historical metrics endpoint to dashboard.