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

[Lens] allow user to disable auto apply #125158

Conversation

drewdaemon
Copy link
Contributor

@drewdaemon drewdaemon commented Feb 9, 2022

Summary

Allows user to disable Lens's current auto-apply behavior. This enables them to save on resources associated with rendering a complex or large visualization.

Screen.Recording.2022-02-15.at.4.10.50.PM.mov

With full-screen datasource
Screen Shot 2022-02-16 at 10 31 56 AM

Resolves #74490

Filters and suggestions are still applied automatically. Open for feedback here.

@drewdaemon drewdaemon changed the title 74490/disable auto apply duplicate state strategy [Lens] disable auto apply duplicate state strategy Feb 9, 2022
@drewdaemon drewdaemon force-pushed the 74490/disable-auto-apply-duplicate-state-strategy branch from b85cd65 to 49069af Compare February 10, 2022 15:51
@drewdaemon drewdaemon force-pushed the 74490/disable-auto-apply-duplicate-state-strategy branch from c575d08 to 6e6f127 Compare February 11, 2022 15:30
@drewdaemon drewdaemon force-pushed the 74490/disable-auto-apply-duplicate-state-strategy branch from 900315a to 4811574 Compare February 14, 2022 19:47
Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

This looks really good to me.

There's one "bug" I found:

updateTimeRange(storeDeps.lensServices.data, store.dispatch);

is updating the search session id and the time range which is implicitly applying and sending out another request. To reproduce, set current time range to 15mins, turn off auto apply and wait for 20 seconds, then do a change - it will be applied automatically. I suggest we just turn off this "outdated now" behavior as long as auto apply is not active.

One behavior I found a bit strange: Changing query/filter/timerange is reloading the chart (which is expected), but it's also auto-applying all changes made to the config in the meantime. This is theoretically fine, but it's different than TSVB and Visualize agg based behavior and IMHO it's closer to what a user would expect. cc @ghudgins @MichaelMarcialis what do you think?

@drewdaemon
Copy link
Contributor Author

drewdaemon commented Mar 1, 2022

[the app is] updating the search session id and the time range which is implicitly applying and sending out another request. To reproduce, set current time range to 15mins, turn off auto apply and wait for 20 seconds, then do a change - it will be applied automatically. I suggest we just turn off this "outdated now" behavior as long as auto apply is not active.

Thanks for pinning this down. I had seen the behavior but hadn't yet been able to reproduce reliably. This has been addressed so that, while auto-apply is disabled, the time range is only updated when the user applies changes.

One behavior I found a bit strange: Changing query/filter/timerange is reloading the chart (which is expected), but it's also auto-applying all changes made to the config in the meantime. This is theoretically fine, but it's different than TSVB and Visualize agg based behavior and IMHO it's closer to what a user would expect.

I don't think anyone objected to this last week during my demo at the vis-editors sync, but I still agree with you that it could be unexpected behavior. After digging into it a bit, I found that this behavior is due to syncExistingFields being fired when we get new active data. I had to make it so that it immediately applies changes because otherwise I ran into the following problem:

  • user has auto-apply disabled in localStorage
  • navigates to Lens editor
  • syncExistingFields gets called when data loads
  • apply button turns blue before user has changed anything

It isn't clear how to address both of these issues. Any ideas?

@MichaelMarcialis
Copy link
Contributor

One behavior I found a bit strange: Changing query/filter/timerange is reloading the chart (which is expected), but it's also auto-applying all changes made to the config in the meantime. This is theoretically fine, but it's different than TSVB and Visualize agg based behavior and IMHO it's closer to what a user would expect. cc @ghudgins @MichaelMarcialis what do you think?

Per our discussion in today's vis editors sync, I think the current implementation (auto-applying all changes when a query/filter/date picker change is made) makes sense given that the reason to add this feature was to provide users an option that avoids having to perform multiple, performance-heavy re-renderings of the visualization. Applying only query/filter/date picker changes and no other changes asks that users also apply and re-render their visualization a second time, which goes against that original intent.

@drewdaemon
Copy link
Contributor Author

@elasticmachine merge upstream

@stratoula
Copy link
Contributor

The options on the top work fine now except from one! If I have defined a custom axis name such as here:
image
and clear the value, the apply button is not enabled:
image

…ithub.com:andrewctate/kibana into 74490/disable-auto-apply-duplicate-state-strategy
@drewdaemon
Copy link
Contributor Author

@stratoula thanks for the catch. I fixed it 👍

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
lens 1.1MB 1.1MB +5.0KB
Unknown metric groups

ESLint disabled line counts

id before after diff
lens 37 41 +4

Total ESLint disabled count

id before after diff
lens 39 43 +4

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

LGTM! I tested it once again and I can't find other bugs. Thanx @andrewctate for applying all the comments! 🥇

@drewdaemon drewdaemon merged commit e694507 into elastic:main Mar 4, 2022
lucasfcosta pushed a commit to lucasfcosta/kibana that referenced this pull request Mar 8, 2022
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 8, 2022
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 125158 or prevent reminders by adding the backport:skip label.

@stratoula stratoula added backport:skip This commit does not require backporting and removed backport missing Added to PRs automatically when the are determined to be missing a backport. labels Mar 8, 2022
lucasfcosta pushed a commit to lucasfcosta/kibana that referenced this pull request Mar 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Lens release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Lens] Make "auto-apply" behavior configurable
9 participants