Skip to content

fix: let workspace pages download partial logs for unhealthy workspaces #13761

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

Merged
merged 19 commits into from
Jul 3, 2024
Merged

fix: let workspace pages download partial logs for unhealthy workspaces #13761

Changes from 1 commit
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
f1f5f88
fix: get basic fix in for preventing download logs from blowing up UI
Parkreiner Jul 1, 2024
edd6362
fix: make sure blob units can't go out of bounds
Parkreiner Jul 1, 2024
901aa39
fix: make sure timeout is cleared on component unmount
Parkreiner Jul 1, 2024
e1d8113
fix: reduce risk of shared cache state breaking useAgentLogs
Parkreiner Jul 2, 2024
87f57aa
fix: allow partial downloading of logs
Parkreiner Jul 2, 2024
5bf2baf
fix: make sure useMemo cache is used properly
Parkreiner Jul 2, 2024
5c2316b
wip: commit current progress on updated logs functionality
Parkreiner Jul 2, 2024
edd6569
docs: rewrite comment for clarity
Parkreiner Jul 2, 2024
2eb7d6e
refactor: clean up current code
Parkreiner Jul 2, 2024
fa06515
fix: update styles for unavailable logs
Parkreiner Jul 2, 2024
8fb4496
fix: resolve linter violations
Parkreiner Jul 2, 2024
6935f50
fix: update type signature of getErrorDetail
Parkreiner Jul 2, 2024
f74eda4
fix: revert log/enabled logic for useAgentLogs
Parkreiner Jul 2, 2024
7b76eb7
fix: remove memoization from DownloadLogsDialog
Parkreiner Jul 2, 2024
07ff536
fix: update name of timeout state
Parkreiner Jul 2, 2024
460aa53
refactor: make log web sockets logic more clear
Parkreiner Jul 2, 2024
5a449b4
docs: reword comment for clarity
Parkreiner Jul 2, 2024
eb4f88c
fix: commit current style update progress
Parkreiner Jul 2, 2024
71692b6
fix: finish style updates
Parkreiner Jul 2, 2024
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
Prev Previous commit
Next Next commit
refactor: clean up current code
  • Loading branch information
@Parkreiner
Parkreiner committed Jul 2, 2024
commit 2eb7d6ebd818b5db561e5f9bae76c7c82268dbcd
29 changes: 13 additions & 16 deletions site/src/pages/WorkspacePage/WorkspaceActions/DownloadLogsDialog.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { useTheme, type Interpolation, type Theme } from "@emotion/react";
import Skeleton from "@mui/material/Skeleton";
import { saveAs } from "file-saver";
import JSZip from "jszip";
import { useMemo, useState, type FC, useRef, useEffect } from "react";
import { type FC, useMemo, useState, useRef, useEffect } from "react";
import { UseQueryOptions, useQueries, useQuery } from "react-query";
import { agentLogs, buildLogs } from "api/queries/workspaces";
import type {
Expand Down Expand Up @@ -59,13 +59,14 @@ export const DownloadLogsDialog: FC<DownloadLogsDialogProps> = ({
// This is clunky, but we have to memoize in two steps to make sure that we
// don't accidentally break the memo cache every render. We can't tuck
// everything into a single memo call, because we need to set up React Query
// state between processing the agents, and we can't violate rules of hooks
// state between processing the agents, but we can't violate rules of hooks by
// putting hooks inside of hooks
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know, but I feel it is an over-optimization. If it is fast enough and does not impact UI usage, I don't see why we should complicate things.

Copy link
Member Author

@Parkreiner Parkreiner Jul 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, I was considering that, but didn't really have time to test out how long it would take to keep generating blobs every render

To be clear, we basically have two choices:

  • Keep the clunky but functional memoization
  • Remove the memoization altogether

The original memoization has worse performance than not having memoization at all, because we're still recalculating the blobs every render, but React has to compare dependencies and decide if it should re-calculate the value

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, it was my fault. I usually use "useMemo" to compute a variable value but it would probably be better to be a function. I would rather remove memorization altogether to make things simple.

type AgentInfo = Readonly<{
agents: readonly WorkspaceAgent[];
queries: readonly UseQueryOptions<readonly WorkspaceAgentLog[]>[];
logOptionsArray: readonly UseQueryOptions<readonly WorkspaceAgentLog[]>[];
}>;

const { agents, queries } = useMemo<AgentInfo>(() => {
const { agents, logOptionsArray } = useMemo<AgentInfo>(() => {
const allAgents = workspace.latest_build.resources.flatMap(
(resource) => resource.agents ?? [],
);
Expand All @@ -77,7 +78,7 @@ export const DownloadLogsDialog: FC<DownloadLogsDialogProps> = ({

return {
agents: uniqueAgents,
queries: uniqueAgents.map((agent) => {
logOptionsArray: uniqueAgents.map((agent) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need to calculate the query options here and not mount it and pass it in the query? I think it is kinda far from what is used and I took some time to link logOptionsArray to agentQueryOptions

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, this was an over-optimization on my part. I basically figured that since the query objects shouldn't change unless the agents do, it was safe to throw them all together

return {
...agentLogs(workspace.id, agent.id),
enabled: open,
Expand All @@ -86,13 +87,13 @@ export const DownloadLogsDialog: FC<DownloadLogsDialogProps> = ({
};
}, [workspace, open]);

const agentLogResults = useQueries({ queries });
const agentLogQueries = useQueries({ queries: logOptionsArray });
const allFiles = useMemo<readonly DownloadableFile[]>(() => {
const files: DownloadableFile[] = [buildLogsFile];

agents.forEach((a, i) => {
const name = `${a.name}-logs.txt`;
const txt = agentLogResults[i]?.data?.map((l) => l.output).join("\n");
const txt = agentLogQueries[i]?.data?.map((l) => l.output).join("\n");

let blob: Blob | undefined;
if (txt) {
Expand All @@ -103,7 +104,7 @@ export const DownloadLogsDialog: FC<DownloadLogsDialogProps> = ({
});

return files;
}, [agentLogResults, agents, buildLogsFile]);
}, [agentLogQueries, agents, buildLogsFile]);

const [isDownloading, setIsDownloading] = useState(false);
const isWorkspaceHealthy = workspace.health.healthy;
Expand Down Expand Up @@ -134,7 +135,7 @@ export const DownloadLogsDialog: FC<DownloadLogsDialogProps> = ({
disabled={
isDownloading ||
// If a workspace isn't healthy, let the user download as many logs as
// they can
// they can. Otherwise, wait for everything to come in
(isWorkspaceHealthy && isLoadingFiles)
}
onConfirm={async () => {
Expand Down Expand Up @@ -214,14 +215,10 @@ const DownloadingItem: FC<DownloadingItemProps> = ({ file, giveUpTimeMs }) => {
<span css={styles.listItemSecondary}>
{file.blob ? (
humanBlobSize(file.blob.size)
) : isWaiting ? (
<Skeleton variant="text" width={48} height={12} />
) : (
<>
{isWaiting ? (
<Skeleton variant="text" width={48} height={12} />
) : (
<p css={styles.notAvailableText}>N/A</p>
)}
</>
<p css={styles.notAvailableText}>N/A</p>
)}
</span>
</li>
Expand Down