Page MenuHomeElementl

change run view to show compute log panel with log type toggle
ClosedPublic

Authored by prha on Fri, Apr 16, 11:10 PM.

Details

Summary
  • Changes full-screen modal to structured-log equivalent panel
  • Made log type a query persistent state, so that compute log UI is linkable
  • Changed structured log step start link to open panel

Test Plan

rendered structured logs, stdout/stderr, used download link, made sure logs streamed

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

prha requested review of this revision.Fri, Apr 16, 11:23 PM

This is a great strat.

I think a "stdout"/"stderr" button should be visible even before clicking on the "raw step log" button. It could just default to the first step (or the one that is in scope) Downside it might complicate the UI with a dual-focus problem. Maybe just expand the left-hand toggle?

Eventually we could add a view more akin to the buildkite UI that allows one to view all the logs in one surface somehow

This looks like a huge improvement - the JS looks good but I'll defer to Nick and Sandy on the functional aspects!

js_modules/dagit/packages/core/src/runs/ComputeLogPanel.tsx
51

Offhand it seems a bit odd that the step key isn't passed into getComputeLogDownloadURL - does it return all the logs for all the steps?

90โ€“93

Because there are two of these ContentWrappers rendered and the only purpose of setComputeLogUrl being passed in is so that it can be set to downloadUrl, it feels like the useEffect doesn't belong here. I wonder if computeLogUrl is really still needed in the toolbar? Ideally I think the cleanest solution would move <ComputeLogsProvider> up a level so we don't need state that hoists the computeLogUrl value out of the subtree, OR just move the download link into this ContentWrapper :-p

This revision is now accepted and ready to land.Mon, Apr 19, 7:08 PM
js_modules/dagit/packages/core/src/runs/ComputeLogPanel.tsx
51

yeah, the logData already computes the download url, but it can be either absolute, or relative to the root server URI. a better name might be resolveDownloadURL

90โ€“93

admittedly hacky, but I don't want to instantiate the provider until the user clicks on the raw compute logs button (it sets up a data subscription). But the toolbar needs the data from the provider to set the URL, which I want on the provider parent component.