[GS] add application result provider #68488

pgayvallet · 2020-06-08T08:23:22Z

Summary

Create a new globalSearchProviders plugin and implement the application client-side provider.

Checklist

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

pgayvallet · 2020-06-08T09:53:18Z

src/core/public/application/types.ts

+  // remove optional on fields populated with default values
+  status: AppStatus;
+  navLinkStatus: AppNavLinkStatus;
+  appRoute: string;


Ease consumers usage by transforming optional fields into non-optional as we are injecting default values during registration.

Minor thing, but you could use the Required type, but this may be simpler to read.

pgayvallet · 2020-06-08T09:54:33Z

x-pack/plugins/global_search_providers/kibana.json

@@ -0,0 +1,10 @@
+{
+  "id": "globalSearchProviders",


Unless anyone is against it, I was planning to create a single globalSearchProviders plugin for both the application and (in the following PR) the savedObjects providers.

SGTM. Once #68811 is merged, we should group this plugin with the global_search plugin in the same directory.

Was thinking about this more and maybe this should live in the same plugin. Where do we expect the search UI code to go? Since it needs to be under the Elastic License, it must be in a plugin. I see two options:

A 3rd plugin, like global_search_ui that depends on global_search and global_search_providers

Putting everything in one plugin

cc @myasonik

🤷‍♂️ I'm a terrible person to ask in this case. I've got little context on the tradeoffs of this... Either options seems fine.

I'd maybe err towards separate plugins in case we ever want to experiment with different search strategies, it might be easier to swap between two separate plugins rather than handling it all in one.

But, really, I'm just guessing at a bunch of this so other opinions encouraged.

Was thinking about this more and maybe this should live in the same plugin. Where do we expect the search UI code to go

I was thinking of something like global_search_bar or something like that tbh.

I added a new plugin for the GS providers basically only because I could (tm) and it was maybe a slightly better isolation. But I have no strong feeling toward this. I can move the providers into the global_search plugin if we want to.

For the searchbar / search_ui, I still thinks it makes more sense to have it as a separate plugin, as it's supposed to be 'just' a consumer of the GS API. But here too, I will follow the majority if we think it's better to have everything in a single plugin.

Let's just go with separate plugins for everything and then group them in a single directory once #68811 merges.

pgayvallet · 2020-06-08T10:00:25Z

x-pack/plugins/global_search_providers/public/providers/get_app_results.ts

+      // remove disabled and chromeless app (to avoid returning the login page for example)
+      .filter((app) => app.status === 0 && (app.legacy === true || app.chromeless !== true))


I'm filtering chromeless apps because I don't think it makes any sense to find the login or logout application. I think it's acceptable to declare that our chromeless should all be considered as hidden.

If it's not the case, we would probably need to introduce an additional field in App, as there is currently no way to differenciate chromeless apps that should appear in the results from the one that shouldn't.

If we ever need to change our mind in the future, it seems easiest to assume chromeless is also hidden unless there's also some showInSearch field present too which we could add whenever the need comes up.

pgayvallet · 2020-06-08T10:03:56Z

x-pack/plugins/global_search_providers/public/providers/get_app_results.ts

+  // shortcuts to avoid calculating the distance when there is an exact match somewhere.
+  if (title === term) {
+    return 100;
+  }
+  if (title.startsWith(term)) {
+    return 90;
+  }
+  if (title.includes(term)) {
+    return 75;
+  }
+  const length = Math.max(term.length, title.length);
+  const distance = levenshtein(term, title);
+
+  // maximum lev distance is length, we compute the match ratio (lower distance is better)
+  const ratio = Math.floor((1 - distance / length) * 100);
+  if (ratio >= 60) {
+    return ratio;
+  }
+  return 0;


Note that all the scoring logic in scoreApp is totally arbitrary, as we have to implement it ourselves.

I'm still unsure about the need to rely on levenshtein distance when there is not an exact match. There are not a lot of apps, and titles are +- max 15chars, so the computation time should be negligible though.

@elastic/kibana-core-ui WDYT? Are exact (sub-)matches sufficient when searching for applications, or do you think the levenshtein scoring makes sense here?

Given that the number of apps, I suspect exact matches will be sufficient. It would be good to verify that by actually trying it out, but that's my quick take.

It doesn't seem to complicate much, so I'd rather not use exact match...

Thinking about non-English/non-native English speakers trying to use Kibana when we don't translate our app names, this seems like a nice way to catch typos.

Some examples:

Visualize vs Visualise

Machine Learning vs Mashine vs Machien (actually, thinking about it, there are like a million ways to spell machine so it still sounds right)

Swapping any hard "c" for a "k" (e.g., Discover, Canvas, Metrics)

Maybe not an immediate thing, but maybe worth opening a ticket for, it could be nice to translate the app names for search (e.g., someone translates the word "Discover" into their language and searches that, maybe we could still surface Discover then).

I haven't dug in deep, but I think our existing setup should work with translated app title strings as well. Those are translated before they enter the Core system so we should always be comparing search terms to the translated strings. Is that right @pgayvallet ?

Those are translated before they enter the Core system so we should always be comparing search terms to the translated strings

That is correct. As long as the title of the app is a translated string, we will be performing the comparison on the translated title (and would not really be able to do otherwise without adding a new field to AppBase).

elasticmachine · 2020-06-08T10:44:47Z

Pinging @elastic/kibana-platform (Team:Platform)

…ult-provider

joshdover · 2020-06-18T19:31:08Z

x-pack/plugins/global_search_providers/kibana.json

@@ -0,0 +1,10 @@
+{
+  "id": "globalSearchProviders",


Was thinking about this more and maybe this should live in the same plugin. Where do we expect the search UI code to go? Since it needs to be under the Elastic License, it must be in a plugin. I see two options:

A 3rd plugin, like global_search_ui that depends on global_search and global_search_providers

Putting everything in one plugin

cc @myasonik

joshdover · 2020-06-18T19:33:02Z

x-pack/plugins/global_search_providers/public/index.ts

+export const plugin: PluginInitializer<
+  GlobalSearchProvidersPluginSetup,
+  GlobalSearchProvidersPluginStart,
+  GlobalSearchProvidersPluginSetupDeps,
+  GlobalSearchProvidersPluginStartDeps
+> = (context) => new GlobalSearchProvidersPlugin();


nit: you could just leave these generics empty. Honestly they don't really provide any value, maybe we should just remove them from the PluginInitializer type.

My java background probably. Will remove them.

joshdover · 2020-06-18T19:38:51Z

x-pack/plugins/global_search_providers/public/providers/get_app_results.ts

+  // shortcuts to avoid calculating the distance when there is an exact match somewhere.
+  if (title === term) {
+    return 100;
+  }
+  if (title.startsWith(term)) {
+    return 90;
+  }
+  if (title.includes(term)) {
+    return 75;
+  }
+  const length = Math.max(term.length, title.length);
+  const distance = levenshtein(term, title);
+
+  // maximum lev distance is length, we compute the match ratio (lower distance is better)
+  const ratio = Math.floor((1 - distance / length) * 100);
+  if (ratio >= 60) {
+    return ratio;
+  }
+  return 0;


I haven't dug in deep, but I think our existing setup should work with translated app title strings as well. Those are translated before they enter the Core system so we should always be comparing search terms to the translated strings. Is that right @pgayvallet ?

joshdover · 2020-06-18T19:42:21Z

x-pack/plugins/global_search_providers/public/providers/application.ts

+      return from(applicationPromise).pipe(
+        mergeMap((application) => application.applications$),


I wonder if we should cache this value so we don't need to recompute on each search. Same with the filtering that is done in getAppResults for chromeless apps, etc. I know the perf impact here is probably minimal, but since this may be executed on each keypress, we should go ahead and do the easy wins if doesn't make the code harder to read.

joshdover · 2020-06-18T19:49:04Z

x-pack/plugins/global_search_providers/public/providers/get_app_results.ts

+  // maximum lev distance is length, we compute the match ratio (lower distance is better)
+  const ratio = Math.floor((1 - distance / length) * 100);
+  if (ratio >= 60) {
+    return ratio;
+  }


Would it make sense for all matches that reach this part to be ranked lower than the exact matches above? If so, we should be multiplying the ratio by 74 instead of 100 to lower the ceiling.

If we do that, we will have all 'searchable' apps returned on every search. We can tweak/weight the final ratio differently if we want to, but we will still need a score floor behind which we are excluding results.

Just impacting the final score could look like something like

const ratio = Math.floor((1 - distance / length) * 100); if (ratio >= 60) { return ratio * 0.74; }

Ah yes, that's more what I meant. Let's just keep as-is for now and we can tweak this once we have a UI to test this out with.

…ult-provider

joshdover · 2020-06-26T19:15:02Z

src/core/public/application/types.ts

+  // remove optional on fields populated with default values
+  status: AppStatus;
+  navLinkStatus: AppNavLinkStatus;
+  appRoute: string;


Minor thing, but you could use the Required type, but this may be simpler to read.

joshdover · 2020-06-26T19:16:04Z

x-pack/plugins/global_search_providers/kibana.json

@@ -0,0 +1,10 @@
+{
+  "id": "globalSearchProviders",


Let's just go with separate plugins for everything and then group them in a single directory once #68811 merges.

joshdover · 2020-06-26T19:21:39Z

x-pack/plugins/global_search_providers/public/providers/get_app_results.ts

+  // maximum lev distance is length, we compute the match ratio (lower distance is better)
+  const ratio = Math.floor((1 - distance / length) * 100);
+  if (ratio >= 60) {
+    return ratio;
+  }


Ah yes, that's more what I meant. Let's just keep as-is for now and we can tweak this once we have a UI to test this out with.

…ult-provider

kibanamachine · 2020-06-29T08:36:16Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 183c860

Build metrics

@kbn/optimizer bundle module count

id	value	diff	baseline
globalSearchProviders	3	+3	-

History

💚 Build #55476 succeeded 04d3803
💔 Build #55463 failed 3ef8b1d
💚 Build #54666 succeeded 1a52005
💚 Build #53887 succeeded 5fbb1e9
💛 Build #52996 was flaky 4b109b6

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

* add application result provider * remove empty contracts & cache searchable apps * fix types

add application result provider

be094b9

pgayvallet added release_note:skip Skip the PR/issue when compiling release notes v7.9.0 v8.0.0 labels Jun 8, 2020

pgayvallet added the Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc label Jun 8, 2020

pgayvallet commented Jun 8, 2020

View reviewed changes

pgayvallet marked this pull request as ready for review June 8, 2020 10:44

pgayvallet requested a review from a team as a code owner June 8, 2020 10:44

Merge remote-tracking branch 'upstream/master' into kbn-65633-app-res…

4757227

…ult-provider

ryankeairns mentioned this pull request Jun 8, 2020

[Meta] Kibana global header #62010

Closed

pgayvallet mentioned this pull request Jun 9, 2020

[GS] SavedObject results provider #65222

Closed

Merge remote-tracking branch 'upstream/master' into kbn-65633-app-res…

4b109b6

…ult-provider

pgayvallet added 2 commits June 15, 2020 08:23

Merge remote-tracking branch 'upstream/master' into kbn-65633-app-res…

5fbb1e9

…ult-provider

Merge remote-tracking branch 'upstream/master' into kbn-65633-app-res…

1a52005

…ult-provider

joshdover reviewed Jun 18, 2020

View reviewed changes

pgayvallet added 3 commits June 23, 2020 10:21

Merge remote-tracking branch 'upstream/master' into kbn-65633-app-res…

0454d35

…ult-provider

remove empty contracts & cache searchable apps

3ef8b1d

fix types

04d3803

joshdover approved these changes Jun 26, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into kbn-65633-app-res…

183c860

…ult-provider

pgayvallet merged commit 7e5cff4 into elastic:master Jun 29, 2020

pgayvallet added a commit to pgayvallet/kibana that referenced this pull request Jun 29, 2020

[GS] add application result provider (elastic#68488)

b4b1172

* add application result provider * remove empty contracts & cache searchable apps * fix types

pgayvallet mentioned this pull request Jun 29, 2020

[7.x] [GS] add application result provider (#68488) #70192

Merged

pgayvallet added a commit that referenced this pull request Jun 29, 2020

[GS] add application result provider (#68488) (#70192)

8fe3814

* add application result provider * remove empty contracts & cache searchable apps * fix types

Bamieh pushed a commit to Bamieh/kibana that referenced this pull request Jul 1, 2020

[GS] add application result provider (elastic#68488)

b366b90

* add application result provider * remove empty contracts & cache searchable apps * fix types

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GS] add application result provider #68488

[GS] add application result provider #68488

pgayvallet commented Jun 8, 2020 •

edited

Loading

pgayvallet Jun 8, 2020

joshdover Jun 26, 2020

pgayvallet Jun 8, 2020

joshdover Jun 11, 2020

joshdover Jun 18, 2020

myasonik Jun 18, 2020

pgayvallet Jun 23, 2020

joshdover Jun 26, 2020

pgayvallet Jun 8, 2020

myasonik Jun 11, 2020

pgayvallet Jun 8, 2020 •

edited

Loading

ryankeairns Jun 10, 2020

myasonik Jun 11, 2020

myasonik Jun 11, 2020

joshdover Jun 18, 2020

pgayvallet Jun 23, 2020

elasticmachine commented Jun 8, 2020

joshdover Jun 18, 2020

joshdover Jun 18, 2020

pgayvallet Jun 23, 2020

joshdover Jun 18, 2020

joshdover Jun 18, 2020

joshdover Jun 18, 2020

pgayvallet Jun 23, 2020

joshdover Jun 26, 2020

joshdover Jun 26, 2020

joshdover Jun 26, 2020

joshdover Jun 26, 2020

kibanamachine commented Jun 29, 2020

		// remove disabled and chromeless app (to avoid returning the login page for example)
		.filter((app) => app.status === 0 && (app.legacy === true \|\| app.chromeless !== true))

		return from(applicationPromise).pipe(
		mergeMap((application) => application.applications$),

[GS] add application result provider #68488

[GS] add application result provider #68488

Conversation

pgayvallet commented Jun 8, 2020 • edited Loading

Summary

Checklist

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgayvallet Jun 8, 2020 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Jun 8, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Jun 29, 2020

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

History

pgayvallet commented Jun 8, 2020 •

edited

Loading

pgayvallet Jun 8, 2020 •

edited

Loading