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

Migrate injected UiSettings from legacyMetadata #22779

Open
spalger opened this issue Sep 6, 2018 · 8 comments
Open

Migrate injected UiSettings from legacyMetadata #22779

spalger opened this issue Sep 6, 2018 · 8 comments
Labels
chore Feature:Legacy Removal Issues related to removing legacy Kibana Feature:New Platform Feature:uiSettings old Used to help sort old issues on GH Projects which don't support the Created search term. Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience)

Comments

@spalger
Copy link
Contributor

spalger commented Sep 6, 2018

In #22694 we are moving the creation of the UiSettingsClient from the legacy platform to the new platform, but we intentionally left the UiSettingsClient dependent on the legacy metadata to avoid too many unnecessary changes in this PR.

The legacy metadata currently includes two objects that are merged together to create an internal cache of the ui settings state: uiSettings.defaults and uiSettings.user. Once these objects are merged they are used to lookup information about each settings and are regularly mutated when settings are changed, sometimes by directly setting certain properties and sometimes by merging in responses from the uiSettings API. This makes it hard for me to understand what the state of the internal cache is after a given operation, so I'd like to rethink things a little bit starting by refactoring the injected vars and then refactoring the way they're consumed by the UiSettingsClient.

@spalger spalger added chore Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:New Platform labels Sep 6, 2018
@spalger spalger self-assigned this Sep 6, 2018
@mshustov
Copy link
Contributor

Kibana stores all the uiSettings in Elasticsearch. Shouldn't Elasticsearch be the only source of truth for the uiSettings? A user can change a setting in another tab, or have a long-running session with NP SPA mode. Effectively it means we should not use local state on the client anymore.

@pgayvallet
Copy link
Contributor

pgayvallet commented Nov 12, 2020

Kibana stores all the uiSettings in Elasticsearch. Shouldn't Elasticsearch be the only source of truth for the uiSettings? A user can change a setting in another tab, or have a long-running session with NP SPA mode. Effectively it means we should not use local state on the client anymore.

I would like to agree, however looking at the current implementation of the client-side client in src/core/public/ui_settings/ui_settings_client.ts:

  • We will have to remove the observable-based APIs such as get$, getSaved$, getUpdate$ as they are all only aware of the client-side changes. Not sure how many consumers of those we have.
  • We would have to perform client->server calls everytime get is called. We could batch these, as we do with SavedObjectClient.get, but this will still have impacts on performances (atm we are never calling the server when using get, we always rely on the client-side local cache)
  • We probably will have to remove the underlying batching of client->server settings update calls, which could have a significative impact on performances too. We could be batching these too, but that means still having some kind of cache on the client-side to preserve the updated values until the batch update http call is fired.

So overall, these may not be trivial changes. Do we want to do this at the same time we are removing usages of legacyMetadata, or should we be doing that at a later time?

@spalger
Copy link
Contributor Author

spalger commented Nov 12, 2020

I personally don't think there's value in making uiSettings async. The things that are changed through uiSettings have historically been things which can update on the next refresh without issue. Is that changing or are we just trying to make things more conceptually pure? I think a little client side caching for something like this is pretty practically useful and hasn't caused an issue in the past AFAIK.

@pgayvallet
Copy link
Contributor

pgayvallet commented Dec 1, 2020

The things that are changed through uiSettings have historically been things which can update on the next refresh without issue

That was probably alright when switching application was causing a full page reload. Now that Kibana is a real SPA, the uiSettings are effectively loaded only once, at the initial application startup, which make me wonder if this is still acceptable.

@spalger
Copy link
Contributor Author

spalger commented Dec 1, 2020

Now that Kibana is a real SPA, the uiSettings are effectively loaded only once, at the initial application startup

Yeah, that's something I hadn't considered. Still don't think the work required to make the APIs async/observable and convert all the usage to the updated APIs is worth it but 🤷

@pgayvallet
Copy link
Contributor

Still don't think the work required to make the APIs async/observable and convert all the usage to the updated APIs is worth it

It may not be, I'm genuinely asking the question. Some kind of interval-based resync from the client to the server could also be an option that would preserve the client-side API as synchronous.

@spalger
Copy link
Contributor Author

spalger commented Dec 4, 2020

Yeah, perhaps it could be integrated into app router switching so that the uiSettings won't change while a user is in the middle of using an app, but if they haven't been updated in a while core could refresh them on the next app -> app transition, before mounting the app?

@TinaHeiligers TinaHeiligers added Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) and removed Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc labels Feb 14, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/appex-sharedux (Team:SharedUX)

@petrklapka petrklapka added the old Used to help sort old issues on GH Projects which don't support the Created search term. label Sep 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Feature:Legacy Removal Issues related to removing legacy Kibana Feature:New Platform Feature:uiSettings old Used to help sort old issues on GH Projects which don't support the Created search term. Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience)
Projects
None yet
Development

No branches or pull requests

10 participants