search

elastic / kibana

Your window into the Elastic Stack
https://www.elastic.co/products/kibana
Other
19.62k stars 8.22k forks source link

Unify error reporting across Stack Management UIs #104235

Open cjcenizal opened 3 years ago

cjcenizal commented 3 years ago

Problems

Incompatible SectionError component

Index Management has its own SectionError component. This component is incompatible with the ES UI Shared SectionError component, because they each expect different types of error to be provided to them. ES UI Shared expects the error object to have an error property, whereas Index Management expects it to have statusText and attributes properties.

// ES UI Shared
const {
  cause, // wrapEsError() on the server adds a "cause" array
  message,
  error: errorString,
} = error;

// Index Management
const {
  cause: causeRoot, // wrapEsError() on the server adds a "cause" array
  message,
  statusText,
  attributes: { cause: causeAttributes } = {},
} = error;

statusText: From what I can tell, the statusText property is never actually provided on the error object by the server, so we might be able to ignore that.

attributes: The attributes property is provided in three API route handlers, all within the api/templates/ directory:

Cost: Aside from DX confusion (making UI migrations difficult), this impedes us from improving supportability by enhancing SectionError error reporting with any features added on the ES side (e.g. domain-specific error codes).

Resolution: We need to resolve this incompatibility and then migrate from Index Management's SectionError to the ES UI Shared SectionError.

Incompatibilities with ErrorToast and addError

The Toasts module's addError method accepts a global Error argument. This requires weird hacks to fulfill the interface's expectations.

The Toasts module defines its own RequestError type, which seems like it should be defined as part of the elasticsearch-specification repo, perhaps as the ErrorResponseBase type. If that's not possible, this type should be exported and we should consume it in ES UI Shared and elsewhere in our plugins.

Cost: DX confusion makes it challenging to consistently implement error reporting and error handling in a straightforward manner. These incompatibilities also impeded us from improving supportability by enhancing addError error reporting with any features added on the ES side (e.g. domain-specific error codes).

Resolution: We need to resolve these incompatibilities by refactoring addError to narrowly define the types of errors it can accept. These types of errors should be published in a centralized location and consumed by the relevant plugins.

Related issues

elasticmachine commented 3 years ago

Pinging @elastic/kibana-stack-management (Team:Stack Management)

cjcenizal commented 3 years ago

CC @elastic/kibana-core

pgayvallet commented 3 years ago

Incompatibilities with ErrorToast and addError

We need to resolve these incompatibilities by refactoring addError to narrowly define the types of errors it can accept

We could, but the problem with TS error handling is that, by nature, you can't really be sure what your error's shape is, and you can't restraint a catch block to only specific error types, making any type of specialized-error-type-as-argument patterns potentially dangerous, as the caught error is any anyway, and would match anything.

So, even if we were to specialize the accepted input of toast.addError, it wouldn't be properly handled in most usages of the API:

type Toast = {
   addError: (error: SomeSpecificError | AnotherSpecificError) => ReturnType
}

try {
 explode()
} catch(e) {
  // TS is awesome, e is any.
  Toast.addError(e);
}

Also, this would introduce bad isolation, as core's showErrorDialog implementation would have to handle potentially consumer-specific error types, which we're trying to avoid.

So, if I agree that the hacks around the Toast handling of RequestError is an aberration, AFAIK this is a necessary evil to handle the most common case where the error caught was send by the server, and is 'fine' due to the fact that is a core handled(-ish) error.

If a consumer wants to render specific toast messages (and associated dialogs) for a specialized error type, it should be done via a custom component and using the toast.add({color: 'danger', text: myErrorMountPoint}) API instead.

mshustov commented 3 years ago

We could, but the problem with TS error handling is that, by nature, you can't really be sure what your error's shape is, and you can't restraint a catch block to only specific error types, making any type of specialized-error-type-as-argument patterns potentially dangerous, as the caught error is any anyway, and would match anything.

Starting from TS v4.4, the catch argument will be treated as unknown in strict mode. Considering that we track any usages count in the codebase, we might hope all the errors will be properly typed in the future.

Also, this would introduce bad isolation, as core's showErrorDialog implementation would have to handle potentially consumer-specific error types, which we're trying to avoid.

my overall 👍 However, I don't see anything bad if relax requirements for the input argument:

interface ErrorLike {
  message: string;
  stack?: string;
}

interface RequestError extends ErrorLike { ... }
tsullivan commented 3 years ago

I think this proposal should interest @elastic/kibana-app-services. The search services typically uses a "danger" toast rendered with a custom React component when showing an error. The way it is done uses a custom class that extends Error, and it is extremely hard to find the "important" part of the error. See https://github.com/elastic/kibana/issues/104466#issuecomment-875799693

elasticmachine commented 1 month ago

Pinging @elastic/kibana-management (Team:Kibana Management)