-
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] Improving client side error handling #76743
[ML] Improving client side error handling #76743
Conversation
@elasticmachine merge upstream |
Pinging @elastic/ml-ui (:ml) |
@elasticmachine merge upstream |
const { displayErrorToast } = toastNotificationServiceProvider(notifications.toasts); | ||
displayErrorToast( | ||
error, | ||
i18n.translate('xpack.ml.newJob.wizard.summaryStep.createJobError', { |
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.
Not sure we meant for the same i18n id here but if we are using the same block for both places I wonder if we should consolidate them in one function.
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.
these ids have always been the same, which surprises me, but i guess it doesn't complain if the ids are the same and the text is the same.
I'll remove the duplication.
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.
Removed in 9027b2a
// takes an Error object and and optional response object | ||
// if error is falsy (null) the response object will be used | ||
// notify will show the full expandable stack trace of the response if a response object is used and no error is passed in. | ||
|
||
constructor(error: any, resp: any) { |
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.
I wonder if we can be more explicit here with the error: any
since if error obj is required and if .message
is undefined, the error message shown to the user will be blank.
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.
Updated in 430dbdb
I've also broken the error processing file into multiple files and moved the MLRequestFailure
into common and changed it so it is no longer a subclass of KbnError
, as it didn't gain us anything and meant it had to live in public.
Gave this a quick test. I modified the names of the jobIds to test the delete operations and for some reasons it's showing success messages even though the jobs weren't deleted.
For starting invalid datafeed, I wonder if we should get rid of this |
This is the same behaviour as master. The problem is that the jobs aren't really deleted at the point that the request returns. Instead they are deleted in tasks that happen later. We only get an acknowledgement that the delete request has gone through. |
Changes LGTM 💯 |
@@ -83,12 +83,12 @@ export const useResultsViewConfig = (jobId: string) => { | |||
setIsLoadingJobConfig(false); | |||
} | |||
} catch (e) { | |||
setJobCapsServiceErrorMessage(getErrorMessage(e)); | |||
setJobCapsServiceErrorMessage(extractErrorMessage(e)); |
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.
Noticed while testing this section that errors never get into this block when the error occurs in initializeFromIndexPattern
since that function catches any error and logs it and does not propagate it. This means the results view table just fails silently and is empty. Maybe outside the scope of this PR but would be good to talk about whether the errors should be propagated somehow to code calling the service.
Once the error is propagated it does show up correctly so extractErrorMessage
appears to be extracting things correctly. 👍
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.
yes the errors probably should be propagated. initializeFromIndexPattern
is called a few places and it's only here that we're expecting to catch anything.
I agree that it's outside of the scope of this PR, but definitely worth revisiting.
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.
Gave this a local test and LGTM ⚡
Fixed in 8875cfa |
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.
LGTM
@elasticmachine merge upstream |
💚 Build SucceededBuild metrics@kbn/optimizer bundle module count
async chunks size
page load bundle size
distributable file count
History
To update your PR or re-run it, just comment with: |
* [ML] Improving client side error handling * adding stacktrace to request errors * copying error parsing to transforms * update snapshot * fixing jest tests * adding test and removing debug log output * updating translations * rewriting error extracting code * fixing tests * removing message bar service * removing test code * updating translations * combining job creation error handling * refactoring error files * updating test * fixing bug in DFA deletion * improving mml warning Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
* [ML] Improving client side error handling * adding stacktrace to request errors * copying error parsing to transforms * update snapshot * fixing jest tests * adding test and removing debug log output * updating translations * rewriting error extracting code * fixing tests * removing message bar service * removing test code * updating translations * combining job creation error handling * refactoring error files * updating test * fixing bug in DFA deletion * improving mml warning Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
…s-for-710 * 'master' of github.com:elastic/kibana: (65 commits) Separate url forwarding logic and legacy services (elastic#76892) Bump yargs-parser to v13.1.2+ (elastic#77009) [Ingest Manager] Shared Fleet agent policy action (elastic#76013) [Search] Re-add support for aborting when a connection is closed (elastic#76470) [Search] Remove long-running query pop-up (elastic#75385) [Monitoring] Fix UI error when alerting is not available (elastic#77179) do not log plugin id format warning in dist mode (elastic#77134) [ML] Improving client side error handling (elastic#76743) [Alerting][Connectors] Refactor IBM Resilient: Generic Implementation (phase one) (elastic#74357) [Docs] some basic searchsource api docs (elastic#77038) add cGroupOverrides to the legacy config (elastic#77180) Change saved object bulkUpdate to work across multiple namespaces (elastic#75478) [Security Solution][Resolver] Replace Selectable popover with badges (elastic#76997) Removing ml-state index from archive (elastic#77143) [Security Solution] Add unit tests for histograms (elastic#77081) [Lens] Filters aggregation (elastic#75635) [Enterprise Search] Update WS Overview logic to use new config data (elastic#77122) Cleanup type output before building new types (elastic#77211) [Security Solution] Use safe type in resolver backend (elastic#76969) Use proper lodash syntax (elastic#77105) ... # Conflicts: # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/node_allocation.tsx
Fixes error reporting on the client side introduced by #74965 which caused changes to the error objects returned from kibana.
extractErrorProperties
to extract the error properties from the new error formats.getErrorMessage
function for converting the error to a string. Copies it to the transforms plugin where it is still used and was originally being imported from the ML plugin.mlMessageBarService
, and replaces it with a similar functiongetToastNotificationService
which uses the dependency cache for gettingToastsStart
to retain backwards compatibility with all existingmlMessageBarService
uses.MLRequestFailure
. This allows for the correct error message to be displayed when clicking theSee full error
button in the toast.Previously it would show the client side JS stack, which was unrelated to the actual error.
Before:
After:
Removes
updateJob
,validateJob
andupdateDatafeed
from job service as they are unnecessary and caused two error toasts to appear.Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios
This was checked for breaking API changes and was labeled appropriately