refactor search source warnings to return a single warning for 'is_partial' results #165512

nreese · 2023-09-01T18:38:35Z

This PR replaces individual shard failure and timeout warnings with a single "incomplete data" warning. This work is required for #163381

Test instructions

Install flights and web logs sample data
Create data view kibana_sample_data*. Set time field to timestamp
open discover and select kibana_sample_data* data view

Add filter with custom DSL

{
  "error_query": {
    "indices": [
      {
        "error_type": "exception",
        "message": "local shard failure message 123",
        "name": "kibana_sample_data_logs",
        "shard_ids": [
          0
        ]
      }
    ]
  }
}

…-ref HEAD~1..HEAD --fix'

…fix'

…-ref HEAD~1..HEAD --fix'

…-fix'

…-ref HEAD~1..HEAD --fix'

nreese · 2023-09-05T20:49:56Z

@elasticmachine merge upstream

…-ref HEAD~1..HEAD --fix'

…-fix'

…-ref HEAD~1..HEAD --fix'

nreese · 2023-09-11T14:34:38Z

@elasticmachine merge upstream

nreese · 2023-09-12T15:34:45Z

@elasticmachine merge upstream

…-ref HEAD~1..HEAD --fix'

stratoula · 2023-09-13T05:46:48Z

x-pack/plugins/lens/public/datasources/form_based/utils.tsx

