Upgrade RxJS to 7 #129087

afharo · 2022-03-31T15:20:18Z

Summary

I needed an easy and repetitive task to clear my mind, so I thought I'd try what it would take to upgrade RxJS to the latest.

Fun fact: RxJS recently released 7.5.5, and we were sitting on 6.5.5. We're doing a whole major jump 😜

It might close #79751.

Motivation

Recently, we've identified a few performance issues related to RxJS observables:

[Performance] Kibana RSS / Memory usage seems higher in 8.2 vs previous releases (by 50% sometimes) #128061
https://github.com/elastic/kibana-team/issues/493

This PR is to check how hard would it be to upgrade. If green, we can take a look at https://github.com/elastic/kibana-load-testing/ to measure the effect as compared to main.

Main breaking changes

TS inference has been improved in RxJS v7 so there are some type errors to fix
observable.toPromise() is deprecated. firstValueFrom or lastValueFrom are the way to go now. Refer to https://rxjs.dev/deprecations/to-promise for a deeper explanation.
void Subjects now need to be explicit in their declaration new Subject<void>() or calls to .next() will fail because they expect one parameter.
Updated some snapshots because the internal properties of the observer changed.
Updated redux-observer to 2.0.0 because the previous version did not work with RxJS v7
The internal implementation of the Scheduler now does not depend on the setTimeout, so jest.advanceTimers do not work any longer. We can either delay or use the fakeScheduler helper in our tests.

Performance tests results

It appears that the overall response time has been improved, except for the Canvas scenario:

cc @dmlemeshko to make better sense of the data 🙏

APM data

cc @lizozom and @mshustov, would you mind helping out to retrieve the APM stats to find out how would this affect the memory consumption?

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

…rade-rxjs-to-7

dokmic

Expressions changes LGTM 👍

While searching the ingestion system for any [upcoming deprecations](#129087), I noticed these scripts are no longer used.

walterra

ML changes LGTM.

LeeDr

LGTM - only 2 files (package.json and yarn.lock) have QA team on them. Since this PR is targeting 8.3.0, we have a good amount of time before FF and GA to ensure there's no negative impacts and it helps to get it merged ASAP.

afharo · 2022-04-11T20:01:34Z

removing @elastic/kibana-app-services because this PR already got all the approvals from both sub-teams :)

EDIT: adding @mattkime

vadimkibana · 2022-04-12T09:25:38Z

x-pack/plugins/reporting/public/panel_actions/get_csv_panel_action.tsx

