[ML] Adding file datavisualizer overrides #23194

jgowdyelastic · 2018-09-14T16:26:05Z

Adds the ability to override settings in the file datavisualizer.
When hitting Apply, the endpoint is called again with the user's changes.

Things which will be fixed in a follow up PR:

Better layout for overrides UI.
- Possibly with fields examples when editing field names
- Field name editing may move to a new tab.
All known values will be moved to constants, delimited, json etc
Better value checking when trying to populate super selects. EUISuperSelect is quite fragile and throws a huge error if you try to set a value that isn't in its list.
finish adding the tests, obvs.

elasticmachine · 2018-09-14T18:02:16Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-09-19T17:26:47Z

Pinging @elastic/ml-ui

elasticmachine · 2018-09-19T19:21:38Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

walterra · 2018-09-20T07:56:20Z

x-pack/plugins/ml/public/file_datavisualizer/components/edit_flyout/edit_flyout.js

+    this.setState({ isFlyoutVisible: true });
+  }
+
+  apply = () => {


Suggest to rename this to applyAndClose or similar. (just apply plays tricks on my mind when it's then used in other places in the code and I'd think of Function.prototype.apply())

walterra · 2018-09-20T08:18:33Z

x-pack/plugins/ml/public/file_datavisualizer/components/edit_flyout/edit_flyout.js

+    }
+    return (
+      <div>
+        {flyout}


Suggestion: Could be changed to use inline conditional rendering (https://reactjs.org/docs/conditional-rendering.html#inline-if-with-logical--operator) to make the use of let+if unncessary, something like:

<div> {this.state.isFlyoutVisible && ( <EuiFlyout ... </EuiFlyout> )} </div>

walterra · 2018-09-20T08:25:12Z

x-pack/plugins/ml/public/file_datavisualizer/components/edit_flyout/overrides.js

+  constructor(props) {
+    super(props);
+
+    this.originalSettings = this.props.originalSettings;


Not sure it's necessary to use this... here? As far as I can see it's only used in the constructor and only accessed, not changed further down. So suggest to either use const originalSettings here or this.props.originalSettings further down when it's accessed.

walterra · 2018-09-20T08:26:34Z

x-pack/plugins/ml/public/file_datavisualizer/components/edit_flyout/overrides.js

+      grokPattern,
+      timestampField,
+      timestampFormat,
+    } = this.overrides;


Similar to the above, not sure this.overrides = ... in necessary, could be just } = this.props.overrides; here.

walterra · 2018-09-20T08:35:55Z

x-pack/plugins/ml/public/file_datavisualizer/components/edit_flyout/overrides.js

+
+    this.fields = this.props.fields;
+    this.fieldOptions = this.fields.map(f => ({ value: f, inputDisplay: f }));
+    this.originalColumnNames = (this.state.columnNames !== undefined) ? [...this.state.columnNames] : [];


I think the above three lines should go to render(). The code might work as intended, but my understanding is that props could change and doing this only in the constructor will do this only once and will not pick up changing props.

peteharverson

Just a few minor comments

peteharverson · 2018-09-20T09:10:05Z

x-pack/plugins/ml/public/file_datavisualizer/components/edit_flyout/options.js

+
+export const timestampFormatOptions = [
+  {
+    value: 'dd/MMM/YYYY:HH:mm:ss Z',


Is the inputDisplay <span> contents for any of these not the same as the value? Just wondering if the contents can be generated from the value without duplicating the string? Same with the charset options below.

yes, these could be changed to be functions that generate the lists. I'll make this change in a follow up PR

peteharverson · 2018-09-20T09:16:11Z

x-pack/plugins/ml/public/file_datavisualizer/components/edit_flyout/overrides.js

+          </React.Fragment>
+        }
+        <EuiFormRow
+          label="Time stamp format"


I this this label should probably be Timestamp format

peteharverson · 2018-09-20T09:18:55Z

x-pack/plugins/ml/public/file_datavisualizer/components/edit_flyout/overrides.js

+  }
+}
+
+function convertDelimiter(d) {


Can you add a comment as to what the role of this function is. Why does each delimiter need to have a customDelimiter which is blank string?

peteharverson · 2018-09-20T09:19:15Z

x-pack/plugins/ml/public/file_datavisualizer/components/edit_flyout/overrides.js

+  }
+}
+
+function convertDelimiterBack({ delimiter, customDelimiter }) {


As above, I think the role of this function needs a comment

peteharverson · 2018-09-20T09:20:21Z

...l/public/file_datavisualizer/components/file_datavisualizer_view/file_datavisualizer_view.js

+        // check to see if the settings from the server which haven't been overridden have changed.
+        // e.g. changing the name of the time field which is also the time field
+        // will cause the timestamp_field setting to change.
+        // if any have change, update the originalSettings value


nit - should be if any have changed...

elasticmachine · 2018-09-20T09:30:48Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

walterra

LGTM, just added a few comments about a react component structure.

alvarezmelissa87 · 2018-09-20T10:34:35Z

x-pack/plugins/ml/public/file_datavisualizer/components/edit_flyout/overrides.test.js

+
+    expect(component).toMatchSnapshot();
+  });
+});


