[search source] remove is_partial check by nreese · Pull Request #164506 · elastic/kibana

nreese · 2023-08-22T19:59:34Z

Background

"is partial" has 2 meanings

Results are incomplete because search is still running
Search is finished. Results are incomplete because there are shard failures (either in local or remote clusters)

is_running: Whether the search is still being executed or it has completed
is_partial: When the query is no longer running, indicates whether the search failed or was successfully completed on all shards. While the query is being executed, is_partial is always set to true
note: there is a bug in async search where requests to only local clusters return is_partial:false when there are shard errors on the local cluster. See async search returns is_partial:false when local cluster has shard errors elasticsearch#98725. This should be resolved in 8.11

Kibana's existing search implementation does not align with Elasticsearch's is_running and is_partial flags. Kibana defines "is partial" as definition "1)". Elasticsearch async search defines "is partial" as definition "2)".

This PR aligns Kibana's usage of "is partial" with Elasticsearch's definition. This required the following changes

isErrorResponse renamed to isAbortedResponse. Method no longer returns true when !response.isRunning && !!response.isPartial. Kibana handles results with incomplete data. Note removed export of isErrorResponse from data plugin since its use outside of data plugin does not make sense.
Replace isPartialResponse with isRunningResponse. This aligns Kibana's definition with Elasticsearch async search flags.
Remove isCompleteResponse. The word "complete" is ambiguous. Does it mean the search is finished (no longer running)? Or does it mean the search has all results and there are no shard failures?

…irectory

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

nreese · 2023-08-23T15:34:24Z

@elasticmachine merge upstream

nreese · 2023-08-23T19:16:17Z

@elasticmachine merge upstream

nreese · 2023-08-25T14:34:56Z

@elasticmachine merge upstream

