-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[ui] Fix endless log loading #26446
[ui] Fix endless log loading #26446
Conversation
Deploy preview for dagit-core-storybook ready! ✅ Preview Built with commit c174946. |
60ac241
to
b306940
Compare
@@ -81,12 +81,10 @@ export const LogsScrollingTable = (props: Props) => { | |||
}, [totalHeight, virtualizer]); | |||
|
|||
const content = () => { | |||
if (logs.loading) { | |||
if (logs.allNodeChunks.length === 0 && logs.loading) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fixes the specific bug report.
<NonIdealState icon="spinner" title="Fetching logs..." /> | ||
</ListEmptyState> | ||
<Box margin={{top: 32}} flex={{direction: 'column', alignItems: 'center'}}> | ||
<SpinnerWithText label="Loading run logs…" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tweaking the loading state to better match our loading states elsewhere.
const nodes = [...state.nodes, ...queuedNodes]; | ||
|
||
const copy = state.nodeChunks.slice(); | ||
copy.push(queuedNodes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid copying the entire list of individual logs by using chunks instead.
b306940
to
d0ed589
Compare
js_modules/dagster-ui/packages/ui-core/src/runs/flattenNodeChunks.tsx
Outdated
Show resolved
Hide resolved
88e37b7
to
dbcc916
Compare
@@ -113,9 +114,12 @@ export const StepLogsModalContent = ({ | |||
const [logType, setComputeLogType] = useState<LogType>(LogType.structured); | |||
const [computeLogUrl, setComputeLogUrl] = React.useState<string | null>(null); | |||
|
|||
const firstLogForStep = logs.allNodes.find( | |||
const flatLogs = useMemo(() => flattenNodeChunks(logs.allNodeChunks), [logs]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could try using useThrottledMemo
in case the websocket gets spammed very quickly then we can batch multiple updates to lead to just 1 flattenNodeChunks call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import React, { useState, useEffect } from 'react';
function withThrottle(WrappedComponent, delay = 1000) {
return function ThrottledComponent(props) {
const [throttledProps, setThrottledProps] = useState(props);
useEffect(() => {
const handler = setTimeout(() => setThrottledProps(props), delay);
return () => clearTimeout(handler);
}, [props, delay]);
return <WrappedComponent {...throttledProps} />;
};
}
export default withThrottle;
Or maybe we could use something like the above to throttle the number of re-renders for the logs component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is throttling behavior in LogsProvider
to guard against this. It looks like it is working as intended, though it's pretty old code and could maybe be revisited at some point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I added that throttling. Thinking back on it though I think throttling at the component level would be nicer than just throttling the websocket updates.
dbcc916
to
c174946
Compare
Summary & Motivation
We received a bug report from an org with some very, very long log output on some of their runs. Specifically, hundreds of thousands of large logs for individual runs. I noticed two issues during testing:
[].concat(...chunks)
as the flattening approach, based on benchmarking. it seems to be faster thanflat()
.How I Tested These Changes
View run with neverending log loading. Verify that logs appear and stream in.
Use Chrome profiler to determine that the chunking and flattening behavior appears to be performant at scale. The
filterLogs
function remains expensive, but I think in this case it's because the logs themselves are enormous and require a ton of JSON stringification to support text searching.Changelog
[ui] Fix run log streaming for runs with a large volume of logs.