[licensing] add license fetcher cache #170006
Conversation
pgayvallet
added
Team:Core
| @@ -29,7 +30,7 @@ export function createLicenseUpdate( | |||
| ) { | |||
| const manuallyRefresh$ = new Subject<void>(); | |||
|
|
|||
| const fetched$ = merge(triggerRefresh$, manuallyRefresh$).pipe( | |||
| const fetched$ = merge(triggerRefresh$, manuallyRefresh$.pipe(debounceTime(1000))).pipe( | |||
There was a problem hiding this comment.
Some plugins call licensing.refresh() (exposed on the licensing plugin contract), and they are awaiting for the license back.
Using debounceTime here might have a negative impact, as it will add an extra 1 second delay systematically to those calls. throttleTime is probably a better alternative, but I have to confirm it works, as the description says subsequent emissions are "ignored".
There was a problem hiding this comment.
Yeah, good catch, but actually both are problematic. Using throttleTime will ignore the subsequent emissions, meaning that the wait for refresh could be from up to 30s (the interval based refresh)
TBH, I only added this because I thought it would be a quick win. As we've seen, the use of exhaustMap is likely sufficient.
So probably the correct thing to do is to revert this change and keep the code as it was, wdyt?
There was a problem hiding this comment.
I tested the throttleTime and it indeed ignores emissions, so it's not good either.
The picture they have in their website is misleading, as events that happen during the "throttle period" seem to be taken into account.
There was a problem hiding this comment.
UPDATE: throttleTime has a 3rd parameter, configuration, that serves precisely this purpose:
It isn't documented at all, but by playing around, I think I got to understand the meaning. The default config is:
export const defaultThrottleConfig: ThrottleConfig = {
leading: true,
trailing: false,
};
leading: false, trailing: falseall events are ignored.leading: true, trailing: falsedefault behavior (ignore trailing).leading: false, trailing: truesame behavior asdebounceTime.leading: true, trailing: truelet the first value (aka "leading") pass through, and it debounces subsequent (trailing) ones.
So, for our use case, we need the 4th option.
| return async () => { | ||
| const client = isPromise(clusterClient) ? await clusterClient : clusterClient; | ||
| try { | ||
| const response = await client.asInternalUser.xpack.info(undefined, { maxRetries: 3 }); |
There was a problem hiding this comment.
Just noticed maxRetries: 3 wasn't part of the old code, so IIUC we're now asking elasticsearch-js to retry 3 times in a row before actually reporting error 👍🏼
That's already an improvement, but I wonder if we could have a more resilient strategy using exponential backoff and perhaps a few more retries (5?). That would probably cover https://github.com/elastic/sdh-kibana/issues/4194 already.
Don't want to block the PR on this, it can be tackled with a separate issue.
I also agree that ES probably shouldn't report itself as available if the GET /_xpack API is not.
We can investigate/confirm if that's the case.
There was a problem hiding this comment.
Just noticed maxRetries: 3 wasn't part of the old code, so IIUC we're now asking elasticsearch-js to retry 3 times in a row before actually reporting error 👍🏼
It's actually the default value of the option. I'm not sure why I added it tbh, I think I wanted it to be more explicit. I should just remove it for now.
|
Pinging @elastic/kibana-core (Team:Core) |
💚 Build Succeeded
Metrics [docs]Page load bundle
History
To update your PR or re-run it, just comment with: |
This reverts commit 21c0b0b.
|
This was reverted with d7ab75f. It appears to have extended the runtime of a few test suites: Builds: |
## Summary #170006 was reverted because of significant increases of run duration on some of our FTR test suites, apparently because of the throttling that was added... This PR reopens #170006 without the throttling --------- Co-authored-by: kibanamachine <[email protected]>
Summary
Related to #169788
Fix #117394