[Synonyms UI] Synonyms UI base plugin #203284

efegurkan · 2024-12-06T16:39:18Z

Summary

Creates a plugin for Synonyms UI implementation. It is hidden under the UI flag and config option which is off by default.

POST kbn:/internal/kibana/settings/searchSynonyms:synonymsEnabled
{"value": true}

Serverless Search:

Stack Search

Checklist

Check the PR satisfies following conditions.

Reviewers should verify this PR satisfies this list as well.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The release_note:breaking label should be applied in these situations.
Flaky Test Runner was used on any tests changed
The PR description includes the appropriate Release Notes section, and the correct release_note:* label is applied per the guidelines

…e-plugin

TattdCodeMonkey

It would be good to also add enabling the feature flag in this serverless config.feature_flags.ts and adding some very light FTRs with these changes as well. Just something that checks for the page to render or the link in the side nav that would give a place to build from.

You'll also want to add the app id to the apps list in the serverless FTR config.

And we would need to do the same in x-pack/test/functional/config.base.js when we want to add FTRs functional_search.

TattdCodeMonkey · 2024-12-17T18:52:30Z

x-pack/plugins/search_synonyms/public/plugin.ts

+    core: CoreSetup<AppPluginStartDependencies, SearchSynonymsPluginStart>,
+    _: AppPluginSetupDependencies
+  ): SearchSynonymsPluginSetup {
+    if (!this.config.ui?.enabled && !core.uiSettings.get<boolean>(SYNONYMS_UI_FLAG, false)) {


I'm not sure we need the config.ui for this plugin. Those were added for the other plugins to share backend without sharing the frontend. Now that these plugins should always use both I think we can leave this out.

TattdCodeMonkey · 2024-12-17T18:52:59Z

x-pack/plugins/search_synonyms/server/plugin.ts

+
+    defineRoutes({ router, logger: this.logger });
+
+    plugins.features.registerKibanaFeature({


TattdCodeMonkey · 2024-12-17T18:53:49Z

x-pack/plugins/search_synonyms/server/plugin.ts

+      privileges: {
+        all: {
+          app: ['kibana', PLUGIN_ID],
+          api: [],


we should have privileges for the APIs as well as the UI since we're handling the KibanaFeature from creation.

TattdCodeMonkey · 2024-12-17T18:55:01Z

x-pack/plugins/search_synonyms/server/plugin.ts

+          },
+          ui: [],
+        },
+        read: {


We should implement read privileges with the feature so that we don't have to add it later. We should be able to follow other plugin (not search) for examples.

This will mean that you'll need to setup APIs with correct privileges as well for read vs all.

I experimented a bit on this, I will update these with the next PR where I actually use them otherwise it will complicate. Also will include a cluster permission registration as well since we need those for the APIs.

Do you think changing these freely until we release the feature would cause a problem ? @TattdCodeMonkey

TattdCodeMonkey · 2024-12-17T18:56:26Z

x-pack/plugins/search_synonyms/server/config.ts

+
+const configSchema = schema.object({
+  enabled: schema.boolean({ defaultValue: true }),
+  ui: schema.object({


see other comment, I think we can ditch this config value.

TattdCodeMonkey · 2024-12-17T19:00:06Z

x-pack/plugins/search_synonyms/public/plugin.ts

+      deepLinks: [
+        {
+          id: 'synonyms',
+          path: PLUGIN_PATH,


PLUGIN_PATH = '/synonyms' so the full URL path is going to be /app/elasticsearch/synonyms/synonyms should that be /app/elasticsearch/synonyms/list or something else? the double /synonyms seems like it could be better.

TattdCodeMonkey · 2024-12-17T19:00:55Z

packages/deeplinks/search/constants.ts

@@ -18,6 +18,7 @@ export const SERVERLESS_ES_CONNECTORS_ID = 'serverlessConnectors';
 export const SERVERLESS_ES_WEB_CRAWLERS_ID = 'serverlessWebCrawlers';
 export const ES_SEARCH_PLAYGROUND_ID = 'searchPlayground';
 export const SERVERLESS_ES_SEARCH_INFERENCE_ENDPOINTS_ID = 'searchInferenceEndpoints';
+export const SERVERLESS_ES_SEARCH_SYNONYMS_ID = 'searchSynonyms';


I think we can drop SERVERLESS_ since this app id/deep link will be used in both serverless and stack.

Suggested change

export const SERVERLESS_ES_SEARCH_SYNONYMS_ID = 'searchSynonyms';

export const ES_SEARCH_SYNONYMS_ID = 'searchSynonyms';

afharo

Core changes LGTM

efegurkan · 2024-12-23T15:10:53Z

@elasticmachine merge upstream

…fix'

elasticmachine · 2024-12-23T21:45:04Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: e43981a

Failed CI Steps

Test Failures

[job] [logs] FTR Configs #112 / cloud_security_posture Cloud Posture Dashboard Page Kubernetes Dashboard displays accurate summary compliance score
[job] [logs] Jest Tests #19 / it should allow to add a new value

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`searchSynonyms`	-	31	+31

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`searchSynonyms`	-	5	+5

Async chunks

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

id	before	after	diff
`enterpriseSearch`	2.6MB	2.6MB	+297.0B
`searchSynonyms`	-	3.9KB	+3.9KB
total			+4.2KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`searchSynonyms`	-	4.4KB	+4.4KB
`serverlessSearch`	23.0KB	23.1KB	+150.0B
total			+4.5KB

Unknown metric groups

API count

id	before	after	diff
`searchSynonyms`	-	5	+5

async chunk count

id	before	after	diff
`searchSynonyms`	-	1	+1

ESLint disabled line counts

id	before	after	diff
`searchSynonyms`	-	3	+3

Total ESLint disabled count

id	before	after	diff
`searchSynonyms`	-	3	+3

History

💔 Build #262857 failed 9bcf377
💔 Build #262817 failed 807c206
💚 Build #261487 succeeded 7cf6cf1
💔 Build #261425 failed ba7c1f4
💔 Build #261372 failed 0706eca

Samiul-TheSoccerFan

LGTM, code reviews only

azasypkin · 2024-12-24T11:05:23Z

ACK: will review PR today, sorry for the delay

azasypkin

Overall, LGTM, but there are a number of rough edges that I think we might want to polish before merging this. Sorry if some of the suggestions feel like nitpicking, but adding a new plugin is a rare opportunity for the platform team to share knowledge and best practices with the rest of the Kibana contributors.

Thanks!

azasypkin · 2024-12-24T11:08:23Z

config/serverless.es.yml

+
+
+# Synonyms UI
+xpack.searchSynonyms.enabled: false


note: Since the plugin is disabled for all solutions/project-types, please disable it in the serverless.yml file instead of adding three identical entries in each solution-specific configuration file. This approach makes it much easier to reason about the plugin’s status in Serverless.

But, I’m not sure you even need anything in the configuration files since your plugin is already disabled by default through its configuration schema.

Suggested change

xpack.searchSynonyms.enabled: false

Yeah given that it's disabled by default, I would keep this flag out of the Serverless configs.

azasypkin · 2024-12-24T11:17:17Z

x-pack/plugins/search_synonyms/README.md

@@ -0,0 +1,3 @@
+# Search Synonyms


@gsoldevila I know we’re relocating plugins into solution-specific folders (x-pack/solutions) right now, but I don’t see a dedicated folder for the Search solution. Is the current recommendation to still place new plugins related to the Search solution within x-pack/plugins?

azasypkin · 2024-12-24T11:27:37Z

x-pack/plugins/search_synonyms/server/config.ts

+
+export const config: PluginConfigDescriptor<SearchPlaygroundConfig> = {
+  exposeToBrowser: {
+    enabled: true,


Caution
If the plugin is disabled, the client-side plugin won’t even be initialized to read this value. And if it’s initialized, that would implicitly mean it’s enabled. Why do you need to expose this to the browser? Do you happen to observe a different behavior?

We should only expose configuration to the browser if it’s absolutely necessary, as it increases both the initial payload size and the risk of accidentally exposing something that shouldn’t be exposed (these values are accessible to any user, authenticated or not).

azasypkin · 2024-12-24T11:31:03Z

x-pack/plugins/search_synonyms/server/plugin.ts

+  }
+
+  public setup(core: CoreSetup, plugins: SearchSynonymsPluginSetupDependencies) {
+    this.logger.debug('searchSynonyms: Setup');


question: is there any practical value in this log line (trying to reduce a noise in our logs when we have to enable debug to actually ... debug something 🙂)? Kibana already logs which plugins are enabled/disabled when it starts, and if plugin and all its required dependencies are enabled its setup is always called.

azasypkin · 2024-12-24T11:33:37Z

x-pack/plugins/search_synonyms/server/plugin.ts

+
+    plugins.features.registerKibanaFeature({
+      id: PLUGIN_ID,
+      name: PLUGIN_NAME,


Caution
The name is user-facing and should be a localizable string, even though it currently retains its value across different languages, as this might not always be the case.

azasypkin · 2024-12-24T11:56:04Z

x-pack/plugins/search_synonyms/public/plugin.ts

+    core: CoreSetup<AppPluginStartDependencies, SearchSynonymsPluginStart>,
+    _: AppPluginSetupDependencies
+  ): SearchSynonymsPluginSetup {
+    if (!this.config.enabled || !core.uiSettings.get<boolean>(SYNONYMS_UI_FLAG, false)) {


Caution
The core.uiSettings is deprecated

Suggested change

if (!this.config.enabled || !core.uiSettings.get<boolean>(SYNONYMS_UI_FLAG, false)) {

if (!this.config.enabled || !core.settings.get<boolean>(SYNONYMS_UI_FLAG, false)) {

azasypkin · 2024-12-24T11:58:05Z

x-pack/plugins/search_synonyms/kibana.jsonc

+    "requiredPlugins": [
+      "licensing",
+      "features",
+    ],
+    "optionalPlugins": [
+      "cloud",
+      "console",
+      "usageCollection",
+      "searchNavigation",
+    ],
+    "requiredBundles": [
+      "kibanaReact",
+    ]


Caution
Let's only add dependencies when we know we need them.

Suggested change

"requiredPlugins": [

"licensing",

"features",

],

"optionalPlugins": [

"cloud",

"console",

"usageCollection",

"searchNavigation",

],

"requiredBundles": [

"kibanaReact",

]

"requiredPlugins": [

"features"

],

"optionalPlugins": [

"searchNavigation"

],

"requiredBundles": [

"kibanaReact"

]

azasypkin · 2024-12-24T12:03:24Z

x-pack/plugins/search_synonyms/public/types.ts

+export interface AppPluginStartDependencies {
+  history: AppMountParameters['history'];
+  console?: ConsolePluginStart;
+  searchNavigation: SearchNavigationPluginStart;


Caution
This dependency is declared as optional in the manifest, but here it's defined as required (without ?).

azasypkin · 2024-12-24T12:04:24Z

x-pack/plugins/search_synonyms/public/types.ts

+import { AppMountParameters } from '@kbn/core/public';
+import type { ConsolePluginStart } from '@kbn/console-plugin/public';


optional nit: it feels a bit weird that your imports go after export that uses them...

azasypkin · 2024-12-24T12:07:14Z

x-pack/test_serverless/functional/test_suites/search/config.feature_flags.ts

@@ -37,6 +37,9 @@ export default createTestConfig({
    searchPlayground: {
      pathname: '/app/search_playground',
    },
+    searchSynonyms: {
+      pathname: '/app/search_synonyms',


Caution
Didn't you define your appRoute as /app/elasticsearch/synonyms in public/plugin.ts?

efegurkan added 6 commits December 4, 2024 18:13

Add synonyms plugin and add routes

1b9cbcd

Check config before registering

97c8d0a

Merge branch 'main' of github.com:elastic/kibana into synonyms-ui-bas…

3c4c114

…e-plugin

Update navigation on synonyms plugins

ac08dfa

Fix missing config export

0706eca

Fix quick check failures

ba7c1f4

efegurkan force-pushed the synonyms-ui-base-plugin branch from 7db5d71 to ba7c1f4 Compare December 17, 2024 14:21

efegurkan added release_note:skip Skip the PR/issue when compiling release notes v9.0.0 v8.17.0 Team:EnterpriseSearch labels Dec 17, 2024

efegurkan changed the title ~~[Synonyms UI]Synonyms UI base plugin~~ [Synonyms UI] Synonyms UI base plugin Dec 17, 2024

efegurkan marked this pull request as ready for review December 17, 2024 14:23

efegurkan requested review from a team as code owners December 17, 2024 14:23

efegurkan added backport:version Backport to applied version labels v8.18.0 and removed v8.17.0 labels Dec 17, 2024

FTR fixes

7cf6cf1

Ikuni17 approved these changes Dec 17, 2024

View reviewed changes

TattdCodeMonkey reviewed Dec 17, 2024

View reviewed changes

afharo approved these changes Dec 17, 2024

View reviewed changes

azasypkin self-requested a review December 20, 2024 14:36

Review changes

78c132c

elasticmachine and others added 3 commits December 23, 2024 15:10

Merge branch 'main' into synonyms-ui-base-plugin

88404f4

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

807c206

…fix'

Fix FTRs

9bcf377

Fix privilege FTR

e43981a

Samiul-TheSoccerFan approved these changes Dec 23, 2024

View reviewed changes

efegurkan enabled auto-merge (squash) December 24, 2024 08:13

azasypkin requested changes Dec 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Synonyms UI] Synonyms UI base plugin #203284

[Synonyms UI] Synonyms UI base plugin #203284

efegurkan commented Dec 6, 2024 •

edited

Loading

TattdCodeMonkey left a comment

TattdCodeMonkey Dec 17, 2024

TattdCodeMonkey Dec 17, 2024

TattdCodeMonkey Dec 17, 2024

TattdCodeMonkey Dec 17, 2024

efegurkan Dec 20, 2024

TattdCodeMonkey Dec 17, 2024

TattdCodeMonkey Dec 17, 2024

TattdCodeMonkey Dec 17, 2024

afharo left a comment

efegurkan commented Dec 23, 2024

elasticmachine commented Dec 23, 2024

API count

async chunk count

ESLint disabled line counts

Total ESLint disabled count

Samiul-TheSoccerFan left a comment

azasypkin commented Dec 24, 2024

azasypkin left a comment

azasypkin Dec 24, 2024

sphilipse Dec 24, 2024

azasypkin Dec 24, 2024

azasypkin Dec 24, 2024

azasypkin Dec 24, 2024

azasypkin Dec 24, 2024

azasypkin Dec 24, 2024

azasypkin Dec 24, 2024

azasypkin Dec 24, 2024

azasypkin Dec 24, 2024

azasypkin Dec 24, 2024


		defineRoutes({ router, logger: this.logger });

		plugins.features.registerKibanaFeature({

	export const SERVERLESS_ES_SEARCH_SYNONYMS_ID = 'searchSynonyms';
	export const ES_SEARCH_SYNONYMS_ID = 'searchSynonyms';

	if (!this.config.enabled \|\| !core.uiSettings.get<boolean>(SYNONYMS_UI_FLAG, false)) {
	if (!this.config.enabled \|\| !core.settings.get<boolean>(SYNONYMS_UI_FLAG, false)) {

		import { AppMountParameters } from '@kbn/core/public';
		import type { ConsolePluginStart } from '@kbn/console-plugin/public';

[Synonyms UI] Synonyms UI base plugin #203284

Are you sure you want to change the base?

[Synonyms UI] Synonyms UI base plugin #203284

Conversation

efegurkan commented Dec 6, 2024 • edited Loading

Summary

Checklist

TattdCodeMonkey 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

Choose a reason for hiding this comment

afharo left a comment

Choose a reason for hiding this comment

efegurkan commented Dec 23, 2024

elasticmachine commented Dec 23, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Page load bundle

API count

async chunk count

ESLint disabled line counts

Total ESLint disabled count

History

Samiul-TheSoccerFan left a comment

Choose a reason for hiding this comment

azasypkin commented Dec 24, 2024

azasypkin 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

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

efegurkan commented Dec 6, 2024 •

edited

Loading