fix: let workspace pages download partial logs for unhealthy workspaces #13761
Conversation
| const lastLog = logs?.at(-1); | ||
| const canSetLogId = lastLog !== undefined && lastQueriedLogId.current === 0; | ||
|
|
||
| if (canSetLogId) { | ||
| lastQueriedLogId.current = lastLog.id; |
There was a problem hiding this comment.
Previously: we were checking that the array existed, but not that it actually had data
Really do think that we need to bite the bullet and get TypeScript's noUncheckedIndexedAccess compiler option turned on, because it would've caught this
There was a problem hiding this comment.
This is a good one and would require a ticket and PR by itself.
site/src/pages/WorkspacePage/WorkspaceActions/DownloadLogsDialog.tsx
Outdated
Show resolved
Hide resolved
site/src/pages/WorkspacePage/WorkspaceActions/DownloadLogsDialog.tsx
Outdated
Show resolved
Hide resolved
| files.push({ name, blob }); | ||
| }); | ||
|
|
||
| return files; | ||
| }, [agentLogResults, agents, buildLogsQuery.data, workspace.name]); |
There was a problem hiding this comment.
This was a problem with the previous logic: because agents was getting redefined every render, we were also invalidating the useMemo dependency array every render
site/src/pages/WorkspacePage/WorkspaceActions/DownloadLogsDialog.tsx
Outdated
Show resolved
Hide resolved
| setIsDownloading(true); | ||
| const zip = new JSZip(); | ||
| allFiles.forEach((f) => { | ||
| if (f.blob) { | ||
| zip.file(f.name, f.blob); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Moved this out of the try block, because it should be the part that shouldn't be able to fail
| notAvailableText: (theme) => ({ | ||
| display: "flex", | ||
| flexFlow: "row nowrap", | ||
| alignItems: "center", | ||
| columnGap: "4px", | ||
|
|
||
| "& > span": { | ||
| maxHeight: "fit-content", | ||
| display: "flex", | ||
| alignItems: "center", | ||
| color: theme.palette.error.light, | ||
| }, | ||
|
|
||
| "& > p": { | ||
| opacity: "80%", | ||
| }, | ||
| }), |
There was a problem hiding this comment.
I've been stuck in the pre-Emotion, Backstage-specific version of MUI for the past few months, so please let me know if anything looks funky
| // 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, but we can't violate rules of hooks by | ||
| // putting hooks inside of hooks |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| ]; | ||
|
|
||
| return { | ||
| agents: uniqueAgents, | ||
| logOptionsArray: uniqueAgents.map((agent) => { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
| let i = 0; | ||
| while (size > 1024 && i < units.length) { | ||
| while (size > 1024 && i < BLOB_SIZE_UNITS.length) { |
There was a problem hiding this comment.
Since the BLOB_SIZE_UNITS is only used here, I would not move it to leave outside of the function where it is used.
| .filter((a) => a !== undefined) as WorkspaceAgent[]; | ||
| // The while condition can break if we accidentally exceed the bounds of the | ||
| // array. Have to be extra sure we have a unit at the very end. | ||
| const finalUnit = BLOB_SIZE_UNITS[i] ?? BLOB_SIZE_UNITS.at(-1) ?? "TB"; |
There was a problem hiding this comment.
Sorry – just updated the wording on the comment for more clarity!
site/src/pages/WorkspacePage/WorkspaceActions/DownloadLogsDialog.tsx
Outdated
Show resolved
Hide resolved
I did some QA, and it works as expected, but I have some design considerations:
|
|
@BrunoQuaresma Tried implementing all your feedback, and also added some styling for handling long filenames: |
Addresses the most important parts of #13723
Changes made
useAgentLogsto make it more resistant to data being missing, or having agent log data be updated in other parts of the appScreen.Recording.2024-07-02.at.9.52.11.AM.mov
Notes