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

[Logs UI] Create screen to set up analysis ML jobs #43413

Merged
merged 32 commits into from
Aug 22, 2019

Conversation

Zacqary
Copy link
Contributor

@Zacqary Zacqary commented Aug 15, 2019

Summary

Closes #41877
Based on #43050

Screen Shot 2019-08-19 at 11 37 46 AM

Screen Shot 2019-08-16 at 2 44 05 PM

Screen Shot 2019-08-16 at 2 44 02 PM

Checklist

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

For maintainers

@Zacqary Zacqary added release_note:enhancement Feature:Logs UI Logs UI feature Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.4.0 labels Aug 15, 2019
@Zacqary Zacqary requested a review from a team August 15, 2019 23:03
@elasticmachine
Copy link
Contributor

Pinging @elastic/infra-logs-ui

@elasticmachine
Copy link
Contributor

💔 Build Failed

@Kerry350 Kerry350 self-requested a review August 16, 2019 08:55
@Kerry350
Copy link
Contributor

Kerry350 commented Aug 16, 2019

@Zacqary

I've taken a look at the request failures you were seeing. For the most part these were just small syntax errors (with bucketSpan and timeField), which I've fixed in this commit.

This was the error coming back in relation to that:

{
   "error":{
      "root_cause":[
         {
            "type":"status_exception",
            "reason":"Datafeed aggregations are not parsable"
         }
      ],
      "type":"x_content_parse_exception",
      "reason":"[1:486] [datafeed_config] failed to parse field [aggregations]",
      "caused_by":{
         "type":"status_exception",
         "reason":"Datafeed aggregations are not parsable",
         "caused_by":{
            "type":"illegal_argument_exception",
            "reason":"failed to parse [{bucketSpan}ms]",
            "caused_by":{
               "type":"number_format_exception",
               "reason":"For input string: \"{bucketspan}\""
            }
         }
      }
   },
   "status":400
}

However, this did lead me to uncover a potential problem once the syntax issues were fixed. I started to get this error:

{
   "error":{
      "root_cause":[
         {
            "type":"status_exception",
            "reason":"No node found to start datafeed [datafeed-kibana-logs-ui-kerry-default-log-entry-rate], allocation explanation [cannot start datafeed [datafeed-kibana-logs-ui-kerry-default-log-entry-rate] because index [filebeat-*,kibana_sample_data_logs*] does not exist, is closed, or is still initializing.]"
         }
      ],
      "type":"status_exception",
      "reason":"No node found to start datafeed [datafeed-kibana-logs-ui-kerry-default-log-entry-rate], allocation explanation [cannot start datafeed [datafeed-kibana-logs-ui-kerry-default-log-entry-rate] because index [filebeat-*,kibana_sample_data_logs*] does not exist, is closed, or is still initializing.]"
   },
   "status":409
}

this is with the API sending a string of filebeat-*,kibana_sample_data_logs* using the multiple indices syntax, which is the default in the source configuration (and users can of course configure this to cover as many indices as they need). Back over in #42872, the indexPatternName was set to filebeat-* in the "Testing hints". If I swap this over to a hardcoded filebeat-* everything works as expected, with the jobs and datafeed returning success: true and started: true.

@jgowdyelastic does indexPatternName have to be a singular index pattern? Is there a way we can send multiple indices? I followed through to updateDatafeedIndices() from setupModuleItems() and it looked like it got passed through as is, but I don't know the code well past that point.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@Zacqary Zacqary marked this pull request as ready for review August 19, 2019 20:25
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@sophiec20
Copy link
Contributor

sophiec20 commented Aug 20, 2019

Looking at the elasticsearch ML APIs, a datafeed error occurs on _start if the indices are supplied as a single string rather than an array. Both of the following examples can be PUT however only the first example will _start without error.

PUT _ml/datafeeds/job1
{
    "job_id": "job1",
    "indices": [ "index1-*", "index2-*" ]
}
PUT _ml/datafeeds/job2
{
    "job_id": "job2",
    "indices": [ "index1-*,index2-*" ]
}

# error on _start:
# "type" : "resource_not_found_exception",
# "reason" : "datafeed [job2] cannot retrieve data because index [index1-*,index2-*] does not exist"

It seems to me that the Kibana ML setup process should split the string it receives for INDEX_PATTERN_NAME if it contains commas and create the datafeed correctly using an array of indices.

