Licensing: Move shareReplay to the bottom

[afharo]

Summary

Potentially resolves #129589 (@lizozom can you help me out validating it?).

The license$ observable is consumed on every pre_respose hook in HAPI to add the header kbn-license-sig:

https://github.com/elastic/kibana/blob/3730dd0779ed8efd74aee88f57422781ec1ac122/x-pack/plugins/licensing/server/on_pre_response_handler.ts

Moving the shareReplay entry at the bottom of the pipe avoids performing the computation that we run today.

For simple requests like the one specified #129589, it may add up.

For maintainers

• This was checked for breaking API changes and was labeled appropriately

[afharo]

@lizozom can you help me run the tests from #129589 in this PR? I'd like to know if it'll make any impact.

[afharo]

@elasticmachine merge upstream

[afharo]

Running the test script linked in #129589, these are the side-by-side results:

(index)	rpm	statusCode	total time	total runs	avg runtime (main)	avg runtime (this PR)	avg size
0	'200'	'200'	1099	100	10.99	11.85	29
1	'500'	'200'	2834	250	11.336	10.816	29
2	'1000'	'200'	5756	500	11.512	10.908	29
3	'2000'	'200'	10807	1000	10.807	10.483	29
4	'3000'	'200'	16391	1500	10.927333333333333	9.376	29
5	'5000'	'200'	23359	2500	9.3436	9.0944	29
6	'7500'	'200'	32581	3750	8.688266666666667	8.44	29
7	'10000'	'200'	38630	5000	7.726	7.5372	29
8	'15000'	'200'	45082	7500	6.010933333333333	6.1912	29
9	'20000'	'200'	62240	10000	6.224	5.5565	29
10	'25000'	'200'	76010	12500	6.0808	6.26752	29
11	'30000'	'200'	93136	15000	6.209066666666667	6.3468	29
12	'40000'	'200'	7947177	19828 / 19767	400.805779705467	510.22345323013104	29
13	'40000'	'undefined'	NaN	172 / 233	NaN	NaN	NaN

Not a big change... although it's true that this PR achieved the shortest avg runtime (when loaded). But it may be circumstantial... 🤷

[elasticmachine]

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

[pgayvallet]

Assigning @gsoldevila to review as he was the last one to touch this observable chain and IIRC we spent some time discussing the order and flow of the licensing observable at the time, so there may be a reason why it was in that exact order.

[afharo]

@elasticmachine merge upstream

[kibana-ci]

💚 Build Succeeded

• Buildkite Build
• Commit: d561eb7

Metrics [docs]

✅ unchanged

History

• 💚 Build #78727 succeeded 47ea15a
• 💚 Build #72389 succeeded c7b53f3

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

[gsoldevila]

LGTM, this will cause the hasLicenseInfoChanged to be executed once per emission, rather than once per emission per subscription.

[gsoldevila]

That being said, I don't think this will bring any substantial performance improvements, as it will only reduce the number of calls for operators above the shareReplay:

const license$: Observable<ILicense> = fetched$.pipe(
    startWith(...startWithArgs),
>    pairwise(),
>    filter(([previous, next]) => hasLicenseInfoChanged(previous, next!)),
>    map(([, next]) => next!),
    shareReplay(1)
  );

Note that:

• The fetch operation (part of the fetched$ pipeline) was already "shared" when the shareReplay() was on top.
• Any subsequent pipelines that follow license$ will be executed once per subscription per emission (unless they too are shared). Thus, in order to reduce the number of calls to a potential hotspot / bottleneck, we must make sure we place a shareReplay() after the conflicting operator.

[afharo]

don't think this will bring any substantial performance improvements

I agree! The performance improvements will be very low:

• In limited CPUs, it could be a couple of ms at best.
• On an M1 Mac, we can see from my previous comment that it's just unperceivable.

However, #129589 tested out downloading /translations/en.json, whose average response times were between 10-20ms: In fact, it highlighted that the difference between full Kibana and without X-Pack plugins (which includes licensing) was 4ms.

Bearing in mind that licensing is the only plugin that intercepts the asset requests and the small difference between full Kibana and without X-Pack plugins, I think running these extra operators on every request may be the reason.

[gsoldevila]

That's a fair point! Let's see if we can shave those 4ms off then.
Either way, the fix won't do any harm and it makes the logic more efficient, so it's already worth it.