-          return Object.values(state.layers).flatMap((layer) =>
+    if (warning.type === 'incomplete') {
+      return hasUnsupportedDownsampledAggregationFailure(warning)
+        ? Object.values(state.layers).flatMap((layer) =>


@dej611 can you do a sanity check on the TSDB behavior?

There's a specific FTR test for this case. I see it passing, so all good by me 👍

stratoula

This looks good to me, I tested all editors and everything seems as expected. I just want a sanity check from Marco for downsampling warnings. He knows this area better than me

jughosta

LGTM 👍
Thanks!

MichaelMarcialis

Thanks, @nreese. This looks good to me. I left a few small comments below, but nothing worth holding you up over. Assuming you're able to review those comments, approving now so we don't hold you up further.

I noticed that the warning callout's dismiss button is using a primary (blue) colored icon. Would it be more fitting to use a warning (yellow) colored icon instead?
Is it necessary to show the same warning(s) both in a callout above the table and using the warning count and popover on the visualization embed? Would it make sense to suppress the warning count and popover from the visualization embed to avoid the warning redundancy (and simply rely on the warning callout above the table)? I believe the visualization embeds were developed with the ability to suppress warnings/errors.

MichaelMarcialis · 2023-09-13T16:43:46Z

src/plugins/data/public/incomplete_results_modal/_incomplete_results_modal.scss

+.shardFailureModal__desc {
+  // set for IE11, since without it depending on the content the width of the list
+  // could be much higher than the available screenspace
+  max-width: 686px;
+}


Do we still support IE11? If not, is this style necessary?

Is IE11 still supported? This css is just moving an existing css file so this is not being added, just changing locations.

It looks like IE11 is unsupported as of 7.9+ (leaving aside that this isn't new code): https://www.elastic.co/support/matrix.

MichaelMarcialis · 2023-09-13T16:51:40Z

src/plugins/data/public/incomplete_results_modal/open_incomplete_results_modal_button.tsx

    >
      <FormattedMessage
-        id="data.search.searchSource.fetch.shardsFailedModal.showDetails"
+        id="data.search.searchSource.fetch.incompleteResultsModal.showDetails"
        defaultMessage="Show details"


Would it be possible to change the Show details text to View details to match the verbiage we'll be using in the upcoming CCS updates?

If we're changing it to View details here, we should also update the error callout used in Discover too for consistency:

kibana/src/plugins/discover/public/components/common/error_callout.tsx

Lines 43 to 45 in 09faf89

const showErrorMessage = i18n.translate('discover.errorCalloutShowErrorMessage', {

defaultMessage: 'Show details',

});

MichaelMarcialis · 2023-09-13T17:18:37Z

src/plugins/data/public/search/fetch/extract_warnings.ts

-        defaultMessage: 'Data might be incomplete because your request timed out',
+      type: 'incomplete',
+      message: i18n.translate('data.search.searchSource.fetch.incompleteResultsMessage', {
+        defaultMessage: 'The data might be incomplete or wrong.',


I understand we're trying to make the messaging more general-purpose to account for multiple issues. But would it make sense to reference shards and partial data within the warning somehow? Perhaps something like: Index shards returned partial data. Information displayed here may be incomplete or incorrect.?

At this level, the messaging should be high level. It would not be practical to explain why there is incomplete data in this message since there are some many different reasons for incomplete data and such a small space. More knowledgable users (or curious users) can explore the "detailed why" by clicking "view details". In the future, this will open #166021. This is give the user all of the context of why data is incomplete with screen real estate to clearly explain what is missing.

nreese · 2023-09-13T18:12:28Z

I noticed that the warning callout's dismiss button is using a primary (blue) colored icon. Would it be more fitting to use a warning (yellow) colored icon instead?

Resolved with 8f4f8cf

Is it necessary to show the same warning(s) both in a callout above the table and using the warning count and popover on the visualization embed? Would it make sense to suppress the warning count and popover from the visualization embed to avoid the warning redundancy (and simply rely on the warning callout above the table)? I believe the visualization embeds were developed with the ability to suppress warnings/errors.

This is existing behavior and is not impacted by this PR. I will leave it up to the @elastic/kibana-data-discovery team to make this change in the future.

davismcphee

No code review, just responding to some feedback.

Is it necessary to show the same warning(s) both in a callout above the table and using the warning count and popover on the visualization embed? Would it make sense to suppress the warning count and popover from the visualization embed to avoid the warning redundancy (and simply rely on the warning callout above the table)? I believe the visualization embeds were developed with the ability to suppress warnings/errors.

This is existing behavior and is not impacted by this PR. I will leave it up to the @elastic/kibana-data-discovery team to make this change in the future.

These warnings are for different requests, since the vis request is separate from the data grid request. In some cases one may return a warning while the other succeeds, and you would only see one warning in the UI in these cases.

It may make sense from a UX perspective to capture the vis warnings and somehow merge them with the data grid warnings, but currently the warnings are shown where they occurred and I would consider any attempt at merging them as a separate effort from this work.

davismcphee · 2023-09-13T20:11:29Z

src/plugins/data/public/incomplete_results_modal/_incomplete_results_modal.scss

+.shardFailureModal__desc {
+  // set for IE11, since without it depending on the content the width of the list
+  // could be much higher than the available screenspace
+  max-width: 686px;
+}


It looks like IE11 is unsupported as of 7.9+ (leaving aside that this isn't new code): https://www.elastic.co/support/matrix.

davismcphee · 2023-09-13T20:16:15Z

src/plugins/data/public/incomplete_results_modal/open_incomplete_results_modal_button.tsx

    >
      <FormattedMessage
-        id="data.search.searchSource.fetch.shardsFailedModal.showDetails"
+        id="data.search.searchSource.fetch.incompleteResultsModal.showDetails"
        defaultMessage="Show details"


If we're changing it to View details here, we should also update the error callout used in Discover too for consistency:

kibana/src/plugins/discover/public/components/common/error_callout.tsx

Lines 43 to 45 in 09faf89

const showErrorMessage = i18n.translate('discover.errorCalloutShowErrorMessage', {

defaultMessage: 'Show details',

});

stratoula

Visualizations team changes LGTM!

kibana-ci · 2023-09-14T14:09:13Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 8888e3f

Failed CI Steps

FTR Configs #40

Test Failures

[job] [logs] FTR Configs #40 / Search examples handling warnings with search source fetch should show search warnings in results tab

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`discover`	650	651	+1
`lens`	1157	1162	+5
`visualizations`	340	345	+5
total			+11

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/es-types`	12	19	+7
`data`	2584	2577	-7
total			-0

Async chunks

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

id	before	after	diff
`data`	54.1KB	54.3KB	+243.0B
`discover`	556.2KB	555.5KB	-726.0B
`lens`	1.4MB	1.4MB	-249.0B
`unifiedHistogram`	48.6KB	48.6KB	-6.0B
`visualizations`	265.2KB	265.5KB	+328.0B
total			-410.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
`data`	26	24	-2

Page load bundle

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

id	before	after	diff
`data`	408.4KB	407.5KB	-835.0B

Unknown metric groups

API count

id	before	after	diff
`@kbn/es-types`	12	19	+7
`@kbn/search-response-warnings`	16	15	-1
`data`	3311	3308	-3
total			+3

History

💚 Build #159006 succeeded 85c16f0
💚 Build #158873 succeeded 7f5bb92
💚 Build #158472 succeeded eb54f57
💔 Build #158410 failed cec504f
💔 Build #158017 failed 9cd9aac
💔 Build #157392 failed 0a4e19d

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

Found duplicated type ShardFailure in data plugin. Same type removed from `src/plugins/data/public/shard_failure_modal/shard_failure_types.ts` in #165512. This PR removes some unneeded types from `src/plugins/data/common/search/search_source/types.ts`. --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>

nreese and others added 9 commits September 5, 2023 08:36

extract incomplete warning and handle in toasts

d4164ab

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

d93aedb

…-ref HEAD~1..HEAD --fix'

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

2046549

…fix'

tslint

7d15531

another test case

74f7c88

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

e87efd7

…-ref HEAD~1..HEAD --fix'

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

3d3c439

…-fix'

fix discover

be16158

remove console.log

1385564

nreese force-pushed the incomplete_warning branch from 799d11a to 1385564 Compare September 5, 2023 14:41

tslint

47ca3f9

nreese force-pushed the incomplete_warning branch from d4a7466 to 47ca3f9 Compare September 5, 2023 14:53

kibanamachine and others added 6 commits September 5, 2023 15:27

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

353c528

…-fix'

tslint packages/kbn-search-response-warnings

1619d17

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

d3e1b37

…-ref HEAD~1..HEAD --fix'

update lens for incomplete warning

f9ca751

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

60452d8

…-ref HEAD~1..HEAD --fix'

tslint visualizations plugin

eb50b70

kibanamachine and others added 3 commits September 5, 2023 16:49

Merge branch 'main' into incomplete_warning

e75281d

i18n fixes

45868ce

round 1 of tslint discover

ee013d6

nreese force-pushed the incomplete_warning branch from 42bb386 to ee013d6 Compare September 5, 2023 21:56

tslint discover

243464b

nreese force-pushed the incomplete_warning branch from a2d7804 to 243464b Compare September 5, 2023 23:16

kibanamachine and others added 5 commits September 5, 2023 23:21

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

c3ce686

…-ref HEAD~1..HEAD --fix'

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

95dae5e

…-fix'

tslint

a69611b

fix jest tests

201681f

display shard failures

10765c4

add jest test for IncompleteResultsModal component

b724b92

nreese requested a review from jughosta September 7, 2023 17:45

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

0a4e19d

…-ref HEAD~1..HEAD --fix'

pgayvallet approved these changes Sep 11, 2023

View reviewed changes

Merge branch 'main' into incomplete_warning

9cd9aac

kibanamachine and others added 3 commits September 12, 2023 11:34

Merge branch 'main' into incomplete_warning

cec504f

fix search source example functional tests

6939dd5

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

eb54f57

…-ref HEAD~1..HEAD --fix'

sorenlouv approved these changes Sep 12, 2023

View reviewed changes

stratoula reviewed Sep 13, 2023

View reviewed changes

jughosta approved these changes Sep 13, 2023

View reviewed changes

MichaelMarcialis approved these changes Sep 13, 2023

View reviewed changes

nreese added 2 commits September 13, 2023 12:09

change callout close button color to warning to match callout

8f4f8cf

rename 'Show details' to 'View details'

7f5bb92

davismcphee reviewed Sep 13, 2023

View reviewed changes

Merge branch 'main' into incomplete_warning

85c16f0

stratoula approved these changes Sep 14, 2023

View reviewed changes

nreese added 2 commits September 14, 2023 06:58

remove IE11 CSS rule

2df8a8a

update text to 'View details'

8888e3f

nreese merged commit f3b280f into elastic:main Sep 14, 2023
21 checks passed

kibanamachine added the backport:skip This commit does not require backporting label Sep 14, 2023

nreese mentioned this pull request Sep 19, 2023

remove duplicative types from search_source types #166760

Merged

davismcphee mentioned this pull request Sep 20, 2023

[Discover] Improve shard error message formatting #161098

Merged

4 tasks

nreese mentioned this pull request Jan 16, 2024

Show warning if _clusters.skipped is greater than 0. #118736

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor search source warnings to return a single warning for 'is_partial' results #165512

refactor search source warnings to return a single warning for 'is_partial' results #165512

nreese commented Sep 1, 2023 •

edited

nreese commented Sep 5, 2023

nreese commented Sep 11, 2023

nreese commented Sep 12, 2023

stratoula Sep 13, 2023 •

edited

dej611 Sep 14, 2023

stratoula left a comment

jughosta left a comment

MichaelMarcialis left a comment

MichaelMarcialis Sep 13, 2023

nreese Sep 13, 2023

davismcphee Sep 13, 2023

MichaelMarcialis Sep 13, 2023

davismcphee Sep 13, 2023

MichaelMarcialis Sep 13, 2023

nreese Sep 13, 2023 •

edited

nreese commented Sep 13, 2023

davismcphee left a comment

davismcphee Sep 13, 2023

davismcphee Sep 13, 2023

stratoula left a comment

kibana-ci commented Sep 14, 2023

API count

	const showErrorMessage = i18n.translate('discover.errorCalloutShowErrorMessage', {
	defaultMessage: 'Show details',
	});

refactor search source warnings to return a single warning for 'is_partial' results #165512

refactor search source warnings to return a single warning for 'is_partial' results #165512

Conversation

nreese commented Sep 1, 2023 • edited

Test instructions

nreese commented Sep 5, 2023

nreese commented Sep 11, 2023

nreese commented Sep 12, 2023

stratoula Sep 13, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stratoula left a comment

Choose a reason for hiding this comment

jughosta left a comment

Choose a reason for hiding this comment

MichaelMarcialis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nreese Sep 13, 2023 • edited

Choose a reason for hiding this comment

nreese commented Sep 13, 2023

davismcphee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stratoula left a comment

Choose a reason for hiding this comment

kibana-ci commented Sep 14, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Public APIs missing exports

Page load bundle

API count

History

nreese commented Sep 1, 2023 •

edited

stratoula Sep 13, 2023 •

edited

nreese Sep 13, 2023 •

edited