Skip to content
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 task instance state and name filter to task instances tab in a dagrun #45215

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
18 changes: 14 additions & 4 deletions airflow/ui/src/components/SearchBar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,19 @@ type Props = {
readonly buttonProps?: ButtonProps;
readonly defaultValue: string;
readonly groupProps?: InputGroupProps;
readonly hideAdvanced?: boolean;
readonly onChange: (value: string) => void;
readonly placeHolder: string;
};

export const SearchBar = ({ buttonProps, defaultValue, groupProps, onChange, placeHolder }: Props) => {
export const SearchBar = ({
buttonProps,
defaultValue,
groupProps,
hideAdvanced = false,
onChange,
placeHolder,
}: Props) => {
const handleSearchChange = useDebouncedCallback((val: string) => onChange(val), debounceDelay);

const [value, setValue] = useState(defaultValue);
Expand Down Expand Up @@ -61,9 +69,11 @@ export const SearchBar = ({ buttonProps, defaultValue, groupProps, onChange, pla
size="xs"
/>
) : undefined}
<Button fontWeight="normal" height="1.75rem" variant="ghost" width={140} {...buttonProps}>
Advanced Search
</Button>
{Boolean(hideAdvanced) ? undefined : (
<Button fontWeight="normal" height="1.75rem" variant="ghost" width={140} {...buttonProps}>
Advanced Search
</Button>
)}
Comment on lines +72 to +76
Copy link
Member

@pierrejeambrun pierrejeambrun Jan 7, 2025

Choose a reason for hiding this comment

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

I am not aware of a SearchBar that actually leverage this feature. (with a working button).

As you mentioned in the description. This can be removed if unused.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be a better practice to make the prop showAdvanced and force pages to opt in to it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is some API work done which will be used in advanced search. For now I can keep it as opt out here.

#45420

</>
}
startElement={<FiSearch />}
Expand Down
128 changes: 122 additions & 6 deletions airflow/ui/src/pages/Run/TaskInstances.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,18 +16,21 @@
* specific language governing permissions and limitations
* under the License.
*/
import { Box, Link } from "@chakra-ui/react";
import { Box, Link, createListCollection, HStack, type SelectValueChangeDetails } from "@chakra-ui/react";
import type { ColumnDef } from "@tanstack/react-table";
import { Link as RouterLink, useParams } from "react-router-dom";
import { useCallback, useState } from "react";
import { Link as RouterLink, useParams, useSearchParams } from "react-router-dom";

import { useTaskInstanceServiceGetTaskInstances } from "openapi/queries";
import type { TaskInstanceResponse } from "openapi/requests/types.gen";
import type { TaskInstanceResponse, TaskInstanceState } from "openapi/requests/types.gen";
import { DataTable } from "src/components/DataTable";
import { useTableURLState } from "src/components/DataTable/useTableUrlState";
import { ErrorAlert } from "src/components/ErrorAlert";
import { SearchBar } from "src/components/SearchBar";
import Time from "src/components/Time";
import { Status } from "src/components/ui";
import { getDuration } from "src/utils";
import { Select, Status } from "src/components/ui";
import { SearchParamsKeys, type SearchParamsKeysType } from "src/constants/searchParams";
import { capitalize, getDuration } from "src/utils";
import { getTaskInstanceLink } from "src/utils/links";

const columns: Array<ColumnDef<TaskInstanceResponse>> = [
Expand Down Expand Up @@ -82,12 +85,74 @@ const columns: Array<ColumnDef<TaskInstanceResponse>> = [
},
];

const stateOptions = createListCollection<{ label: string; value: TaskInstanceState | "all" | "none" }>({
items: [
{ label: "All States", value: "all" },
Copy link
Member

Choose a reason for hiding this comment

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

I believe this can be further simplified, by having nothing selected at all 'undefined' value, that is considered all implicitly so we do not have to manually handle an "all". Because that also mess up the typing to have that extra virtual 'state'.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think it needs to be some version of undefined because otherwise the clear X appears when it shouldn't

Screenshot 2025-01-07 at 1 29 24 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added clearable by default. Now I have made it so that it's clearable only when a state is selected and it's not shown for "all states" when no state is selected.

{ label: "Scheduled", value: "scheduled" },
{ label: "Queued", value: "queued" },
{ label: "Running", value: "running" },
{ label: "Success", value: "success" },
{ label: "Restarting", value: "restarting" },
{ label: "Failed", value: "failed" },
{ label: "Up For Retry", value: "up_for_retry" },
{ label: "Up For Reschedule", value: "up_for_reschedule" },
{ label: "Upstream failed", value: "upstream_failed" },
{ label: "Skipped", value: "skipped" },
{ label: "Deferred", value: "deferred" },
{ label: "Removed", value: "removed" },
Comment on lines +90 to +102
Copy link
Member

Choose a reason for hiding this comment

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

I think a task instance can also have no status => state is null, not yet scheduled.

Copy link
Contributor Author

@tirkarthi tirkarthi Jan 9, 2025

Choose a reason for hiding this comment

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

react-select doesn't seem to allow selecting an option with value as null . Backend seems to accept "none" and transform it as None state in the query. { label: "No status", value: "none" } helps But selecting "No status" will make UI list it as "None" . Maybe displaying selected options part needs to be special cased to handle "none" .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in c67eec9 . Please feel free to suggest if there is a better way to handle this state. Thanks.

{ label: "No Status", value: "none" },
],
});

const STATE_PARAM = "state";

export const TaskInstances = () => {
const { dagId = "", runId = "" } = useParams();
const [searchParams, setSearchParams] = useSearchParams();
const { setTableURLState, tableURLState } = useTableURLState();
const { pagination, sorting } = tableURLState;
const [sort] = sorting;
const orderBy = sort ? `${sort.desc ? "-" : ""}${sort.id}` : "-start_date";
const filteredState = searchParams.getAll(STATE_PARAM);
const hasFilteredState = filteredState.length > 0;
const { NAME_PATTERN: NAME_PATTERN_PARAM }: SearchParamsKeysType = SearchParamsKeys;

const [taskDisplayNamePattern, setTaskDisplayNamePattern] = useState(
searchParams.get(NAME_PATTERN_PARAM) ?? undefined,
);

const handleStateChange = useCallback(
({ value }: SelectValueChangeDetails<string>) => {
const [val, ...rest] = value;

if ((val === undefined || val === "all") && rest.length === 0) {
searchParams.delete(STATE_PARAM);
} else {
searchParams.delete(STATE_PARAM);
value.filter((state) => state !== "all").map((state) => searchParams.append(STATE_PARAM, state));
}
setTableURLState({
pagination: { ...pagination, pageIndex: 0 },
sorting,
});
setSearchParams(searchParams);
},
[pagination, searchParams, setSearchParams, setTableURLState, sorting],
);

const handleSearchChange = (value: string) => {
if (value) {
searchParams.set(NAME_PATTERN_PARAM, value);
} else {
searchParams.delete(NAME_PATTERN_PARAM);
}
setTableURLState({
pagination: { ...pagination, pageIndex: 0 },
sorting,
});
setTaskDisplayNamePattern(value);
setSearchParams(searchParams);
};

const { data, error, isFetching, isLoading } = useTaskInstanceServiceGetTaskInstances(
{
Expand All @@ -96,13 +161,64 @@ export const TaskInstances = () => {
limit: pagination.pageSize,
offset: pagination.pageIndex * pagination.pageSize,
orderBy,
state: hasFilteredState ? filteredState : undefined,
taskDisplayNamePattern: Boolean(taskDisplayNamePattern) ? taskDisplayNamePattern : undefined,
},
undefined,
{ enabled: !isNaN(pagination.pageSize) },
);

return (
<Box>
<Box pt={4}>
<HStack>
<Select.Root
collection={stateOptions}
maxW="250px"
multiple
onValueChange={handleStateChange}
value={hasFilteredState ? filteredState : ["all"]}
>
<Select.Trigger
{...(hasFilteredState ? { clearable: true } : {})}
colorPalette="blue"
isActive={Boolean(filteredState)}
>
<Select.ValueText>
{() =>
hasFilteredState ? (
<HStack gap="10px">
{filteredState.map((state) => (
<Status key={state} state={state as TaskInstanceState}>
{state === "none" ? "No Status" : capitalize(state)}
</Status>
))}
</HStack>
) : (
"All States"
)
}
</Select.ValueText>
</Select.Trigger>
<Select.Content>
{stateOptions.items.map((option) => (
<Select.Item item={option} key={option.label}>
{option.value === "all" ? (
option.label
) : (
<Status state={option.value as TaskInstanceState}>{option.label}</Status>
)}
</Select.Item>
))}
</Select.Content>
</Select.Root>
<SearchBar
buttonProps={{ disabled: true }}
defaultValue={taskDisplayNamePattern ?? ""}
hideAdvanced
onChange={handleSearchChange}
placeHolder="Search Tasks"
/>
</HStack>
<DataTable
columns={columns}
data={data?.task_instances ?? []}
Expand Down
1 change: 1 addition & 0 deletions airflow/ui/src/utils/stateColor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
export const stateColor = {
deferred: "mediumpurple",
failed: "red",
none: "lightblue",
null: "lightblue",
queued: "gray",
removed: "lightgrey",
Expand Down