Don't expose Elasticsearch client as Observable by mshustov · Pull Request #53824 · elastic/kibana

mshustov · 2019-12-27T15:31:11Z

Summary

Closes #53267
TODO:

decide whether we want to use observables internally in context.
decide whether we want to maintain both admin & data client
see related discussion in the parent task.

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~[ ] This was checked for cross-browser compatibility, including a check against IE11~~
~~[ ] 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
~~[ ] This was checked for keyboard-only and screenreader accessibility~~

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

Dev docs

Elasticsearch clients aren't exposed via the Observable interface anymore. Elasticsearch client provides a static API that handles all elasticsearch config updates under the hood, transparent to the end-user.

const client = core.elasticsearch.dataClient;
const data = await client.callAsInternalUser('endpoint');

elasticmachine · 2019-12-27T15:31:13Z

Pinging @elastic/kibana-platform (Team:Platform)

azasypkin

Security/Spaces changes LGTM.

pgayvallet

This seems definitely better in term of developer experience.

pgayvallet · 2020-01-06T09:55:00Z

x-pack/legacy/plugins/alerting/server/plugin.ts

    plugins: AlertingPluginsSetup
  ): Promise<PluginSetupContract> {
-    this.adminClient = await core.elasticsearch.adminClient$.pipe(first()).toPromise();
+    this.adminClient = core.elasticsearch.adminClient;


Proof by usage that the PR was needed.

src/core/server/elasticsearch/cluster_client.ts

pgayvallet · 2020-01-06T10:14:13Z

src/core/server/elasticsearch/elasticsearch_service.mock.ts

-  createSetupContract: createSetupContractMock,
+  createInternalSetup: createInternalSetupContractMock,
+  createSetup: createSetupContractMock,


I think I'm in favor of this 'new' naming without the contract suffix, however this is inconsistent with every other service mocks we got. Should we decide to go with this and change the others when we can?

I think we should unify this naming. Right now we have inconsistency across the codebase. example
Should we add this to NP conventions and fix all the mocks?

This is minor, but I think we should

created #54229

pgayvallet · 2020-01-06T10:23:25Z

src/core/server/elasticsearch/elasticsearch_service.ts

+          async callAsCurrentUser(
+            endpoint: string,
+            clientParams: Record<string, any> = {},
+            options?: CallAPIOptions
+          ) {


NIT: does explicitly typing the clients to IClusterClient avoid to redefine the callAsXXXUser parameter types?

nope. TS complaints Parameter '...' implicitly has an 'any' type

pgayvallet · 2020-01-06T10:33:47Z

src/core/server/elasticsearch/elasticsearch_service.test.ts

+  it('returns data and admin client as a part of the contract', async () => {
+    const mockAdminClusterClientInstance = elasticsearchServiceMock.createClusterClient();
+    const mockDataClusterClientInstance = elasticsearchServiceMock.createClusterClient();
+    MockClusterClient.mockImplementationOnce(
+      () => mockAdminClusterClientInstance
+    ).mockImplementationOnce(() => mockDataClusterClientInstance);
+
+    const setupContract = await elasticsearchService.setup(deps);


Do we have any reasons to keep this now? (the filter I mean. Comment is unrelated to the actual test file)

kibana/src/core/server/elasticsearch/elasticsearch_service.ts

Lines 61 to 68 in bde2895

filter(() => {

if (this.subscription !== undefined) {

this.log.error('Clients cannot be changed after they are created');

return false;

}

return true;

}),

As I understand, our code is ready for runtime config updates, but we don't have such requirements at the moment.

kibanamachine · 2020-01-08T08:48:57Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: c0d4246

History

💚 Build #17997 succeeded 8befcb5
💚 Build #17377 succeeded 69df6fe

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

* expose ES clients without observables * expose observable-less api to plugins * update core api and mocks * update plugins * NP SO & legacy use updated API * update SO tests * update TSDocs * update types * update docs * document createCluster analog in np * typo

* master: (55 commits) [ui/public/utils] Copy rarely used items to where they are consumed (elastic#53819) set AppArch team as an owner of the search endpoints (elastic#54131) Don't expose Elasticsearch client as Observable (elastic#53824) [SIEM] Cleanup unnecessary use of enzyme-to-json (elastic#53980) fix ui exports doc (elastic#54138) change markdown element title (elastic#54194) [Logs UI] Refactor log position to hooks (elastic#53540) [SIEM] Implement NP Plugin Setup (elastic#54030) [DOCS] Updates ML links (elastic#53613) sort renovate packages in config Spaces - fix flakey api tests (elastic#54154) Remove dependency that was causing effect to re-execute infinitely. (elastic#54160) [dev/run] expose unexpected flags as more than just names (elastic#54080) [DOCS] Moves index pattern doc to Discover (elastic#53347) [SIEM] Cleanup React imports (elastic#53981) Update eslint related packages (elastic#54107) [Uptime] Added date range filter into expanded list query (elastic#52609) [SIEM] Add react/display-name eslint rule (elastic#53107) [SIEM] Enable eslint prefer-template rule (elastic#53983) Elasticsearch snapshots automation (elastic#53706) ...

….com:mbondyra/kibana into IS-50036_show-sum-for-a-field-with-formatter * 'IS-50036_show-sum-for-a-field-with-formatter' of github.com:mbondyra/kibana: [Telemetry] Fix license page crashing on telemetry.enabled: fa… (elastic#54174) [ui/public/utils] Copy rarely used items to where they are consumed (elastic#53819) set AppArch team as an owner of the search endpoints (elastic#54131) Don't expose Elasticsearch client as Observable (elastic#53824) [SIEM] Cleanup unnecessary use of enzyme-to-json (elastic#53980)

rudolf

~~Sorry for the late review :(~~

src/core/server/elasticsearch/elasticsearch_service.ts

rudolf · 2020-01-09T12:45:13Z

Oh ignore me, I missed the publishReplay(1)

kibana/src/core/server/elasticsearch/elasticsearch_service.ts

Lines 90 to 92 in 56041f0

    
             ), 
        
             publishReplay(1) 
        
           ) as ConnectableObservable<CoreClusterClients>;

mshustov added 4 commits December 27, 2019 18:24

expose ES clients without observables

bd04930

expose observable-less api to plugins

50eef07

update core api and mocks

8a4939b

update plugins

2968a29

mshustov added Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// Feature:New Platform release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. v7.6.0 labels Dec 27, 2019

mshustov added 7 commits December 30, 2019 08:23

Merge branch 'master' into issue-53267-no-more-observables

fa96ff1

NP SO & legacy use updated API

cecc2de

update SO tests

77fe747

update TSDocs

48db5d5

update types

f8812eb

update docs

2d31ebe

document createCluster analog in np

69df6fe

mshustov marked this pull request as ready for review December 30, 2019 09:33

mshustov requested review from a team as code owners December 30, 2019 09:33

azasypkin approved these changes Jan 2, 2020

View reviewed changes

Merge branch 'master' into issue-53267-no-more-observables

8befcb5

pgayvallet approved these changes Jan 6, 2020

View reviewed changes

mshustov added 2 commits January 8, 2020 10:11

Merge branch 'master' into issue-53267-no-more-observables

2c8513a

typo

c0d4246

mshustov merged commit 56041f0 into elastic:master Jan 8, 2020

mshustov deleted the issue-53267-no-more-observables branch January 8, 2020 09:01

mshustov mentioned this pull request Jan 8, 2020

[7.x] Don't expose Elasticsearch client as Observable (#53824) #54228

Merged

mshustov added the backported label Jan 8, 2020

rudolf reviewed Jan 9, 2020

View reviewed changes

Ander-MZ mentioned this pull request Feb 25, 2020

Plugin no longer works on 7.6.X flexmonster/pivot-kibana#20

Closed

	filter(() => {
	if (this.subscription !== undefined) {
	this.log.error('Clients cannot be changed after they are created');
	return false;
	}

	return true;
	}),

Conversation

mshustov commented Dec 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Dev docs

Uh oh!

elasticmachine commented Dec 27, 2019

Uh oh!

azasypkin left a comment

Choose a reason for hiding this comment

Uh oh!

pgayvallet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgayvallet Jan 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Jan 8, 2020

💚 Build Succeeded

History

Uh oh!

rudolf left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rudolf commented Jan 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mshustov commented Dec 27, 2019 •

edited

Loading

pgayvallet Jan 6, 2020 •

edited

Loading

rudolf left a comment •

edited

Loading

rudolf commented Jan 9, 2020 •

edited

Loading