Refactor Plugins to access elasticsearch from CoreStart#59915
Refactor Plugins to access elasticsearch from CoreStart#59915rudolf merged 28 commits intoelastic:masterfrom
Conversation
|
Pinging @elastic/kibana-platform (Team:Platform) |
|
Having Elasticsearch only available in setup causes a lot ugly ways to access Elasticsearch 😞 some of these could be improved by refactoring more code "downstream", but there are some plugins which I don't think can be improved like actions. Actions would have to expose it's client through an async getter which would make the dev experience worse for all plugins depending on the actions client. |
lizozom
left a comment
There was a problem hiding this comment.
Code owner changes are in a single file.
Side note:
Love the new way to access elasticsearch, though not crazy about the fact we use [0] to access core on the start services (core.getStartServices())[0].elasticsearch.client).
2260086 to
f0bff3e
Compare
x-pack/plugins/oss_telemetry/server/lib/tasks/visualizations/task_runner.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
This looks more like a hack than a solution to me. Snapshot and Restore app has a very similar logic than watcher to create the ES client. Should we move all the logic of the client creation + registering routes directly in the start() method?
There was a problem hiding this comment.
Yeah I'm not very happy with how this looks 😞
We want all routes to be registered before we start the HTTP server and that requires that it's done during setup(). The alternative would be to turn client into a promise returning function. This makes it slightly less convenient to use, but since route handlers are already async functions adding another await might not make much difference.
What do you think of (I only refactored one route to illustrate):
diff --git a/x-pack/plugins/watcher/server/plugin.ts b/x-pack/plugins/watcher/server/plugin.ts
index 88cecf5335..75f727b738 100644
--- a/x-pack/plugins/watcher/server/plugin.ts
+++ b/x-pack/plugins/watcher/server/plugin.ts
@@ -17,6 +17,7 @@ import {
Plugin,
PluginInitializerContext,
} from 'kibana/server';
+import { once } from 'lodash';
import { PLUGIN } from '../common/constants';
import { Dependencies, LicenseStatus, RouteDependencies } from './types';
import { LICENSE_CHECK_STATE } from '../../licensing/server';
@@ -31,7 +32,7 @@ import { registerLoadHistoryRoute } from './routes/api/register_load_history_rou
import { elasticsearchJsPlugin } from './lib/elasticsearch_js_plugin';
export interface WatcherContext {
- client: IScopedClusterClient;
+ getClient: () => Promise<IScopedClusterClient>;
}
export class WatcherServerPlugin implements Plugin<void, void, any, any> {
@@ -52,19 +53,15 @@ export class WatcherServerPlugin implements Plugin<void, void, any, any> {
getLicenseStatus: () => this.licenseStatus,
};
- const getWatcherEsClient = async () => {
+ const getWatcherEsClient = once(async () => {
const [coreStart] = await getStartServices();
const config = { plugins: [elasticsearchJsPlugin] };
return coreStart.elasticsearch.legacy.createClient('watcher', config);
- };
+ });
+
http.registerRouteHandlerContext('watcher', (ctx, request) => {
return {
- client: {
- callAsCurrentUser: async (...args) =>
- (await getWatcherEsClient()).asScoped(request).callAsCurrentUser(...args),
- callAsInternalUser: async (...args) =>
- (await getWatcherEsClient()).asScoped(request).callAsInternalUser(...args),
- },
+ getClient: async () => (await getWatcherEsClient()).asScoped(request),
};
});
diff --git a/x-pack/plugins/watcher/server/routes/api/register_list_fields_route.ts b/x-pack/plugins/watcher/server/routes/api/register_list_fields_route.ts
index d72e5ad2f8..89a9daf56a 100644
--- a/x-pack/plugins/watcher/server/routes/api/register_list_fields_route.ts
+++ b/x-pack/plugins/watcher/server/routes/api/register_list_fields_route.ts
@@ -40,7 +40,7 @@ export function registerListFieldsRoute(deps: RouteDependencies) {
const { indexes } = request.body;
try {
- const fieldsResponse = await fetchFields(ctx.watcher!.client, indexes);
+ const fieldsResponse = await fetchFields(await ctx.watcher!.getClient(), indexes);
const json = fieldsResponse.status === 404 ? { fields: [] } : fieldsResponse;
const fields = Fields.fromUpstreamJson(json);
return response.ok({ body: fields.downstreamJson });There was a problem hiding this comment.
Yes this is much better 😊 Thanks for making this change!
Depending on the route handler, we might need to externalize the client fetching once at the top whenever it is needed in multiple places.
There was a problem hiding this comment.
Why is it "legacy" es? (coreStart.elasticsearch.legacy.createClient)
There was a problem hiding this comment.
We're going to introduce the new Elasticsearch-js client soon (Kibana is currently using a deprecated version). So while we're making these breaking changes we thought it's a good idea to move these into a legacy namespace so that introducing the new client won't be another breaking change.
There was a problem hiding this comment.
Since creating a custom ES client and then using the request handler context to share it with routes is such a common pattern I did a quick POC of another approach. Core can expose a createScopedClient method from the request handler context that allows you to create a custom client scoped to the incoming request.
@elastic/kibana-platform what do you think of something like:
diff --git a/src/core/server/elasticsearch/elasticsearch_service.ts b/src/core/server/elasticsearch/elasticsearch_service.ts
index 9785bd6700..135767d931 100644
--- a/src/core/server/elasticsearch/elasticsearch_service.ts
+++ b/src/core/server/elasticsearch/elasticsearch_service.ts
@@ -70,6 +70,7 @@ export class ElasticsearchService
clientConfig?: Partial<ElasticsearchClientConfig>
) => ICustomClusterClient;
private adminClient?: IClusterClient;
+ private customClients?: WeakMap<object, ICustomClusterClient>;
constructor(private readonly coreContext: CoreContext) {
this.kibanaVersion = coreContext.env.packageInfo.version;
@@ -182,9 +183,15 @@ export class ElasticsearchService
kibanaVersion: this.kibanaVersion,
}).pipe(takeUntil(this.stop$), shareReplay({ refCount: true, bufferSize: 1 }));
+ this.customClients = new WeakMap();
this.createClient = (type: string, clientConfig: Partial<ElasticsearchClientConfig> = {}) => {
+ if (this.customClients!.has({ type, clientConfig })) {
+ return this.customClients!.get({ type, clientConfig })!;
+ }
const finalConfig = merge({}, config, clientConfig);
- return this.createClusterClient(type, finalConfig, deps.http.getAuthHeaders);
+ const customClient = this.createClusterClient(type, finalConfig, deps.http.getAuthHeaders);
+ this.customClients!.set({ type, clientConfig }, customClient);
+ return customClient;
};
return {
diff --git a/src/core/server/index.ts b/src/core/server/index.ts
index 4a1ac8988e..92755d9d48 100644
--- a/src/core/server/index.ts
+++ b/src/core/server/index.ts
@@ -322,6 +322,12 @@ export interface RequestHandlerContext {
elasticsearch: {
dataClient: IScopedClusterClient;
adminClient: IScopedClusterClient;
+ legacy: {
+ createScopedClient: (
+ type: string,
+ clientConfig?: Partial<ElasticsearchClientConfig>
+ ) => IScopedClusterClient;
+ };
};
uiSettings: {
client: IUiSettingsClient;
diff --git a/src/core/server/server.ts b/src/core/server/server.ts
index 86f1b5153d..25b82ce64e 100644
--- a/src/core/server/server.ts
+++ b/src/core/server/server.ts
@@ -244,8 +244,15 @@ export class Server {
typeRegistry: this.coreStart!.savedObjects.getTypeRegistry(),
},
elasticsearch: {
- adminClient: coreSetup.elasticsearch.adminClient.asScoped(req),
- dataClient: coreSetup.elasticsearch.dataClient.asScoped(req),
+ adminClient: this.coreStart!.elasticsearch.legacy.client.asScoped(req),
+ dataClient: this.coreStart!.elasticsearch.legacy.client.asScoped(req),
+ legacy: {
+ createScopedClient: (
+ type: string,
+ clientConfig: Partial<ElasticsearchClientConfig>
+ ) =>
+ this.coreStart!.elasticsearch.legacy.createClient(type, clientConfig).asScoped(req),
+ },
},
uiSettings: {
client: uiSettingsClient,
diff --git a/x-pack/plugins/watcher/server/plugin.ts b/x-pack/plugins/watcher/server/plugin.ts
index dcbc7e6526..a807866115 100644
--- a/x-pack/plugins/watcher/server/plugin.ts
+++ b/x-pack/plugins/watcher/server/plugin.ts
@@ -10,7 +10,6 @@ declare module 'kibana/server' {
}
}
-import { once } from 'lodash';
import {
CoreSetup,
IScopedClusterClient,
@@ -53,21 +52,11 @@ export class WatcherServerPlugin implements Plugin<void, void, any, any> {
getLicenseStatus: () => this.licenseStatus,
};
- const getWatcherEsClient = once(async () => {
- const [coreStart] = await getStartServices();
- const config = { plugins: [elasticsearchJsPlugin] };
- return coreStart.elasticsearch.legacy.createClient('watcher', config);
- });
- http.registerRouteHandlerContext('watcher', (ctx, request) => {
- return {
- client: {
- callAsCurrentUser: async (...args) =>
- (await getWatcherEsClient()).asScoped(request).callAsCurrentUser(...args),
- callAsInternalUser: async (...args) =>
- (await getWatcherEsClient()).asScoped(request).callAsInternalUser(...args),
- },
- };
- });
+ const config = { plugins: [elasticsearchJsPlugin] };
+
+ http.registerRouteHandlerContext('watcher', (ctx, request) => ({
+ client: ctx.core.elasticsearch.legacy.createScopedClient('watcher', config),
+ }));
registerListFieldsRoute(routeDependencies);
registerLoadHistoryRoute(routeDependencies);There was a problem hiding this comment.
Makes total sense to me. Especially useful for plugins that need to install plugins to the client to support endpoints that do not currently exist in the legacy client (ML comes to mind).
There was a problem hiding this comment.
Ok. It makes sense to me. But with this WeakMap every client becomes a singleton, which introduces a lot of side effects (how would we handle es config updates, for example?). I'd avoid adding stateful logic as long as possible and would rather keep once in the plugin.
There was a problem hiding this comment.
But with this WeakMap every client becomes a singleton, which introduces a lot of side effects
+1. We would need something like a deepEqual test on the clientConfig to get the proper behavior instead of this.customClients!.has({ type, clientConfig }) I think?
There was a problem hiding this comment.
I agree that keeping this state in Core introduces some complexity. But if we keep this state in the plugin (with e.g. a once) then Core cannot automatically scope the client to the incoming request which is how most context API's work. I think whether there is a singleton in Core or in the Plugin the problem of how we update Elasticsearch config remains the same.
You're right, WeakMap.has uses reference equality for objects, so the suggestion won't work as is.
I'll revert the watcher changes for now and then open a new PR to discuss and flesh out how this API might look and work.
There was a problem hiding this comment.
This looks more like a hack than a solution to me. Snapshot and Restore app has a very similar logic than watcher to create the ES client. Should we move all the logic of the client creation + registering routes directly in the start() method?
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
jloleysens
left a comment
There was a problem hiding this comment.
Tested upgrade assistant and remote clusters locally and works as before 👍
* x-pack/watcher: use Elasticsearch from CoreStart * x-pack/upgrade_assistant: use Elasticsearch from CoreStart * x-pack/actions: use Elasticsearch from CoreStart * x-pack/alerting: use Elasticsearch from CoreStart * x-pack/lens: use Elasticsearch from CoreStart * expressions: use Elasticsearch from CoreStart * x-pack/remote_clusters: remove unused Elasticsearch dependency on CoreSetup * x-pack/oss_telemetry: use Elasticsearch from CoreStart * Cleanup after #59886 * x-pack/watcher: create custom client only once * Revert "x-pack/watcher: create custom client only once" This reverts commit 78fc4d2. * Revert "x-pack/watcher: use Elasticsearch from CoreStart" This reverts commit b621af9. * x-pack/task_manager: use Elasticsearch from CoreStart * x-pack/event_log: use Elasticsearch from CoreStart * x-pack/alerting: use Elasticsearch from CoreStart * x-pack/apm: use Elasticsearch from CoreStart * x-pack/actions: use Elasticsearch from CoreStart * PR Feedback * APM review nits * Remove unused variable * Remove unused variable * x-pack/apm: better typesafety Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
|
Friendly reminder: Looks like this PR hasn’t been backported yet. |
|
Friendly reminder: Looks like this PR hasn’t been backported yet. |
3 similar comments
|
Friendly reminder: Looks like this PR hasn’t been backported yet. |
|
Friendly reminder: Looks like this PR hasn’t been backported yet. |
|
Friendly reminder: Looks like this PR hasn’t been backported yet. |
|
@rudolf Has this been backported? If so could you please add the |
* x-pack/watcher: use Elasticsearch from CoreStart * x-pack/upgrade_assistant: use Elasticsearch from CoreStart * x-pack/actions: use Elasticsearch from CoreStart * x-pack/alerting: use Elasticsearch from CoreStart * x-pack/lens: use Elasticsearch from CoreStart * expressions: use Elasticsearch from CoreStart * x-pack/remote_clusters: remove unused Elasticsearch dependency on CoreSetup * x-pack/oss_telemetry: use Elasticsearch from CoreStart * Cleanup after elastic#59886 * x-pack/watcher: create custom client only once * Revert "x-pack/watcher: create custom client only once" This reverts commit 78fc4d2. * Revert "x-pack/watcher: use Elasticsearch from CoreStart" This reverts commit b621af9. * x-pack/task_manager: use Elasticsearch from CoreStart * x-pack/event_log: use Elasticsearch from CoreStart * x-pack/alerting: use Elasticsearch from CoreStart * x-pack/apm: use Elasticsearch from CoreStart * x-pack/actions: use Elasticsearch from CoreStart * PR Feedback * APM review nits * Remove unused variable * Remove unused variable * x-pack/apm: better typesafety Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
#64329) * Refactor Plugins to access elasticsearch from CoreStart (#59915) * x-pack/watcher: use Elasticsearch from CoreStart * x-pack/upgrade_assistant: use Elasticsearch from CoreStart * x-pack/actions: use Elasticsearch from CoreStart * x-pack/alerting: use Elasticsearch from CoreStart * x-pack/lens: use Elasticsearch from CoreStart * expressions: use Elasticsearch from CoreStart * x-pack/remote_clusters: remove unused Elasticsearch dependency on CoreSetup * x-pack/oss_telemetry: use Elasticsearch from CoreStart * Cleanup after #59886 * x-pack/watcher: create custom client only once * Revert "x-pack/watcher: create custom client only once" This reverts commit 78fc4d2. * Revert "x-pack/watcher: use Elasticsearch from CoreStart" This reverts commit b621af9. * x-pack/task_manager: use Elasticsearch from CoreStart * x-pack/event_log: use Elasticsearch from CoreStart * x-pack/alerting: use Elasticsearch from CoreStart * x-pack/apm: use Elasticsearch from CoreStart * x-pack/actions: use Elasticsearch from CoreStart * PR Feedback * APM review nits * Remove unused variable * Remove unused variable * x-pack/apm: better typesafety Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> * Fix event log tests Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Summary
Refactors some plugins to use the elasticsearch client from start instead of setup after it was deprecated it #59886. I'm not familiar with the functionality of the plugins and haven't tested any of the changes, please test your plugin before giving a 👍
The first commit is from #59886 and should be ignored. I don't want the Core API changes to block on the refactoring work.Checklist
Delete any items that are not applicable to this PR.
For maintainers