Extract KQL autocomplete to a plugin by lukasolson · Pull Request #20747 · elastic/kibana

lukasolson · 2018-07-12T21:33:21Z

This PR extracts the autocomplete functionality of Kibana's query language into a plugin. We intend to add additional autocomplete providers for other languages, including those provided by a basic license like SQL.

In the new platform we will have plugin contracts, and this also paves the way to accomplishing this.

…asic

lukasolson · 2018-07-12T21:34:09Z

src/ui/public/autocomplete_providers/index.js

-function escapeSpecialCharacters(string) {
-  return string.replace(/[\\():<>"*]/g, '\\$&'); // $& means the whole matched string
-}
+const autocompleteProviders = new Map();


This is basically a registry, but I didn't actually want to use a registry because it relies on Private which would make it difficult to include in APM.

lukasolson · 2018-07-12T21:34:39Z

src/ui/public/kuery/suggestions/conjunction.js

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


This is just a rename.

lukasolson · 2018-07-12T21:35:33Z

x-pack/plugins/kuery_autocomplete/index.js

+  uiExports: {
+    hacks: ['plugins/kuery_autocomplete/hacks'],
+  },
+  init: () => {}


For some reason if I don't include this, the plugin doesn't load...

Hmm, works for me, plugins without init functions are nothing new: https://github.com/elastic/kibana/blob/master/src/core_plugins/kbn_vislib_vis_types/index.js. Maybe you mean you're not seeing the log line about its status? That's because plugins without an init() function can't actually have a status, since the only way to set it would be with an init() function.

elasticmachine · 2018-07-12T22:44:19Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

Bargs

LGTM. I tested with OSS, basic, and trial licenses to make sure the right thing happened in each case. Not 100% sure if hacks are the best way to do this, but I don't know of a better way, so I'll leave that to someone more familiar with what's happening with the plugin APIs to comment on.

epixa · 2018-07-18T21:27:40Z

I won't have time to review this before I go on vacation, but I want to make sure it gets merged/backported for 6.4 FF. @lukasolson please ask someone else to take a look at this in my stead.

bmcconaghy

LGTM, follows a similar pattern to how we've handled other xpack/oss interactions in management apps (xpack console autocomplete, rollup support). I made a couple of naming suggestions but they are not blockers.

bmcconaghy · 2018-07-19T20:32:40Z

src/ui/public/autocomplete_providers/index.js

-// See the Keyword rule in kuery.peg
-function escapeAndOr(string) {
-  return string.replace(/(\s+)(and|or)(\s+)/ig, '$1\\$2$3');
+export function setAutocompleteProvider(language, provider) {


I would suggest renaming this to "addAutocompleteProvider" to make it clear that multiple providers can be added.

👍 Agreed, I'll update this

bmcconaghy · 2018-07-19T20:38:01Z

src/ui/public/query_bar/directive/query_bar.js


-        this.suggestions = this.getRecentSearchSuggestions(query);
-        if (this.localQuery.language !== 'kuery' || !this.getKuerySuggestions) return;
+        const suggestionsProvider = getAutocompleteProvider(language);


this mismatch b/t suggestionsProvider and autocompleteProvider is weird and possibly confusing.

👍 Agreed, I'll update this

elasticmachine · 2018-07-20T22:02:12Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

lukasolson · 2018-07-20T22:19:35Z

Jenkins, test this

elasticmachine · 2018-07-20T23:44:47Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

spalger · 2018-07-21T06:03:40Z

x-pack/plugins/kuery_autocomplete/index.js

+export const kueryAutocomplete = (kibana) => new kibana.Plugin({
+  id: 'kuery_autocomplete',
+  publicDir: resolve(__dirname, 'public'),
+  require: ['kibana', 'xpack_main'],


this is unnecessary, this plugin doesn't actually require the kibana or xpack_main plugins AFAICT

spalger · 2018-07-21T06:03:50Z

x-pack/plugins/kuery_autocomplete/index.js

+  id: 'kuery_autocomplete',
+  publicDir: resolve(__dirname, 'public'),
+  require: ['kibana', 'xpack_main'],
+  config: (Joi) => Joi.object({


Also unnecessary, these are the defaults.

spalger

Things seem to work well, I sent lukasolson#15 to switch to a new uiExports type, and I think you could trim down the plugin specification a bit

JackRyanson · 2018-07-21T22:16:19Z

guys it appears to me you're removing the Kuery autocomplete from OSS and making it into xpack only is this correct?

JackRyanson · 2018-07-21T22:16:19Z

guys it appears to me you're removing the Kuery autocomplete from OSS and making it into xpack only is this correct?

JackRyanson · 2018-07-22T16:05:52Z

@Spager @lukasolson sorry to insist but it is a quite important for those that are integrating Kibana in more complex environments where the right to modify the code is key. (basic xpack might be free but there is no right to use a modified code even if one purchases the licence ). Please advice is there a roadmap of features that used to be open source and are going to get locked? if so could you please share for us to plan. thanks in advance

clintongormley · 2018-07-23T11:56:03Z

Hi @JackRyanson

You are correct - this PR moves Kuery autocomplete from an experimental feature in OSS to Basic. Regarding your question about whether there is a roadmap to move open source features to licensed, no, not at all. We have never moved a supported feature from open source to licensed. In fact, we'd only ever consider moving supported features in the opposite direction. Kuery autocomplete doesn't fall into that box because it was experimental. Usually we'd try to have the licensing decided before we first publish a feature, but we reserve the right to make changes to experimental features.

May I ask what changes you want to make to autocomplete? Are they things that would be generally useful? If so, why not try to get your changes incorporated into the released code so that they can benefit everybody?

JackRyanson · 2018-07-23T15:18:24Z

Autocomplete is pretty much the kind of things that benefits from customization in advanced integrations, the DSL might be modified e.g. with specialized functions.

Just to make sure i understand this is THE core autocomplete of kibana , just released as highlight feature in 6.3 - not available anymoe in OSS from 6.4 ? (or is it something else)
https://www.elastic.co/blog/kibana-6-3-0-released .

You talk about contributing but i dont understand : how would i possibly want to contribute to something which has a licence which prevents one from using a modified version or on modified kibana code even after paying for the licence (my current understanding of your licence) .

Just to be clear, you obviously have the right to do what you want here but i do think is in everyones interest to have a clear policy, and i believe Elastic will miss out big time if you lose the ability to flexibly integrate the stack

[uiExports] switch to new autocompleteProviders export type

elasticmachine · 2018-07-23T21:35:24Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…autocompleteBasic

elasticmachine · 2018-07-24T01:13:03Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-07-24T18:54:40Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

* fix: move autocomplete to x-pack basic * fix: apm support * fix: renames * [uiExports] switch to new autocompleteProviders export type * fix: remove unnecessary stuff from the plugin spec

lukasolson · 2018-07-24T19:34:51Z

6.x (6.4.0): 3bbb9c5

clintongormley · 2018-07-25T11:22:17Z

You talk about contributing but i dont understand : how would i possibly want to contribute to something which has a licence which prevents one from using a modified version or on modified kibana code even after paying for the licence (my current understanding of your licence) .

By contributing, your changes would be included and you'd be able to use the default distribution, plus you will have helped others.

JackRyanson · 2018-07-25T13:24:32Z

@clintongormley the default distribution is useless in advanced integration projects.

as per my previous message - "default distribution" is useless we need to integrate with other components and that goes at code level.

I am not an opensource fanboy : we are using X Pack for security/alerting/support and all, but what you're doing here is extending it to where it blocks and locks Ui capabilities which are general purpose and we need to retain the ability to customize as in "you gotta use our stock or nothing".

You guys - the elastic company i guess - moving in an integration unfriendly way locking stuff away with the excuse of "free" ==== FOSS is very worrisome and at least in our case will force us into alternatives to xpack, which frankly exist. It appears a lose lose for all.

sorenlouv · 2018-07-25T21:15:30Z

@lukasolson Kuery Bar in APM no longer works after this change.
getAutocompleteProvider('kuery') returns undefined and thus no suggestions are returned.
Let me know if I should do anything on my end to fix that.

Going forward I think it would be a good idea to notify the teams affected by a change, so they can manually test it.

lukasolson · 2018-07-25T21:18:38Z

@sqren Oh goodness, my fault, I manually tested APM up until that last commit (definitely should have tested there too...)

lukasolson added 3 commits July 11, 2018 16:08

fix: move autocomplete to x-pack basic

52697b5

Merge remote-tracking branch 'upstream/master' into fix/autocompleteB…

b06d41a

…asic

fix: apm support

d2620a3

lukasolson self-assigned this Jul 12, 2018

lukasolson requested a review from Bargs July 12, 2018 21:33

lukasolson added review :Discovery v7.0.0 v6.4.0 labels Jul 12, 2018

lukasolson commented Jul 12, 2018

View reviewed changes

src/ui/public/kuery/suggestions/conjunction.js

@@ -1,41 +0,0 @@

/*

Copy link

Contributor Author

lukasolson Jul 12, 2018

Choose a reason for hiding this comment

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

This is just a rename.

lukasolson commented Jul 12, 2018

View reviewed changes

Bargs approved these changes Jul 13, 2018

View reviewed changes

lukasolson requested a review from epixa July 18, 2018 19:06

epixa removed their request for review July 18, 2018 21:26

lukasolson requested a review from spalger July 18, 2018 22:06

bmcconaghy approved these changes Jul 20, 2018

View reviewed changes

lukasolson added 2 commits July 20, 2018 13:16

fix: renames

1956f3e

Merge branch 'master' into fix/autocompleteBasic

352a354

spalger mentioned this pull request Jul 21, 2018

[uiExports] switch to new autocompleteProviders export type lukasolson/kibana#15

Merged

spalger reviewed Jul 21, 2018

View reviewed changes

[uiExports] switch to new autocompleteProviders export type

caf318c

spalger reviewed Jul 21, 2018

View reviewed changes

Merge pull request #15 from spalger/fix/autocompleteBasic

b9dc3dc

[uiExports] switch to new autocompleteProviders export type

lukasolson added 2 commits July 23, 2018 16:44

Merge remote-tracking branch 'origin/fix/autocompleteBasic' into fix/…

e1fad8d

…autocompleteBasic

fix: remove unnecessary stuff from the plugin spec

dfde48b

Merge branch 'master' into fix/autocompleteBasic

1f16ce3

lukasolson merged commit ecc0f5e into elastic:master Jul 24, 2018

lukasolson mentioned this pull request Jul 25, 2018

Fix Kuery autocomplete in APM #21249

Merged

lukasolson deleted the fix/autocompleteBasic branch October 31, 2018 16:43

Conversation

lukasolson commented Jul 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Jul 12, 2018

💚 Build Succeeded

Uh oh!

Bargs left a comment

Choose a reason for hiding this comment

Uh oh!

epixa commented Jul 18, 2018

Uh oh!

bmcconaghy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Jul 20, 2018

💔 Build Failed

Uh oh!

lukasolson commented Jul 20, 2018

Uh oh!

elasticmachine commented Jul 20, 2018

💔 Build Failed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spalger left a comment

Choose a reason for hiding this comment

Uh oh!

JackRyanson commented Jul 21, 2018

Uh oh!

JackRyanson commented Jul 21, 2018

Uh oh!

JackRyanson commented Jul 22, 2018

Uh oh!

clintongormley commented Jul 23, 2018

Uh oh!

JackRyanson commented Jul 23, 2018

Uh oh!

elasticmachine commented Jul 23, 2018

💔 Build Failed

Uh oh!

elasticmachine commented Jul 24, 2018

💔 Build Failed

Uh oh!

elasticmachine commented Jul 24, 2018

💚 Build Succeeded

Uh oh!

lukasolson commented Jul 24, 2018

Uh oh!

clintongormley commented Jul 25, 2018

Uh oh!

JackRyanson commented Jul 25, 2018

Uh oh!

sorenlouv commented Jul 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lukasolson commented Jul 25, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

sorenlouv commented Jul 25, 2018 •

edited

Loading