[ML] New Platform server shim: update data visualizer routes to use new platform router #56739
Conversation
darnautov
added
:ml
v8.0.0
release_note:skip
darnautov
requested review from
alvarezmelissa87,
peteharverson and
jgowdyelastic
|
Pinging @elastic/ml-ui (:ml) |
| count: number; | ||
| trueCount: number; | ||
| falseCount: number; | ||
| [key: string]: number | string; |
There was a problem hiding this comment.
Is this needed? Don't we just use falseCount and trueCount ?
There was a problem hiding this comment.
yeah it's required because of https://github.com/elastic/kibana/pull/56739/files#diff-45e703fa74c02467ebd253aa40c78e0bR876
| @@ -277,7 +367,7 @@ export class DataVisualizer { | |||
| aggs: buildSamplerAggregation(aggs, samplerShardSize), | |||
| }; | |||
|
|
|||
| const resp = await this.callWithRequest('search', { | |||
| const resp = await this.context.core.elasticsearch.dataClient.callAsCurrentUser('search', { | |||
There was a problem hiding this comment.
Should we be using the same approach that @alvarezmelissa87 used in the calendars route and calendar_manager.ts?
const actualClient = isLegacy === true ? client : client.ml!.mlClient.callAsCurrentUser;
There was a problem hiding this comment.
Yeah, I think we should be using the mlClient.
The mlClient is set up in plugin.ts with the plugins option to extend the elasticsearch client.
const mlClient = core.elasticsearch.createClient('ml', { plugins: [elasticsearchJsPlugin] });
http.registerRouteHandlerContext('ml', (context, request) => {
return {
mlClient: mlClient.asScoped(request),
};
});
That's the difference between context.core.elasticsearch.dataClient.callAsCurrentUser and using .ml!.mlClient.callAsCurrentUser. We should always use the mlClient to make sure it's consistent and so we don't have to worry about changing things later if we add more extensions to the elasticsearch client.
There was a problem hiding this comment.
Changed to mlClient
| ? SAMPLER_TOP_TERMS_SHARD_SIZE | ||
| : samplerShardSize, | ||
| }; | ||
|
|
||
| stats.topValues = _.get(aggregations, [...topAggsPath, 'buckets'], []); |
There was a problem hiding this comment.
Are these lines still needed with the lines you've added above?
There was a problem hiding this comment.
Good catch, deleted
|
Seeing these error toasts pop up when I try to use the data visualizer with an index not based on time series:
The user shouldn't need to see those schema-related errors. In Everything else works as expected. Apart from that mentioned above, the code LGTM. |
thanks @alvarezmelissa87, pushed a fix for schema validation 6c3510a |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
…ew platform router (elastic#56739) * [ML] data_visualizer TS refactor, NP router * [ML] fix schema, add apiDoc * [ML] update apiDoc order * [ML] validate_cardinality with NP router * [ML] use mlClient * [ML] remove redundant code * [ML] support legacy callWithRequest for job validation * [ML] fix schema validation # Conflicts: # x-pack/legacy/plugins/ml/server/routes/apidoc.json
…ew platform router (#56739) (#56873) * [ML] data_visualizer TS refactor, NP router * [ML] fix schema, add apiDoc * [ML] update apiDoc order * [ML] validate_cardinality with NP router * [ML] use mlClient * [ML] remove redundant code * [ML] support legacy callWithRequest for job validation * [ML] fix schema validation # Conflicts: # x-pack/legacy/plugins/ml/server/routes/apidoc.json
Summary
Part of #49743. Updates data visualizer routes to use the new platform router.
Checklist
[ ] 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](https://github.com/elastic/kibana/blob/master/packages/kbn-i18n/README.md)[ ] Unit or functional tests were updated or added to match the most common scenarios[ ] This was checked for keyboard-only and screenreader accessibility