Note, in order for the ML analysis results to be meaningful for an event rate analysis, for this single search the resulting documents require the same time field throughout.

cc @jgowdyelastic

@jasonrhodes
Copy link
Member

Both of the following examples can be PUT however only the first example will _start without error.

PUT _ml/datafeeds/job1
{
    "job_id": "job1",
    "indices": [ "index1-*", "index2-*" ]
}

@sophiec20 if this example works, can we just split our string on commas before we send it? That would be a very easy thing for us to do, even if ML eventually splits on commas too.

@jasonrhodes
Copy link
Member

can we just split our string on commas before we send it? That would be a very easy thing for us to do, even if ML eventually splits on commas too.

Ah I think I understand now -- we don't call the datafeeds endpoints, we call the module endpoint, so this isn't something we can fix on our end.

@jgowdyelastic
Copy link
Member

#43686 created to enable the use of a comma separated index pattern in the modules/setup endpoint

* you may not use this file except in compliance with the Elastic License.
*/

import * as rt from 'io-ts';
Copy link
Member

Choose a reason for hiding this comment

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

Can someone explain to me why io-ts gets called rt?

@Kerry350
Copy link
Contributor

@jgowdyelastic Awesome, thank you!

@Kerry350
Copy link
Contributor

Just pushed the change here for ensuring the sample data index doesn't get passed to the ML setup.

} else {
return indexPattern;
}
};
Copy link
Member

@jasonrhodes jasonrhodes Aug 22, 2019

Choose a reason for hiding this comment

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

Would this work with a filter() too? If so that might be easier to follow later when we want to remember what this does and safely rip it out ... maybe it's just me but I always forget how indices and -1 and Array#splice all work :P

return indices.filter(index => index !== SAMPLE_DATA_INDEX).join(',')

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 I'll change this on the "cleanup / bug fixing" PR so I don't need to trigger another build cycle.

Copy link
Member

@jasonrhodes jasonrhodes left a comment

Choose a reason for hiding this comment

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

Everything here LGTM, great work @Kerry350 and @Zacqary

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@Kerry350 Kerry350 merged commit 40b28c7 into elastic:master Aug 22, 2019
@Zacqary Zacqary deleted the 41877-ml-setup-screen branch August 22, 2019 15:16
Kerry350 pushed a commit to Zacqary/kibana that referenced this pull request Aug 23, 2019
* Add empty analysis tab

* Add ml capabilities check

* Add job status checking functionality

* Add a loading page for the job status check

* Change types / change method for deriving space ID / change setup requirement filtering check

* Use new structure

* Add module setup to log analysis jobs hook

* Add ID to path

* [Logs UI] Add analyis setup landing screen

* Add function to set up ML module on click

* Use partial type for start and end props

* Add start and end time selection

* Fix syntax

* Change seconds timestamp to ms

* Update wording

* Use FormControlLayout to clear datepickers

* Update wording about earlier start date

* Remove specific point in time wording

* Fix typechecking

* Reload analysis page on successful job creation

* Add error handling for setup failure

* Update description ton of feature to reflect 7.4 feature set

* Add toggleable default message

* Revert to EuiFormControlLayout until eui changes are pushed

* Remove sample data index if user has it set
Zacqary added a commit that referenced this pull request Aug 23, 2019
* Add empty analysis tab

* Add ml capabilities check

* Add job status checking functionality

* Add a loading page for the job status check

* Change types / change method for deriving space ID / change setup requirement filtering check

* Use new structure

* Add module setup to log analysis jobs hook

* Add ID to path

* [Logs UI] Add analyis setup landing screen

* Add function to set up ML module on click

* Use partial type for start and end props

* Add start and end time selection

* Fix syntax

* Change seconds timestamp to ms

* Update wording

* Use FormControlLayout to clear datepickers

* Update wording about earlier start date

* Remove specific point in time wording

* Fix typechecking

* Reload analysis page on successful job creation

* Add error handling for setup failure

* Update description ton of feature to reflect 7.4 feature set

* Add toggleable default message

* Revert to EuiFormControlLayout until eui changes are pushed

* Remove sample data index if user has it set
@weltenwort weltenwort added release_note:skip Skip the PR/issue when compiling release notes and removed release_note:enhancement labels Sep 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Logs UI Logs UI feature release_note:skip Skip the PR/issue when compiling release notes Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.4.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Logs UI] Create screen to set up analysis ML jobs
8 participants