Re: [PR] Improve duration chart [airflow]
github-actions[bot] closed pull request #55339: Improve duration chart URL: https://github.com/apache/airflow/pull/55339 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Improve duration chart [airflow]
github-actions[bot] commented on PR #55339: URL: https://github.com/apache/airflow/pull/55339#issuecomment-3690641944 This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Improve duration chart [airflow]
pwdsudinym-ui commented on code in PR #55339:
URL: https://github.com/apache/airflow/pull/55339#discussion_r2373909559
##
airflow-core/src/airflow/ui/src/components/DurationChart.tsx:
##
@@ -236,6 +236,9 @@ export const DurationChart = ({
stacked: true,
ticks: {
maxTicksLimit: 3,
+callback: (value) => {
+ return(dayjs(value).format(getLabelFormat(entries)))
+}
Review Comment:
Good catch, I didn't notice that the x-axis labels were all the same.
I'm not sure that this is a timezone issue.
I spent some time investigating this today. It seems a little more difficult
than I thought. If we use the callback function for ticks, the values currently
being passed to the callback are `0, 1, 2, ...` which I think is what gives us
the same number across the x axis.
I tried to enable `type: time` with `import
"chartjs-adapter-dayjs-4/dist/chartjs-adapter-dayjs-4.esm";` but that just
ended up leading to the bars disappearing
https://github.com/user-attachments/assets/b316a4b4-75d1-4105-a8a4-4edead94e225";
/>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Improve duration chart [airflow]
pierrejeambrun commented on code in PR #55339:
URL: https://github.com/apache/airflow/pull/55339#discussion_r2371828305
##
airflow-core/src/airflow/ui/src/components/DurationChart.tsx:
##
@@ -236,6 +236,9 @@ export const DurationChart = ({
stacked: true,
ticks: {
maxTicksLimit: 3,
+callback: (value) => {
+ return(dayjs(value).format(getLabelFormat(entries)))
+}
Review Comment:
That's not working. It's not taking into account the timezone.
Also it always show a wrong value for me:
https://github.com/user-attachments/assets/42535f84-37ef-4cd0-bb68-9e12c7ec2b6e";
/>
https://github.com/user-attachments/assets/6bd2dd5e-316e-4791-b0a2-b818fc150156";
/>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Improve duration chart [airflow]
pwdsudinym-ui commented on code in PR #55339:
URL: https://github.com/apache/airflow/pull/55339#discussion_r2396036654
##
airflow-core/src/airflow/ui/src/components/DurationChart.tsx:
##
@@ -236,6 +236,9 @@ export const DurationChart = ({
stacked: true,
ticks: {
maxTicksLimit: 3,
+callback: (value) => {
+ return(dayjs(value).format(getLabelFormat(entries)))
+}
Review Comment:
@pierrejeambrun let me know if this makes sense / if you have any advice on
how to proceed. I'm not sure this is super easy without significantly changing
the Duration chart.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Improve duration chart [airflow]
pierrejeambrun commented on code in PR #55339:
URL: https://github.com/apache/airflow/pull/55339#discussion_r2426781887
##
airflow-core/src/airflow/ui/src/components/DurationChart.tsx:
##
@@ -236,6 +236,9 @@ export const DurationChart = ({
stacked: true,
ticks: {
maxTicksLimit: 3,
+callback: (value) => {
+ return(dayjs(value).format(getLabelFormat(entries)))
+}
Review Comment:
Might need some refinement, but you can use something like this:
```typescript
callback: (_value, index) =>
formatDate((entries)[index]?.run_after, selectedTimezone,
"HH:mm:ss"),
```
https://github.com/user-attachments/assets/1dc3a4d3-9ea3-4489-9aa3-405a7fc04a09";
/>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Improve duration chart [airflow]
pwdsudinym-ui commented on PR #55339: URL: https://github.com/apache/airflow/pull/55339#issuecomment-3321962706 @pierrejeambrun @bbovenzi let me know if this looks good now :) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Improve duration chart [airflow]
pwdsudinym-ui commented on code in PR #55339:
URL: https://github.com/apache/airflow/pull/55339#discussion_r2373909559
##
airflow-core/src/airflow/ui/src/components/DurationChart.tsx:
##
@@ -236,6 +236,9 @@ export const DurationChart = ({
stacked: true,
ticks: {
maxTicksLimit: 3,
+callback: (value) => {
+ return(dayjs(value).format(getLabelFormat(entries)))
+}
Review Comment:
Good catch, I didn't notice that the x-axis labels were all the same.
I'm not sure that this is a timezone issue.
I spent some time investigating this today. It seems a little more difficult
than I thought. If we use the callback function for ticks, the values currently
being passed to the callback are `0, 1, 2, ...` which I think is what gives us
the same number across the x axis.
I tried to enable `type: time` on the x axis to give the chart more info
with which to parse the label. I added the adapter `import
"chartjs-adapter-dayjs-4/dist/chartjs-adapter-dayjs-4.esm";` The callback
values did end up being the correct unix timestamps, but the bars ended up
disappearing for some reason
https://github.com/user-attachments/assets/b316a4b4-75d1-4105-a8a4-4edead94e225";
/>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Improve duration chart [airflow]
pwdsudinym-ui commented on code in PR #55339:
URL: https://github.com/apache/airflow/pull/55339#discussion_r2373909559
##
airflow-core/src/airflow/ui/src/components/DurationChart.tsx:
##
@@ -236,6 +236,9 @@ export const DurationChart = ({
stacked: true,
ticks: {
maxTicksLimit: 3,
+callback: (value) => {
+ return(dayjs(value).format(getLabelFormat(entries)))
+}
Review Comment:
Good catch, I didn't notice that the x-axis labels were all the same.
I'm not sure that this is a timezone issue.
I spent some time investigating this today. It seems a little more difficult
than I thought. If we use the callback function for ticks, the values currently
being passed to the callback are `0, 1, 2, ...` which I think is what gives us
the same number across the x axis.
I tried to enable `type: time` on the x axis to give the chart more info
with which to parse the label. I added the adapter `import
"chartjs-adapter-dayjs-4/dist/chartjs-adapter-dayjs-4.esm";` but that just
ended up leading to the bars disappearing
https://github.com/user-attachments/assets/b316a4b4-75d1-4105-a8a4-4edead94e225";
/>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Improve duration chart [airflow]
pwdsudinym-ui commented on code in PR #55339:
URL: https://github.com/apache/airflow/pull/55339#discussion_r2373946839
##
airflow-core/src/airflow/ui/src/components/DurationChart.tsx:
##
@@ -236,6 +236,9 @@ export const DurationChart = ({
stacked: true,
ticks: {
maxTicksLimit: 3,
+callback: (value) => {
+ return(dayjs(value).format(getLabelFormat(entries)))
+}
Review Comment:
I'm open to ideas but I think the question I'm trying to get at here is is:
how do I get all the times to play well with each other:
- We want onHover to show full date, but it uses the label to determine the
date to show
- We want the x axis to show truncated time, but this also uses the label to
determine what to show
- not using x axis `type: time` makes the callback values `0, 1, 2, ...`
- x axis `type: time` causes the bars to disappear
- x axis `type: time` will also cause time based scaling instead of just 1
bar after another like a classic bar chart
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Improve duration chart [airflow]
pwdsudinym-ui commented on code in PR #55339:
URL: https://github.com/apache/airflow/pull/55339#discussion_r2373909559
##
airflow-core/src/airflow/ui/src/components/DurationChart.tsx:
##
@@ -236,6 +236,9 @@ export const DurationChart = ({
stacked: true,
ticks: {
maxTicksLimit: 3,
+callback: (value) => {
+ return(dayjs(value).format(getLabelFormat(entries)))
+}
Review Comment:
Good catch, I didn't notice that the x-axis labels were all the same. I'm
not sure that this is a timezone issue though. I hadn't noticed before, but my
screenshot above has 12-31 as the time for all despite the onhover being a
different day.
I spent some time investigating this today. It seems a little more difficult
than I thought.
If we use the callback function for ticks, the values currently being passed
to the callback are `0, 1, 2, ...` which I think when converted with dayjs give
us the same x axis label. Not using the callback function lets chartjs auto
detect the label to show so it's not a problem on the main branch as of now.
I tried to enable `type: time` on the x axis to give the chart more info
with which to parse the label. I added the adapter `import
"chartjs-adapter-dayjs-4/dist/chartjs-adapter-dayjs-4.esm";` The callback
values did end up being the correct unix timestamps, but the bars ended up
disappearing for some reason
https://github.com/user-attachments/assets/b316a4b4-75d1-4105-a8a4-4edead94e225";
/>
I'm also not sure this is what we want because then the chart can have gaps
if scaled based on time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Improve duration chart [airflow]
pwdsudinym-ui commented on code in PR #55339:
URL: https://github.com/apache/airflow/pull/55339#discussion_r2373909559
##
airflow-core/src/airflow/ui/src/components/DurationChart.tsx:
##
@@ -236,6 +236,9 @@ export const DurationChart = ({
stacked: true,
ticks: {
maxTicksLimit: 3,
+callback: (value) => {
+ return(dayjs(value).format(getLabelFormat(entries)))
+}
Review Comment:
Good catch, I didn't notice that the x-axis labels were all the same.
I'm not sure that this is a timezone issue.
I spent some time investigating this today. It seems a little more difficult
than I thought. If we use the callback function for ticks, the values currently
being passed to the callback are `0, 1, 2, ...` which I think is what gives us
the same number across the x axis.
I tried to enable `type: time` on the x axis with `import
"chartjs-adapter-dayjs-4/dist/chartjs-adapter-dayjs-4.esm";` but that just
ended up leading to the bars disappearing
https://github.com/user-attachments/assets/b316a4b4-75d1-4105-a8a4-4edead94e225";
/>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Improve duration chart [airflow]
pwdsudinym-ui commented on code in PR #55339:
URL: https://github.com/apache/airflow/pull/55339#discussion_r2373909559
##
airflow-core/src/airflow/ui/src/components/DurationChart.tsx:
##
@@ -236,6 +236,9 @@ export const DurationChart = ({
stacked: true,
ticks: {
maxTicksLimit: 3,
+callback: (value) => {
+ return(dayjs(value).format(getLabelFormat(entries)))
+}
Review Comment:
Good catch, I didn't notice that the x-axis labels were all the same.
I'm not sure that this is a timezone issue.
I spent some time investigating this today. It seems a little more difficult
than I thought. If we use the callback function for ticks, the values currently
being passed to the callback are `0, 1, 2, ...` which I think is what gives us
the same number across the x axis.
I tried to enable `type: time` with `import
"chartjs-adapter-dayjs-4/dist/chartjs-adapter-dayjs-4.esm";` but that just
ended up leading to the bars disappearing
https://github.com/user-attachments/assets/b316a4b4-75d1-4105-a8a4-4edead94e225";
/>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Improve duration chart [airflow]
pierrejeambrun commented on code in PR #55339:
URL: https://github.com/apache/airflow/pull/55339#discussion_r2371828305
##
airflow-core/src/airflow/ui/src/components/DurationChart.tsx:
##
@@ -236,6 +236,9 @@ export const DurationChart = ({
stacked: true,
ticks: {
maxTicksLimit: 3,
+callback: (value) => {
+ return(dayjs(value).format(getLabelFormat(entries)))
+}
Review Comment:
That's not working. It's not taking into account the timezone.
Also it always show 13:00:00 for me, which is not correct.
https://github.com/user-attachments/assets/42535f84-37ef-4cd0-bb68-9e12c7ec2b6e";
/>
https://github.com/user-attachments/assets/6bd2dd5e-316e-4791-b0a2-b818fc150156";
/>
##
airflow-core/src/airflow/ui/src/components/DurationChart.tsx:
##
@@ -236,6 +236,9 @@ export const DurationChart = ({
stacked: true,
ticks: {
maxTicksLimit: 3,
+callback: (value) => {
+ return(dayjs(value).format(getLabelFormat(entries)))
+}
Review Comment:
That's not working. It's not taking into account the timezone.
Also it always show 01:00:00 for me, which is not correct.
https://github.com/user-attachments/assets/42535f84-37ef-4cd0-bb68-9e12c7ec2b6e";
/>
https://github.com/user-attachments/assets/6bd2dd5e-316e-4791-b0a2-b818fc150156";
/>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Improve duration chart [airflow]
bbovenzi commented on code in PR #55339:
URL: https://github.com/apache/airflow/pull/55339#discussion_r2341232590
##
airflow-core/src/airflow/ui/src/components/DurationChart.tsx:
##
@@ -143,10 +145,16 @@ export const DurationChart = ({
label: translate("durationChart.runDuration"),
},
],
- labels: entries.map((entry: RunResponse) =>
dayjs(entry.run_after).format("-MM-DD, hh:mm:ss")),
+ labels: entries.map((entry: RunResponse) =>
dayjs(entry.run_after).format("-MM-DD, hh:mm")),
Review Comment:
Yeah, I think that makes sense to me.
There are other issues with this bar chart, which may require a bigger
refactor but this still helps
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Improve duration chart [airflow]
pwdsudinym-ui commented on code in PR #55339:
URL: https://github.com/apache/airflow/pull/55339#discussion_r2365702395
##
airflow-core/src/airflow/ui/src/components/DurationChart.tsx:
##
@@ -114,6 +118,24 @@ export const DurationChart = ({
value: (ctx: PartialEventContext) => average(ctx, 0),
};
+ const getLabelFormat = (entries: Array) => {
+const timestamps = entries.map(entry => dayjs(entry.run_after));
+const minTime = dayjs.min(timestamps);
+const maxTime = dayjs.max(timestamps);
+
+// satisfy null typecheck for dayjs.min/max
+if (minTime === null || maxTime === null) {
+ return "MM-DD";
+}
+const diffInDays = maxTime.diff(minTime, 'days');
+
+if (diffInDays < 1) {
+ return "hh:mm:ss"
+} else {
+ return "MM-DD"
+}
+ };
+
Review Comment:
Cool, moved this outside the component
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Improve duration chart [airflow]
pierrejeambrun commented on PR #55339: URL: https://github.com/apache/airflow/pull/55339#issuecomment-3303125183 This makes me realize that we might have an issue if runs are over a period greater than 1 day, and there are more than 1 dag runs per day. We will end up with the same `"MM-DD"` making it impossible to identify runs. Also extreme case over years. (less probable but possible). I think we should do what is proposed here for the `labels`, but in the 'tooltip' on hover a Bar, we should keep the date formatter with a full datetime. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Improve duration chart [airflow]
pierrejeambrun commented on code in PR #55339:
URL: https://github.com/apache/airflow/pull/55339#discussion_r2355576449
##
airflow-core/src/airflow/ui/src/components/DurationChart.tsx:
##
@@ -114,6 +118,24 @@ export const DurationChart = ({
value: (ctx: PartialEventContext) => average(ctx, 0),
};
+ const getLabelFormat = (entries: Array) => {
+const timestamps = entries.map(entry => dayjs(entry.run_after));
+const minTime = dayjs.min(timestamps);
+const maxTime = dayjs.max(timestamps);
+
+// satisfy null typecheck for dayjs.min/max
+if (minTime === null || maxTime === null) {
+ return "MM-DD";
+}
+const diffInDays = maxTime.diff(minTime, 'days');
+
+if (diffInDays < 1) {
+ return "hh:mm:ss"
+} else {
+ return "MM-DD"
+}
+ };
+
Review Comment:
Can you move this function outside of the component, so we do not redefine
it on every render.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Improve duration chart [airflow]
pwdsudinym-ui commented on code in PR #55339:
URL: https://github.com/apache/airflow/pull/55339#discussion_r2350134074
##
airflow-core/src/airflow/ui/src/pages/Dag/Overview/Overview.tsx:
##
@@ -76,7 +76,15 @@ export const Overview = () => {
timestampLte: endDate,
});
+<<< HEAD
Review Comment:
woops, updated!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Improve duration chart [airflow]
bbovenzi commented on code in PR #55339:
URL: https://github.com/apache/airflow/pull/55339#discussion_r2349942957
##
airflow-core/src/airflow/ui/src/pages/Dag/Overview/Overview.tsx:
##
@@ -76,7 +76,15 @@ export const Overview = () => {
timestampLte: endDate,
});
+<<< HEAD
Review Comment:
Merge conflict error?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Improve duration chart [airflow]
pwdsudinym-ui commented on code in PR #55339:
URL: https://github.com/apache/airflow/pull/55339#discussion_r2342545988
##
airflow-core/src/airflow/ui/src/components/DurationChart.tsx:
##
@@ -143,10 +145,16 @@ export const DurationChart = ({
label: translate("durationChart.runDuration"),
},
],
- labels: entries.map((entry: RunResponse) =>
dayjs(entry.run_after).format("-MM-DD, hh:mm:ss")),
+ labels: entries.map((entry: RunResponse) =>
dayjs(entry.run_after).format("-MM-DD, hh:mm")),
Review Comment:
Realized hh:mm is ambiguous for >1d and <7d so I just went:
- MM-DD for >1d
- hh:mm:ss for <1d
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
