Page MenuHomePhabricator

extract run graph utils from partition-specific graphs
ClosedPublic

Authored by prha on Sep 16 2020, 12:25 AM.

Details

Summary

Based off of D4307, which created graphs for non-partitioned pipelines, using execution time as the X-axis

Test Plan

Rendered partition dashboard, schedule dashboard, saw rendered graphs

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

prha requested review of this revision.Sep 16 2020, 12:43 AM

This looks great! I added a few inline comments but I like the idea of creating a base class for these graphs, the chartJS stuff is pretty verbose and it's always a bummer to have a ton of chart config in the middle of a component.

js_modules/dagit/src/RunGraph.tsx
17

It might be nice to document in a comment up here that the Y value within each PointValue is freeform, but the X value needs to be the start time of the run because of the logic in _fillPartitions.

It might also be nice to export the _getExecutionTime method as something more opaque (eg: getXForRun(run)) so that anyone who adds another graph using this component uses the exact same code for creating X values.

46

I added the chart.js types to the project so we should be able to make this return an instance of ChartOptions - if it's not too much of a pain to add here, I think it'd be nice to be able to cmd-click through to their types to investigate these options.

(If it complains about scales you could say const scales: ChartOptions['scales'] = to reference the type of the object member in the other type.)

117

This is a really dumb nit but it might be better to call this _getRunStartTime just to avoid introducing another word with an equivalent meaning - though I like execution time better than start time ...

This revision is now accepted and ready to land.Oct 5 2020, 7:55 PM