Page MenuHomePhabricator

Overhauled run history view
ClosedPublic

Authored by bengotow on Wed, Aug 21, 6:54 PM.

Details

Reviewers
alangenfeld
Group Reviewers
Restricted Project
Commits
R1:c4a68162810b: Overhauled run history view
Summary

This diff replaces the old run history tab that showed runs of a particular pipeline with a new one that is at the top level of the Dagit UI and allows you to explore the config, etc. of previous runs and view summary stats.

This diff also allows you to pass ?q= in the query string of a run URL to pre-filter the logs, and pass ?config=&mode=&solidSubset=[] to the /:pipeline/execute URL to create a new tab with that configuration.

Right now this view auto-refreshes every 15 seconds and requires the logs for every run (to compute summary statistics like the number of failed expectations). This will be fixed later.

Test Plan

Run snapshot tests

Diff Detail

Repository
R1 dagster
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

bengotow created this revision.Wed, Aug 21, 6:54 PM
bengotow edited the summary of this revision. (Show Details)Wed, Aug 21, 6:56 PM
bengotow updated this revision to Diff 3902.Wed, Aug 21, 6:59 PM
  • Update snapshot tests
Harbormaster failed remote builds in B3120: Diff 3902!
alangenfeld requested changes to this revision.Wed, Aug 21, 10:27 PM
alangenfeld added a subscriber: alangenfeld.
alangenfeld added inline comments.
js_modules/dagit/src/CustomAlertProvider.tsx
10–51

this seems like its still under construction - i am guessing a string array use for a bool isn't the intended end state

js_modules/dagit/src/TopNav.tsx
27–54

should we do some renames or change the url scheme? this is very confusing

js_modules/dagit/src/execute/PipelineExecutionRoot.tsx
26–44

oof this is gnarly

34–40

sidenote: i personally would like it if we turned on always bracket {} for ifs

js_modules/dagit/src/runs/RunHistory.tsx
32–36

should probably handle print out for hours/minutes as well - real pipes will be slower than our toy ones

465–469

this magic str replace logic feels a bit dicey to bury in render at least hoist to a fn

This revision now requires changes to proceed.Wed, Aug 21, 10:27 PM
sashank added inline comments.
js_modules/dagit/src/runs/RunHistory.tsx
32–36

We use short form durations like 533ms, 2.1s, 4m52s, 34m12s, 1h4m in the dagit log viewer. Maybe something similar would be a good fit here.

https://dagster.phacility.com/source/dagster/browse/master/python_modules/dagster/dagster/utils/timing.py$6

Thanks for the comments—will get these addressed tomorrow,

js_modules/dagit/src/CustomAlertProvider.tsx
10–51

Ahh good call, I think this is a bit shoehorned in here, will see if I can come up with a better set of props for this. I think we may actually be able to have the custom alert callsites just pass a React component for the content and that'd probably be more clear than these msg + pre + lang props.

js_modules/dagit/src/TopNav.tsx
27–54

Yeah we've worked ourselves into a bit of an odd corner because the URL scheme is just /:pipeline/:tab, and now we have things that we need to special case as "not-pipelines". I think we can safely change this but I wasn't sure if there were places where the dagit CLI linked to the web UI. Will double check.

js_modules/dagit/src/execute/PipelineExecutionRoot.tsx
26–44

Yeah... these Redirect components are a bit of an odd concept overall, since it's sort of an official impl of a rendering side-effect. I can break this file into two components and do this in componentDidMount (where the side effect should really be), but I think in that case we'll still have an implicit this.context.router.history.replace, which isn't much better :-/

34–40

Oh this is interesting, I didn't realize prettier wrapped this without adding the brackets. I think that might be an option, let me see. Totally agree this is not great.

js_modules/dagit/src/runs/RunHistory.tsx
32–36

Ahh good call - thanks folks!

465–469

Ahh sounds good - actually it looks like this also exists in LogsRowComponents.tsx so I can merge the two into a helper.

