Add SearchStrategyRegistry and defaultSearchStrategy to support existing search behavior, and integrate it with CallClient. #20497

cjcenizal · 2018-07-05T19:31:54Z

Addresses initial steps in #20004:

Create a registry for adding a handler that can create a search based on index pattern type.
Refactor Courier to have a clear integration point for this registry.

Handling errors

When you try to access a rollup index pattern in Discover without the correct search strategy (e.g. you've downgraded from X-Pack to OSS Kibana), you'll see an error:

This requires mentioning an X-Pack feature in OSS, but I talked about this with @AlonaNadler and we think it's acceptable since the user has already been using X-Pack to create the rollup originally.

elasticmachine · 2018-07-05T20:45:02Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…ing search behavior, and integrate it with CallClient.

elasticmachine · 2018-07-05T23:35:26Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…h uiExport to avoid race conditions with other plugins.

elasticmachine · 2018-07-06T19:20:36Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

spalger · 2018-07-06T19:28:01Z

src/ui/public/courier/fetch/call_client.js

-        esPromise = es.msearch({ body: serializedFetchParams });
-        const clientResponse = await esPromise;
-        await respond(clientResponse.responses);
+        esPromise = Promise.all(esPromises);


We'll need to update the esPromise logic to handle the esPromises array because it needs access to the individual esPromise.abort() methods

Also, would you mind trying to write a test that validates that promises from the esClient are aborted correctly when a single request in a batch is aborted? I imagine it's going to be tricky but would probably be a good idea to have given the scope of this refactor.

We'll need to update the esPromise logic to handle the esPromises array because it needs access to the individual esPromise.abort() methods

Can you see any problem with just removing the code that calls esPromise.abort()? What's the point of aborting them?

jen-huang · 2018-07-06T22:58:19Z

src/ui/public/courier/search_strategy/search_strategy_registry.js

+
+  searchRequests.forEach(searchRequest => {
+    const matchingSearchStrategy = searchStrategies.find(searchStrategy => searchStrategy.isValidForSearchRequest(searchRequest));
+    const { id } = matchingSearchStrategy;


getting an error for a rollup index pattern in Discover, as it doesn't match any registered search strategies:

should handle cases where there is no matching search strategy. maybe we set strategy id to default?

cjcenizal · 2018-07-06T23:05:26Z

@jen-huang I ran into this error myself and then smacked my forehead with my palm. Technically, this situation won't ever arise, because you'll only be able to create a rollup index pattern using the UI in our feature/rollups branch, and that's where I'm also going to add the rollup specific strategy (WIP here: #20505). This branch is just supposed to create some abstractions we can use without changing any functionality. So I just solved the problem for myself by deleting the rollup index pattern, which is a more realistic scenario.

That said, I wonder if we really need to gracefully handle cases where there's no matching strategy? That would mean that either a) a plugin developer relies on courier for searching but failed to import 'uiExports/search' or b) we've created an index pattern which has a type defined that our UI doesn't currently support. Both of those seem like error cases to me, so I think we should probably handle them by throwing an informative error. WDYT?

elasticmachine · 2018-07-06T23:30:27Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

bmcconaghy

LGTM

… accepts.

spalger · 2018-07-16T20:24:22Z

src/ui/public/courier/fetch/call_client.js

+        // Assigning searchRequests to strategies means that the responses come back in a different
+        // order than the original searchRequests. So we'll put them back in order so that we can
+        // use the order to associate each response with the original request.
+        const responsesInOriginalRequestOrder = new Array(searchRequestsAndStatuses.length);


Nothing "wrong" with this here, but I think it would be logical to use the searchRequests array instead of the searchRequestsAndStatuses array here, below, and in respondToSearchRequests(), don't you think? Especially since respondToSearchRequests() is reading the status from searchRequestsAndStatuses by index anyway.

At least for me, using searchRequestsAndStatuses makes the code a bit easier to follow by reducing the number of variables I have to keep in mind. In terms of data structures being consumed, searchRequests stops being important immediately, and searchRequestsAndStatuses, requestsToFetch, and searchStrategiesWithRequests become the important ones. It would be a bit confusing to me if I were to read this code and see searchRequests was reintroduced -- I'd wonder what the intention behind that was.

spalger · 2018-07-16T20:29:07Z

src/core_plugins/kibana/public/discover/directives/unsupported_rollup.js

+    <EuiFlexGroup justifyContent="center">
+      <EuiFlexItem grow={false} className="discoverNoResults">
+        <EuiCallOut
+          title="OSS Kibana doesn't support rollup index patterns. Please use X-Pack to search this index pattern."


Do you think users will understand "OSS Kibana"? I wonder if we should rephrase this to instead say something like: "Index Patterns based on rollup indices require the rollup plugin from X-Pack which is either not installed or disabled."

I like this a lot. @gchaps thoughts?

spalger · 2018-07-16T20:31:34Z

src/ui/public/courier/fetch/call_client.js

+        // order than the original searchRequests. So we'll put them back in order so that we can
+        // use the order to associate each response with the original request.
+        const responsesInOriginalRequestOrder = new Array(searchRequestsAndStatuses.length);
+        segregatedResponses.forEach((responses, strategyIndex) => {


Maybe replace responses on this line with { responses } to avoid some of the redundancy here?

Maybe I should just change the interface to only return the responses array. WDYT?

Either works for me

spalger

A couple nitpicks, but went through the code pretty thoroughly and pulled it down and searching/aborting/etc seem to work great.

jen-huang · 2018-07-16T20:39:52Z

src/ui/public/courier/search_strategy/search_strategy_registry.js

+  return Boolean(getSearchStrategy(indexPattern));
+};
+
+const isRollupIndexPattern = indexPattern => {


I don't think we should have any rollup references in OSS code. when we embarked on this journey, we made the decision to avoid any cross stream code for x-pack features. maybe this can be abstracted out as part of as part of registered search strategies? see my next comments for ideas 😄

jen-huang · 2018-07-16T20:44:46Z

src/core_plugins/kibana/public/discover/controllers/discover.js

@@ -777,5 +777,16 @@ function discoverController(
    return loadedIndexPattern;
  }

+  // Block the UI from loading if the user has loaded a rollup index pattern but it isn't
+  // supported.
+  $scope.isUnsupportedRollup = (


when rollup reference is abstracted away, maybe this can be something like:

import { hasSearchStategyForIndexPattern, isIndexPatternSupported, renderUnsupportedIndexPatternMessage, } from 'ui/courier'; if( !hasSearchStategyForIndexPattern($route.current.locals.ip.loaded) || !isIndexPatternSupported($route.current.locals.ip.loaded) ) { // psuedo code - implementation will need adjustment for rendering logic! :) $scope.unsupportedMessage = renderUnsupportedIndexPatternMessage(); return; }

I redact my previous review after chatting with CJ and realizing that the unsupported rollup messages were are due to giving user hints when they're using OSS. X-Pack code isn't reachable in that scenario.

The comprise to support this type of error in OSS, while avoiding cross stream OSS/X-Pack code and rollup references, is to check whether the index pattern is the default type, instead of checking for rollup type.

jen-huang · 2018-07-16T20:48:53Z

src/core_plugins/kibana/public/discover/directives/index.js

@@ -25,8 +25,14 @@ import {
  DiscoverNoResults,
 } from './no_results';

+import {


references to rollup message can be removed with implementation of renderUnsupportedIndexPatternMessage or similar

jen-huang · 2018-07-16T20:49:29Z

src/core_plugins/kibana/public/discover/directives/unsupported_rollup.js

@@ -0,0 +1,43 @@
+/*


then, this component can be moved somewhere inside x-pack/rollups directory as part of rollup PR

jen-huang · 2018-07-16T20:50:07Z

src/core_plugins/kibana/public/discover/index.html

@@ -60,6 +60,10 @@ <h1 tabindex="0" id="kui_local_breadcrumb" class="kuiLocalBreadcrumb">

      <div class="discover-wrapper col-md-10">
        <div class="discover-content">
+          <discover-unsupported-rollup


and this will use generic $scope.unsupportedMessage or similar, instead of referencing rollups directly

cjcenizal · 2018-07-16T22:37:26Z

@spalger @jen-huang I've updated the error message and removed references in the code to rollups (except for comments). I've also changed callClient to expect a strategy's searching promise to resolve with a responses array.

spalger · 2018-07-16T23:04:44Z

Still LGTM

elasticmachine · 2018-07-16T23:50:02Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-07-17T01:19:22Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

spalger · 2018-07-17T04:54:11Z

src/ui/public/courier/search_strategy/default_search_strategy.js

@@ -68,12 +68,12 @@ export const defaultSearchStrategy = {
      failedSearchRequests,
    } = await serializeAllFetchParams(allFetchParams, searchRequests, serializeFetchParams);

-    const searching = es.msearch({ body: serializedFetchParams }).then(({ responses }) => responses);
-    const abort = searching.abort;
+    const searching = es.msearch({ body: serializedFetchParams });


gchaps · 2018-07-17T16:05:01Z

@cjcenizal A minor edit. Add a comma before which and I think we can remove "either"

Index patterns based on rollup indices require the rollup plugin from X-pack, which is not installed or disabled

gchaps · 2018-07-17T17:33:30Z

@cjcenizal @alexfrancoeur

re: the wording of Please use X-Pack to search this index pattern or Please install an Elastic licensed version of Kibana to use rollups.

How about:

"OSS Kibana doesn't support rollup index patterns. You must use the default distribution of Kibana and a Basic license at a minimum."

I picked "default distribution" due to the wording here: https://www.elastic.co/downloads/kibana

jen-huang

The new changes to abstract out the index pattern type are great! I tested locally and am able to get the unsupported message to show in Discover.

I think we need to apply the same logic (that checks default index pattern type and if it has a search strategy) to search calls made by visualizations. I still get the same Courier fetch: Cannot read property 'id' of undefined when trying to create a visualization for a rollup pattern.

cjcenizal · 2018-07-17T21:28:57Z

I think we need to apply the same logic (that checks default index pattern type and if it has a search strategy) to search calls made by visualizations. I still get the same Courier fetch: Cannot read property 'id' of undefined when trying to create a visualization for a rollup pattern.

@jen-huang I agree but I'd like to make those changes in a separate PR to keep the size of this one down and to merge it in ASAP.

jen-huang

Since the error in visualizations for rollups won't affect anyone else due to rollup code not being in master, LGTM!

elasticmachine · 2018-07-18T00:25:50Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…gy-registry

elasticmachine · 2018-07-18T01:47:38Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…ting search behavior, and integrate it with CallClient. (elastic#20497) * Add SearchStrategyRegistry and defaultSearchStrategy to support existing search behavior, and integrate it with CallClient. * Move fetch param logic from CallClient into defaultSearchStrategy. * Move defaultSearchStrategy configuration into kibana plugin via search uiExport to avoid race conditions with other plugins. * Add call-out react directive. * Show error in Discover if user tries to access a rollup index pattern without the right search strategy. Sentence-case copy in field chooser. * Add tests with multiple searchStrategies and fix errors in logic.

…ting search behavior, and integrate it with CallClient. (#20497) (#20912) * Add SearchStrategyRegistry and defaultSearchStrategy to support existing search behavior, and integrate it with CallClient. * Move fetch param logic from CallClient into defaultSearchStrategy. * Move defaultSearchStrategy configuration into kibana plugin via search uiExport to avoid race conditions with other plugins. * Add call-out react directive. * Show error in Discover if user tries to access a rollup index pattern without the right search strategy. Sentence-case copy in field chooser. * Add tests with multiple searchStrategies and fix errors in logic.

cjcenizal added the WIP Work in progress label Jul 5, 2018

cjcenizal force-pushed the search-strategy-registry branch from 2ca6559 to 973c580 Compare July 5, 2018 21:30

Add SearchStrategyRegistry and defaultSearchStrategy to support exist…

2bd4962

…ing search behavior, and integrate it with CallClient.

cjcenizal added :Discovery v7.0.0 v6.4.0 and removed WIP Work in progress labels Jul 5, 2018

cjcenizal requested review from jen-huang and bmcconaghy July 5, 2018 22:21

cjcenizal force-pushed the search-strategy-registry branch from 973c580 to 2bd4962 Compare July 5, 2018 22:22

Fix typo.

1c1b99a

cjcenizal mentioned this pull request Jul 5, 2018

[META] Rollups #20004

Closed

52 tasks

cjcenizal changed the title ~~Add SearchStrategyRegistry with a default strategy to support existing search behavior, and integrate it with CallClient.~~ Add SearchStrategyRegistry and defaultSearchStrategy to support existing search behavior, and integrate it with CallClient. Jul 6, 2018

cjcenizal added 2 commits July 6, 2018 10:55

Move fetch param logic from CallClient into defaultSearchStrategy.

a0386ea

Move defaultSearchStrategy configuration into kibana plugin via searc…

ebd3544

…h uiExport to avoid race conditions with other plugins.

cjcenizal requested a review from spalger July 6, 2018 18:02

Fix typo in comment.

27ba85c

spalger reviewed Jul 6, 2018

View reviewed changes

Fix bug where Dashboard Only Mode was broken.

8a7e05f

jen-huang reviewed Jul 6, 2018

View reviewed changes

bmcconaghy approved these changes Jul 9, 2018

View reviewed changes

cjcenizal added 3 commits July 9, 2018 11:19

Don't throw ABORTED.

0a8442b

Add comments and refactor for clarity.

9777a60

Refactor strategy interface to be more flexible with the arguments it…

31925d5

… accepts.

spalger reviewed Jul 16, 2018

View reviewed changes

spalger approved these changes Jul 16, 2018

View reviewed changes

jen-huang requested changes Jul 16, 2018

View reviewed changes

cjcenizal added 2 commits July 16, 2018 15:33

Check for default type index pattern instead of rollup type.

4aba1fa

CallClient now expects search to resolve with responses array.

4c51a51

Fix tests.

1c4e668

spalger reviewed Jul 17, 2018

View reviewed changes

jen-huang reviewed Jul 17, 2018

View reviewed changes

jen-huang approved these changes Jul 17, 2018

View reviewed changes

Tweak copy.

ff73efb

Merge branch 'master' of github.com:elastic/kibana into search-strate…

e6379d8

…gy-registry

cjcenizal merged commit a36b87a into elastic:master Jul 18, 2018

cjcenizal deleted the search-strategy-registry branch July 18, 2018 01:49

cjcenizal mentioned this pull request Jul 18, 2018

[6.x] Add SearchStrategyRegistry and defaultSearchStrategy to support existing search behavior, and integrate it with CallClient. (#20497) #20912

Merged

cjcenizal mentioned this pull request Jul 23, 2018

Register objects to initialize and execute the Courier's refresh cycle #4921

Closed

markov00 mentioned this pull request Aug 24, 2018

fixing flaky vsualize data_table_nontimeindex test #22288

Merged

cjcenizal mentioned this pull request Aug 30, 2018

Fix regression in CallClient, which caused request errors like timeouts to result in fatal errors #22558

Merged

Add SearchStrategyRegistry and defaultSearchStrategy to support existing search behavior, and integrate it with CallClient. #20497

Add SearchStrategyRegistry and defaultSearchStrategy to support existing search behavior, and integrate it with CallClient. #20497

Conversation

cjcenizal commented Jul 5, 2018 • edited Loading

Handling errors

elasticmachine commented Jul 5, 2018

💚 Build Succeeded

elasticmachine commented Jul 5, 2018

💚 Build Succeeded

elasticmachine commented Jul 6, 2018

💔 Build Failed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jen-huang Jul 6, 2018 • edited Loading

Choose a reason for hiding this comment

cjcenizal commented Jul 6, 2018 • edited Loading

elasticmachine commented Jul 6, 2018

💚 Build Succeeded

bmcconaghy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalger left a comment

Choose a reason for hiding this comment

jen-huang Jul 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jen-huang Jul 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal commented Jul 16, 2018

spalger commented Jul 16, 2018

elasticmachine commented Jul 16, 2018

💔 Build Failed

elasticmachine commented Jul 17, 2018

💚 Build Succeeded

Choose a reason for hiding this comment

gchaps commented Jul 17, 2018

gchaps commented Jul 17, 2018

jen-huang left a comment

Choose a reason for hiding this comment

cjcenizal commented Jul 17, 2018

jen-huang left a comment • edited Loading

Choose a reason for hiding this comment

elasticmachine commented Jul 18, 2018

💔 Build Failed

elasticmachine commented Jul 18, 2018

💚 Build Succeeded

cjcenizal commented Jul 5, 2018 •

edited

Loading

jen-huang Jul 6, 2018 •

edited

Loading

cjcenizal commented Jul 6, 2018 •

edited

Loading

jen-huang Jul 16, 2018 •

edited

Loading

jen-huang Jul 16, 2018 •

edited

Loading

jen-huang left a comment •

edited

Loading