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

Update incomplete data messaging #169578

Merged
merged 55 commits into from
Oct 27, 2023
Merged

Update incomplete data messaging #169578

merged 55 commits into from
Oct 27, 2023

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Oct 23, 2023

Closes #167906

PR breaks monolith component <SearchResponseWarnings/> into 3 separate components: <SearchResponseWarningsBadge/>, <SearchResponseWarningsCallout/>, and <SearchResponseWarningsEmptyPrompt/>. These components are designed to display a single messages when provided warnings from multiple requests and display better messaging around partial results. PR also removes message from SearchResponseWarning type.

Collaborated with @gchaps on copy.

Test setup

  1. install sample web logs data set
  2. install sample flights data set
  3. Create data view.
    1. Set Index pattern to kibana_sample_data*
    2. Set Time field to timestamp
  4. Open discover
  5. Select kibana_sample_data* data view
  6. set time range to last 24 hours
  7. Add filter
    {
      "error_query": {
        "indices": [
          {
            "error_type": "exception",
            "message": "shard failure message 123",
            "name": "kibana_sample_data_logs",
            "shard_ids": [
              0
            ]
          }
        ]
      }
    }
    
  1. save search as kibana_sample_data*

Search response warnings callout

  1. Open saved search created in test setup
    Screenshot 2023-10-24 at 8 49 19 AM
  2. Click "expand" icon on left of first row in documents table
  3. Click "Surrounding documents"
  4. Re-enable "kibana_sample_data_logs failure" filter
    Screenshot 2023-10-24 at 8 51 22 AM

Search response warnings empty prompt

  1. Open saved search created in test setup
  2. Add filter DistanceKilometers is -1
    Screenshot 2023-10-24 at 8 44 13 AM

Search response warnings badge

  1. create new dashboard
  2. add saved search created during test setup
    Screenshot 2023-10-26 at 9 15 21 AM

Search response warnings toast

  1. create new table aggregation visualization
  2. Use saved search created during test setup as source
    Screenshot 2023-10-24 at 2 59 41 PM

@andreadelrio
Copy link
Contributor

Looking good! Here's a suggestion to match the other empty prompts we show in Discover. Can you change the type of the button to use EuiButton instead?

Current
Screenshot 2023-10-24 at 8 44 13 AM

Suggested
image

@nreese can you let me know how this popover works? Could we add any more metadata to let the user know the difference between the options? (e.g. predecessor #1, predecessor #2, etc.)

image

@nreese
Copy link
Contributor Author

nreese commented Oct 26, 2023

@nreese can you let me know how this popover works? Could we add any more metadata to let the user know the difference between the options? (e.g. predecessor #1, predecessor #2, etc.)

The popover uses the request name as the button link. I can update to include a number if the names are duplicate

@nreese
Copy link
Contributor Author

nreese commented Oct 26, 2023

@elasticmachine merge upstream

@nreese
Copy link
Contributor Author

nreese commented Oct 26, 2023

@andreadelrio

Here's a suggestion to match the other empty prompts we show in Discover. Can you change the type of the button to use EuiButton instead?

Empty prompt now displays an EuiButton

@nreese can you let me know how this popover works? Could we add any more metadata to let the user know the difference between the options? (e.g. predecessor #1, predecessor #2, etc.)

Duplicate request names are now suffixed with a count to avoid duplicate text in UI.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
data 476 479 +3
discover 687 690 +3
lens 1133 1136 +3
visualizations 362 365 +3
total +12

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/search-response-warnings 7 12 +5

Async chunks

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

id before after diff
data 47.4KB 46.8KB -552.0B
discover 585.7KB 585.6KB -113.0B
lens 1.4MB 1.4MB +985.0B
visualizations 268.3KB 267.9KB -432.0B
total -112.0B

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
@kbn/search-response-warnings 2 1 -1

Page load bundle

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

id before after diff
data 410.1KB 411.1KB +1.0KB
Unknown metric groups

async chunk count

id before after diff
data 4 3 -1
discover 20 19 -1
lens 23 22 -1
visualizations 15 14 -1
total -4

ESLint disabled line counts

id before after diff
@kbn/search-response-warnings 1 0 -1

Total ESLint disabled count

id before after diff
@kbn/search-response-warnings 1 0 -1

History

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

Copy link
Contributor

@jughosta jughosta left a comment

Choose a reason for hiding this comment

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

LGTM 👍
Thanks!

@andreadelrio
Copy link
Contributor

Duplicate request names are now suffixed with a count to avoid duplicate text in UI.

Thanks for the changes. When I look at what you've implemented it seems like a count (3 predecessors, 4 predecessors and so on), not a number representing an order. Is that the intent? Could we instead do 1. predecessor, 2. predecessor, 3. predecessor, ... , 7. anchor, ... through 13. sucessor? I'm trying to think of what will be of most help to the user when they look at this list.

image image

@nreese
Copy link
Contributor Author

nreese commented Oct 27, 2023

Could we instead do 1. predecessor, 2. predecessor, 3. predecessor, ... , 7. anchor, ... through 13. sucessor? I'm trying to think of what will be of most help to the user when they look at this list.

The problem is that the component does not have any context about the requests (like order). Request creators need to provide meaningful names because they have required context. In this case, that would be up to Discover to provide sequential naming, or better yet - timestamped names. I would consider this outside the scope of this PR and a fix that @elastic/kibana-data-discovery team could make later.

@andreadelrio
Copy link
Contributor

Could we instead do 1. predecessor, 2. predecessor, 3. predecessor, ... , 7. anchor, ... through 13. sucessor? I'm trying to think of what will be of most help to the user when they look at this list.

The problem is that the component does not have any context about the requests (like order). Request creators need to provide meaningful names because they have required context. In this case, that would be up to Discover to provide sequential naming, or better yet - timestamped names. I would consider this outside the scope of this PR and a fix that @elastic/kibana-data-discovery team could make later.

@kertal or @davismcphee can we please track this need somewhere? I'll approve now but want to make sure this doesn't get lost.

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.

Design changes LGTM. Hoping for follow-up to improve the View details popover in Surrounding Documents cc @elastic/kibana-data-discovery

Copy link
Contributor

@drewdaemon drewdaemon left a comment

Choose a reason for hiding this comment

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

Works for me! Thanks! (code review for visualizations code)

@nreese nreese merged commit 4b27288 into elastic:main Oct 27, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Oct 27, 2023
@davismcphee
Copy link
Contributor

@andreadelrio @nreese created an issue here: #170089.

davismcphee added a commit that referenced this pull request Nov 16, 2023
## Summary

Looks like a stray `0` snuck into Surrounding Documents as part of
#169578. This PR removes it:
<img width="447" alt="surrounding_docs_0"
src="https://github.com/elastic/kibana/assets/25592674/0eb9d88d-36b9-44df-a193-ebd6b2cf2468">

### Checklist

Delete any items that are not applicable to this PR.

- [ ] 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)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [ ] [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
- [ ] Any UI touched in this PR is usable by keyboard only (learn more
about [keyboard accessibility](https://webaim.org/techniques/keyboard/))
- [ ] Any UI touched in this PR does not create any new axe failures
(run axe in browser:
[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),
[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))
- [ ] If a plugin configuration key changed, check if it needs to be
allowlisted in the cloud and added to the [docker
list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)
- [ ] 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))
- [ ] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)

### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
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. Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.12.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[search source] update warning messaging to include more details about clusters
9 participants