Skip to content

Add error handling to expression runner#37968

Merged
flash1293 merged 4 commits into
elastic:masterfrom
flash1293:feature/expression-service-error-handling
Jun 5, 2019
Merged

Add error handling to expression runner#37968
flash1293 merged 4 commits into
elastic:masterfrom
flash1293:feature/expression-service-error-handling

Conversation

@flash1293
Copy link
Copy Markdown
Contributor

This PR adds additional error handling capabilities to the expressions service of the data plugin.

It introduces a new option onRenderFailure which gets called if a render target element is passed in but the result of the expression can't be rendered because it isn't of type render or because the desired renderer is not registered.

There are two reasons for having such an API instead of just shifting this responsibility to the consumer who can inspect the result of the run themselves:

  • It requires knowledge about expression internals (type, registries and stuff) which should not be necessary to run and render them. onRenderFailure is a more friendly semantic level for high level usage.
  • It integrates nice with React which would make it difficult the get the result of the interpreter run

Another way of building this could be throwing exceptions if the rendering doesn't work. This could be used in React by adding an error boundary. However I feel that an error callback feels more consistent in our code base and makes it easier to ignore errors if you are not interested in them.

@flash1293 flash1293 added non-issue Indicates to automation that a pull request should not appear in the release notes v8.0.0 :AppArch v7.3.0 labels Jun 4, 2019
@flash1293 flash1293 requested review from lukeelmers and ppisljar June 4, 2019 10:13
@elasticmachine
Copy link
Copy Markdown
Contributor

Pinging @elastic/kibana-app-arch

@elasticmachine
Copy link
Copy Markdown
Contributor

💚 Build Succeeded

@ppisljar
Copy link
Copy Markdown
Contributor

ppisljar commented Jun 4, 2019

seems we are mixing promises and callbacks ? to me it feels that if i have a promise, it will either resolve if everything goes fine or reject if it fails and i wouldn't be expecting i need to pass callbacks in. Also, what happens on error when i passed the callback in ? my promise will still resolve, so i will need to have special logic in my code (inside .then) to handle this correctly.

if i understand correctly main reason to go with this approach is to make it easier consumable from react ? we have the renderer react component which is supposed to do that, so probably we should make that one consume the rejected promises and the end consumer doesn't need to worry about this.

@flash1293
Copy link
Copy Markdown
Contributor Author

flash1293 commented Jun 4, 2019

Thanks @ppisljar, that's way better. I updated the PR.

On thing I'm uncertain about: Now the runner only rejects the promise if it also tries to render but this doesn't work, even when the result is an error. Calling the runner without an element to render to always resolves, even if the result of the interpreter run is of type: 'error'. Should the runner always reject in this case?

Or in other words should does this test https://github.com/elastic/kibana/pull/37968/files#diff-d1401db958bbe72cabce0674ffadc926R133 document the correct behavior?

@ppisljar
Copy link
Copy Markdown
Contributor

ppisljar commented Jun 4, 2019

yes, lets for now reject in that case.

@elasticmachine
Copy link
Copy Markdown
Contributor

💚 Build Succeeded

@flash1293
Copy link
Copy Markdown
Contributor Author

@ppisljar @lukeelmers Errors are now rejected even if the runner is not rendering

@flash1293
Copy link
Copy Markdown
Contributor Author

Jenkins, test this.

@elasticmachine
Copy link
Copy Markdown
Contributor

💚 Build Succeeded

Copy link
Copy Markdown
Contributor

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

code LGTM

@flash1293 flash1293 merged commit 4c80466 into elastic:master Jun 5, 2019
@flash1293 flash1293 deleted the feature/expression-service-error-handling branch June 5, 2019 08:31
flash1293 added a commit to flash1293/kibana that referenced this pull request Jun 5, 2019
patrykkopycinski pushed a commit to patrykkopycinski/kibana that referenced this pull request May 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

non-issue Indicates to automation that a pull request should not appear in the release notes v7.3.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants