Migrate server-side ES domain to packages #136297

pgayvallet · 2022-07-13T14:46:18Z

Summary

Fix #136283

Create the following packages
- @kbn/core-elasticsearch-server
- @kbn/core-elasticsearch-client-server-internal
- @kbn/core-elasticsearch-client-server-mocks
- @kbn/core-elasticsearch-server-internal
- @kbn/core-elasticsearch-server-mocks
Move the content of src/core/server/elasticsearch accordingly
Adapt internal and external usages / imports

…-ref HEAD~1..HEAD --fix'

…ide-es-to-packages

…-ref HEAD~1..HEAD --fix'

elasticmachine · 2022-07-15T07:00:54Z

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

cjcenizal

Changes to imports in Upgrade Assistant tests LGTM.

elasticmachine · 2022-07-15T13:46:16Z

Pinging @elastic/fleet (Team:Fleet)

elasticmachine · 2022-07-15T13:46:17Z

Pinging @elastic/uptime (Team:uptime)

ymao1

Response Ops changes LGTM

juliaElastic

Fleet change LGTM

TinaHeiligers · 2022-07-18T17:07:52Z

packages/core/base/core-base-common/src/index.ts

@@ -9,3 +9,5 @@
 export type { PluginOpaqueId, PluginName, DiscoveredPlugin } from './plugins';
 export { PluginType } from './plugins';
 export { EUI_STYLES_GLOBAL } from './eui';
+export { ServiceStatusLevels } from './service_status';


core-base-common seems to be morphing into a "catch-all" I-don't-know-where-to-put-this-thing package. It's fine for now as the package is still very small. During the cleanup phase or slightly prior to that, we should audit everything in core-base-* packages and see if we could organize the components into a domain-like structure.

core-base-common seems to be morphing into a "catch-all" I-don't-know-where-to-put-this-thing package

Yup, I totally agree. The intent here is to avoid the 'lets create 99 another one-file packages before the end of the migration' effect. We move all the 'misc' stuff here, and we'll revisit once everything has been moved.

During the cleanup phase or slightly prior to that, we should audit everything in core-base-* packages and see if we could organize the components into a domain-like structure

Fully agree too.

From a quick search through core, it's elasticsearch, status, and savedObjects that all use the types from status. Am I missing any other core domains in this list?

No, AFAIK for now only the ES and SO services declare their statuses.

ashokaditya

security_solution/server/endpoint changes LGTM.

…ide-es-to-packages

nkhristinin

Reviewed security-solution-platform files. LGTM!

fkanout

Actionable Observability changes LGTM.

marshallmain

Detection alerts changes LGTM

TinaHeiligers · 2022-07-19T17:58:56Z

packages/core/elasticsearch/core-elasticsearch-client-server-internal/src/cluster_client.ts

-} from './retry_unauthorized';
+  ICustomClusterClient,
+} from '@kbn/core-elasticsearch-server';
+import type { ElasticsearchClientConfig } from '@kbn/core-elasticsearch-server';


nit: combine imports from @kbn/core-elasticsearch-server

TinaHeiligers · 2022-07-19T18:08:40Z

packages/core/elasticsearch/core-elasticsearch-client-server-internal/src/create_transport.ts

