[ML] DF Analytics Regression results: Ensure error handling and table sort works correctly #49929

alvarezmelissa87 · 2019-10-31T21:12:15Z

Summary

This PR addresses all comments set as followups in #49667
including:

sorting issue fix (query then sort would empty the table)
Adds label to table for number of documents obtained

Error handling for initial jobConfig loading in results view. If the jobConfig fails to load this error message is shown instead of an infinite loading screen.

small code improvements

@walterra - I'd particularly like your thoughts on removing the clearTable workaround you originally added for EuiInMemoryTable to deal with issues with column sort. It appears to work as expected without the workaround - looks like Eui may have updated that component to fix the original issue as I can't reproduce it. I'll link a particular fix here if I can find one.

Checklist

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

This was checked for cross-browser compatibility, including a check against IE11
Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
~~- [ ] Documentation was added for features that require explanation or tutorials~~
Unit or functional tests were updated or added to match the most common scenarios
~~- [ ] This was checked for keyboard-only and screenreader accessibility~~

For maintainers

~~- [ ] This was checked for breaking API changes and was labeled appropriately~~
~~- [ ] This includes a feature addition or change that requires a release note and was labeled appropriately~~

elasticmachine · 2019-10-31T21:12:18Z

Pinging @elastic/ml-ui (:ml)

elasticmachine · 2019-10-31T22:33:28Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 649baf0

elasticmachine · 2019-11-01T03:15:29Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 3cbca2f

darnautov

LGTM, just some minor suggestions

darnautov · 2019-11-01T08:34:32Z

...ics/pages/analytics_exploration/components/regression_exploration/regression_exploration.tsx

+export const ExplorationTitle: React.SFC<{ jobId: string }> = ({ jobId }) => (
+  <EuiTitle size="xs">
+    <span>
+      {i18n.translate('xpack.ml.dataframe.analytics.regressionExploration.jobIdTitle', {


FormattedMessage? 👀

Using FormattedMessage with EuiTitle shows an Types of property 'children' are incompatible TypeScript error. Since we've got some cleanup to do with replacing other .translate with FormattedMessage it might be best to update them in a single PR once we have that fix upstream. I can double check with the eui team to see what the timeline for that is.

darnautov · 2019-11-01T08:37:11Z

...ics/pages/analytics_exploration/components/regression_exploration/regression_exploration.tsx

+      if (e.message !== undefined) {
+        setJobConfigErrorMessage(e.message);
+      } else {
+        setJobConfigErrorMessage(JSON.stringify(e));
+      }


I've seen this logic in a couple of places already, maybe we should create a function (or custom hook) to handle it?

For now I think since this logic is fairly short and clear it's okay as is. 👍

peteharverson · 2019-11-01T11:46:06Z

...me_analytics/pages/analytics_exploration/components/regression_exploration/results_table.tsx

+            helpText={i18n.translate(
+              'xpack.ml.dataframe.analytics.regressionExploration.documentsShownHelpText',
+              {
+                defaultMessage: 'Showing first 1000 documents',


Maybe you should parameterize the 1000 and pass in SEARCH_SIZE from data_frame_analytics/common/analytics.ts ?

Definitely should be - fixed in 90a54cc
Thanks 😄

peteharverson

LGTM
I think in a follow up we should address issues around field types - setting the columnType for each column using the field caps API (which explains why what look like numeric data sometimes get left aligned as strings), and disabling column sort when the type is text

elasticmachine · 2019-11-01T15:53:17Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 90a54cc

… sort works correctly (elastic#49929) * add error handling for regression jobConfig fetch * fix sorting change causing blank table by removing table render timeout * Add label to table for number of docs obtained * parameterize searchSize in documents fetched text

… sort works correctly (#49929) (#49954) * add error handling for regression jobConfig fetch * fix sorting change causing blank table by removing table render timeout * Add label to table for number of docs obtained * parameterize searchSize in documents fetched text

alvarezmelissa87 added 2 commits October 31, 2019 15:43

add error handling for regression jobConfig fetch

55e827e

fix sorting change causing blank table by removing table render timeout

649baf0

alvarezmelissa87 added :ml v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:Data Frame Analytics ML data frame analytics features v7.6.0 labels Oct 31, 2019

alvarezmelissa87 requested review from walterra, darnautov, peteharverson and jgowdyelastic October 31, 2019 21:12

alvarezmelissa87 requested review from a team as code owners October 31, 2019 21:12

alvarezmelissa87 self-assigned this Oct 31, 2019

Add label to table for number of docs obtained

3cbca2f

darnautov approved these changes Nov 1, 2019

View reviewed changes

peteharverson reviewed Nov 1, 2019

View reviewed changes

parameterize searchSize in documents fetched text

90a54cc

peteharverson approved these changes Nov 1, 2019

View reviewed changes

alvarezmelissa87 merged commit 568b8d3 into elastic:master Nov 1, 2019

alvarezmelissa87 mentioned this pull request Nov 1, 2019

[7.x] [ML] DF Analytics Regression results: Ensure error handling and table sort works correctly (#49929) #49954

Merged

alvarezmelissa87 deleted the ml-regression-ui-followups branch November 1, 2019 16:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML] DF Analytics Regression results: Ensure error handling and table sort works correctly #49929

[ML] DF Analytics Regression results: Ensure error handling and table sort works correctly #49929

alvarezmelissa87 commented Oct 31, 2019 •

edited

Loading

elasticmachine commented Oct 31, 2019

elasticmachine commented Oct 31, 2019

elasticmachine commented Nov 1, 2019

darnautov left a comment

darnautov Nov 1, 2019

alvarezmelissa87 Nov 1, 2019

darnautov Nov 1, 2019

alvarezmelissa87 Nov 1, 2019

peteharverson Nov 1, 2019

alvarezmelissa87 Nov 1, 2019

peteharverson left a comment

elasticmachine commented Nov 1, 2019

[ML] DF Analytics Regression results: Ensure error handling and table sort works correctly #49929

[ML] DF Analytics Regression results: Ensure error handling and table sort works correctly #49929

Conversation

alvarezmelissa87 commented Oct 31, 2019 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Oct 31, 2019

elasticmachine commented Oct 31, 2019

💚 Build Succeeded

elasticmachine commented Nov 1, 2019

💚 Build Succeeded

darnautov left a comment

Choose a reason for hiding this comment

darnautov Nov 1, 2019

Choose a reason for hiding this comment

alvarezmelissa87 Nov 1, 2019

Choose a reason for hiding this comment

darnautov Nov 1, 2019

Choose a reason for hiding this comment

alvarezmelissa87 Nov 1, 2019

Choose a reason for hiding this comment

peteharverson Nov 1, 2019

Choose a reason for hiding this comment

alvarezmelissa87 Nov 1, 2019

Choose a reason for hiding this comment

peteharverson left a comment

Choose a reason for hiding this comment

elasticmachine commented Nov 1, 2019

💚 Build Succeeded

alvarezmelissa87 commented Oct 31, 2019 •

edited

Loading