Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove unneeded usages of isErrorResponse #164609

Merged
merged 23 commits into from
Aug 28, 2023

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Aug 23, 2023

data.search uses poll_search to fetch data. poll_search checks for isErrorResponse and throws whenisErrorResponse is true. Note searchSource.search uses data.search to perform searches.

   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.

@nreese
Copy link
Contributor Author

nreese commented Aug 23, 2023

@elasticmachine merge upstream

@nreese nreese force-pushed the remove_isErrorResponse branch from 61fa2f6 to d71119f Compare August 23, 2023 21:28
@nreese nreese force-pushed the remove_isErrorResponse branch from 5a73b8c to 9a0bc67 Compare August 24, 2023 18:25
@nreese nreese force-pushed the remove_isErrorResponse branch from 709e82e to c3189d0 Compare August 24, 2023 19:45
@nreese
Copy link
Contributor Author

nreese commented Aug 24, 2023

@elasticmachine merge upstream

@nreese nreese marked this pull request as ready for review August 24, 2023 21:34
@nreese nreese requested review from a team as code owners August 24, 2023 21:34
@nreese nreese requested review from a team as code owners August 24, 2023 21:34
@nreese nreese added Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas release_note:skip Skip the PR/issue when compiling release notes v8.11.0 labels Aug 24, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-presentation (Team:Presentation)

@nreese nreese added the technical debt Improvement of the software architecture and operational architecture label Aug 24, 2023
Copy link
Contributor

@rylnd rylnd left a comment

Choose a reason for hiding this comment

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

I'm not very familiar with the search_strategy code so I'm going to defer on approving this, but it does look like that translation can now be removed as well 👍

@nreese nreese requested a review from rylnd August 24, 2023 22:05
@nreese
Copy link
Contributor Author

nreese commented Aug 24, 2023

I'm not very familiar with the search_strategy code so I'm going to defer on approving this

Are there any edge cases that have broken search in security solution before? You could try testing those

@rylnd
Copy link
Contributor

rylnd commented Aug 24, 2023

Are there any edge cases that have broken search in security solution before? You could try testing those

I defer to my previous statement 😅

Apologies, I came here from the general ping to security-solution (which is a weird consequence of the CODEOWNERS structure ?); the Threat Hunting teams listed in reviewers are going to have the best context here.

Copy link
Contributor

@darnautov darnautov left a comment

Choose a reason for hiding this comment

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

ML changes LGTM

Copy link
Contributor

@dej611 dej611 left a comment

Choose a reason for hiding this comment

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

Code review only 👍

@logeekal
Copy link
Contributor

Are there any edge cases that have broken search in security solution before? You could try testing those

Overall changes in security solutions look good to me but I deferred the approval before I discuss with my team regarding any edge that might be affected. Although, mostly cypress tests should already cover it.

Copy link
Member

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

LGTM, tested a few different error types and functionality hasn't changed.

Copy link
Member

@machadoum machadoum left a comment

Choose a reason for hiding this comment

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

threat-hunting-explore changes LGTM!

@nreese
Copy link
Contributor Author

nreese commented Aug 28, 2023

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 4473 4472 -1

Async chunks

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

id before after diff
aiops 593.7KB 593.6KB -102.0B
securitySolution 16.0MB 16.0MB -4.9KB
threatIntelligence 56.6KB 56.5KB -60.0B
triggersActionsUi 1.5MB 1.5MB -175.0B
total -5.2KB

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.6KB 404.5KB -48.0B

History

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

Copy link
Contributor

@PhilippeOberti PhilippeOberti left a comment

Choose a reason for hiding this comment

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

LGTM for the Protections Experience team! :)

Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

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

LGTM

@nreese nreese merged commit 0113eb7 into elastic:main Aug 28, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Aug 28, 2023
jloleysens added a commit to jloleysens/kibana that referenced this pull request Aug 29, 2023
* main: (40 commits)
  Adjust migrations and elasticsearch service settings for serverless. (elastic#165050)
  [Security Solution] expandable flyout - add investigate in timeline f… (elastic#165025)
  [SecuritySolution] Hide create dashboard button from listing (elastic#164476)
  Construct HTTP log message only if needed (elastic#165057)
  [Security Solution] expandable flyout - add no data message in entities details and entities overview components (elastic#164955)
  Add functional tests for serverless security management UIs (elastic#164886)
  [api-docs] 2023-08-29 Daily api_docs build (elastic#165056)
  [Cloud Security][CIS GCP]cis gcp now use updated gcp field name + small last minute changes (elastic#164792)
  [Security Solution] Expandable flyout - update risk classification ui in entities overview (elastic#165022)
  [Security Solution] Fixes Preconfigured Connectors not working with Assistant (elastic#164900)
  [Security Solution] Coverage Overview follow-up 2 (elastic#164986)
  [DOCS] Add cross-link for other encryption key settings (elastic#165014)
  chore(slo): general enhancement (elastic#164723)
  Revert "[SOR] Allow optionally downgrading documents with a higher version model in API READ methods" (elastic#164991)
  [OAS] Add more Elasticsearch query rule examples (elastic#164386)
  [security_solution_cypress] Add support for options in EsArchiver.load (elastic#164988)
  [Event Log] Skip setting assets to hidden in serverless (elastic#164767)
  remove unneeded usages of isErrorResponse (elastic#164609)
  [Enterprise Search] Make network drive connector platinum (elastic#165007)
  [RAM] update api key to become public (elastic#164883)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas technical debt Improvement of the software architecture and operational architecture v8.11.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.