Great to have the full snapshot test here! It'd be a good idea to add tests for the conditionals for the form rows in the render as well. Just checking that a particular label with the expected text exists when a condition is met (or something like that) should be enough.
This way we can make sure what's expected is showing up under different conditions and it's not as susceptible to implementation detail changes as snapshots.

yes, i agree. This test was added as a placeholder with the intention of adding tests to cover everything at a later stage.
The rest of the tests will be added in a subsequent PR.

alvarezmelissa87 · 2018-09-20T10:56:19Z

x-pack/plugins/ml/public/file_datavisualizer/components/results_view/results_view.js


-    this.state = {};
-  }
+  console.log(results);



Nit - just making sure we want to log the results here... :)

I'm leaving log statements in for the time being as this is still on a feature branch and very much a work in progress.

peteharverson

Latest changes LGTM

walterra · 2018-09-20T11:53:51Z

Latest changes LGTM 🎉

alvarezmelissa87

👍 LGTM

elasticmachine · 2018-09-20T12:11:37Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-09-20T13:52:29Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-09-20T15:31:21Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-09-20T16:17:41Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

* [ML] File datavisualizer initial commit (#22828) * [ML] File datavisualizer initial commit * removing mocked data and adding initial stats * adding card styling to fields * Revert "". accidentally added with no commit message This reverts commit d762d20b706e6a770e631f863b9e7d8879bb7ee6. * adding date type to timestamp field * renaming FileStats to FieldsStats * code clean up * changes based on review * changes to error handling * [ML] Adding file datavisualizer overrides (#23194) * [ML] Adding file datavisualizer overrides * improvements to overrides * removing comment * small refactor * removing accidentally added file * updates based on review * fixing broken test * adding missing grok pattern override * fixing test * [ML] Refactoring override option lists (#23424) * [ML] Refactoring override option lists * moving lists out of class * updating test snapshot * [ML] Fixing field editing (#23500) * [ML] Changes to timestamp formats (#23498) * [ML] Changes to timestamp formats * updating test snapshot * [ML] Allow Datavisualizer use on basic license (#23748) * [ML] Allow ML use on basic license * removing timeout change * adding permission checks * updating tests * removing unnecessary checks * [ML] Adds new page for choosing file or index based data visualizer (#23763) * [ML] Adding license check to datavisualizer landing page (#23809) * [ML] Adding license check to datavisualizer landing page * removing comments * updating redirect to landing page * [ML] Adding ability to upload data to elasticsearch from datavisualizer (#24042) * [ML] Initial work for delimited file upload * adding results links cards * adding nav menu * removing accidental debugger * initial work for importing semi structured text * using ingest pipeline for import * adding json importer and better error reporting * better progress steps * time range added to results links * first import only creates index and pipeline * adding status constants * using status constants * adding explanation comment * updating yarn.lock * changes based on review * fixing space * fixing space again, stort it out git * removing oversized background container causing constant scrollbar * [ML] Adding basic license check when loading privileges (#24173) * [ML] Adding basic license check * missing import * [ML] Adds an About panel to the file data visualizer landing page (#24260) * [ML] Adds an About panel to the file data visualizer landing page * [ML] Remove unnecessary style from file data visualizer scss * [ML] Adding better error reporting for reading and importing data (#24269) * [ML] Adding better error reporting for reading and importing data * changes to endpoint errors * displaying errors * step logic refactor * removing log statements * [ML] Switch file data visualizer to use Papa Parse for CSV parsing (#24329) * [ML] Fixes layout of Data Visualizer selector page for IE (#24387) * [ML] Adding ability to override various settings when importing data (#24346) * [ML] Adding ability to override various settings when importing data * second commit with most of the outstanding code * improving index pattern name validation * better index pattern matching * adding comments * adding empty index pattern check * changes based on review * fixing test

* [ML] File datavisualizer initial commit (elastic#22828) * [ML] File datavisualizer initial commit * removing mocked data and adding initial stats * adding card styling to fields * Revert "". accidentally added with no commit message This reverts commit d762d20b706e6a770e631f863b9e7d8879bb7ee6. * adding date type to timestamp field * renaming FileStats to FieldsStats * code clean up * changes based on review * changes to error handling * [ML] Adding file datavisualizer overrides (elastic#23194) * [ML] Adding file datavisualizer overrides * improvements to overrides * removing comment * small refactor * removing accidentally added file * updates based on review * fixing broken test * adding missing grok pattern override * fixing test * [ML] Refactoring override option lists (elastic#23424) * [ML] Refactoring override option lists * moving lists out of class * updating test snapshot * [ML] Fixing field editing (elastic#23500) * [ML] Changes to timestamp formats (elastic#23498) * [ML] Changes to timestamp formats * updating test snapshot * [ML] Allow Datavisualizer use on basic license (elastic#23748) * [ML] Allow ML use on basic license * removing timeout change * adding permission checks * updating tests * removing unnecessary checks * [ML] Adds new page for choosing file or index based data visualizer (elastic#23763) * [ML] Adding license check to datavisualizer landing page (elastic#23809) * [ML] Adding license check to datavisualizer landing page * removing comments * updating redirect to landing page * [ML] Adding ability to upload data to elasticsearch from datavisualizer (elastic#24042) * [ML] Initial work for delimited file upload * adding results links cards * adding nav menu * removing accidental debugger * initial work for importing semi structured text * using ingest pipeline for import * adding json importer and better error reporting * better progress steps * time range added to results links * first import only creates index and pipeline * adding status constants * using status constants * adding explanation comment * updating yarn.lock * changes based on review * fixing space * fixing space again, stort it out git * removing oversized background container causing constant scrollbar * [ML] Adding basic license check when loading privileges (elastic#24173) * [ML] Adding basic license check * missing import * [ML] Adds an About panel to the file data visualizer landing page (elastic#24260) * [ML] Adds an About panel to the file data visualizer landing page * [ML] Remove unnecessary style from file data visualizer scss * [ML] Adding better error reporting for reading and importing data (elastic#24269) * [ML] Adding better error reporting for reading and importing data * changes to endpoint errors * displaying errors * step logic refactor * removing log statements * [ML] Switch file data visualizer to use Papa Parse for CSV parsing (elastic#24329) * [ML] Fixes layout of Data Visualizer selector page for IE (elastic#24387) * [ML] Adding ability to override various settings when importing data (elastic#24346) * [ML] Adding ability to override various settings when importing data * second commit with most of the outstanding code * improving index pattern name validation * better index pattern matching * adding comments * adding empty index pattern check * changes based on review * fixing test

* [ML] File datavisualizer initial commit (#22828) * [ML] File datavisualizer initial commit * removing mocked data and adding initial stats * adding card styling to fields * Revert "". accidentally added with no commit message This reverts commit d762d20b706e6a770e631f863b9e7d8879bb7ee6. * adding date type to timestamp field * renaming FileStats to FieldsStats * code clean up * changes based on review * changes to error handling * [ML] Adding file datavisualizer overrides (#23194) * [ML] Adding file datavisualizer overrides * improvements to overrides * removing comment * small refactor * removing accidentally added file * updates based on review * fixing broken test * adding missing grok pattern override * fixing test * [ML] Refactoring override option lists (#23424) * [ML] Refactoring override option lists * moving lists out of class * updating test snapshot * [ML] Fixing field editing (#23500) * [ML] Changes to timestamp formats (#23498) * [ML] Changes to timestamp formats * updating test snapshot * [ML] Allow Datavisualizer use on basic license (#23748) * [ML] Allow ML use on basic license * removing timeout change * adding permission checks * updating tests * removing unnecessary checks * [ML] Adds new page for choosing file or index based data visualizer (#23763) * [ML] Adding license check to datavisualizer landing page (#23809) * [ML] Adding license check to datavisualizer landing page * removing comments * updating redirect to landing page * [ML] Adding ability to upload data to elasticsearch from datavisualizer (#24042) * [ML] Initial work for delimited file upload * adding results links cards * adding nav menu * removing accidental debugger * initial work for importing semi structured text * using ingest pipeline for import * adding json importer and better error reporting * better progress steps * time range added to results links * first import only creates index and pipeline * adding status constants * using status constants * adding explanation comment * updating yarn.lock * changes based on review * fixing space * fixing space again, stort it out git * removing oversized background container causing constant scrollbar * [ML] Adding basic license check when loading privileges (#24173) * [ML] Adding basic license check * missing import * [ML] Adds an About panel to the file data visualizer landing page (#24260) * [ML] Adds an About panel to the file data visualizer landing page * [ML] Remove unnecessary style from file data visualizer scss * [ML] Adding better error reporting for reading and importing data (#24269) * [ML] Adding better error reporting for reading and importing data * changes to endpoint errors * displaying errors * step logic refactor * removing log statements * [ML] Switch file data visualizer to use Papa Parse for CSV parsing (#24329) * [ML] Fixes layout of Data Visualizer selector page for IE (#24387) * [ML] Adding ability to override various settings when importing data (#24346) * [ML] Adding ability to override various settings when importing data * second commit with most of the outstanding code * improving index pattern name validation * better index pattern matching * adding comments * adding empty index pattern check * changes based on review * fixing test

[ML] Adding file datavisualizer overrides

e78cc45

jgowdyelastic added 2 commits September 19, 2018 18:16

improvements to overrides

a197bf4

removing comment

8656dfd

jgowdyelastic requested review from peteharverson, walterra and alvarezmelissa87 September 19, 2018 17:26

jgowdyelastic self-assigned this Sep 19, 2018

jgowdyelastic added :ml Feature:New Feature New feature not correlating to an existing feature label labels Sep 19, 2018

small refactor

ba9966e

removing accidentally added file

ca3c3c4

walterra reviewed Sep 20, 2018

View reviewed changes

peteharverson reviewed Sep 20, 2018

View reviewed changes

walterra approved these changes Sep 20, 2018

View reviewed changes

alvarezmelissa87 reviewed Sep 20, 2018

View reviewed changes

updates based on review

460e23c

peteharverson approved these changes Sep 20, 2018

View reviewed changes

alvarezmelissa87 approved these changes Sep 20, 2018

View reviewed changes

fixing broken test

6eff145

jgowdyelastic added 2 commits September 20, 2018 14:55

adding missing grok pattern override

4f3703e

fixing test

1bdb840

jgowdyelastic merged commit a265611 into elastic:feature/file-datavisualizer Sep 20, 2018

jgowdyelastic mentioned this pull request Sep 22, 2018

[ML] Refactoring override option lists #23424

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML] Adding file datavisualizer overrides #23194

[ML] Adding file datavisualizer overrides #23194

jgowdyelastic commented Sep 14, 2018 •

edited

Loading

elasticmachine commented Sep 14, 2018

elasticmachine commented Sep 19, 2018

elasticmachine commented Sep 19, 2018

walterra Sep 20, 2018

walterra Sep 20, 2018

walterra Sep 20, 2018

walterra Sep 20, 2018

walterra Sep 20, 2018

peteharverson left a comment

peteharverson Sep 20, 2018

jgowdyelastic Sep 20, 2018

peteharverson Sep 20, 2018

peteharverson Sep 20, 2018

peteharverson Sep 20, 2018

peteharverson Sep 20, 2018

elasticmachine commented Sep 20, 2018

walterra left a comment

alvarezmelissa87 Sep 20, 2018

jgowdyelastic Sep 20, 2018

alvarezmelissa87 Sep 20, 2018

jgowdyelastic Sep 20, 2018

peteharverson left a comment

walterra commented Sep 20, 2018

alvarezmelissa87 left a comment

elasticmachine commented Sep 20, 2018

elasticmachine commented Sep 20, 2018

elasticmachine commented Sep 20, 2018

elasticmachine commented Sep 20, 2018

[ML] Adding file datavisualizer overrides #23194

[ML] Adding file datavisualizer overrides #23194

Conversation

jgowdyelastic commented Sep 14, 2018 • edited Loading

elasticmachine commented Sep 14, 2018

💔 Build Failed

elasticmachine commented Sep 19, 2018

elasticmachine commented Sep 19, 2018

💔 Build Failed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peteharverson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Sep 20, 2018

💔 Build Failed

walterra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peteharverson left a comment

Choose a reason for hiding this comment

walterra commented Sep 20, 2018

alvarezmelissa87 left a comment

Choose a reason for hiding this comment

elasticmachine commented Sep 20, 2018

💔 Build Failed

elasticmachine commented Sep 20, 2018

💚 Build Succeeded

elasticmachine commented Sep 20, 2018

💔 Build Failed

elasticmachine commented Sep 20, 2018

💚 Build Succeeded

jgowdyelastic commented Sep 14, 2018 •

edited

Loading