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.

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

[Discover] Improve shard error message formatting #161098

Merged
merged 20 commits into from
Jul 26, 2023

Conversation

jughosta
Copy link
Contributor

@jughosta jughosta commented Jul 3, 2023

Summary

This PR updates UI of shards error modal.

Updated design

Screenshot 2023-07-07 at 10 05 00

Jul-07-2023 10-20-13

Before

image 2

For testing, please follow instruction from #41649 and drop targetfield on Lens page.

Checklist

@jughosta jughosta added release_note:enhancement backport:skip This commit does not require backporting Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. labels Jul 3, 2023
@jughosta jughosta self-assigned this Jul 3, 2023
@jughosta jughosta marked this pull request as ready for review July 3, 2023 15:04
@jughosta jughosta requested review from a team as code owners July 3, 2023 15:04
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

LGTM 👍 thx for taking care of this. Tested locally and works as expected.

Bildschirmfoto 2023-07-04 um 08 32 01

2 observations, potential follow ups

shard errors in Discover are still toasts ... they don't storm, but like error messages we should inline them. Same with embeddables on a dashboard ... there they could in theory create a storm, when you have a lot embeddables with different shard errors. Also on a dashboard it's not clear which toast belongs to which embeddables in such cases ... something we should aim to improve rather earlier than later, give that we should also aim to surface CCS to the user (like timeouts) ... but for sure nothing for this PR... here's the issue:
#155216

Copy link
Contributor

@andreadelrio andreadelrio left a comment

Choose a reason for hiding this comment

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

@jughosta Great job! thanks for implementing so fast. LGTM. Just left a nit about the Show details button.

@andreadelrio
Copy link
Contributor

@sophiec20 can you please let us know what you think of this proposal?

@sophiec20
Copy link
Contributor

sophiec20 commented Jul 25, 2023

One small point of feedback - what size font is the page heading?... does it need to be so dominant?... it is the largest and boldest font on the entire page (including the base page) ..

Otherwise LGTM and thank you ..

@jughosta
Copy link
Contributor Author

Thanks, @sophiec20! Reduced the title size to 16px:

Screenshot 2023-07-26 at 13 20 31

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

I just tested it in Lens and is much better now.

I would personally have the Close button and the Copy response CTAs in a fixed position in the bottom, so even if you change tabs, you see this information in the exact same place.

image

But possibly this was the design (?)

@jughosta
Copy link
Contributor Author

@stratoula Good point! Updated:

Screenshot 2023-07-26 at 14 43 40

@stratoula
Copy link
Contributor

Awww much better Julia, thanx ❤️

@jughosta jughosta enabled auto-merge (squash) July 26, 2023 13:10
@jughosta jughosta merged commit 41e2363 into elastic:main Jul 26, 2023
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #23 / endpoint When on the Endpoint Policy Details Page when on Ingest Policy Edit Package Policy page should allow updates to policy items

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
data 458 462 +4

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
data 53.0KB 53.9KB +922.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
data 404.1KB 404.0KB -65.0B

History

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

cc @jughosta

ThomThomson pushed a commit to ThomThomson/kibana that referenced this pull request Aug 1, 2023
- Closes elastic#156645

## Summary

This PR updates UI of shards error modal.

### Updated design
<img width="500" alt="Screenshot 2023-07-07 at 10 05 00"
src="https://github.com/elastic/kibana/assets/1415710/a099a436-61fd-4522-b231-88a0d1179061">

![Jul-07-2023
10-20-13](https://github.com/elastic/kibana/assets/1415710/23069476-9aae-4c76-9e39-586a382dcf70)



### Before
<img width="500" alt="image 2"
src="https://github.com/elastic/kibana/assets/4016496/d3c067d9-2d18-4fcc-8db0-573defcb9b44">


For testing, please follow instruction from
elastic#41649 and drop `targetfield` on
Lens page.


### Checklist

- [x] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [x] This renders correctly on smaller devices using a responsive
layout. (You can test this [in your
browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))
- [x] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)

---------

Co-authored-by: Andrea Del Rio <[email protected]>
Co-authored-by: Stratoula Kalafateli <[email protected]>
@thekofimensah
Copy link

This is an aside, I've always thought that the 10 pop ups that appear simultaneously whenever I get a shard error is overwhelming and the need to close each pop up is similarly annoying. Is there any attempt to address this? Or has this been dealt with in the more recent versions?

@davismcphee
Copy link
Contributor

Hi @thekofimensah, I agree that seeing long lists of toast messages is really annoying. There's still work left to do, but we've been taking steps to address this issue in recent releases. These are some of the other improvements we've made already:

We're also currently working on overhauling how we handle and surface shard failures and cross cluster search errors, so users can expect more improvements in these areas soon. Hope this helps!

@thekofimensah
Copy link

thekofimensah commented Sep 21, 2023 via email

@jughosta jughosta deleted the 156645-update-shards-error-modal branch September 21, 2023 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:enhancement Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. v8.10.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve shard error message formatting
10 participants