Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Ingest Pipelines] Error messages #70167

Merged
merged 12 commits into from
Jul 2, 2020
Merged

[Ingest Pipelines] Error messages #70167

merged 12 commits into from
Jul 2, 2020

Conversation

jloleysens
Copy link
Contributor

@jloleysens jloleysens commented Jun 29, 2020 •
edited
Loading

Summary

Compresses error feedback when attempting to create or update a pipeline.

Currently pipeline errors are reported as one continuous string (see screenshot). This PR adds code for parsing and displaying errors in a more user friendly way.

How to test

  1. Start Kibana on a basic license
  2. Go the ingest pipelines app in the management section
  3. Create a new pipeline, adding a processor with an empty {} for config (so you know it is invalid).
  4. Duplicate the above processor a 6 times
  5. Create the pipeline and the error messages should be present at the top of the creation form along with a toast of the error.

Additional notes

Replicates the pattern in Snapshot & Restore of collapsible indices list

Screenshots

Current errors

Screenshot 2020-06-18 at 11 59 00

After changes

Screenshot 2020-06-30 at 11 53 33

Screenshot 2020-06-30 at 11 53 53

@jloleysens jloleysens requested a review from a team as a code owner June 29, 2020 07:49
@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@jloleysens jloleysens added Feature:Ingest Node Pipelines Ingest node pipelines management release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.9.0 v8.0.0 labels Jun 29, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/es-ui (Team:Elasticsearch UI)

@cjcenizal
Copy link
Contributor

@jloleysens Could you add a screenshot of how the error looks with these changes?

@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@jloleysens
Copy link
Contributor Author

@cjcenizal Made some minor adjustments to copy and layout and updated the PR description with screenshots.

@mdefazio
Copy link
Contributor

This is much better! Does the 'Show 3 errors' need to read 'Show 3 more errors'? or '+ 3 more'?

Can we limit the number shown by default (before hiding them) down to 4 or 5 or so?

@jloleysens
Copy link
Contributor Author

@mdefazio I took the copy from the collapsible list we have in snapshot & restore which does not have the "more" and "less" in it. We can ask someone from docs to also weigh in.

Currently the number of errors is limited to 5, we can go down to 4 if you think that would look better?

@mdefazio
Copy link
Contributor

@mdefazio I took the copy from the collapsible list we have in snapshot & restore which does not have the "more" and "less" in it. We can ask someone from docs to also weigh in.

Currently the number of errors is limited to 5, we can go down to 4 if you think that would look better?

Ah sorry, I mixed up the screenshots trying to go too fast. 5 is good. :D

My initial reaction was that it wasn't clear enough that hitting the link would show 3 additional errors. If there are 5 shown, and 5 hidden, maybe this would be confusing. I think it's good we stay consistent, but might be worth having someone from docs weigh in.

alisonelizabeth
alisonelizabeth reviewed Jun 30, 2020
View reviewed changes
Copy link
Contributor

@alisonelizabeth alisonelizabeth left a comment

Choose a reason for hiding this comment

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

This makes the errors much easier to parse. Nice work @jloleysens!

I agree with @mdefazio regarding the "more/less" copy - I think it's worth reaching out to the docs team to see their thoughts. I also wonder if it would be more readable if the list items were formatted something like: "bytes processor: [field] required property is missing". WDYT?

Also, is it possible to add some tests for this change?

...plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_form_error.tsx Outdated Show resolved Hide resolved
...plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_form_error.tsx Outdated Show resolved Hide resolved
@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

elasticmachine and others added 3 commits July 1, 2020 07:48
@elasticmachine
Merge branch 'master' into ingest-pipelines/error-messages
3176d9b
@jloleysens
Refactor i18n texts and change wording
ba2a04b 
Also added missing translation and refactored maximum errors in
collapsed state to external constant
@jloleysens
use io-ts, add CIT and unit tests
7861f0b
@jloleysens
Copy link
Contributor Author

@alisonelizabeth @mdefazio thanks both for the feedback.

I believe I have addressed the feedback points provided. Would you please take another look?

alisonelizabeth
alisonelizabeth approved these changes Jul 1, 2020
View reviewed changes
Copy link
Contributor

@alisonelizabeth alisonelizabeth left a comment

Choose a reason for hiding this comment

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

Latest changes LGTM. Thanks for adding tests!

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
ingestPipelines 245 +5 240

History

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

@jloleysens jloleysens merged commit 83beede into elastic:master Jul 2, 2020
@jloleysens jloleysens deleted the ingest-pipelines/error-messages branch July 2, 2020 09:02
@jloleysens jloleysens mentioned this pull request Jul 2, 2020
Merged
jloleysens added a commit to jloleysens/kibana that referenced this pull request Jul 2, 2020
@jloleysens @elasticmachine
[Ingest Pipelines] Error messages (elastic#70167)
385ed6d 
* improved error messages

* traverse recursive error struct

* add check for object with keys

* update button position and copy

* size adjustments

* Refactor i18n texts and change wording

Also added missing translation and refactored maximum errors in
collapsed state to external constant

* use io-ts, add CIT and unit tests

* refactor error utilities to separate file

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jul 2, 2020
@gmmorris
Merge branch 'master' into alerting/built-in-alerts-feature
0e001e6 
* master: (46 commits)
  [Visualize] Add missing advanced settings and custom label for pipeline aggs (elastic#69688)
  Use dynamic: false for config saved object mappings (elastic#70436)
  [Ingest Pipelines] Error messages (elastic#70167)
  [APM] Show transaction rate per minute on Observability Overview page (elastic#70336)
  Filter out error when calculating a label (elastic#69934)
  [Visualizations] Each visType returns its supported triggers (elastic#70177)
  [Telemetry] Report data shippers (elastic#64935)
  Reduce SavedObjects mappings for Application Usage (elastic#70475)
  [Lens] fix dimension label performance issues (elastic#69978)
  Skip failing endgame tests (elastic#70548)
  [SIEM] Reenabling Cypress tests (elastic#70397)
  [SIEM][Security Solution][Endpoint] Endpoint Artifact Manifest Management + Artifact Download and Distribution (elastic#67707)
  [Security] Adds field mapping support to rule creation (elastic#70288)
  SECURITY-ENDPOINT: add fields for events to metadata document (elastic#70491)
  Fixed assertion in hybrid index pattern test to iterate through indices (elastic#70130)
  [SIEM][Exceptions] - Exception builder component (elastic#67013)
  [Ingest Manager] Rename data sources to package configs (elastic#70259)
  skip suites blocking es snapshot promomotion (elastic#70532)
  [Metrics UI] Fix asynchronicity and error handling in Snapshot API (elastic#70503)
  fix export response (elastic#70473)
  ...
jloleysens added a commit that referenced this pull request Jul 2, 2020
@jloleysens @elasticmachine
[Ingest Pipelines] Error messages (#70167) (#70567)
561fccc 
* improved error messages

* traverse recursive error struct

* add check for object with keys

* update button position and copy

* size adjustments

* Refactor i18n texts and change wording

Also added missing translation and refactored maximum errors in
collapsed state to external constant

* use io-ts, add CIT and unit tests

* refactor error utilities to separate file

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

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@alisonelizabeth alisonelizabeth mentioned this pull request Jul 17, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alisonelizabeth alisonelizabeth alisonelizabeth approved these changes

@mdefazio mdefazio Awaiting requested review from mdefazio

Assignees
No one assigned
Labels
Feature:Ingest Node Pipelines Ingest node pipelines management release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.9.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jloleysens @elasticmachine @cjcenizal @mdefazio @kibanamachine @alisonelizabeth