-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ML] Show better file structure finder explanations #62316
[ML] Show better file structure finder explanations #62316
Conversation
Pinging @elastic/ml-ui (:ml) |
c03ffff
to
09c4d8e
Compare
@elasticmachine merge upstream |
@elasticmachine merge upstream |
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. |
...c/application/datavisualizer/file_based/components/explanation_flyout/explanation_flyout.tsx
Outdated
Show resolved
Hide resolved
...ic/application/datavisualizer/file_based/components/import_view/importer/message_importer.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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.
* 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 |
There was a problem hiding this comment.
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.
@@ -9,9 +9,13 @@ import { ResponseError, CustomHttpResponseOptions } from 'kibana/server'; | |||
|
|||
export function wrapError(error: any): CustomHttpResponseOptions<ResponseError> { | |||
const boom = isBoom(error) ? error : boomify(error, { statusCode: error.status }); | |||
const statusCode = boom.output.statusCode; |
There was a problem hiding this comment.
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! 😄
There was a problem hiding this comment.
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.
There was a problem hiding this 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 ⚡️
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
* [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 <[email protected]>
* [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 <[email protected]> Co-authored-by: Elastic Machine <[email protected]>
* 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) ...
The
find_file_structure
endpoint is now always called with theexplain
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:
They are also displayed when the analysis has failed:
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 optionalattributes
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