-} from '@elastic/transport';
-import type { TransportOptions } from '@elastic/transport/lib/Transport';
-import { Transport } from '@elastic/elasticsearch';
+import {


Aside: are we going to standardize combining imports from the same place in this manner rather than declaring type imports separately?

TinaHeiligers

I left a bunch of comments and questions but these aren't strictly related to this PR.
The tradeoffs, compromises, and tasks left for future work are all more than reasonable, considering that migrating the elasticsearch core domain is one of our largest.
Great work!
LGTM.

TinaHeiligers · 2022-07-19T18:19:51Z

...re/elasticsearch/core-elasticsearch-client-server-internal/src/scoped_cluster_client.test.ts

 import { ScopedClusterClient } from './scoped_cluster_client';

+const createEsClient = () => ({} as unknown as ElasticsearchClient);


Aside: I feel we need to come up with a simple rule of thumb on how to approach internal implementations' unit tests using what will be public mocks. With our dependency structure in the Kibana-as-packages world, we forbid that.

A rule of thumb could be:

if manual stubbing is easy, replace the mock with a manual stub.

if a mock depends on other mocks, create duplicates of these for use internally.

Does that sound reasonable as an interim solution?

TinaHeiligers · 2022-07-19T18:36:36Z

packages/core/elasticsearch/core-elasticsearch-client-server-mocks/src/mocks.ts

-import { ICustomClusterClient } from './cluster_client';
-import { PRODUCT_RESPONSE_HEADER } from '../supported_server_response_check';
+import type { ElasticsearchClient, ICustomClusterClient } from '@kbn/core-elasticsearch-server';
+import { PRODUCT_RESPONSE_HEADER } from '@kbn/core-elasticsearch-client-server-internal';


What's the issue of mocks depending on implementations? I thought it was the reverse we can't have. In other words, we cannot have the implementations using mocks.

If its just a bundle size concern, in that we're forcing a dependency just for a header string, then yes, we can come back later and change it.

TinaHeiligers · 2022-07-19T19:21:53Z

...ages/core/elasticsearch/core-elasticsearch-server-internal/src/elasticsearch_service.test.ts

-// Mocking the module to disable caching for tests
-jest.mock('../ui_settings/cache');


hehehe, see the reviewers on #133017

TinaHeiligers · 2022-07-19T20:00:23Z

packages/core/elasticsearch/core-elasticsearch-server/src/client/scopeable_request.ts

+ * Fake request object created manually by Kibana plugins.
+ * @public
+ */
+export interface FakeRequest {


I wondered where these went. Moving ScopeableRequest to its own file with the same name nicely separates out concerns.

TinaHeiligers · 2022-07-19T20:27:59Z

src/core/server/core_usage_data/core_usage_data_service.ts

@@ -20,8 +20,9 @@ import type { CoreContext, CoreService } from '@kbn/core-base-server-internal';
 import type { LoggingConfigType } from '@kbn/core-logging-server-internal';
 import type { Logger } from '@kbn/logging';
 import type { HttpConfigType, InternalHttpServiceSetup } from '@kbn/core-http-server-internal';
+import type { ElasticsearchServiceStart } from '@kbn/core-elasticsearch-server';
+import type { ElasticsearchConfigType } from '@kbn/core-elasticsearch-server-internal';


Using exposed internal types within core is needed but there's nothing preventing external consumers from using the internal types either. It's maybe too early to say, but how should we approach the need for sometimes having to use internal core domain types outside of core?

It's maybe too early to say, but how should we approach the need for sometimes having to use internal core domain types outside of core?

The initial idea was to simply forbid import from -internal package by consumers outside of Core via an ESlint rule. Note that now that we progressed quite a bit in the migration, we can say that this will cause problem given we saw that some 'internal' stuff is (at least currently) exported from these packages by our entrypoints and consumed externally.

One option could be to extract those parts to dedicated 'util' packages, or maybe have a way to flag things that are allowed to import from internal packages. I'm not sure yet to be honest. We definitely need to think about it once we complete the rest of the migration.

TinaHeiligers · 2022-07-19T20:36:05Z

src/core/server/index.ts

+export { ElasticsearchConfig, pollEsNodesVersion } from '@kbn/core-elasticsearch-server-internal';
+export type {
+  NodesVersionCompatibility,
+  PollEsNodesVersionOptions,


Seems like we both had the same thoughts.

TinaHeiligers · 2022-07-19T20:56:22Z

src/core/server/saved_objects/migrations/actions/search_for_outdated_documents.test.ts

@@ -8,10 +8,11 @@

 import { catchRetryableEsClientErrors } from './catch_retryable_es_client_errors';
 import { errors as EsErrors } from '@elastic/elasticsearch';
-jest.mock('./catch_retryable_es_client_errors');


Does the order here matter? I ask because the order between the mock import and the file mock is switched in some tests but not in others.

jest.mock order does not matter, and the consensus was to have them after the import lines

jest.doMock, or importing test.mocks.ts files performing jest.doMock order matter, and should usually be done before the imports

jest.mock is hoisted above import when used at top level and hoisted to the the beginning of the block when used in a block (test function scope, etc), same for jest.unmock. jest.doMock and jest.dontMock serve the same purpose but aren't hoisted.

TinaHeiligers · 2022-07-19T20:58:51Z

...ugins/cloud_security_posture/server/routes/compliance_dashboard/compliance_dashboard.test.ts

-import // eslint-disable-next-line @kbn/eslint/no-restricted-paths
-'@kbn/core/server/elasticsearch/client/mocks';


me too 🤔

…ide-es-to-packages

kibana-ci · 2022-07-20T07:46:49Z

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

[job] [logs] Fleet Cypress Tests / Add Integration - Real API should install integration without policy
[job] [logs] Security Solution Tests #2 / EQL rules Detection rules, EQL Creates and enables a new EQL rule

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`core`	355	356	+1

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/core-elasticsearch-client-server-internal`	-	9	+9
`@kbn/core-elasticsearch-client-server-mocks`	-	32	+32
`@kbn/core-elasticsearch-server`	-	51	+51
`@kbn/core-elasticsearch-server-internal`	-	29	+29
`@kbn/core-elasticsearch-server-mocks`	-	15	+15
`core`	705	664	-41
total			+95

Any counts in public APIs

Total count of every any typed public API. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats any for more detailed information.

id	before	after	diff
`@kbn/core-elasticsearch-client-server-mocks`	-	1	+1
`@kbn/core-elasticsearch-server-mocks`	-	1	+1
total			+2

Async chunks

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

id	before	after	diff
`core`	136.4KB	136.4KB	+3.0B

Page load bundle

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

id	before	after	diff
`core`	342.3KB	342.9KB	+616.0B

Unknown metric groups

API count

id	before	after	diff
`@kbn/core-base-common`	12	20	+8
`@kbn/core-elasticsearch-client-server-internal`	-	11	+11
`@kbn/core-elasticsearch-client-server-mocks`	-	36	+36
`@kbn/core-elasticsearch-server`	-	99	+99
`@kbn/core-elasticsearch-server-internal`	-	33	+33
`@kbn/core-elasticsearch-server-mocks`	-	15	+15
`core`	2472	2470	-2
total			+200

ESLint disabled line counts

id	before	after	diff
`actions`	22	20	-2
`cloudSecurityPosture`	12	7	-5
`fleet`	53	51	-2
`lists`	38	13	-25
`monitoring`	31	25	-6
`monitoringCollection`	2	0	-2
`ruleRegistry`	6	2	-4
`securitySolution`	439	425	-14
`stackAlerts`	23	22	-1
`synthetics`	62	61	-1
`upgradeAssistant`	11	9	-2
total			-64

References to deprecated APIs

id	before	after	diff
`@kbn/core-elasticsearch-server-internal`	-	2	+2

Total ESLint disabled count

id	before	after	diff
`actions`	28	26	-2
`cloudSecurityPosture`	13	8	-5
`fleet`	61	59	-2
`lists`	43	18	-25
`monitoring`	38	32	-6
`monitoringCollection`	2	0	-2
`ruleRegistry`	8	4	-4
`securitySolution`	513	499	-14
`stackAlerts`	24	23	-1
`synthetics`	68	67	-1
`upgradeAssistant`	11	9	-2
total			-64

Unreferenced deprecated APIs

id	before	after	diff
`@kbn/core-elasticsearch-server`	-	1	+1

History

💛 Build #58528 was flaky 195b75b
💛 Build #57985 was flaky 15e3b29
💛 Build #57853 was flaky 1988d60
💔 Build #57814 failed 366daa8
💔 Build #57774 failed 70b2de0
💔 Build #57760 failed 4133c84

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

pgayvallet and others added 10 commits July 13, 2022 15:38

create es types package

6550e9f

start moving client types to @kbn/core-elasticsearch-server

9d272fa

move ElasticsearchClientConfig to package

5a7f812

start adapting usages

7d3620b

start fixing imports

21610bf

fix more imports

0f4953b

just a bit more

a07f24d

move service types

add4b0e

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

fa83d21

…-ref HEAD~1..HEAD --fix'

fix more internal types

b03557b

pgayvallet added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.4.0 labels Jul 14, 2022

pgayvallet and others added 18 commits July 14, 2022 10:39

Merge remote-tracking branch 'upstream/main' into kbn-136283-server-s…

64c3bea

…ide-es-to-packages

move retry call cluster helpers outside of client package

d108f7a

move client code and mocks to packages

7892891

fix imports

76a8c84

adapt external usages

73d9556

adapt more external usages

675f1eb

Merge remote-tracking branch 'upstream/main' into kbn-136283-server-s…

d9a632f

…ide-es-to-packages

adapt more external usages 2

2a3d8f2

fix mocked module

edc8434

create empty domain packages

4b722cf

more external usages fix

cf50cba

move ALL the things (again)

22b5cbc

mock external import fix

e640dfe

fix tests, add test dependencies

a4ee98f

fix some internal usages

e7e3e8c

fix more internal usages

77aa97e

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

4133c84

…-ref HEAD~1..HEAD --fix'

fix cli_setup usage

70b2de0

pgayvallet requested a review from a team as a code owner July 15, 2022 07:00

pgayvallet requested review from pzl and joeypoon July 15, 2022 07:00

pgayvallet added backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes labels Jul 15, 2022

cjcenizal approved these changes Jul 15, 2022

View reviewed changes

botelastic bot added Team:Fleet Team label for Observability Data Collection Fleet team Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability labels Jul 15, 2022

miltonhultgren approved these changes Jul 18, 2022

View reviewed changes

ari-aviran approved these changes Jul 18, 2022

View reviewed changes

ymao1 approved these changes Jul 18, 2022

View reviewed changes

juliaElastic approved these changes Jul 18, 2022

View reviewed changes

shahzad31 approved these changes Jul 18, 2022

View reviewed changes

TinaHeiligers reviewed Jul 18, 2022

View reviewed changes

ashokaditya approved these changes Jul 19, 2022

View reviewed changes

Merge remote-tracking branch 'upstream/main' into kbn-136283-server-s…

195b75b

…ide-es-to-packages

nkhristinin approved these changes Jul 19, 2022

View reviewed changes

fkanout approved these changes Jul 19, 2022

View reviewed changes

pgayvallet removed request for pzl and joeypoon July 19, 2022 08:32

marshallmain approved these changes Jul 19, 2022

View reviewed changes

TinaHeiligers reviewed Jul 19, 2022

View reviewed changes

TinaHeiligers approved these changes Jul 19, 2022

View reviewed changes

Merge remote-tracking branch 'upstream/main' into kbn-136283-server-s…

491ecc1

…ide-es-to-packages

pgayvallet merged commit 3508350 into elastic:main Jul 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate server-side ES domain to packages #136297

Migrate server-side ES domain to packages #136297

pgayvallet commented Jul 13, 2022 •

edited

Loading

elasticmachine commented Jul 15, 2022

cjcenizal left a comment

elasticmachine commented Jul 15, 2022

elasticmachine commented Jul 15, 2022

ymao1 left a comment

juliaElastic left a comment

TinaHeiligers Jul 18, 2022

pgayvallet Jul 19, 2022 •

edited

Loading

TinaHeiligers Jul 19, 2022 •

edited

Loading

pgayvallet Jul 20, 2022

ashokaditya left a comment

nkhristinin left a comment

fkanout left a comment

marshallmain left a comment

TinaHeiligers Jul 19, 2022

TinaHeiligers Jul 19, 2022

TinaHeiligers left a comment

TinaHeiligers Jul 19, 2022

TinaHeiligers Jul 19, 2022

TinaHeiligers Jul 19, 2022

TinaHeiligers Jul 19, 2022

TinaHeiligers Jul 19, 2022

pgayvallet Jul 20, 2022

TinaHeiligers Jul 19, 2022

TinaHeiligers Jul 19, 2022

pgayvallet Jul 20, 2022

TinaHeiligers Jul 19, 2022

kibana-ci commented Jul 20, 2022

API count

ESLint disabled line counts

References to deprecated APIs

Total ESLint disabled count

Unreferenced deprecated APIs

		import { ScopedClusterClient } from './scoped_cluster_client';

		const createEsClient = () => ({} as unknown as ElasticsearchClient);

		// Mocking the module to disable caching for tests
		jest.mock('../ui_settings/cache');

		import // eslint-disable-next-line @kbn/eslint/no-restricted-paths
		'@kbn/core/server/elasticsearch/client/mocks';

Migrate server-side ES domain to packages #136297

Migrate server-side ES domain to packages #136297

Conversation

pgayvallet commented Jul 13, 2022 • edited Loading

Summary

elasticmachine commented Jul 15, 2022

cjcenizal left a comment

Choose a reason for hiding this comment

elasticmachine commented Jul 15, 2022

elasticmachine commented Jul 15, 2022

ymao1 left a comment

Choose a reason for hiding this comment

juliaElastic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgayvallet Jul 19, 2022 • edited Loading

Choose a reason for hiding this comment

TinaHeiligers Jul 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashokaditya left a comment

Choose a reason for hiding this comment

nkhristinin left a comment

Choose a reason for hiding this comment

fkanout left a comment

Choose a reason for hiding this comment

marshallmain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TinaHeiligers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibana-ci commented Jul 20, 2022

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Module Count

Public APIs missing comments

Any counts in public APIs

Async chunks

Page load bundle

API count

ESLint disabled line counts

References to deprecated APIs

Total ESLint disabled count

Unreferenced deprecated APIs

History

pgayvallet commented Jul 13, 2022 •

edited

Loading

pgayvallet Jul 19, 2022 •

edited

Loading

TinaHeiligers Jul 19, 2022 •

edited

Loading