[search source] return rawResponse on search failure by nreese · Pull Request #168389 · elastic/kibana

nreese · 2023-10-09T17:33:44Z

Problem

/bsearch and /search APIs only return error key from elasticsearch error response. This is problematic because Inspector needs rawResponse to populate "Clusters and shards"

While working on this issue, I discovered another problem with how error responses are added to inspector requestResponder. The Error instance is added as json key. This is a little awkward since the response tab just stringifies the contents of json, thus stringifing the Error object instead of just the error body returned from API. This PR address this problem by setting json to either attributes or { message }.

Solution

PR updates /bsearch and /search APIs to return { attributes: { error: ErrorCause, rawResponse }} for failed responses. Solution avoided changing KbnServerError and reportServerError since these methods are used extensivly throughout Kibana (see #167544 (comment) for more details). Instead, KbnSearchError and reportSearchError are created to report search error messages.

Test

install web logs sample data set
open discover

add filter

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

Open inspector. Verify "Clusters and shards" tab is visible and populated. Verify "Response" tab shows "error" and "rawResponse" keys.

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

…-fix'

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

nreese · 2023-10-10T15:24:35Z

@elasticmachine merge upstream

elasticmachine · 2023-10-10T18:12:23Z

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

nreese · 2023-10-11T17:06:01Z

@elasticmachine merge upstream

nreese · 2023-10-11T18:14:10Z

@elasticmachine merge upstream

nreese · 2023-10-11T19:21:00Z

@elasticmachine merge upstream

davismcphee

Code changes LGTM! Just left a couple of minor comments.

Otherwise my only concern is related to backward compatibility. Does changing the error response of the /search and /bsearch endpoints risk breaking backward compatibility in a Serverless upgrade scenario (i.e. backend and frontend versions off by 1)? This is less of a concern while Serverless is in private preview, but something to be mindful of regardless.

Also, unrelated to this PR, but I noticed the inspector uses X failured shard(s) in the title, which looks a little odd. Should it instead say X failed shard(s)?

davismcphee · 2023-10-12T18:25:51Z

src/plugins/data/server/search/report_search_error.ts

+  if (e instanceof KbnSearchError) return e;
+  return new KbnSearchError(
+    e.message ?? 'Unknown error',
+    e instanceof errors.ResponseError ? e.statusCode! : 500,


How do we know e.statusCode isn't null here?

davismcphee · 2023-10-12T18:30:03Z

src/plugins/data/server/search/routes/search.test.ts

-    expect(error.body.attributes).toBe(indexNotFoundException.error);
+    expect(error.body.attributes).toEqual({
+      error: indexNotFoundException.error,
+      rawResponse: undefined,


Is there a scenario we can test for here where rawResponse isn't undefined too?

I can add a test case to API integration tests that verifies rawResponse is returned. These tests are all just mocks, where as API integration tests test actual code paths.

That instead also works for me 👍

nreese · 2023-10-12T19:41:20Z

Also, unrelated to this PR, but I noticed the inspector uses X failured shard(s) in the title, which looks a little odd. Should it instead say X failed shard(s)?

I am not sure I see the problem. It says "1 failed shard" when there is a single shard failure and "3 failed shards" when there are multiple shard failures. This seems like proper english. Could you explain further?

https://github.com/elastic/kibana/blob/main/src/plugins/inspector/public/views/requests/components/details/clusters_view/clusters_table/shards_view/shard_failure_flyout.tsx#L35

{i18n.translate('inspector.requests.clusters.shards.flyoutTitle', {
              defaultMessage:
                '{failedShardCount} failured {failedShardCount, plural, one {shard} other {shards}}',
              values: { failedShardCount: failures.length },
            })}

davismcphee · 2023-10-12T20:36:10Z

I am not sure I see the problem. It says "1 failed shard" when there is a single shard failure and "3 failed shards" when there are multiple shard failures. This seems like proper english. Could you explain further?

"1 failed shard" makes sense, but currently it's "1 failured shard". There's an additional "ur" in "failed" currently.

nreese · 2023-10-12T21:09:37Z

"1 failed shard" makes sense, but currently it's "1 failured shard". There's an additional "ur" in "failed" currently.

Thanks, I see that now. I can open a separate PR to resolve

drewdaemon

Lens changes make sense to me. I left one question.

Search changes are owned by Discovery, so nothing needed from me there.

drewdaemon · 2023-10-13T20:54:34Z

x-pack/plugins/lens/public/editor_frame_service/error_helper.tsx

+    if (e.attributes?.error?.reason) {
+      return getNestedErrorClause(e.attributes.error);
+    }
+    if (e.attributes?.error?.caused_by) {


Not sure I understand why this guard was inserted. It doesn't look like we gated the logic by the existence of caused_by before.

attributes.error is typed as optional.

It looks like the previous logic assumed that caused_by was defined (type cast) which it must always have been since getNestedErrorClause would have errored if passed undefined.

Given this, I think changing from casting to checking is okay because it should not change the amount of times this branch gets entered.

nreese · 2023-10-22T20:32:20Z

@elasticmachine merge upstream

angorayc · 2023-10-23T11:03:23Z

x-pack/test/security_solution_cypress/cypress/e2e/explore/dashboards/entity_analytics.cy.ts

+  // Skipping to unblock: https://github.com/elastic/kibana/pull/168389
+  describe.skip('With anomalies data', () => {


I can reproduce this when running locally with Network throttling. Create an issue to follow it up: #169507

Could you please check if the error you had was the same as #168709, if so, can you please try unskipping the test?

I removed skip, it is not longer needed now that test has been fixed

drewdaemon · 2023-10-23T13:55:14Z

x-pack/plugins/lens/public/editor_frame_service/error_helper.tsx

+    if (e.attributes?.error?.reason) {
+      return getNestedErrorClause(e.attributes.error);
+    }
+    if (e.attributes?.error?.caused_by) {


It looks like the previous logic assumed that caused_by was defined (type cast) which it must always have been since getNestedErrorClause would have errored if passed undefined.

Given this, I think changing from casting to checking is okay because it should not change the amount of times this branch gets entered.

PhilippeOberti

LGTM for the Threat Hunting Investigations team!

kibana-ci · 2023-10-23T16:09:43Z

💚 Build Succeeded

Buildkite Build
Commit: a17c45c

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`	2547	2539	-8

Async chunks

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

id	before	after	diff
`lens`	1.4MB	1.4MB	+145.0B
`securitySolution`	13.0MB	13.0MB	+136.0B
`timelines`	30.0KB	30.2KB	+136.0B
total			+417.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.4KB	407.8KB	+361.0B

Unknown metric groups

API count

id	before	after	diff
`data`	3202	3194	-8

History

💛 Build #169780 was flaky 940dcb5
💔 Build #167341 failed 91ece0d
💔 Build #167118 failed 45b3a47
💔 Build #167090 failed 034e64b
💔 Build #167067 failed 8831278
💔 Build #166714 failed 127150b

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

nreese and others added 10 commits October 5, 2023 12:54

[search source] return rawResponse when search fails

fe435c5

pull error from new structure

55656c3

cleanup handleSearchError

7921fd3

reportSearchError

90de42d

tslint

7dc2d0d

send attributes to inspector instead of Error object

b79a744

throw KbnSearchError when search fails

43db56b

fix integration tests

17b9b84

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

1313601

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

lens tslint

9191337

nreese force-pushed the kbn_search_error branch from ce23841 to 9191337 Compare October 9, 2023 18:44

nreese and others added 11 commits October 9, 2023 12:48

cleanup

37403ee

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

cef72de

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

timelines tslint

cfc6e46

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

78e32f4

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

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

7295273

…-fix'

security_solution tslint

86e35f2

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

e5b159b

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

fix unit tests

5ce3155

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

e9c0137

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

update expects for delete api test

fab4790

fix serverless verify_error

8d6693e

kibanamachine and others added 2 commits October 10, 2023 11:24

Merge branch 'main' into kbn_search_error

fe0b2f8

fix integration tests

127150b

nreese marked this pull request as ready for review October 10, 2023 18:11

nreese requested review from a team as code owners October 10, 2023 18:12

nreese added release_note:skip Skip the PR/issue when compiling release notes Team:DataDiscovery Discover, search (data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. t// v8.12.0 labels Oct 10, 2023

nreese added the Feature:Search Querying infrastructure in Kibana label Oct 10, 2023

Merge branch 'main' into kbn_search_error

8831278

Merge branch 'main' into kbn_search_error

034e64b

kibanamachine and others added 2 commits October 11, 2023 15:21

Merge branch 'main' into kbn_search_error

45b3a47

skipping problematic test

15ba4bf

MadameSheema requested a review from a team as a code owner October 12, 2023 13:39

Merge branch 'main' into kbn_search_error

91ece0d

davismcphee approved these changes Oct 12, 2023

View reviewed changes

drewdaemon reviewed Oct 13, 2023

View reviewed changes

Merge branch 'main' into kbn_search_error

940dcb5

angorayc reviewed Oct 23, 2023

View reviewed changes

angorayc mentioned this pull request Oct 23, 2023

Flaky Cypress test: dashboards/entity_analytics.cy.ts #169507

Open

drewdaemon approved these changes Oct 23, 2023

View reviewed changes

remove skipping test as fix has been merged

a17c45c

PhilippeOberti approved these changes Oct 23, 2023

View reviewed changes

nreese merged commit adf3b8b into elastic:main Oct 23, 2023

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

		// Skipping to unblock: https://github.com/elastic/kibana/pull/168389
		describe.skip('With anomalies data', () => {

Conversation

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

Uh oh!

Problem

Solution

Test

Uh oh!

nreese commented Oct 10, 2023

Uh oh!

elasticmachine commented Oct 10, 2023

Uh oh!

nreese commented Oct 11, 2023

Uh oh!

nreese commented Oct 11, 2023

Uh oh!

nreese commented Oct 11, 2023

Uh oh!

davismcphee left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

davismcphee commented Oct 12, 2023

Uh oh!

nreese commented Oct 12, 2023

Uh oh!

drewdaemon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nreese commented Oct 22, 2023

Uh oh!

angorayc Oct 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PhilippeOberti left a comment

Choose a reason for hiding this comment

Uh oh!

kibana-ci commented Oct 23, 2023

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Async chunks

Page load bundle

API count

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

nreese commented Oct 9, 2023 •

edited

Loading

nreese commented Oct 12, 2023 •

edited

Loading

angorayc Oct 23, 2023 •

edited

Loading