Skip to content

[ML] Show better file structure finder explanations#62316

Merged
jgowdyelastic merged 9 commits intoelastic:masterfrom
jgowdyelastic:show-better-find-file-structure-finder-information
Apr 7, 2020
Merged

[ML] Show better file structure finder explanations#62316
jgowdyelastic merged 9 commits intoelastic:masterfrom
jgowdyelastic:show-better-find-file-structure-finder-information

Conversation

@jgowdyelastic
Copy link
Copy Markdown
Member

@jgowdyelastic jgowdyelastic commented Apr 2, 2020 •
edited
Loading

The find_file_structure endpoint is now always called with the explain flag to allows it to show the logical steps it has gone through to produce its results.
These are available in a flyout when analysis has been successful:
image

They are also displayed when the analysis has failed:
image

Also converts all simple functional components and non-react utility code to typescript.
More complicated react components have been left as javascript for now.

Also updates the wrapError function used on all of our endpoint errors to allow an optional attributes property to contain the body of the originally throw error.

cc @droberts195

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@jgowdyelastic jgowdyelastic marked this pull request as ready for review April 3, 2020 14:43
@jgowdyelastic jgowdyelastic requested a review from a team as a code owner April 3, 2020 14:43
@jgowdyelastic jgowdyelastic self-assigned this Apr 3, 2020
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Apr 3, 2020

Pinging @elastic/ml-ui (:ml)

@jgowdyelastic jgowdyelastic force-pushed the show-better-find-file-structure-finder-information branch from c03ffff to 09c4d8e Compare April 3, 2020 18:57
@jgowdyelastic
Copy link
Copy Markdown
Member Author

jgowdyelastic commented Apr 4, 2020

@elasticmachine merge upstream

@jgowdyelastic
Copy link
Copy Markdown
Member Author

jgowdyelastic commented Apr 6, 2020

@elasticmachine merge upstream

@droberts195
Copy link
Copy Markdown

droberts195 commented Apr 6, 2020

Thanks for doing this. I think it will make it much easier for people to self-diagnose common problems with file uploads, such as CSV files with different numbers of fields on some lines.

peteharverson
peteharverson approved these changes Apr 6, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

Really nice new functionality! Left a couple of minor comments, but otherwise LGTM.

alvarezmelissa87
alvarezmelissa87 reviewed Apr 6, 2020
View reviewed changes
...c/application/datavisualizer/file_based/components/explanation_flyout/explanation_flyout.tsx Outdated
* you may not use this file except in compliance with the Elastic License.
*/
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
Copy link
Copy Markdown
Contributor

@alvarezmelissa87 alvarezmelissa87 Apr 6, 2020

Choose a reason for hiding this comment

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

Nit - double copyright comments at the top of this file.

alvarezmelissa87
alvarezmelissa87 reviewed Apr 6, 2020
View reviewed changes
x-pack/plugins/ml/server/client/error_wrapper.ts

export function wrapError(error: any): CustomHttpResponseOptions<ResponseError> {
const boom = isBoom(error) ? error : boomify(error, { statusCode: error.status });
const statusCode = boom.output.statusCode;
Copy link
Copy Markdown
Contributor

@alvarezmelissa87 alvarezmelissa87 Apr 6, 2020

Choose a reason for hiding this comment

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

This is used for all the server routes - just would like to confirm it works as expected for those and won't have unexpected side effects. From what I can see it looks fine but just want to confirm this was kept in mind when it was updated. Thanks! 😄

Copy link
Copy Markdown
Member Author

@jgowdyelastic jgowdyelastic Apr 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It's good to question this! I forgot to add a comment about the change I had to make.
I'll update the description now but basically the only way to get additional information in the kibana error response is to add it in an attributes object.
The original code set the whole boom object as the body, this would be turned into a single string message.
From looking through customError in KibanaResponse and in my testing, it appears these two objects result in the same error message.

{
  body: boom,
  ...
}

and

{
  body: {
    message: boom,
 }
  ...
}

So i used the latter to be able to also add an optional attributes object to the body.
i couldn't use

{
  body: {
    ...boom,
 }
  ...
}

as the spread operator corrupted the boom object. Which was the cause of the failing unrelated tests on this PR originally.

alvarezmelissa87
alvarezmelissa87 approved these changes Apr 6, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@alvarezmelissa87 alvarezmelissa87 left a comment

Choose a reason for hiding this comment

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

Left a small comment but overall LGTM ⚡️

@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Apr 6, 2020

💚 Build Succeeded

History

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

@jgowdyelastic jgowdyelastic merged commit 64f27ca into elastic:master Apr 7, 2020
@jgowdyelastic jgowdyelastic deleted the show-better-find-file-structure-finder-information branch April 7, 2020 07:47
@jgowdyelastic jgowdyelastic mentioned this pull request Apr 7, 2020
Merged
jgowdyelastic added a commit to jgowdyelastic/kibana that referenced this pull request Apr 7, 2020
@jgowdyelastic @elasticmachine
[ML] Show better file structure finder explanations (elastic#62316)
6c6e812 
* [ML] Show better file structure finder explanations

* more typescript changes

* changing function format

* fixing some types

* fixing translation id

* fix boom error reporting

* changes based on review

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
jgowdyelastic added a commit that referenced this pull request Apr 7, 2020
@jgowdyelastic @elasticmachine
[ML] Show better file structure finder explanations (#62316) (#62735)
efc68c2 
* [ML] Show better file structure finder explanations

* more typescript changes

* changing function format

* fixing some types

* fixing translation id

* fix boom error reporting

* changes based on review

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Apr 7, 2020
@gmmorris
Merge branch 'master' into alerting/tls-disable
a328e16 
* master: (36 commits)
  [data.search.aggs] Remove service getters from agg types (elastic#61628)
  fixing APM internationalization (elastic#62757)
  fix: 🐛 correctly create error on no_matching_indices (elastic#61257)
  [Lens] Remove all legacy imports (elastic#62596)
  Add label for ace editor (elastic#62588)
  [ML] Show better file structure finder explanations (elastic#62316)
  Fix old pathes in eslintrc (elastic#62580)
  [Uptime] Improve Telemetry test (elastic#62428)
  [SIEM] Adds sort rules Cypress test (elastic#62700)
  [Uptime]Abstracted 'access:uptime-read' tag into a wrapper for… (elastic#62576)
  fixing bug (elastic#62577)
  [Maps] Allow updating requestType for ESGeoGridSource (elastic#62365)
  [Maps] do not show circle border when symbol size is zero (elastic#62644)
  [Maps] Always show current zoom level (elastic#62684)
  bc5 siem rules merge (elastic#62679)
  Revert "[Monitoring] Cluster state watch to Kibana alerting (elastic#61685)"
  Fix visual tests (elastic#62660)
  [Telemetry] update crypto packages (elastic#62469)
  [DOCS] Removed references to left (elastic#60807)
  [Maps] Move layers to np maps (elastic#61877)
  ...
@droberts195 droberts195 mentioned this pull request Apr 7, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alvarezmelissa87 alvarezmelissa87 alvarezmelissa87 approved these changes

@peteharverson peteharverson peteharverson approved these changes

@walterra walterra Awaiting requested review from walterra

@darnautov darnautov Awaiting requested review from darnautov

Assignees

@jgowdyelastic jgowdyelastic

Labels

Feature:File and Index Data Viz ML file and index data visualizer :ml release_note:enhancement review v7.8.0 v8.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@jgowdyelastic @elasticmachine @droberts195 @kibanamachine @alvarezmelissa87 @peteharverson