alangenfeld added a comment.EditedFri, Aug 23, 6:51 PM

did some work stacked on top of here:

  • the runs in the previous runs section seem to always be grey dot
  • we might want to interpolate run time between the 15s polls since currently the clock steps with the poll rate for currently running runs
bengotow updated this revision to Diff 4032.Tue, Aug 27, 5:59 PM
  • Update snapshot tests
  • Remove complexity from CustomAlertProvider in favor of passing a ReactNode
  • Change paths to /explore/<pipeline> instead of <pipeline>/explore
  • Remove occurrences of if \([^\)]*\)$ in the TS (two line no-bracket ifs)
  • Consolidat time and stepKey formatting to the same utils
  • Tick up the elapsed time when runs are in-flight
  • Rebase

Hey @alangenfeld - this should be ready for another go. I rebased on master today but I wasn't able to reproduce the problem you saw with all the historical runs showing gray dots. Any idea what might have caused that?

I also added interpolation of the elapsed time strings so that they increment each second. I think that worked out great, but because we only refresh every 15 seconds, it is possible to see the elapsed time go DOWN when a run finishes, which is slightly odd. Creating a subscription for this view might be the best long-term fix but I think this will be OK for now.

sashank added inline comments.Tue, Aug 27, 9:01 PM
js_modules/dagit/src/Util.tsx
80

Nit: ms instead of msec

88

I'm not sure if this is better, but would it be easier to read 5h 2m 10s and 2m 10s than 5:02:10 and 2:10.

The "minutes:seconds" case could can read like "hours:minutes"

js_modules/dagit/src/runs/RunHistory.tsx
360

Passing in a component here seems to be causing a React error when I click the "View Configuration" button.

Error: https://gist.github.com/helloworld/9a5697c981b4fb0b3eca3b89dfa52dc3

372

Should this route be execute/{run.pipeline.name}..?

alangenfeld accepted this revision.Wed, Aug 28, 7:54 PM

shipit

js_modules/dagit/src/execute/PipelineExecutionRoot.tsx
24–38

discussed in person - just do some clean up here:

  • use hooks?
  • move separate session page
This revision is now accepted and ready to land.Wed, Aug 28, 7:54 PM
bengotow updated this revision to Diff 4075.Wed, Aug 28, 9:32 PM
bengotow marked an inline comment as done.
  • Switch <StorageProvider> to a hook to clean up the redirect
  • Move the hack to a separate page to keep things really nice and clean
bengotow updated this revision to Diff 4076.Wed, Aug 28, 9:36 PM
  • Move the hack out of the main pipeline execution page
bengotow updated this revision to Diff 4079.Wed, Aug 28, 10:14 PM
  • Revert changes to apollo version
bengotow updated this revision to Diff 4082.Wed, Aug 28, 10:41 PM
  • Fix item missing in package cache
bengotow updated this revision to Diff 4083.Wed, Aug 28, 10:55 PM
  • Update snapshot tests
  • Remove complexity from CustomAlertProvider in favor of passing a ReactNode
  • Change paths to /explore/<pipeline> instead of <pipeline>/explore
  • Remove occurrences of if \([^\)]*\)$ in the TS (two line no-bracket ifs)
  • Consolidat time and stepKey formatting to the same utils
  • Tick up the elapsed time when runs are in-flight
  • Rebase
  • Switch <StorageProvider> to a hook to clean up the redirect
  • Move the hack to a separate page to keep things really nice and clean
  • Move the hack out of the main pipeline execution page
  • Revert changes to apollo version
  • Fix item missing in package cache
  • Rebase on master
bengotow updated this revision to Diff 4084.Wed, Aug 28, 10:59 PM
  • Update app snapshot tests to reflect new page URLs
bengotow updated this revision to Diff 4085.Wed, Aug 28, 11:01 PM
  • Update autogenerated types
This revision was landed with ongoing or failed builds.Wed, Aug 28, 11:07 PM
This revision was automatically updated to reflect the committed changes.