Fix isErrorResponse when cluster details are provided by lukasolson · Pull Request #166544 · elastic/kibana

lukasolson · 2023-09-14T22:10:53Z

Summary

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests 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)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk	Probability	Severity	Mitigation/Notes
Multiple Spaces—unexpected behavior in non-default Kibana Space.	Low	High	Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks.	High	Low	Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled.	Medium	High	Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

This was checked for breaking API changes and was labeled appropriately

thomasneirynck

quick 2c.

I can verify this fixes the issue for CCS deployments, showing the expected warning

Without this fix, in 8.10, this shows an error.

thomasneirynck · 2023-09-14T23:24:37Z

+  return (
+    !response ||
+    !response.rawResponse ||
+    (!response.isRunning && !!response.isPartial && !response.rawResponse?._clusters?.details)


Just for reference:

This basically determines that ES will handle the isPartial flag differently for CCS?

Yeah, I should add a comment there. Basically, the behavior for CCS with ccs_minimize_roundtrips=true is to set is_partial to true if the search is complete but there are some shard failures (and thus partial results), then have details about the failures inside the _clusters.details section. This will also likely be the behavior of CCS with ccs_minimize_roundtrips=false and non-CCS after elastic/elasticsearch#98913 is resolved.

I've added some comments to the code in 0859897.

This will also likely be the behavior of CCS with ccs_minimize_roundtrips=false

Chatted offline with @quux00 and he confirmed this is correct for the non-CCS situation (this work is still pending), but already merged for the CSS-part in 8.11/main.

elasticmachine · 2023-09-14T23:35:29Z

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

davismcphee

Thanks for fixing this! I confirmed that without the fix in 8.10, an error is shown. And with the fix in this PR, a warning is shown that matches the local cluster behaviour.

Local:

Remote:

We should try to get confirmation from someone in Security who encountered the issue too if possible, but I don't want to hold hold this up so I'm approving now for when we're ready.

davismcphee

Oops, I didn't realize the build failed before submitting my review... I guess we'll need to investigate what's going on there first.

kertal · 2023-09-15T06:18:54Z

@elasticmachine merge upstream

thomasneirynck · 2023-09-15T14:38:32Z

@elasticmachine merge upstream

thomasneirynck · 2023-09-15T14:41:58Z

cc @yuliacech @stephmilovic Would be great if you could take a look at this as well from Security-side. Thx!

…a into fix/isErrorResponse8.10

kibana-ci · 2023-09-15T20:28:21Z

💚 Build Succeeded

Buildkite Build
Commit: 42c9107

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`	2576	2574	-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`	404.5KB	404.6KB	+133.0B

History

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

cc @lukasolson

sophiec20 · 2023-09-18T08:57:08Z

Is there risk this might have impact for the visualizations team?

And is there an impact for CSS if the remote clusters are still 8.9.x and local is 8.10.x ?

kertal · 2023-09-18T09:38:15Z

Is there risk this might have impact for the visualizations team?

I don't think there is but @elastic/kibana-visualizations will know better

thomasneirynck · 2023-09-18T11:57:13Z

@elasticmachine merge upstream

stephmilovic

This clears the partial results error in Security Solution. Thanks for the quick fix, LGTM!

timductive · 2023-09-18T15:00:21Z

And is there an impact for CSS if the remote clusters are still 8.9.x and local is 8.10.x?

@lukasolson I want to make sure we don't lose sophie's question here, have we validated this with clusters on different versions?

lukasolson · 2023-09-18T16:37:47Z

And is there an impact for CSS if the remote clusters are still 8.9.x and local is 8.10.x?

I just verified that this fix works for this scenario as well (and that it is also broken in this scenario without this fix).

drewdaemon

This PR resolves the issue for visualizations as well

Lens editor

Before

After

TSVB

Before

After

Aggs-based

Before

After

…ic#166544) ## Summary Fixes elastic#166528. ### 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) ### Risk Matrix Delete this section if it is not applicable to this PR. Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release. When forming the risk matrix, consider some of the following examples and how they may potentially impact the change: | Risk | Probability | Severity | Mitigation/Notes | |---------------------------|-------------|----------|-------------------------| | Multiple Spaces—unexpected behavior in non-default Kibana Space. | Low | High | Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces. | | Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks. | High | Low | Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure. | | Code should gracefully handle cases when feature X or plugin Y are disabled. | Medium | High | Unit tests will verify that any feature flag or plugin combination still results in our service operational. | | [See more potential risk examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx) | ### 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) --------- Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

## Summary Cherry picks #166544 into main. Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Fix isErrorResponse when cluster details are provided

016f723

lukasolson self-assigned this Sep 14, 2023

Fix isCompleteResponse and rawResponse behavior

1c083be

thomasneirynck reviewed Sep 14, 2023

View reviewed changes

Merge branch '8.10' into fix/isErrorResponse8.10

703b118

lukasolson marked this pull request as ready for review September 14, 2023 23:35

lukasolson added the release_note:fix label Sep 14, 2023

davismcphee approved these changes Sep 15, 2023

View reviewed changes

davismcphee reviewed Sep 15, 2023

View reviewed changes

Merge branch '8.10' into fix/isErrorResponse8.10

6725d1b

Merge branch '8.10' into fix/isErrorResponse8.10

676ded5

lukasolson added 3 commits September 15, 2023 09:40

Fix isCompleteResponse and add comments

0859897

Merge branch 'fix/isErrorResponse8.10' of github.com:lukasolson/kiban…

457e7bb

…a into fix/isErrorResponse8.10

Fix isCompleteResponse

42c9107

kertal reviewed Sep 18, 2023

View reviewed changes

Comment thread src/plugins/data/common/search/utils.ts

Merge branch '8.10' into fix/isErrorResponse8.10

0662b07

stephmilovic approved these changes Sep 18, 2023

View reviewed changes

drewdaemon approved these changes Sep 18, 2023

View reviewed changes

davismcphee added v8.10.2 and removed v8.10.1 labels Sep 18, 2023

davismcphee merged commit d317259 into elastic:8.10 Sep 18, 2023

lukasolson mentioned this pull request Sep 18, 2023

[Search] Fix isErrorResponse when cluster details are provided #166667

Merged

piergm reviewed Sep 19, 2023

View reviewed changes

Comment thread src/plugins/data/common/search/utils.ts

lukasolson added a commit that referenced this pull request Sep 21, 2023

[Search] Fix isErrorResponse when cluster details are provided (#166667)

22a9f4a

## Summary Cherry picks #166544 into main. Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

This was referenced Oct 1, 2023

[8.10] fix(slo): Handle partial indicator url state (#167247) #167722

Merged

[8.10] backport SLO fixes #168231

Merged

Conversation

lukasolson commented Sep 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Risk Matrix

For maintainers

Uh oh!

thomasneirynck left a comment

Choose a reason for hiding this comment

Uh oh!

thomasneirynck Sep 14, 2023

Choose a reason for hiding this comment

Uh oh!

lukasolson Sep 14, 2023

Choose a reason for hiding this comment

Uh oh!

lukasolson Sep 15, 2023

Choose a reason for hiding this comment

Uh oh!

thomasneirynck Sep 20, 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!

elasticmachine commented Sep 14, 2023

Uh oh!

davismcphee left a comment

Choose a reason for hiding this comment

Uh oh!

davismcphee left a comment

Choose a reason for hiding this comment

Uh oh!

kertal commented Sep 15, 2023

Uh oh!

thomasneirynck commented Sep 15, 2023

Uh oh!

thomasneirynck commented Sep 15, 2023

Uh oh!

kibana-ci commented Sep 15, 2023

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Page load bundle

History

Uh oh!

Uh oh!

sophiec20 commented Sep 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kertal commented Sep 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thomasneirynck commented Sep 18, 2023

Uh oh!

stephmilovic left a comment

Choose a reason for hiding this comment

Uh oh!

timductive commented Sep 18, 2023 • edited by lukasolson Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lukasolson commented Sep 18, 2023

Uh oh!

drewdaemon left a comment

Choose a reason for hiding this comment

Lens editor

Before

After

TSVB

Before

After

Aggs-based

Before

After

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

lukasolson commented Sep 14, 2023 •

edited

Loading

thomasneirynck Sep 20, 2023 •

edited

Loading

sophiec20 commented Sep 18, 2023 •

edited

Loading

kertal commented Sep 18, 2023 •

edited

Loading

timductive commented Sep 18, 2023 •

edited by lukasolson

Loading