Page MenuHomeElementl

[dagit] Use Python event types in log filter view
ClosedPublic

Authored by dish on Feb 18 2021, 5:41 PM.

Details

Summary

This diff aims to improve the clarity of type filtering in the logs view on a Run.

Currently, each log event has a "type", but our filter suggestions don't clearly map to the various displayed types, and many of the event types cannot be chosen at all. In master, we currently support ['expectation', 'materialization', 'engine', 'input', 'output', 'pipeline'], and we just do a string search to see if the string appears anywhere in the GraphQL typename. This behavior exists at an unusual intersection of _limited_, _confusing_, and _magical_.

Moreover, the displayed English strings in the tags aren't themselves searchable -- you kind of have to guess what to type in in order to get the right suggestion and subsequent filtered view.

Instead, I propose that we align all of the "type" values:

  • The DagsterEventType python enum
  • The text displayed in the tag on the logs table in Dagit
  • The filter suggestions in the logs filter input

Specifically, the python enum values will be exposed and rendered. The English strings will be replaced with the specific python values.

I'd then type type: and get the entire list of event type values, each of which correspond exactly to what is displayed on the log rows.

Test Plan

Load Dagit, view a Run. Use type: filter in logs table, verify correct suggestions and filtering behavior upon selection. Verify that the python values themselves are what appear in the log row tags.

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Feb 18 2021, 5:58 PM
Harbormaster failed remote builds in B26107: Diff 31874!
Harbormaster returned this revision to the author for changes because remote builds failed.Feb 18 2021, 6:23 PM
Harbormaster failed remote builds in B26113: Diff 31880!

We may want to toy with how exactly we show the tags for the events - they feel a little aggressive in current screen shot. Also think trying to call out the "log message" ie not-a-dagster-event types more clearly could be a nice improvement.

Overall I think this direction makes sense - better to just be transparent with whats happening under the hood if we need to accept the values back as input via search as well. The the complexity cost of maintaining some mapping scheme on output and input for the event types seems not worth it if the only benefit is "all caps underscore names are not elegant"

The the complexity cost of maintaining some mapping scheme on output and input for the event types seems not worth it if the only benefit is "all caps underscore names are not elegant"

Yeah, the English tag strings might be a little nicer when just visually scanning the logs, but as long as we're aiming to support concrete type filtering by typing in the suggestion input, it's going to be hard to cleanly match the suggestions with the English text. Options like type:"Pipeline Failed" as suggestions/tokens would not be great. Personally I don't really even mind the UPPERCASE_STRING tags -- it's a bunch of logs, so there shouldn't be much surprise that there would be machine-generated values in there.

dish requested review of this revision.Feb 18 2021, 7:48 PM

screenshot looks good. some of those events are deprecated and we should consider not rendering them

This looks good to me, I don't think the capitalized strings in the logs are too scary...

It's a bit of a API nit, but it's a little strange that each event has an event.eventType and an event.__typename which are in practice locked together string constants? Reminds me a bit of run.id and run.runId. It seems like eventType is a re-implementation of the automatic __typename concept with values we like better :-/ It would be nice if each event had a specific eventType value at the Typescript level, since at runtime only a specific fixed value is used for each event type, but I imagine the schema generator may not support that...

I wonder if we could instead create a "__typename to eventType" mapping on the front-end that is required by Typescript to contain every DagsterEventType and every typename? We could use the TS never check to make sure that any new types that are added have mappings defined...

Either way though I don't think it's a big deal!

This revision is now accepted and ready to land.Feb 22 2021, 9:42 PM

Resolve conflicts, rebase.