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.

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

Jump to bottom

[Dashboard] Add task detail page with logs #35328

Merged
merged 8 commits into from
May 16, 2023
Merged

[Dashboard] Add task detail page with logs #35328

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2ff46de
Add task detail page with task logs
alanwguo May 12, 2023
e376e61
Use state-api task_id and actor_id apis to fetch logs instead of file…
alanwguo May 12, 2023
d39423b
fixup
alanwguo May 12, 2023
356315a
update driver jobs to show message informing why driver logs are not …
alanwguo May 15, 2023
c47df84
fix tests
alanwguo May 15, 2023
387f71f
Fix styling of autoscaler cards on the OverviewPage
alanwguo May 15, 2023
aa3779f
add docs url
alanwguo May 15, 2023
f828790
fixup
alanwguo May 15, 2023
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
View file
Open in desktop
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
View file
Open in desktop
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
Loading