[ML] Client-Side Job Validation v6.3

elasticmachine commented 6 years ago

Original comment by @walterra:

(Client-Side means x-pack-kibana instead of x-pack-elasticsearch in this context)

This is a follow up issue to LINK REDACTED to keep track of the actual implementation.

UI/Job Validation Framework

[x] General boilerplate/framework to support validation checks including UI prototype LINK REDACTED
[x] Refactor UI to use EUI/React LINK REDACTED LINK REDACTED
[x] Enable Buttons in jobs list and wizards LINK REDACTED LINK REDACTED
[x] Change UI to display "success" messages for test categories LINK REDACTED
[x] Include job validation message status in messages.json LINK REDACTED
[x] Deduplicate messages LINK REDACTED
[x] Loading indicator / status icon for job validation button LINK REDACTED
[x] Add check for duplicated detectors to population wizard LINK REDACTED
[x] "Learn more" links should link to current version LINK REDACTED
[x] Fix "undefined" fieldname LINK REDACTED

Bugs

[x] supress result message when model memory checks are not executed https://github.com/elastic/kibana/issues/18517
[x] check for valid format of model memory limit https://github.com/elastic/kibana/issues/18518

Checks v6.3

[x] Basic checks (previously advanced job wizard form checks) moved to server LINK REDACTED
[x] Check cardinality of fields
- [x] Basic checks LINK REDACTED
- [x] by fields and other tweaks LINK REDACTED
- [x] Rules for skipping checks LINK REDACTED
[x] Check bucket_span
- [x] invalid if zero LINK REDACTED
- [x] test if > 1 day LINK REDACTED
- [x] test if different from bucket-estimation (trigger INFO level message) LINK REDACTED (disabled for now)
[x] Check influencers
- [x] are there any? LINK REDACTED
- [x] Rules for skipping checks LINK REDACTED
[x] Improve start/end time handling
- [x] Add optional support for start/end timestamps to the node API endpoint. Use this for the job validation checks triggered from wizards instead of hijacking data_count. LINK REDACTED
- [x] If start/end is not set, try to fallback to timespan of original data LINK REDACTED
- [x] Check for data before the unix epoch (see LINK REDACTED)
- [x] Check if time range is 25 times the bucket span LINK REDACTED or at least 2h LINK REDACTED
[x] Check against model memory limit LINK REDACTED
[x] Check job does not contain duplicate detectors (same function and field names) LINK REDACTED
[x] Check for non-aggregatable fields LINK REDACTED LINK REDACTED
[x] If model_plot_config enabled and cardinality of by/over/partition >100 then warn this is a resource intensive job LINK REDACTED

Unit Tests

They should cover all possible message ids being returned.

[x] Basic tests LINK REDACTED LINK REDACTED
[x] Cardinality LINK REDACTED
[x] Bucket Span LINK REDACTED
[x] Influencers LINK REDACTED
[x] Time Range LINK REDACTED
[x] Model Memory Limit LINK REDACTED

elasticmachine commented 6 years ago

Original comment by @sophiec20:

Awesome first review... feedback coming...

A1: Repeated error for each detector - any way to de-dup? Double over_field value check. and also

A2: Links to blogs I don't think we should link to blogs from inside the UI. It seems a bit too temporary and unable to version control. We could link to a page in the docs, which could then link to a blog - however should we do this either? @lcawl thoughts? is there precedence in say upgrade wiz or add data?

A3: bucket span test is very quick (and always seems to be 15m) - is this actually plugged in?

elasticmachine commented 6 years ago

Original comment by @walterra:

Yes those blog links are temporary, Lisa is already working on replacing them here: LINK REDACTED

elasticmachine commented 6 years ago

Original comment by @jgowdyelastic:

Just making a note here so I don't forget. The current way that the validation button has been added to the job wizards might need addressing. The job config is passed to the mlValidateJob directive by a function call: job="getJobFromConfig()" this means this function is called on every digest cycle. Also as a side effect it means the job id validation is called every digest cycle. So the user gets this error message as soon as the page loads:

Perhaps a reference to getJobFromConfig could be passed across instead and only called when the Validate job button is pressed.

elasticmachine commented 6 years ago

Original comment by @walterra:

there's now a check in place to detect duplicate detectors LINK REDACTED (already in master/6.x)
job validation message deduplication is added in LINK REDACTED
the above problem in the wizards described by James is solved in LINK REDACTED

elasticmachine commented 6 years ago

Original comment by @walterra:

I cleaned up the issue's description an moved validation checks planned for v6.4 to https://github.com/elastic/kibana/issues/18368.

walterra commented 6 years ago

Created a follow up issue to track further progress for v6.3.x https://github.com/elastic/kibana/issues/18898

elastic / kibana