[Search] Add response status helpers #78006

lizozom · 2020-09-21T12:34:33Z

Summary

Dev Docs

Introduces the following search helpers

isCompleteResponse
isErrorResponse
isPartialResponse

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
This was checked for keyboard-only and screenreader accessibility
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2020-09-21T12:34:36Z

Pinging @elastic/kibana-app-arch (Team:AppArch)

lizozom · 2020-09-21T15:20:36Z

@elasticmachine merge upstream

lukasolson

Changes LGTM... I'm surprised at how many places this code is already duplicated, so these utilities are definitely helpful. Left a couple of comments below but nothing that should prevent this PR from moving forward.

lukasolson · 2020-09-22T05:38:13Z

examples/search_examples/public/components/app.tsx

@@ -162,7 +164,7 @@ export const SearchExamplesApp = ({
              text: mountReactNode(message),
            });
            searchSubscription$.unsubscribe();
-          } else if (response.isPartial && !response.isRunning) {


Do we know this is an actual documented error case? Is there a reliable way to reproduce this error?

I pinged the security team, for them to consider reducing code duplication.

And this is definitely a documented error case (this is what delayed the release of 7.8 or 7.7 I remember)

lukasolson · 2020-09-22T05:39:08Z

x-pack/plugins/data_enhanced/public/search/search_interceptor.ts

@@ -66,12 +67,12 @@ export class EnhancedSearchInterceptor extends SearchInterceptor {
    return this.runSearch(request, combinedSignal, strategy).pipe(
      expand((response) => {
        // If the response indicates of an error, stop polling and complete the observable
-        if (!response || (!response.isRunning && response.isPartial)) {
+        if (isErrorResponse(response)) {
          return throwError(new AbortError());


We need to revisit the error we throw here. I think we need to forward the error returned by ES.

Yep, this is handled in the error alignment PR

https://github.com/elastic/kibana/pull/77788/files#diff-75f11add5ff5d57a12822fe0c5a52536R91

lizozom · 2020-09-22T13:17:53Z

@elasticmachine merge upstream

kibanamachine · 2020-09-22T14:55:17Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 1bcf744

Build metrics

@kbn/optimizer bundle module count

id	value	diff	baseline
data	568	+1	567

async chunks size

id	value	diff	baseline
securitySolution	10.2MB	+702.0B	10.2MB

page load bundle size

id	value	diff	baseline
data	1.5MB	+1.5KB	1.5MB
dataEnhanced	177.2KB	+32.0B	177.1KB
total		+1.5KB

distributable file count

id	value	diff	baseline
default	45943	+1	45942
oss	26652	+1	26651
total		+2

History

💛 Build #75888 was flaky fc27485
💛 Build #75834 was flaky 41adfc3

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

patrykkopycinski

LGTM Thank you @lizozom 💪

XavierM

@lizozom like we discussed and we will follow your advice to create a generic function to manage all of this in one place. So next time, you won't have to change it everywhere. Thanks a lot!!!

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

* master: (31 commits) skip tests for old pacakge (elastic#78194) [Ingest Pipelines] Add url generator for ingest pipelines app (elastic#77872) [Lens] Rename "telemetry" to "stats" (elastic#78125) [CSM] Url search (elastic#77516) [Drilldowns] Config to disable URL Drilldown (elastic#77887) [Lens] Combined histogram/range aggregation for numbers (elastic#76121) Remove legacy plugins support (elastic#77599) 'Auto' interval must be correctly calculated for natural numbers (elastic#77995) [CSM] fix ingest data retry order messed up (elastic#78163) Add response status helpers (elastic#78006) Bump react-beautiful-dnd (elastic#78028) [Security Solution][Detection Engine] Bubbles up more error messages from ES queries to the UI (elastic#78004) Index pattern - refactor constructor (elastic#77791) Add `xpack.security.sameSiteCookies` to docker allow list (elastic#78192) Remove [key: string]: any; from IIndexPattern (elastic#77968) Remove requirement for manage_index_templates privilege for Index Management (elastic#77377) [Ingest Manager] Agent bulk actions UI (elastic#77690) [Metrics UI] Add inventory view timeline (elastic#77804) Reporting/Docs: Updates for setting to enable CSV Download (elastic#78101) Update to latest rum-react (elastic#78193) ...

Add response status helpers

41adfc3

lizozom added Feature:Search Querying infrastructure in Kibana release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. v8.0.0 Team:AppArch v7.10.0 labels Sep 21, 2020

lizozom requested review from a team as code owners September 21, 2020 12:34

lizozom self-assigned this Sep 21, 2020

Merge branch 'master' into search/response-helpers

fc27485

lukasolson approved these changes Sep 22, 2020

View reviewed changes

Merge branch 'master' into search/response-helpers

1bcf744

patrykkopycinski approved these changes Sep 22, 2020

View reviewed changes

XavierM approved these changes Sep 23, 2020

View reviewed changes

lizozom merged commit 82af937 into elastic:master Sep 23, 2020

lizozom mentioned this pull request Sep 23, 2020

[7.x] Add response status helpers (#78006) #78250

Merged

lizozom added a commit to lizozom/kibana that referenced this pull request Sep 23, 2020

Add response status helpers (elastic#78006)

62875cd

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

lizozom added a commit that referenced this pull request Sep 23, 2020

Add response status helpers (#78006) (#78250)

ee9a2e5

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

nreese mentioned this pull request Aug 22, 2023

Handling of cross cluster search errors #163381

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Search] Add response status helpers #78006

[Search] Add response status helpers #78006

lizozom commented Sep 21, 2020

elasticmachine commented Sep 21, 2020

lizozom commented Sep 21, 2020

lukasolson left a comment

lukasolson Sep 22, 2020

lizozom Sep 22, 2020

lukasolson Sep 22, 2020

lizozom Sep 22, 2020

lizozom commented Sep 22, 2020

kibanamachine commented Sep 22, 2020

patrykkopycinski left a comment

XavierM left a comment •

edited

Loading

[Search] Add response status helpers #78006

[Search] Add response status helpers #78006

Conversation

lizozom commented Sep 21, 2020

Summary

Dev Docs

Checklist

For maintainers

elasticmachine commented Sep 21, 2020

lizozom commented Sep 21, 2020

lukasolson left a comment

Choose a reason for hiding this comment

lukasolson Sep 22, 2020

Choose a reason for hiding this comment

lizozom Sep 22, 2020

Choose a reason for hiding this comment

lukasolson Sep 22, 2020

Choose a reason for hiding this comment

lizozom Sep 22, 2020

Choose a reason for hiding this comment

lizozom commented Sep 22, 2020

kibanamachine commented Sep 22, 2020

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

async chunks size

page load bundle size

distributable file count

History

patrykkopycinski left a comment

Choose a reason for hiding this comment

XavierM left a comment • edited Loading

Choose a reason for hiding this comment

XavierM left a comment •

edited

Loading