Page MenuHomePhabricator

add modal for viewing stdout/stderr logs
ClosedPublic

Authored by prha on Tue, Aug 27, 11:27 PM.

Details

Summary

Adds stdout/stderr links in the PipelineRun view. Pops open a Dialog
that live updates via GraphQL subscriptions.

Depends on D895, D893

Test Plan

Viewed stdout/stderr using the compute_log_spew_pipeline

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 created this revision.Tue, Aug 27, 11:27 PM
prha added a reviewer: Restricted Project.Tue, Aug 27, 11:44 PM
alangenfeld resigned from this revision.Wed, Aug 28, 8:23 PM
alangenfeld added subscribers: sashank, bengotow, alangenfeld.

ill let @sashank / @bengotow take this one

sashank added inline comments.Wed, Aug 28, 8:37 PM
js_modules/dagit/src/runs/LogViewer.tsx
47 ↗(On Diff #4050)

Nit: setOpenContentType? I usually see hooks named [stateVar, setStateVar] for consistency.

150 ↗(On Diff #4050)

Could we change the location of the output file to tmp/dagster/runs/${runId}? We already create this directory, and we wouldn't have to pass the user's username over gql.

js_modules/dagit/src/runs/PipelineRun.tsx
215 ↗(On Diff #4050)

nice :)

prha updated this revision to Diff 4128.Thu, Aug 29, 11:02 PM

switching to use string instead of [string] for stdout/stderr in the graphql endpoint
updating the file path

prha planned changes to this revision.Wed, Sep 4, 9:37 PM
prha updated this revision to Diff 4245.Thu, Sep 5, 12:13 AM

merge stdout/stderr views

prha updated this revision to Diff 4246.Thu, Sep 5, 12:32 AM

fix tests

schrockn resigned from this revision.Thu, Sep 5, 1:22 AM
schrockn added a subscriber: schrockn.

Like @alangenfeld, I'm not really qualified to review this.

One high-level thought is that if there end up being performance problems with the raw log streaming we might want to consider just straight HTTP chunked encoding and totally bypass graphql.

prha updated this revision to Diff 4247.Thu, Sep 5, 2:20 AM

fix snapshot tests

prha updated this revision to Diff 4261.Thu, Sep 5, 6:46 AM

missing export

prha updated this revision to Diff 4269.Thu, Sep 5, 3:29 PM

add generated types

prha updated this revision to Diff 4272.Thu, Sep 5, 4:45 PM

add feature flag gating

prha updated this revision to Diff 4324.Fri, Sep 6, 6:05 PM

rebase

prha updated this revision to Diff 4335.Fri, Sep 6, 6:41 PM

rebase

sashank accepted this revision.Fri, Sep 6, 7:01 PM

This is amazing! Just have one small nit, but otherwise looks good to me.

This isn't essential, but it would be nice to have the status indicator for the step within the log view too, so that the user knows when the step is still running and when the step ends/fails.

js_modules/dagit/src/plan/ComputeLogModal.tsx
83

Nit: It might be better to break this up into two Object.assigns since it's a bit hard to parse when it's nested. Also some intermediary variables with names would be helpful to understand what's going on.

This revision is now accepted and ready to land.Fri, Sep 6, 7:01 PM
prha updated this revision to Diff 4374.Fri, Sep 6, 11:51 PM

shashank comments

prha updated this revision to Diff 4376.Sat, Sep 7, 12:00 AM

update snapshot

prha updated this revision to Diff 4377.Sat, Sep 7, 12:10 AM

update snapshots

This revision was automatically updated to reflect the committed changes.
prha marked an inline comment as done.