Skip to content
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

[ML] Adds support for kuery to job wizards #26094

Merged
merged 3 commits into from
Nov 23, 2018

Conversation

peteharverson
Copy link
Contributor

@peteharverson peteharverson commented Nov 22, 2018

Summary

Adds support for saved searches created using Kuery to the job wizards:

  • Single Metric
  • Multi Metric
  • Population
  • Advanced
  • Recognizer

Note the Data Visualizer does not yet support Kuery (would require a change to the search bar in the view to support Kuery as well as lucene syntax). If a Kuery based saved search is selected for the Data Visualizer a warning is shown to the user that Kuery is not supported.

The previous createSearchItems function in jobs/new_job/utils/new_job_utils.js has been replaced with a new SearchItemsProvider which builds the ES query for use in the job datafeed using the Kibana BuildESQueryProvider from the @kbn/es-query package.

Checklist

For maintainers

Fixes #21062 and #22870

@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Member

@jgowdyelastic jgowdyelastic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a couple of suggestions, but LGTM

import { addItemToRecentlyAccessed } from 'plugins/ml/util/recently_accessed';
import { mlJobService } from 'plugins/ml/services/job_service';


export function getQueryFromSavedSearch(formConfig) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getQueryFromSavedSearch is no longer used anywhere, so could be removed.

} else if (typeof q.query === 'object' &&
typeof q.query.query_string === 'object' && q.query.query_string.query !== '') {
query.query_string.query = q.query.query_string.query;
function createSearchItemsFromRoute($route) {
Copy link
Member

@jgowdyelastic jgowdyelastic Nov 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as this function is now wrapped in a provider, $route doesn't need to be passed in here and can be added to the provider's parameters above.
The function's name change may also then not be necessary, it could go back to being createSearchItems

// Takes the $route object to retrieve the indexPattern and savedSearch from the url
export function SearchItemsProvider(Private) {

const buildESQuery = Private(BuildESQueryProvider);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to see this change, this a lot better than using the getQueryFromSavedSearch function that I had created.
When this was originally written is wasn't possible to include BuildESQueryProvider to create the query object as it was using relative links internally that caused issues. I think because we were then inside x-pack-kibana

@@ -42,14 +42,16 @@ const module = uiModules.get('apps/ml');
module.controller('MlNewJobStepJobType',
function (
$scope,
$route) {
$route,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$route injection can be removed if you go with my suggestion above.

Copy link
Contributor

@walterra walterra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎉

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💔 Build Failed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants