Skip to content

Improve duration chart #55339

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.

Sign up for GitHub

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

Closed
pwdsudinym-ui wants to merge 5 commits into from
Closed

Improve duration chart #55339

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
232eab3
Improve duration chart
pwdsudinym-ui Sep 7, 2025
8f3be61
Dynamic label format for DurationChart
pwdsudinym-ui Sep 11, 2025
a22e912
Resolve merge conflict for autoRefreshEnabled
pwdsudinym-ui Sep 15, 2025
2ff7a31
Move getLabelFormat out, full date on hover
pwdsudinym-ui Sep 20, 2025
ceac5e5
Change back to default date format
pwdsudinym-ui Sep 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions airflow-core/src/airflow/ui/src/components/DurationChart.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import {
import type { PartialEventContext } from "chartjs-plugin-annotation";
import annotationPlugin from "chartjs-plugin-annotation";
import dayjs from "dayjs";
import minMax from "dayjs/plugin/minMax";
import { Bar } from "react-chartjs-2";
import { useTranslation } from "react-i18next";
import { useNavigate } from "react-router-dom";
Expand All @@ -38,6 +39,8 @@ import type { TaskInstanceResponse, GridRunsResponse } from "openapi/requests/ty
import { getComputedCSSVariableValue } from "src/theme";
import { DEFAULT_DATETIME_FORMAT } from "src/utils/datetimeUtils";

dayjs.extend(minMax);

ChartJS.register(
CategoryScale,
LinearScale,
Expand All @@ -59,10 +62,30 @@ type RunResponse = GridRunsResponse | TaskInstanceResponse;

const getDuration = (start: string, end: string | null) => dayjs.duration(dayjs(end).diff(start)).asSeconds();

const getLabelFormat = (entries: Array<RunResponse>) => {
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"
}
};

export const DurationChart = ({
autoRefreshEnabled,
entries,
kind,
}: {
readonly autoRefreshEnabled?: boolean;
readonly entries: Array<RunResponse> | undefined;
readonly kind: "Dag Run" | "Task Instance";
}) => {
Expand Down Expand Up @@ -165,6 +188,12 @@ export const DurationChart = ({
}}
datasetIdKey="id"
options={{
animation: {
delay: 0,
duration: autoRefreshEnabled ? 0 : 1000,
easing: "easeOutQuart",
loop: false,
},
Comment thread
pwdsudinym-ui marked this conversation as resolved.
onClick: (_event, elements) => {
const [element] = elements;

Expand Down Expand Up @@ -207,6 +236,9 @@ export const DurationChart = ({
stacked: true,
ticks: {
maxTicksLimit: 3,
callback: (value) => {
return(dayjs(value).format(getLabelFormat(entries)))
}
Comment on lines +239 to +241

@pierrejeambrun pierrejeambrun Sep 23, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.
Screenshot 2025-09-23 at 12 18 02

Screenshot 2025-09-23 at 12 18 11

@pwdsudinym-ui pwdsudinym-ui Sep 24, 2025 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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
image
I'm also not sure this is what we want because then the chart can have gaps if scaled based on time.

@pwdsudinym-ui pwdsudinym-ui Sep 24, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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

@pwdsudinym-ui pwdsudinym-ui Oct 1, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@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.

@pierrejeambrun pierrejeambrun Oct 13, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Might need some refinement, but you can use something like this:

                callback: (_value, index) =>
                  formatDate((entries)[index]?.run_after, selectedTimezone, "HH:mm:ss"),
Screenshot 2025-10-13 at 18 15 21

},
title: { align: "end", display: true, text: translate("common:dagRun.runAfter") },
},
Expand Down
12 changes: 11 additions & 1 deletion airflow-core/src/airflow/ui/src/pages/Dag/Overview/Overview.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,12 @@ export const Overview = () => {
});

const refetchInterval = useAutoRefresh({});

const autoRefreshEnabled =
Boolean(useAutoRefresh({ dagId })) &&
gridRuns &&
gridRuns.length > 0 &&
isStatePending(gridRuns[0]?.state);

return (
<Box m={4} spaceY={4}>
Expand Down Expand Up @@ -130,7 +136,11 @@ export const Overview = () => {
{isLoadingRuns ? (
<Skeleton height="200px" w="full" />
) : (
<DurationChart entries={gridRuns?.slice().reverse()} kind="Dag Run" />
<DurationChart
autoRefreshEnabled={autoRefreshEnabled}
entries={gridRuns?.slice().reverse()}
kind="Dag Run"
/>
)}
</Box>
{assetEventsData && assetEventsData.total_entries > 0 ? (
Expand Down
Loading