[ML] DFA: ensure at least one field is included in analysis before job can be created #65320

alvarezmelissa87 · 2020-05-05T17:59:35Z

Summary

Checks the field_selection return from the _explain api to ensure that there is at least one other field included that is not also required.

If there is not at least one field available for the analysis creation will be disabled and user will see an error message.

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
This was checked for cross-browser compatibility, including a check against IE11

elasticmachine · 2020-05-05T18:00:29Z

Pinging @elastic/ml-ui (:ml)

jgowdyelastic

Code LGTM

peteharverson · 2020-05-06T11:59:38Z

...lytics/pages/analytics_management/components/create_analytics_form/create_analytics_form.tsx

@@ -48,6 +48,8 @@ import {
 } from '../../../../common/analytics';
 import { shouldAddAsDepVarOption, OMIT_FIELDS } from './form_options_validation';

+const requiredFieldsErrorText = 'At least one field must be included in the analysis.';


This needs to be internationalized.

Updated in 592d87510e6ab0e58d2e72e2c1dbe30cf093bf50

peteharverson · 2020-05-06T12:16:02Z

...lytics/pages/analytics_management/components/create_analytics_form/create_analytics_form.tsx

@@ -565,7 +587,9 @@ export const CreateAnalyticsForm: FC<CreateAnalyticsFormProps> = ({ actions, sta
            <Fragment>
              <EuiFormRow
                fullWidth
-                isInvalid={maxDistinctValuesError !== undefined}
+                isInvalid={


I wonder if it might make more sense to show the invalid field warning below the 'Excluded fields' control, as it's most likely to appear when the user selects too many fields to exclude?

🤔 Good point, @peteharverson. Updated in 592d87510e6ab0e58d2e72e2c1dbe30cf093bf50

walterra · 2020-05-06T15:35:46Z

...lytics/pages/analytics_management/components/create_analytics_form/create_analytics_form.tsx

+      const fieldSelection: FieldSelectionItem[] = resp.field_selection;
+
+      let hasRequiredFields = false;
+      if (fieldSelection) {


Is this if part necessary? Asking because the assignment looks like it will always be an array. Maybe this whole part can even become something like:

const hasRequiredFields = resp.field_selection.some(field => field.is_included === true && field.is_required === false);

Wrapping in an if statement is mostly an extra safety precaution for something going wrong silently in the backend or changes to it that would make field_selection undefined. Looking at the docs, there doesn't seem to be a guarantee that that field will always be populated. The for loop prevents us from having to create a function on every go through. Though in this case the optimization is probably not significant. 🤔

I think for now I'd lean toward keeping it though I'm happy to update if you feel strongly (the readability is a bit better with your suggestion)

Thanks for clarifying! Maybe we can then update the response type DfAnalyticsExplainResponse to something like field_selection?: FieldSelectionItem[]; so TypeScript will force us to consider that the field could be not returned?

Type updated in 4477264

peteharverson

Tested latest edits and LGTM

walterra

LGTM pending the discussed TS update

kibanamachine · 2020-05-07T20:22:21Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 4477264

History

💚 Build #46031 succeeded d4a4b2d
💔 Build #45989 failed 592d87510e6ab0e58d2e72e2c1dbe30cf093bf50
💚 Build #45634 succeeded cb5fdc43b64d4704779981988f16c6e4066a7afd

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

…b can be created (elastic#65320) * ensure at least one field besides depVar included in analysis * show requiredFieldsError above excluded fields * update jest test * update fieldSelection explainResponse type

…b can be created (#65320) (#65773) * ensure at least one field besides depVar included in analysis * show requiredFieldsError above excluded fields * update jest test * update fieldSelection explainResponse type

…b can be created (#65320) (#65774) * ensure at least one field besides depVar included in analysis * show requiredFieldsError above excluded fields * update jest test * update fieldSelection explainResponse type

…or-part-mvp-2 * 'master' of github.com:elastic/kibana: (259 commits) SavedObjects bulkCreate API should return migrationVersion and strip the type & namespace from the id (elastic#65150) Drilldown count tooltip (elastic#65105) plugins logs start with "plugins." prefix (elastic#65710) [ML] Fix pagination reset on search query update. (elastic#65668) [SIEM] Add types to the mappings objects so extra keys cannot be introduced [apm] Update machine learning flyout and service maps docs (elastic#65517) change api endpoint and throw error (elastic#65790) [Maps] remove SLA percentage metric (elastic#65718) [Reporting] APM integration for baseline performance measurements (elastic#59967) fix(NA): noParse regex for windows on kbn optimizer (elastic#65755) [ML] DFA: ensure at least one field is included in analysis before job can be created (elastic#65320) [Data plugin] cleanup - remove unused getRoutes / routes from indexPattern object (elastic#65683) Removed skip to enable test. (elastic#65575) [Lens] Type safe migrations (elastic#65576) [Canvas] Fix nav link behavior in Canvas (elastic#65590) [Event log] Fix flaky test (elastic#65658) [Alerting] changes preconfigured actions config from array to object (elastic#65397) remove immediate functions from esqueue worker cycles (elastic#65375) [Metrics UI] Fix isAbove to only display when threshold set (elastic#65540) draft search profiler accessibility tests (elastic#62357) ... # Conflicts: # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_form_fields.tsx

alvarezmelissa87 added the release_note:fix label May 5, 2020

alvarezmelissa87 requested review from walterra, peteharverson and jgowdyelastic May 5, 2020 17:59

alvarezmelissa87 requested a review from a team as a code owner May 5, 2020 17:59

alvarezmelissa87 self-assigned this May 5, 2020

alvarezmelissa87 changed the title ~~ensure at least one field besides depVar included in analysis~~ [ML] DFA: ensure at least one field is included in analysis before job can be created May 5, 2020

alvarezmelissa87 added :ml Feature:Data Frame Analytics ML data frame analytics features v7.8.0 v8.0.0 labels May 5, 2020

jgowdyelastic approved these changes May 5, 2020

View reviewed changes

peteharverson reviewed May 6, 2020

View reviewed changes

walterra reviewed May 6, 2020

View reviewed changes

peteharverson approved these changes May 6, 2020

View reviewed changes

alvarezmelissa87 added 3 commits May 6, 2020 13:02

ensure at least one field besides depVar included in analysis

defb910

show requiredFieldsError above excluded fields

6a8cbb0

update jest test

d4a4b2d

alvarezmelissa87 force-pushed the ml-analytics-index-check branch from 55c471a to d4a4b2d Compare May 6, 2020 17:03

walterra approved these changes May 7, 2020

View reviewed changes

update fieldSelection explainResponse type

4477264

alvarezmelissa87 merged commit 034f259 into elastic:master May 7, 2020

alvarezmelissa87 mentioned this pull request May 7, 2020

[7.x] [ML] DFA: ensure at least one field is included in analysis before job can be created (#65320) #65773

Merged

alvarezmelissa87 mentioned this pull request May 7, 2020

[7.8] [ML] DFA: ensure at least one field is included in analysis before job can be created (#65320) #65774

Merged

alvarezmelissa87 deleted the ml-analytics-index-check branch May 7, 2020 20:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML] DFA: ensure at least one field is included in analysis before job can be created #65320

[ML] DFA: ensure at least one field is included in analysis before job can be created #65320

alvarezmelissa87 commented May 5, 2020 •

edited

Loading

elasticmachine commented May 5, 2020

jgowdyelastic left a comment

peteharverson May 6, 2020

alvarezmelissa87 May 6, 2020

peteharverson May 6, 2020

alvarezmelissa87 May 6, 2020 •

edited

Loading

walterra May 6, 2020

alvarezmelissa87 May 6, 2020 •

edited

Loading

walterra May 7, 2020 •

edited

Loading

alvarezmelissa87 May 7, 2020

peteharverson left a comment

walterra left a comment

kibanamachine commented May 7, 2020

[ML] DFA: ensure at least one field is included in analysis before job can be created #65320

[ML] DFA: ensure at least one field is included in analysis before job can be created #65320

Conversation

alvarezmelissa87 commented May 5, 2020 • edited Loading

Summary

Checklist

elasticmachine commented May 5, 2020

jgowdyelastic left a comment

Choose a reason for hiding this comment

peteharverson May 6, 2020

Choose a reason for hiding this comment

alvarezmelissa87 May 6, 2020

Choose a reason for hiding this comment

peteharverson May 6, 2020

Choose a reason for hiding this comment

alvarezmelissa87 May 6, 2020 • edited Loading

Choose a reason for hiding this comment

walterra May 6, 2020

Choose a reason for hiding this comment

alvarezmelissa87 May 6, 2020 • edited Loading

Choose a reason for hiding this comment

walterra May 7, 2020 • edited Loading

Choose a reason for hiding this comment

alvarezmelissa87 May 7, 2020

Choose a reason for hiding this comment

peteharverson left a comment

Choose a reason for hiding this comment

walterra left a comment

Choose a reason for hiding this comment

kibanamachine commented May 7, 2020

💚 Build Succeeded

History

alvarezmelissa87 commented May 5, 2020 •

edited

Loading

alvarezmelissa87 May 6, 2020 •

edited

Loading

alvarezmelissa87 May 6, 2020 •

edited

Loading

walterra May 7, 2020 •

edited

Loading