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

[Dashboard] Add task detail page with logs #35328

Merged
merged 8 commits into from
May 16, 2023
Merged
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
16 changes: 12 additions & 4 deletions dashboard/client/src/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import dayjs from "dayjs";
import duration from "dayjs/plugin/duration";
import React, { Suspense, useEffect, useState } from "react";
import { HashRouter, Navigate, Route, Routes } from "react-router-dom";
import ActorDetailPage from "./pages/actor/ActorDetail";
import ActorDetailPage, { ActorDetailLayout } from "./pages/actor/ActorDetail";
import { ActorLayout } from "./pages/actor/ActorLayout";
import Loading from "./pages/exception/Loading";
import JobList, { JobsLayout } from "./pages/job";
Expand Down Expand Up @@ -33,6 +33,7 @@ import { ServeApplicationsListPage } from "./pages/serve/ServeApplicationsListPa
import { ServeLayout } from "./pages/serve/ServeLayout";
import { ServeReplicaDetailPage } from "./pages/serve/ServeReplicaDetailPage";
import { ServeHttpProxyDetailPage } from "./pages/serve/ServeSystemActorDetailPage";
import { TaskPage } from "./pages/task/TaskPage";
import { getNodeList } from "./service/node";
import { lightTheme } from "./theme";

Expand Down Expand Up @@ -201,16 +202,23 @@ const App = () => {
<Route
element={
<JobDetailActorDetailWrapper>
<ActorDetailPage />
<ActorDetailLayout />
Copy link
Member

Choose a reason for hiding this comment

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

it's unclear how this works - wouldn't you need to specify the children/descendant routes here or does this somehow route to the ActorDetailLayout path below?

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 don't quite understand the question. The ActorDetailPage is a child of this ActorDetailLayout

Copy link
Member

Choose a reason for hiding this comment

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

ah I misread the code, the Layout is wrapped so ActorDetail and Task are children, makes sense

</JobDetailActorDetailWrapper>
}
path="actors/:actorId"
/>
>
<Route element={<ActorDetailPage />} path="" />
<Route element={<TaskPage />} path="tasks/:taskId" />
</Route>
<Route element={<TaskPage />} path="tasks/:taskId" />
</Route>
</Route>
<Route element={<ActorLayout />} path="actors">
<Route element={<Actors />} path="" />
<Route element={<ActorDetailPage />} path=":actorId" />
<Route element={<ActorDetailLayout />} path=":actorId">
<Route element={<ActorDetailPage />} path="" />
<Route element={<TaskPage />} path="tasks/:taskId" />
</Route>
</Route>
<Route element={<Metrics />} path="metrics" />
<Route element={<ServeLayout />} path="serve">
Expand Down
197 changes: 147 additions & 50 deletions dashboard/client/src/common/MultiTabLogViewer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,7 @@ const useStyles = makeStyles((theme) =>

export type MultiTabLogViewerTabDetails = {
title: string;
nodeId: string | null;
filename?: string;
};
} & LogViewerData;

export type MultiTabLogViewerProps = {
tabs: MultiTabLogViewerTabDetails[];
Expand All @@ -45,6 +43,10 @@ export const MultiTabLogViewer = ({

const currentTab = tabs.find((tab) => tab.title === value);

if (tabs.length === 0) {
return <Typography>No logs to display.</Typography>;
}

return (
<div className={className}>
<Box
Expand All @@ -59,52 +61,56 @@ export const MultiTabLogViewer = ({
alignItems="stretch"
flexGrow={1}
>
<Tabs
className={classes.tabs}
value={value}
onChange={(_, newValue) => {
setValue(newValue);
}}
indicatorColor="primary"
>
{tabs.map(({ title }) => (
<Tab key={title} label={title} value={title} />
))}
{otherLogsLink && (
<Tab
label={
<Box display="flex" alignItems="center">
Other logs &nbsp; <RiExternalLinkLine size={20} />
</Box>
}
onClick={(event) => {
// Prevent the tab from changing
setValue(value);
}}
component={Link}
to={otherLogsLink}
target="_blank"
rel="noopener noreferrer"
/>
)}
</Tabs>
{(tabs.length > 1 || otherLogsLink) && (
Copy link
Member

Choose a reason for hiding this comment

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

do you have a screenshot of what the page looks like with only one tab?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot 2023-05-15 at 3 07 02 PM

<Tabs
className={classes.tabs}
value={value}
onChange={(_, newValue) => {
setValue(newValue);
}}
indicatorColor="primary"
>
{tabs.map(({ title }) => (
<Tab key={title} label={title} value={title} />
))}
{otherLogsLink && (
<Tab
label={
<Box display="flex" alignItems="center">
Other logs &nbsp; <RiExternalLinkLine size={20} />
</Box>
}
onClick={(event) => {
// Prevent the tab from changing
setValue(value);
}}
component={Link}
to={otherLogsLink}
target="_blank"
rel="noopener noreferrer"
/>
)}
</Tabs>
)}

{!currentTab ? (
<Typography color="error">Please select a tab.</Typography>
) : (
tabs.map(({ title, nodeId, filename }) => (
<HideableBlock
key={title}
visible={title === currentTab?.title}
keepRendered
>
<StateApiLogViewer
nodeId={nodeId}
filename={filename}
height={expanded ? 800 : 300}
/>
</HideableBlock>
))
tabs.map((tab) => {
const { title, ...data } = tab;
return (
<HideableBlock
key={title}
visible={title === currentTab?.title}
keepRendered
>
<StateApiLogViewer
data={data}
height={expanded ? 800 : 300}
/>
</HideableBlock>
);
})
)}
</Box>
<IconButton
Expand All @@ -119,23 +125,114 @@ export const MultiTabLogViewer = ({
);
};

export type StateApiLogViewerProps = {
nodeId?: string | null;
type TextData = {
contents: string;
};
type FileData = {
nodeId: string | null;
filename?: string;
};
type ActorData = {
actorId: string | null;
suffix: "out" | "err";
};
type TaskData = {
taskId: string | null;
suffix: "out" | "err";
};

type LogViewerData = TextData | FileData | ActorData | TaskData;

const isLogViewerDataText = (data: LogViewerData): data is TextData =>
"contents" in data;

const isLogViewerDataActor = (data: LogViewerData): data is ActorData =>
"actorId" in data;

const isLogViewerDataTask = (data: LogViewerData): data is TaskData =>
"taskId" in data;

export type StateApiLogViewerProps = {
height?: number;
data: LogViewerData;
};

export const StateApiLogViewer = ({
height = 300,
data,
}: StateApiLogViewerProps) => {
if (isLogViewerDataText(data)) {
return <TextLogViewer height={height} contents={data.contents} />;
} else if (isLogViewerDataActor(data)) {
return <ActorLogViewer height={height} {...data} />;
} else if (isLogViewerDataTask(data)) {
return <TaskLogViewer height={height} {...data} />;
} else {
return <FileLogViewer height={height} {...data} />;
}
};

const TextLogViewer = ({
height = 300,
contents,
}: {
height: number;
contents: string;
}) => {
return <LogViewer log={contents} height={height} />;
};

const FileLogViewer = ({
height = 300,
nodeId,
filename,
}: {
height: number;
} & FileData) => {
const apiData = useStateApiLogs({ nodeId, filename }, filename);
return <ApiLogViewer apiData={apiData} height={height} />;
};

const ActorLogViewer = ({
height = 300,
}: StateApiLogViewerProps) => {
const { downloadUrl, log, path, refresh } = useStateApiLogs(nodeId, filename);
actorId,
suffix,
}: {
height: number;
} & ActorData) => {
const apiData = useStateApiLogs(
{ actorId, suffix },
`actor-log-${actorId}.${suffix}`,
Copy link
Member

Choose a reason for hiding this comment

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

it seems like passing actorId is only a partial improvement and not actually used for filtering. wouldn't it not be used since it's really just using the file name (path) here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what do you mean by not used for filtering?

Copy link
Member

Choose a reason for hiding this comment

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

disregard the earlier message, isactorId used to identify the node the log file lives on? if so, could you add a code comment to StateApiLogInput that only one of node/actor/task should be specified?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

isn't that clear by the type definition?

type TextData = {
  contents: string;
};
type FileData = {
  nodeId: string | null;
  filename?: string;
};
type ActorData = {
  actorId: string | null;
  suffix: "out" | "err";
};
type TaskData = {
  taskId: string | null;
  suffix: "out" | "err";
};

type LogViewerData = TextData | FileData | ActorData | TaskData;

Copy link
Member

Choose a reason for hiding this comment

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

I would still add the comment. If someone were to look at the base StateApiLogInput type, it's not clear that the fields should really be used as a oneOf

);
return <ApiLogViewer apiData={apiData} height={height} />;
};

const TaskLogViewer = ({
height = 300,
taskId,
suffix,
}: {
height: number;
} & TaskData) => {
const apiData = useStateApiLogs(
{ taskId, suffix },
`task-log-${taskId}.${suffix}`,
);
return <ApiLogViewer apiData={apiData} height={height} />;
};

const ApiLogViewer = ({
apiData: { downloadUrl, log, path, refresh },
height = 300,
}: {
apiData: ReturnType<typeof useStateApiLogs>;
height: number;
}) => {
return typeof log === "string" ? (
<LogViewer
log={log}
path={path}
downloadUrl={downloadUrl}
downloadUrl={downloadUrl !== null ? downloadUrl : undefined}
height={height}
onRefreshClick={() => {
refresh();
Expand Down
8 changes: 1 addition & 7 deletions dashboard/client/src/components/AutoscalerStatusCards.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ const formatClusterStatus = (title: string, cluster_status: string) => {
return (
<div>
<Box marginBottom={2}>
<Typography variant="h6">{title}</Typography>
<Typography variant="h3">{title}</Typography>
</Box>
{cluster_status_rows.map((i, key) => {
// Format the output.
Expand Down Expand Up @@ -64,9 +64,6 @@ export const NodeStatusCard = ({ cluster_status }: StatusCardProps) => {
overflow: "hidden",
overflowY: "scroll",
}}
sx={{ borderRadius: "16px" }}
marginLeft={1}
marginRight={1}
>
{cluster_status?.data
? formatNodeStatus(cluster_status?.data.clusterStatus)
Expand All @@ -82,9 +79,6 @@ export const ResourceStatusCard = ({ cluster_status }: StatusCardProps) => {
overflow: "hidden",
overflowY: "scroll",
}}
sx={{ border: 1, borderRadius: "1", borderColor: "primary.main" }}
marginLeft={1}
marginRight={1}
>
{cluster_status?.data
? formatResourcesStatus(cluster_status?.data.clusterStatus)
Expand Down
Loading