Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Transformations: De-emphasize non-applicable transformations #76373

Merged
merged 20 commits into from
Oct 13, 2023

Conversation

codeincarnate
Copy link
Collaborator

@codeincarnate codeincarnate commented Oct 11, 2023

What is this feature?

This PR adds support to transformations for an applicator function which can be used to determine if the transformation is applicable to the current data.

Why do we need this feature?

Many transformations have base conditions needed for them to work correctly. Currently however, we don't do any checks to see if these base conditions are met. This makes for a confusing experience, as it's currently only possible to tell through experimentation or from prior knowledge what given transformations will work on a given data set.

Who is this feature for?

Users of transformations

Which issue(s) does this PR fix?:

Fixes #74199

Special notes for your reviewer:

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@codeincarnate codeincarnate requested a review from a team October 11, 2023 14:42
@codeincarnate codeincarnate requested review from a team as code owners October 11, 2023 14:42
@codeincarnate codeincarnate requested review from oscarkilhed, mdvictor, joshhunt, L-M-K-B and academo and removed request for a team October 11, 2023 14:42
@grafana-delivery-bot grafana-delivery-bot bot added this to the 10.2.x milestone Oct 11, 2023
@codeincarnate
Copy link
Collaborator Author

Oops, forgot to put this in draft 😢. Still needs a little to hammer out.

@joshhunt
Copy link
Contributor

neat! 📸

@oscarkilhed
Copy link
Contributor

Nice! Will review in the morning!

@codeincarnate
Copy link
Collaborator Author

Added a few things:

  • Moved to applicability levels. Under the hood there are four right now, but we reduce that to two. (i.e. we ignore the impossible outcome and the highly applicable outcome)
  • Adds HighlyApplicable, which would allow transformations to be highlighted in the future based on data (i.e. promote heatmap if heatmap data is found)
  • Updates the display to show a grayscale icon if disabled, add disabled to the title, and to add an info icon which contains the applicability message to help the user understand the source of the error.

Currently although they're marked as disabled they can still be used as we don't wish to make these unusable for cases that we haven't taken into account.

@codeincarnate
Copy link
Collaborator Author

codeincarnate commented Oct 13, 2023

@ryantxu if you could take another look it would be greatly appreciated. I moved to the applicability levels per your suggestion and I think we can get quite a lot from this! Would be great to know your thoughts in regard to the implementation there.

Copy link
Contributor

@oscarkilhed oscarkilhed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good!
There's a few other transformations that don't really work with all kinds of data. TimeSeriesToTable for instance, doesn't work with multiple fields in the same series. But adding and tuning these rules could be left for follow up PRs i guess.

@oscarkilhed
Copy link
Contributor

Two thoughts.
Maybe it's a good idea to sort the disabled transformations to the bottom. I also wouldn't mind if they were a bit more de-emphasized, the gray-scale on the icons is nice, but it's not super obvious.

The transformations are still selectable. I'm a bit torn on this, I think right now they need to be enabled, because there is no way to insert a transformation between two transformations, a transformation could be applicable before one of the applied transformations, but not after it. But it's also a bit weird having a transformation say that it's disabled but you can still apply it.

@codeincarnate
Copy link
Collaborator Author

Thanks for the comments Oscar!

  1. In terms of the conditions, I was hoping to get a decent base set. I think we can go pretty arbitrarily deep in this so didn't go crazy for this PR.
  2. In terms of the sorting to the bottom, there was some concern that they would end up being somewhat hidden. Given that we are pushing to move to a drawer interface for new transformations though this may be ok. I'll circle back with @catherineymgui about that.
  3. I'll play with some ways to make things look less uhhh selectable 😆

@codeincarnate
Copy link
Collaborator Author

@mdvictor @oscarkilhed if you could take another look at the presentation it could be appreciated 🙏

Copy link
Contributor

@oscarkilhed oscarkilhed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! 🎉

@codeincarnate codeincarnate merged commit 5e9dac6 into main Oct 13, 2023
18 checks passed
@codeincarnate codeincarnate deleted the transform/find-applicable branch October 13, 2023 16:21
Jguer pushed a commit that referenced this pull request Oct 13, 2023
* Stub types

* Update applicator

* Type and functionality updates

* Format time applicator

* Group by applicator

* Groupy to matrix applicator

* Use applicability levels

* Add  organize fields applicator

* Add applicator to heatmap

* Add disabled icons

* Update presentation

* Prettier

* Why run prettier once when you can do it twice?

* Update presentation

* Prettier

* Update presentation

* Prefer object syntax

* Betterer and Prettier
@zerok zerok modified the milestones: 10.2.x, 10.2.0 Oct 23, 2023
darrenjaneczek pushed a commit that referenced this pull request Oct 23, 2023
* Stub types

* Update applicator

* Type and functionality updates

* Format time applicator

* Group by applicator

* Groupy to matrix applicator

* Use applicability levels

* Add  organize fields applicator

* Add applicator to heatmap

* Add disabled icons

* Update presentation

* Prettier

* Why run prettier once when you can do it twice?

* Update presentation

* Prettier

* Update presentation

* Prefer object syntax

* Betterer and Prettier
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transformations: Make it harder to select unusable transformations from UI
5 participants