Improve ui settings performance#84513
Conversation
mshustov
added
Team:Core
|
Pinging @elastic/kibana-core (Team:Core) |
pgayvallet
left a comment
pgayvallet
left a comment
There was a problem hiding this comment.
LGTM. The perf improvement for 95pct looks amazing. Do you have the numbers for 50 and 75pct just FMI?
| * under the License. | ||
| */ | ||
| const oneSec = 1000; | ||
| const defMaxAge = 5 * oneSec; |
There was a problem hiding this comment.
A side question for my info: How was a default of 5 seconds decided? I couldn't find a reference to how the decision was made in #82218
There was a problem hiding this comment.
I chose a value that is slightly longer than Global Information: 4636ms before the fix.
|
@pgayvallet added numbers for 50th & 75th percentiles |
|
Do we have other options to address the UI settings performance besides caching? For HA deployments of Kibana, we'll potentially have a stale cache entry for five seconds. Almost none of our automated tests run against a HA deployment of Kibana, so our automated tests won't catch the side-effects this introduces. At a minimum, we shouldn't be backporting this to be released in a patch version because this change is inherently "unsafe" with our current level of testing. |
Yeah, it’s listed as a tradeoff in the parent issue We can reduce cache TTL from 5s to 1s to minimize race condition probability for the current implementation. WDYT? @kobelb
Got it, removed the |
💚 Build SucceededMetrics [docs]Distributable file count
History
To update your PR or re-run it, just comment with: |
|
😳 I think I previously skipped over a very important part of the issue description:
Is my understanding correct that this cache is only per-request with a max TTL of 5 seconds? If so, that seems much safer to me, and would be equivalent to loading all of the UI Settings when a HTTP request is initiated and using them throughout the HTTP request. If that's correct, this all LGTM, and apologies for raising the initial concern. |
Yes. That's how it works. |
* remove unused parameter in "read" function * add cache for uiSettings client * add tests for ui_settings client caching * address comments * do not mutate ui_settings_client cache
## Summary
This is a performance enhancement to the UI Settings service. It
introduces a space-aware, process-wide cache layer for the settings
saved object. It applies to `.get('setting-name')` lookups on all UI
settings clients.
### Justification
When a dashboard loads, search-related settings need to be consulted
before each search request. That can mean 20+ saved object lookups (see
#84923) which are each happening
in different requests, so the [per-request
cache](#84513) doesn't cover it.
### Caching approach
`NamespacedCache` is the class that handles caching. It stores both
resolved settings objects and in-flight requests to deduplicate read
operations (`getUserDefined`).
The server-side cache is freshened in three scenarios
- A setting is changed (`setMany`)
- A browser requests `index.html`
- A settings lookup occurs after the TTL has expired
The browser UI settings client is guaranteed to get fresh settings page
loads (this preserves the existing UX for changing settings).
In a multi-node scenario, the node performing a setting change gets fast
consistency. Other nodes get eventual consistency.
### Checklist
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
---------
Co-authored-by: Stratou <efstratia.kalafateli@elastic.co>
…c#262618) ## Summary This is a performance enhancement to the UI Settings service. It introduces a space-aware, process-wide cache layer for the settings saved object. It applies to `.get('setting-name')` lookups on all UI settings clients. ### Justification When a dashboard loads, search-related settings need to be consulted before each search request. That can mean 20+ saved object lookups (see elastic#84923) which are each happening in different requests, so the [per-request cache](elastic#84513) doesn't cover it. ### Caching approach `NamespacedCache` is the class that handles caching. It stores both resolved settings objects and in-flight requests to deduplicate read operations (`getUserDefined`). The server-side cache is freshened in three scenarios - A setting is changed (`setMany`) - A browser requests `index.html` - A settings lookup occurs after the TTL has expired The browser UI settings client is guaranteed to get fresh settings page loads (this preserves the existing UX for changing settings). In a multi-node scenario, the node performing a setting change gets fast consistency. Other nodes get eventual consistency. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios --------- Co-authored-by: Stratou <efstratia.kalafateli@elastic.co>
* remove unused parameter in "read" function * add cache for uiSettings client * add tests for ui_settings client caching * address comments * do not mutate ui_settings_client cache
7 participants
Summary
Closes #82218
Adds caching layer with TTL to uiSettings client.
Every SOC used in UiSettings is scoped to a KibanaRequest, thus the uiSettings cache cannot be shared between users.
Response time 95th ptc on 7.10 branch
50th ptc
75th ptc
Checklist