Loki: Cache extracted labels #75842
Loki: Cache extracted labels #75842
Conversation
|
Backend code coverage report for PR #75842 |
|
Frontend code coverage report for PR #75842
|
|
Hello @gtk-grafana!
Please, if the current pull request addresses a bug fix, label it with the |
...urce/loki/components/monaco-query-field/monaco-completion-provider/CompletionDataProvider.ts
Outdated
Show resolved
Hide resolved
| @@ -84,4 +84,12 @@ export interface ContextFilter { | |||
| description?: string; | |||
| } | |||
|
|
|||
| export interface ExtractedLabelKeys { | |||
| @@ -16,7 +16,10 @@ export class CompletionDataProvider { | |||
| constructor( | |||
| private languageProvider: LanguageProvider, | |||
| private historyRef: HistoryRef = { current: [] } | |||
| ) {} | |||
| ) { | |||
| this.queryToLabelKeysCache = new Map(); | |||
There was a problem hiding this comment.
We could use LRU for the added feature of smarter cache purging (instead of purging the first inserted, we'd be purging the least recently used), but that's a lot of additional overhead
I am curious about what do you mean by but that's a lot of additional overhead, cause we already use LRUCache in LokiLanguageProvider and I personally like that it handles all logic for purging for us, and we just need to set and get. Which makes code much cleaner and it is more consistent with Loki codebase.
There was a problem hiding this comment.
We do already use the LRU cache, but in #75760 I'm proposing to replace it with the browser cache, so I didn't want to go all-in on LRU in case we want to get away from it.
If we're ok using a bit more memory we could certainly use LRU and/or increase the size of the cache, for this MVP I was going for the fastest/lightest possible implementation.
Swapping out for LRU should be trivial as it has the same interface as Map, we'll just need to delete the bit where we manually delete the oldest element and swap out the Map initializer for LRUCache
There was a problem hiding this comment.
I guess the question is how many items do we want to cache, and how long do we want to cache them, and how bad are stale records being served throughout the session?
I guess the question becomes: is the goal of this cache to prevent duplicate requests, or to cache every possible request in a session?
If we want to cache "everything" or as much as possible, we should use LRU, or beef up the size of the map.
If we're worried about stale results we should range snap and check the date ranges when checking the cache (although in the getParserAndLabelKeys we don't have the time range in scope), or stick to a small cache size so results aren't likely to stick around for a long time.
LRU is ~49K, which IMO is a tad overkill for a map interface that keeps track of how many times each element is hit.
gtk-grafana
deleted the
gtk-grafana/logs/issues/75512/extracted-label-keys-memoize
branch
* add simple cache to extracted label values in completion provider (cherry picked from commit 5b63cdb)
* add simple cache to extracted label values in completion provider
What is this feature?
Adds simple Map to cache the previous results of Loki data samples in the Loki language provider.
Why do we need this feature?
loki-data-samples queries are currently triggered directly from the Monaco completion callbacks, and not part of the react/UI application layer, so certain keystrokes in certain contexts will trigger another query. When this query takes a long time to complete, the editor UX is very sluggish and difficult to work with. Since the values returned by this query are extracted label values, which are not expected to change second-to-second, the solution proposed here is to cache requests and serve "stale" labels to prevent needing to wait for an api request whenever the user presses spacebar or comma inside a stream selector.
TL;DR;
To prevent duplicate API calls getting labels in autocomplete, we cache 2 unique query strings and their associated labels.
Who is this feature for?
Users of Loki code editor.
Which issue(s) does this PR fix?:
Fixes #75512
Special notes for your reviewer:
This is the lightest implementation I could imagine in terms of CPU/memory consumption, worried about cases where this list of labels is quite long.
We could use LRU for the added feature of smarter cache purging (instead of purging the first inserted, we'd be purging the least recently used), but that's a lot of additional overhead, and the API between map and LRU are very similar, so swapping it out would be very easy.
Please check that: