Page MenuHomeElementl

[dagit] Simplify logs filtering input
ClosedPublic

Authored by dish on Feb 17 2021, 8:27 PM.

Details

Summary

Relates to #3688

The current logs filter input uses TokenizingField. We don't really need strict tokens here -- plain text with suggestions is fine. To that end, create:

  • useSuggestionsForString, a hook that accepts a value string and a function to determine suggestions, and exposes the list of suggestions and suggestion-selection behavior.
  • LogsFilterInput, which replaces the existing tokenized input. This will probably be something that can be abstracted out for use in place of our other tokenized inputs so that we can be consistent everywhere, but I'm one-offing it for now while we validate that this does what we need.

I also added some more value options to the type filter. This filter is a liiiitle hacky, as it just hunts for the type string within the __typename of the row node. This doesn't necessarily match up with the text in the actual type <Tag>, which isn't great. But I think if we want this to behave differently, it should be done in a followup.

Additionally, I made a couple changes to clean up the clutter of the LogsToolbar:

  • Remove the View Raw Step Output link, which is a horizontal space hog and seems redundant given that the logs themselves have this link. (Unless I'm missing something?)
  • Remove the "Clear" button, which doesn't seem critical.
  • Reduce the horizontal padding of the bar a little bit.

I have included a test and a storybook example for the hook. Video of LogsToolbar behavior attached in comments.

Test Plan

Buildkite.

Run Dagit, navigate to a run. Use logs toolbar to perform filtering behavior:

  • Test each of the suggestion options, using keyboard and trackpad to select suggestions and populate the input.
  • Verify that the URL updates correctly as the filter input changes, and that the logs filter correctly as well.

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

dish requested review of this revision.Feb 17 2021, 8:33 PM

Rebase, fix which code is in the diff

This looks awesome! Thanks for writing tests too since I imagine this will get pulled into the other places we have a tokenizing text field.

The only possible regression I see here is that the LogsToolbar no longer accepts external modifications to filter.logsQuery, since it bakes that value into the initial value of it's state? I can't quite remember, but I think clicking a step in the gannt chart would reset the selected steps and also the logsFilter and that linkage may be broken. That could be OK but I know it was a subject of debate when we first set it up 😅

Excited to replace the restrictive tokenizing version of this!

This revision is now accepted and ready to land.Feb 23 2021, 4:08 PM

The only possible regression I see here is that the LogsToolbar no longer accepts external modifications to filter.logsQuery, since it bakes that value into the initial value of it's state?

Good catch, I think this might be an oversight on my part. I'll see if I can put that back.

This revision was automatically updated to reflect the committed changes.