-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Index patterns api - load field list on server #81218
Index patterns api - load field list on server #81218
Conversation
…ve_legacy_es_client_usage
…:mattkime/kibana into field_caps_remove_legacy_es_client_usage
…legacy_es_client_usage
…tkime/kibana into single_call_for_index_pattern_fields
Pinging @elastic/kibana-app-arch (Team:AppArch) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One proposed change and afterwards I don't think there will be any changes in the alerting code :)
@@ -301,6 +301,7 @@ export class AlertingPlugin { | |||
): (request: KibanaRequest) => Services { | |||
return (request) => ({ | |||
callCluster: elasticsearch.legacy.client.asScoped(request).callAsCurrentUser, | |||
esClient: elasticsearch.client.asScoped(request).asCurrentUser, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be able to use scopedClusterClient
below.
esClient: elasticsearch.client.asScoped(request).asCurrentUser, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rubberstamping the ES UI codeowner request since the portions of the rollup plugin that were touched aren't owned by us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code LGTM
💚 Build SucceededMetrics [docs]distributable file count
page load bundle size
History
To update your PR or re-run it, just comment with: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed Kibana App code change only. Code LGTM
Index patterns api - load field list on server
Sorry, this caused failures on master once merged so I had to revert. https://kibana-ci.elastic.co/job/elastic+kibana+master/9408/execution/node/459/log |
asScopedToClient: async ( | ||
savedObjectsClient: SavedObjectsClientContract, | ||
elasticsearchClient: ElasticsearchClient | ||
) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mattkime Just stumbled across this change now -- I'm wondering if it would be simpler to instead do:
asScoped: async (request: KibanaRequest) => {...}
Since both the scoped saved objects & es clients can be derived from a kibana request:
const savedObjectsClient = savedObjects.getScopedClient(req);
const elasticsearchClient = elasticsearch.client.asScoped(req);
The reason I called this asScopedToClient
in the first place was because it mirrored the signature of uiSettings.asScopedToClient
which only takes in a scoped SO client. Now that we've changed it, the naming is a bit more confusing IMHO.
In the search service (both low level & search source), we've used asScoped(req: KibanaRequest)
, so this would also make things consistent with the other search services.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought this was following the general principle of asking for for the more specific dependencies over more general. That said, I'm definitely open to improvements in the code. Mind opening a PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
principle of asking for for the more specific dependencies over more general
Yeah, that's a fair point and I agree the whole KibanaRequest
might not always be necessary. I guess it's more of a consistency-vs-explicitness tradeoff. I think I was getting hung up on the naming here in particular though; I'll give it some thought.
Summary
The server side index patterns api can now load the field list. Previously it would error if a field list refresh was required.
The regular and rollup index pattern field list loading methods were merged. Rollup index patterns require additional functionality over regular index patterns when it comes to loading the field list, but this won't be necessary once rollups v2 is released.
Checklist
Delete any items that are not applicable to this PR.
For maintainers