`data.search` uses [poll_search](https://github.com/elastic/kibana/blob/main/src/plugins/data/common/search/poll_search.ts) to fetch data. `poll_search` checks for `isErrorResponse` and throws when`isErrorResponse` is true. **Note** `searchSource.search` uses `data.search` to perform searches. ```javascript return from(search()).pipe( expand(() => { const elapsedTime = Date.now() - startTime; return timer(getPollInterval(elapsedTime)).pipe(switchMap(search)); }), tap((response) => { if (isErrorResponse(response)) { throw response ? new Error('Received partial response') : new AbortError(); } }), takeWhile<Response>(isPartialResponse, true), takeUntil<Response>(aborted$) ); ``` Subscribers to `data.search` and `searchSource.search` do not need to check for `isErrorResponse` in `next`. `poll_search` has already thrown then the response is `isErrorResponse` so these checks are dead code blocks that are never executed. This PR removes these dead code blocks. Breaking this change out of #164506 so that it can be reviewed in isolated. --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>

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

…r partial results

elasticmachine · 2023-09-26T15:21:15Z

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

sphilipse

ent-search changes LGTM

elasticmachine · 2023-09-27T10:46:55Z

Pinging @elastic/uptime (Team:uptime)

nreese · 2023-09-27T12:24:26Z

@elasticmachine merge upstream

darnautov

AIOps changes LGTM

lukasolson

Added a couple of minor comments below, overall LGTM!

stratoula

The Visualizations team review was asked due to co-ownership with the discovery team. Lukas approval will be enough to allow merging as all changes are search related and the discovery team owns them 👍

PhilippeOberti

LGTM for the Protections Experience and the Threat Hunting Investigations teams!

nreese · 2023-10-02T14:32:13Z

@elasticmachine merge upstream

nreese · 2023-10-02T16:21:20Z

@elasticmachine merge upstream

kibana-ci · 2023-10-02T19:05:02Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: c000d44

Failed CI Steps

Test Failures

[job] [logs] Defend Workflows Cypress Tests #4 / Artifact pages Trusted applications should update Endpoint Policy on Endpoint when adding Trusted application name should update Endpoint Policy on Endpoint when adding Trusted application name
[job] [logs] FTR Configs #53 / copy to space with security copy to spaces user with no access from the default space single-namespace types "before each" hook for "should return 403 when copying to space with conflicts when overwriting"
[job] [logs] Security Solution Cypress Tests #9 / Ransomware Detection Alerts Ransomware in Timelines Renders ransomware entries in timelines table Renders ransomware entries in timelines table

Metrics [docs]

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
`data`	2553	2549	-4

Async chunks

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

id	before	after	diff
`aiops`	595.8KB	595.8KB	-3.0B
`securitySolution`	13.0MB	13.0MB	-93.0B
`synthetics`	861.1KB	861.1KB	-33.0B
`threatIntelligence`	56.5KB	56.5KB	-1.0B
`ux`	166.0KB	166.0KB	-1.0B
total			-131.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`	407.9KB	407.4KB	-510.0B
`observabilityShared`	47.0KB	47.0KB	-33.0B
total			-543.0B

Unknown metric groups

API count

id	before	after	diff
`data`	3288	3282	-6

History

💔 Build #164611 failed 425c6ad
💔 Build #164559 failed 3b3ab03
💛 Build #164215 was flaky 0102f27
💔 Build #164211 failed 9f3f920
💛 Build #164026 was flaky 66296c8

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

nreese and others added 7 commits August 22, 2023 13:25

replace isPartialResponse with isRunningResponse

c8b1654

replace isCompleteResponse with isRunningResponse in src/plugins

c32a659

replace isCompleteResponse with isRunningResponse in examples directory

cf73a0b

replace isCompleteResponse with isRunningResponse in x-pack/plugins d…

66c52c1

…irectory

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

2126bcd

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

tslint

8625e38

do not throw Received partial response

d9e3c80

nreese mentioned this pull request Aug 23, 2023

remove unneeded usages of isErrorResponse #164609

Merged

nreese force-pushed the issue_163381 branch from e4801bc to d9e3c80 Compare August 23, 2023 19:16

kibanamachine and others added 3 commits August 23, 2023 15:16

Merge branch 'main' into issue_163381

36c75b4

fix poll_search test

e0f05a9

remove broken test case

066c32c

Merge branch 'main' into issue_163381

7b8262a

tylerperk added the release highlight label Aug 25, 2023

nreese mentioned this pull request Aug 25, 2023

search with shard errors from local cluster no longer shows results in kibana (after elasticsearch 98725 is resolved) #164893

Closed

nreese and others added 6 commits August 28, 2023 11:55

merge with main

95796c4

fix merge conflict

e4bdfba

fix another merge conflict mistake

efbb44b

remove unneeded uses of isErrorResponse

f5af0a2

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

4667979

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

tslint

8c00a67

nreese force-pushed the issue_163381 branch from d70e0d0 to 8c00a67 Compare August 28, 2023 19:09

nreese added 4 commits August 28, 2023 13:39

add missing import

7f6095f

rename isErrorResponse to isMalformedResponse

689f4f5

update test comments to reflect that execption is no longer thrown fo…

5859f4b

…r partial results

clean up useMemo dependencies

608b010

nreese added release_note:skip Skip the PR/issue when compiling release notes v8.11.0 Feature:Search Querying infrastructure in Kibana Team:DataDiscovery Discover, search (data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. t// labels Sep 26, 2023

sphilipse approved these changes Sep 27, 2023

View reviewed changes

botelastic Bot added the Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability label Sep 27, 2023

shahzad31 approved these changes Sep 27, 2023

View reviewed changes

Merge branch 'main' into issue_163381

a5c7ea0

darnautov approved these changes Sep 28, 2023

View reviewed changes

merge with main

66296c8

jcger approved these changes Sep 29, 2023

View reviewed changes

lukasolson approved these changes Sep 29, 2023

View reviewed changes

Comment thread src/plugins/data/public/search/search_interceptor/search_interceptor.test.ts

Comment thread src/plugins/data/public/search/search_interceptor/search_response_cache.test.ts

nreese added 3 commits September 29, 2023 17:12

restore search_response_cache test

0214eb2

restore search_interceptor test

9f3f920

tslint

0102f27

stratoula reviewed Oct 2, 2023

View reviewed changes

angorayc approved these changes Oct 2, 2023

View reviewed changes

PhilippeOberti approved these changes Oct 2, 2023

View reviewed changes

Merge branch 'main' into issue_163381

3b3ab03

Merge branch 'main' into issue_163381

425c6ad

patrykkopycinski added the ci:skip-cypress-osquery Skips osquery cypress checks label Oct 2, 2023

Merge branch 'main' into issue_163381

c000d44

nreese merged commit b5bcf69 into elastic:main Oct 2, 2023

kibanamachine added the backport:skip This PR does not require backporting label Oct 2, 2023

Conversation

nreese commented Aug 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Background

Uh oh!

nreese commented Aug 23, 2023

Uh oh!

nreese commented Aug 23, 2023

Uh oh!

nreese commented Aug 25, 2023

Uh oh!

elasticmachine commented Sep 26, 2023

Uh oh!

sphilipse left a comment

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Sep 27, 2023

Uh oh!

nreese commented Sep 27, 2023

Uh oh!

darnautov left a comment

Choose a reason for hiding this comment

Uh oh!

lukasolson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

stratoula left a comment

Choose a reason for hiding this comment

Uh oh!

PhilippeOberti left a comment

Choose a reason for hiding this comment

Uh oh!

nreese commented Oct 2, 2023

Uh oh!

nreese commented Oct 2, 2023

Uh oh!

kibana-ci commented Oct 2, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Public APIs missing comments

Async chunks

Page load bundle

API count

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

15 participants

nreese commented Aug 22, 2023 •

edited

Loading