[Telemetry plugin] Do not await for getOptInStatus() on stop()

[gsoldevila]

In the scope of graceful shutdown, we want to ensure all async operations are correctly finished when stopping the Kibana server.

[elasticmachine]

Pinging @elastic/kibana-core (Team:Core)

[afharo]

I'm not sure the proposed changes will trigger the optIn call in the setup method. Can we proactively test that?

That said, a huge ++ to removing the await of this.optInPromise in the stop method.

[afharo]

Suggested change

	const internalRepositoryClient = await firstValueFrom(
	// if the Observable completes without emitting, firstValueFrom will trigger an exception
	this.savedObjectsInternalClient$.pipe(catchError((err) => of(undefined)))
	);
	const internalRepositoryClient = await firstValueFrom(
	this.savedObjectsInternalClient$,
	{ defaultValue: undefined },
	);

[afharo]

Why 3s? AFAIK, this.config$ should be immediate (we use the subscriber to account for config reloads)

[gsoldevila]

If the config does not change no new emissions will happen. Thus, the RxJS pipeline defined in the constructor will get stuck, cause we are using exhaustMap.

I was using this 3 second timeout to unblock the pipeline and proceed to the next emission of the timer(), which will takeUntil(this.pluginStop$) and complete the RxJS pipeline.

Perhaps another option would be to use switchMap instead of exhaustMap, this way, we would be cancelling the firstValueFrom(this.config$) every OPT_IN_POLL_INTERVAL_MS if it hasn't finished. WDYT?

[gsoldevila]

Ideally, the config$ Observable that we obtain from ConfigService should complete on stop, but this does not seem to be the case ATM:

https://github.com/elastic/kibana/blob/main/packages/kbn-config/src/config_service.ts#L250-L255

[gsoldevila]

FYI I just tried the switchMap approach and it works. It is a bit simpler and does not require this 3 seconds timeout, so I updated the PR with these changes.

[gsoldevila]

I'm not sure the proposed changes will trigger the optIn call in the setup method. Can we proactively test that?

Good catch! Just added a unit test for that.

[gsoldevila]

I just noticed we could remove the async keyword from this function, but it would alter the setup contract. IDK if it's worth creating a separate task WDYT?

[afharo]

The plan is to deprecate that method in favour of the observable: #122632 Let's keep it as is for now :)

[afharo]

I have a concern about switchMap: IMO, it may lead to accumulating calls to getOptInStatus if OPT_IN_POLL_INTERVAL_MS is faster than the request.

What do you think?

[afharo]

can this lead to memory leaks?

If getOptInStatus takes longer than OPT_IN_POLL_INTERVAL_MS to execute, switchMap will trigger a new call on every tick of the timer.

[gsoldevila]

You've got a very valid point here. From this article I thought that the inner Observable was cancelled with each new higher order emission, including cancelling ongoing HTTP calls. Then, I performed a test to make sure, and realised it must be the Angular HTTP client doing the magic behind the scenes (using an abort signal or similar).

I'll try to find another way

[gsoldevila]

I switched back to exhaustMap and moved the takeUntil operator after the exhaustMap. This way, the RxJS pipeline seems to be completed() correctly.

[kibana-ci]

💛 Build succeeded, but was flaky

• Buildkite Build
• Commit: a89a2df
• Storybooks Preview
• Webpack Bundle Analyzer

Failed CI Steps

• Jest Tests #6

Test Failures

• [job] [logs] Jest Tests #6 / APMEventClient cancels a search when a request is aborted

Metrics [docs]

✅ unchanged

History

• 💛 Build #53767 was flaky 042b74a
• 💔 Build #53605 failed 7806a0e
• 💛 Build #53388 was flaky bc8ee21
• 💔 Build #53325 failed df83e2b

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

[afharo]

LGTM! As long as the CI goes green :)