Skip to content

[Search] Add response status helpers#78006

Merged
lizozom merged 3 commits intoelastic:masterfrom
lizozom:search/response-helpers
Sep 23, 2020
Merged

[Search] Add response status helpers#78006
lizozom merged 3 commits intoelastic:masterfrom
lizozom:search/response-helpers

Conversation

@lizozom
Copy link
Copy Markdown
Contributor

@lizozom lizozom commented Sep 21, 2020

Summary

Dev Docs

Introduces the following search helpers

  • isCompleteResponse
  • isErrorResponse
  • isPartialResponse

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@lizozom 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 lizozom requested review from a team as code owners September 21, 2020 12:34
@lizozom lizozom self-assigned this Sep 21, 2020
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Sep 21, 2020

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

@lizozom
Copy link
Copy Markdown
Contributor Author

lizozom commented Sep 21, 2020

@elasticmachine merge upstream

lukasolson
lukasolson approved these changes Sep 22, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@lukasolson lukasolson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

examples/search_examples/public/components/app.tsx
text: mountReactNode(message),
});
searchSubscription$.unsubscribe();
} else if (response.isPartial && !response.isRunning) {
Copy link
Copy Markdown
Contributor

@lukasolson lukasolson Sep 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@lizozom lizozom Sep 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

x-pack/plugins/data_enhanced/public/search/search_interceptor.ts
// 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());
Copy link
Copy Markdown
Contributor

@lukasolson lukasolson Sep 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@lizozom lizozom Sep 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, this is handled in the error alignment PR

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

@lizozom
Copy link
Copy Markdown
Contributor Author

lizozom commented Sep 22, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Sep 22, 2020

💚 Build Succeeded

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

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

patrykkopycinski
patrykkopycinski approved these changes Sep 22, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@patrykkopycinski patrykkopycinski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM Thank you @lizozom 💪

XavierM
XavierM approved these changes Sep 23, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@XavierM XavierM left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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!!!

@lizozom lizozom merged commit 82af937 into elastic:master Sep 23, 2020
@lizozom lizozom mentioned this pull request Sep 23, 2020
Merged
lizozom added a commit to lizozom/kibana that referenced this pull request Sep 23, 2020
@lizozom @elasticmachine
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
@lizozom @elasticmachine
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>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Sep 23, 2020
@gmmorris
Merge branch 'master' into actions/pagerduty-dedup
097b99d 
* 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)
  ...
@nreese nreese mentioned this pull request Aug 22, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@XavierM XavierM XavierM approved these changes

@lukasolson lukasolson lukasolson approved these changes

@patrykkopycinski patrykkopycinski patrykkopycinski approved these changes

Assignees

@lizozom lizozom

Labels

Feature:Search Querying infrastructure in Kibana release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. v7.10.0 v8.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@lizozom @elasticmachine @kibanamachine @XavierM @lukasolson @patrykkopycinski