Paths

Table of Contentst

Differential D9181

allow step failure events to have no serialized error
ClosedPublic
Actions

Authored by • dgibson on Aug 2 2021, 1:06 PM.

Tags

None

Referenced Files

	Unknown Object (File)
	Sun, Aug 14, 8:18 AM

	Unknown Object (File)
	Sat, Aug 13, 4:33 AM

	Unknown Object (File)
	Fri, Aug 12, 4:23 PM

	Unknown Object (File)
	Fri, Aug 12, 4:23 PM

	Unknown Object (File)
	Thu, Aug 11, 10:58 PM

	Unknown Object (File)
	Thu, Aug 11, 7:22 AM

	Unknown Object (File)
	Sat, Aug 6, 7:13 AM

	Unknown Object (File)
	Sun, Jul 31, 5:24 AM

View All 48 Files

Subscribers

• jordansanders

Details

Reviewers

• dish
alangenfeld
• prha

Commits

R1:6b8d319f822a: allow step failure events to have no serialized error

Summary

StepFailureData lets the error passed in be empty, but then when the event log query goes to create the event, it throws if there is no error set.

This diff makes the server more resilient to empty errors, and changes the frontend to handle step failure events with no serialized error data (falling back to rendering the message of the event instead of the full error, which we have pretty good frontend support for already)

However, before we should land this, we should gain understanding of why this serverside graphql error seems to *completely break the dagit server and stop it from serving any more request until the server is restarted*, at least when it is thrown in certain situations.

Test Plan

BK, raise a step failed error using the k8s executor, dagit no longer bricks, logs from that run render reasonably

Diff Detail

Repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• dgibson created this revision.Aug 2 2021, 1:06 PM

• dgibson added inline comments.Aug 2 2021, 1:15 PM

js_modules/dagit/packages/core/src/runs/LogsRow.tsx
82	@alangenfeld i looked over https://sourcegraph.com/github.com/dagster-io/dagster/-/commit/b57b91ea7666da41a3dbc610116aed500c647d84 and it wasn't immediately clear to me what the consequences were of removing this line - the reason I did was that it looked bad for step failure events that were just a single line (due to having no stack trace to fill up space - you couldn't even see the context, it was just a big "Click to expand").

Harbormaster completed remote builds in B34541: Diff 42707.Aug 2 2021, 1:49 PM

• dgibson requested review of this revision.Aug 2 2021, 1:49 PM

what condition did we add where we have a step failure with no exception?

js_modules/dagit/packages/core/src/runs/LogsRow.tsx
82	I believe the thinking was that the context header based on the source type is hidden information if the message isn't long enough to trigger the dialog window

step failure event with no serialized error info: https://sourcegraph.com/github.com/dagster-io/dagster/-/blob/python_modules/libraries/dagster-k8s/dagster_k8s/executor.py?L233:42

The data classes don't yell and allow an empty error to be passed in (and such events are now in user DBs for better or for worse)

make sure we update the mypy type on StepFailureData which is also not set as Optional

mypy

alangenfeld accepted this revision.Aug 2 2021, 5:02 PM

alangenfeld added inline comments.

js_modules/dagit/packages/core/src/runs/LogsRow.tsx
82	we should have @dish do a follow up pass for cleanup on these to be safe

This revision is now accepted and ready to land.Aug 2 2021, 5:02 PM

Harbormaster failed remote builds in B34550: Diff 42721!Aug 2 2021, 5:07 PM

• dish added inline comments.Aug 2 2021, 5:11 PM

js_modules/dagit/packages/core/src/runs/LogsRow.tsx
82	Do you have before/after screenshots?

up

@dish before:

after:

This revision was landed with ongoing or failed builds.Aug 2 2021, 5:39 PM

Closed by commit R1:6b8d319f822a: allow step failure events to have no serialized error (authored by • dgibson). · Explain Why

This revision was automatically updated to reflect the committed changes.

• dgibson added a commit: R1:6b8d319f822a: allow step failure events to have no serialized error.

Harbormaster completed remote builds in B34555: Diff 42729.Aug 2 2021, 6:14 PM

Revision Contents
Changeset List

Path

Size

js_modules/

dagit/

packages/

core/

src/

graphql/

6 lines

runs/

14 lines

LogsRowStructuredContent.tsx

3 lines

types/

LogsRowStructuredFragment.ts

LogsScrollingTableMessageFragment.ts

PipelineRunLogsSubscription.ts

RunLogsQuery.ts

RunPipelineRunEventFragment.ts

python_modules/

dagster-graphql/

dagster_graphql/

implementation/

6 lines

schema/

logs/

6 lines

dagster/

dagster/

core/

execution/

plan/

4 lines

Diff 42731

js_modules/dagit/packages/core/src/graphql/schema.graphql

Loading...

js_modules/dagit/packages/core/src/runs/LogsRow.tsx

Loading...

js_modules/dagit/packages/core/src/runs/LogsRowStructuredContent.tsx

Loading...

js_modules/dagit/packages/core/src/runs/types/LogsRowStructuredFragment.ts

Loading...

js_modules/dagit/packages/core/src/runs/types/LogsScrollingTableMessageFragment.ts

Loading...

js_modules/dagit/packages/core/src/runs/types/PipelineRunLogsSubscription.ts

Loading...

js_modules/dagit/packages/core/src/runs/types/RunLogsQuery.ts

Loading...

js_modules/dagit/packages/core/src/runs/types/RunPipelineRunEventFragment.ts

Loading...

python_modules/dagster-graphql/dagster_graphql/implementation/events.py

Loading...

python_modules/dagster-graphql/dagster_graphql/schema/logs/events.py

Loading...

python_modules/dagster/dagster/core/execution/plan/objects.py

Loading...