@@ -73,7 +74,7 @@ export class ReportingCsvPanelAction implements ActionDefinition<ActionContext>
 }

 public async getSearchSource(savedSearch: SavedSearch, _embeddable: ISearchEmbeddable) {
- const [{ uiSettings }, { data }] = await this.startServices$.pipe(first()).toPromise();
+ const [{ uiSettings }, { data }] = await firstValueFrom(this.startServices$.pipe(first()));


nit: maybe first() is not necessary here:

Suggested change

const [{ uiSettings }, { data }] = await firstValueFrom(this.startServices$.pipe(first()));

const [{ uiSettings }, { data }] = await firstValueFrom(this.startServices$);

gmmorris · 2022-04-12T16:18:19Z

x-pack/plugins/task_manager/server/polling/delay_on_claim_conflicts.test.ts

+ const delays$ = firstValueFrom(
+ delayOnClaimConflicts(of(maxWorkers), of(pollInterval), taskLifecycleEvents$, 80, 2).pipe(
+ take(2),
+ bufferCount(2)
+ )
+ );


All of these refactoring introducing firstValueFrom seem to have dropped the typing.
Could we bring those back?
Other than that it looks good to me but I don't want to lose the type checks.

As agreed on Slack. Even when the types are correctly inferred, we want to be explicit about them. Added them back in the form of firstValueFrom<number[]>(...)

gmmorris

LGTM once the typing is addressed 👍

kibana-ci · 2022-04-12T19:40:56Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`securitySolution`	2767	2766	-1
`timelines`	276	270	-6
total			-7

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`@kbn/std`	63	59	-4

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`apm`	2.8MB	2.8MB	+20.0B
`canvas`	1.0MB	1.0MB	+27.0B
`cases`	283.8KB	283.8KB	+1.0B
`cloud`	15.1KB	15.1KB	+1.0B
`cloudSecurityPosture`	371.1KB	371.1KB	+25.0B
`controls`	437.6KB	437.6KB	+84.0B
`core`	132.3KB	132.3KB	+1.0B
`dashboard`	295.4KB	295.4KB	+16.0B
`dataViewFieldEditor`	148.0KB	148.0KB	-9.0B
`discover`	407.0KB	407.2KB	+152.0B
`enterpriseSearch`	1.5MB	1.5MB	+1.0B
`fileUpload`	821.1KB	821.1KB	+1.0B
`fleet`	692.7KB	692.7KB	+1.0B
`grokdebugger`	59.1KB	59.1KB	+3.0B
`kibanaReact`	212.0KB	212.0KB	+2.0B
`kibanaUtils`	51.6KB	51.6KB	+1.0B
`lens`	1.0MB	1.0MB	+1.0B
`maps`	2.7MB	2.7MB	+69.0B
`mapsEms`	81.4KB	81.4KB	+1.0B
`ml`	3.3MB	3.3MB	+128.0B
`monitoring`	471.1KB	471.1KB	+1.0B
`osquery`	998.5KB	998.7KB	+145.0B
`security`	493.7KB	493.7KB	+3.0B
`securitySolution`	4.8MB	4.8MB	+93.0B
`snapshotRestore`	258.8KB	258.8KB	+1.0B
`stackAlerts`	202.8KB	202.9KB	+22.0B
`unifiedSearch`	138.3KB	138.3KB	+1.0B
`ux`	170.2KB	170.2KB	+1.0B
`visTypeTimeseries`	470.6KB	470.6KB	+6.0B
total			+799.0B

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`@kbn/std`	2	1	-1

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`cloudSecurityPosture`	4.4KB	4.4KB	+48.0B
`core`	319.1KB	319.1KB	+5.0B
`crossClusterReplication`	11.9KB	11.9KB	-19.0B
`dashboard`	66.7KB	66.7KB	+1.0B
`data`	422.5KB	422.6KB	+85.0B
`dataViewFieldEditor`	24.4KB	24.4KB	-59.0B
`fleet`	110.9KB	110.9KB	+1.0B
`grokdebugger`	4.7KB	4.7KB	-19.0B
`indexLifecycleManagement`	27.1KB	27.0KB	-19.0B
`indexManagement`	27.2KB	27.2KB	+1.0B
`infra`	88.2KB	88.2KB	+21.0B
`kbnUiSharedDeps-npmDll`	4.8MB	4.8MB	-31.1KB
`kbnUiSharedDeps-srcJs`	3.8MB	3.8MB	-766.0B
`kibanaUtils`	67.9KB	67.9KB	+1.0B
`licenseManagement`	10.2KB	10.2KB	-19.0B
`licensing`	8.6KB	8.6KB	-10.0B
`osquery`	16.2KB	16.2KB	+48.0B
`painlessLab`	10.5KB	10.5KB	-19.0B
`reporting`	38.9KB	38.9KB	-18.0B
`searchprofiler`	21.9KB	21.9KB	-19.0B
`securitySolution`	250.5KB	248.3KB	-2.2KB
`telemetry`	27.6KB	27.6KB	+1.0B
`timelines`	286.8KB	282.0KB	-4.8KB
`upgradeAssistant`	19.3KB	19.3KB	+1.0B
total			-38.8KB

Unknown metric groups

API count

id	before	after	diff
`@kbn/std`	96	92	-4

ESLint disabled line counts

id	before	after	diff
`core`	56	57	+1

Total ESLint disabled count

id	before	after	diff
`core`	66	67	+1

History

💔 Build #37658 failed a999efd
💚 Build #37539 succeeded d12ca24
💚 Build #37410 succeeded ef0a373
💛 Build #36376 was flaky f8b103e
💛 Build #36248 was flaky fde178f
💔 Build #36228 failed 7865243

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

…disable-server-side * 'main' of github.com:elastic/kibana: (35 commits) [Uptime] remove latency limit warnings when using monitor management (elastic#129597) [Security Solution] [ReponseOps] Executes Cases Cypress test when there is a change on cases plugin (elastic#129992) Paramaterized Discover tests (elastic#129684) [Security Solution][Investigations] - Minor bug fixes (elastic#130054) [DOCS} Adds technical preview to Lens annotations (elastic#130058) [Security solution] [Endpoint] Revisit blocklist wrong labels (elastic#128773) [Security Solutions] Adds API docs for value lists (elastic#129962) [CI] Move jest tests to spot instances, and fix spot retries in PRs (elastic#130045) chore(NA): upgrades rules_node_js to v5.4.0 (elastic#130051) [SecuritySolution] Remove the cell hovers actions for agent status (elastic#130042) Upgrade RxJS to 7 (elastic#129087) [SecuritySolution] Clean up CaseContext (elastic#130036) Revert "chore(NA): upgrades rules_node_js to v5.4.0 (elastic#130021)" Use RuleDataReader to query for threshold signal history (elastic#129763) Remove securityRulesCancelEnabled setting and set shorter default timeouts (elastic#129769) Upgrade EUI to v54.0.0 (elastic#129653) [Security Solution] More Ransomware exceptionable fields (elastic#130039) Add e2e for the apm integration policy form (elastic#129860) chore(NA): upgrades rules_node_js to v5.4.0 (elastic#130021) [ML] Fix Single Metric Viewer chart failing to load if no points during calendar event (elastic#130000) ... # Conflicts: # x-pack/plugins/screenshotting/server/screenshots/index.test.ts

…rint-media-attempt-2 * 'main' of github.com:elastic/kibana: (75 commits) [Lens] Hide disabled toolbar entries (elastic#129994) Fix explore tables don't display data when a global filter is applied (elastic#130024) [Console] Add option to disable keyboard shortcuts (elastic#128887) [Discover] Update refreshOnClick flaky test (elastic#130001) [Uptime] remove latency limit warnings when using monitor management (elastic#129597) [Security Solution] [ReponseOps] Executes Cases Cypress test when there is a change on cases plugin (elastic#129992) Paramaterized Discover tests (elastic#129684) [Security Solution][Investigations] - Minor bug fixes (elastic#130054) [DOCS} Adds technical preview to Lens annotations (elastic#130058) [Security solution] [Endpoint] Revisit blocklist wrong labels (elastic#128773) [Security Solutions] Adds API docs for value lists (elastic#129962) [CI] Move jest tests to spot instances, and fix spot retries in PRs (elastic#130045) chore(NA): upgrades rules_node_js to v5.4.0 (elastic#130051) [SecuritySolution] Remove the cell hovers actions for agent status (elastic#130042) Upgrade RxJS to 7 (elastic#129087) [SecuritySolution] Clean up CaseContext (elastic#130036) Revert "chore(NA): upgrades rules_node_js to v5.4.0 (elastic#130021)" Use RuleDataReader to query for threshold signal history (elastic#129763) Remove securityRulesCancelEnabled setting and set shorter default timeouts (elastic#129769) Upgrade EUI to v54.0.0 (elastic#129653) ... # Conflicts: # x-pack/plugins/screenshotting/server/formats/pdf/index.ts

afharo and others added 4 commits March 31, 2022 17:19

Upgrade RxJS to 7

b8a86f1

Minor TS complains

cdf9179

More cases

465346b

[CI] Auto-commit changed files from 'yarn kbn run build -i @kbn/pm'

b6accbd

afharo added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting v8.3.0 ci:deploy-cloud labels Apr 1, 2022

afharo added 22 commits April 1, 2022 02:36

Hopefully final round of fixes

826d974

Merge branch 'upgrade-rxjs-to-7' of github.com:afharo/kibana into upg…

61fa51c

…rade-rxjs-to-7

Merge branch 'main' of github.com:elastic/kibana into upgrade-rxjs-to-7

29f4cf3

fromEvent does not handle undefined anymore

283ce1f

Use promises instead of jest.timers to test the debounceTime logic

3853fa7

Update snapshot

38d6d4f

Upgrade redux-observer to 2.0.0 (compatible with v7)

af510fe

Change screenshotting Observable.create to new Observable

13df189

Update security snapshots

61c0df4

Use firstValueFrom with data.search

2a697f4

Fix redux-observable custom type extensions

ca719b4

Change mocks only specifying toPromise

5d6b6c9

APM tests aborted$ should never complete

1f0c7e0

Replace jest.advanceTimersBy with a promise delay

a18127b

Update jest snapshots

ac934dd

Fix expressions loader test

622ac1a

X-Pack jest snapshots

e82fb13

ML embeddables anomaly jest tests

84cb100

Update core snapshots

8523ddc

Jest snapshots for @kbn/shared-ux-components

9dd472c

Fix kbn-optimizer integration tests

bcc2c11

Fix reporting integration tests

7a3ece7

dokmic approved these changes Apr 6, 2022

View reviewed changes

dokmic requested a review from a team April 6, 2022 13:01

wayneseymour added a commit that referenced this pull request Apr 7, 2022

[QA][Code Coverage] Drop dead shell scripts, update docs (#129565)

401ad52

While searching the ingestion system for any [upcoming deprecations](#129087), I noticed these scripts are no longer used.

walterra approved these changes Apr 11, 2022

View reviewed changes

Merge branch 'main' of github.com:elastic/kibana into upgrade-rxjs-to-7

9127b05

paul-tavares approved these changes Apr 11, 2022

View reviewed changes

[CI] Auto-commit changed files from 'yarn kbn run build -i @kbn/pm'

f5c2fdd

LeeDr approved these changes Apr 11, 2022

View reviewed changes

joeypoon approved these changes Apr 11, 2022

View reviewed changes

Merge branch 'main' of github.com:elastic/kibana into upgrade-rxjs-to-7

ef0a373

afharo removed the request for review from a team April 11, 2022 20:00

afharo requested a review from mattkime April 11, 2022 20:16

mattkime approved these changes Apr 11, 2022

View reviewed changes

vadimkibana approved these changes Apr 12, 2022

View reviewed changes

afharo added 2 commits April 12, 2022 12:02

Address nit comment in review

5f203d4

Merge branch 'main' of github.com:elastic/kibana into upgrade-rxjs-to-7

d12ca24

gmmorris reviewed Apr 12, 2022

View reviewed changes

gmmorris approved these changes Apr 12, 2022

View reviewed changes

afharo added 2 commits April 12, 2022 18:30

Address responseOps PR feedback

4619dcf

Merge branch 'main' of github.com:elastic/kibana into upgrade-rxjs-to-7

a999efd

afharo enabled auto-merge (squash) April 12, 2022 16:54

Update snapshots with new rxjs properties

489d844

afharo merged commit 9d5aca5 into elastic:main Apr 12, 2022

afharo deleted the upgrade-rxjs-to-7 branch April 12, 2022 19:40

alexwizp mentioned this pull request Apr 14, 2022

Failing test: Jest Tests.src/plugins/charts/public/services/active_cursor - useActiveCursor should trigger cursor pointer update (chart type: datatable - time based, event type: time) #130177

Closed

kertal mentioned this pull request Jun 20, 2022

[Discover] Replace RxJS firstValueFrom with RxJS lastValueFrom to prevent problem with partial results #134682

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade RxJS to 7 #129087

Upgrade RxJS to 7 #129087

afharo commented Mar 31, 2022 •

edited

Loading

dokmic left a comment

walterra left a comment

LeeDr left a comment

afharo commented Apr 11, 2022 •

edited

Loading

vadimkibana Apr 12, 2022

gmmorris Apr 12, 2022

afharo Apr 12, 2022

gmmorris left a comment

kibana-ci commented Apr 12, 2022

API count

ESLint disabled line counts

Total ESLint disabled count

	const [{ uiSettings }, { data }] = await firstValueFrom(this.startServices$.pipe(first()));
	const [{ uiSettings }, { data }] = await firstValueFrom(this.startServices$);

Upgrade RxJS to 7 #129087

Upgrade RxJS to 7 #129087

Conversation

afharo commented Mar 31, 2022 • edited Loading

Summary

Motivation

Main breaking changes

Performance tests results

APM data

Checklist

For maintainers

dokmic left a comment

Choose a reason for hiding this comment

walterra left a comment

Choose a reason for hiding this comment

LeeDr left a comment

Choose a reason for hiding this comment

afharo commented Apr 11, 2022 • edited Loading

vadimkibana Apr 12, 2022

Choose a reason for hiding this comment

gmmorris Apr 12, 2022

Choose a reason for hiding this comment

afharo Apr 12, 2022

Choose a reason for hiding this comment

gmmorris left a comment

Choose a reason for hiding this comment

kibana-ci commented Apr 12, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Public APIs missing exports

Page load bundle

API count

ESLint disabled line counts

Total ESLint disabled count

History

afharo commented Mar 31, 2022 •

edited

Loading

afharo commented Apr 11, 2022 •

edited

Loading