[App Search] Relevance Tuning stub page and server routes #89308

JasonStoltz · 2021-01-26T15:11:15Z

Summary

This is Part 1 of Relevance Tuning.

It includes a stub page for Relevance Tuning, as well as the corresponding API server routes.

Checklist

Unit or functional tests were updated or added to match the most common scenarios

JasonStoltz · 2021-01-26T15:11:52Z

x-pack/plugins/enterprise_search/public/applications/app_search/routes.ts

@@ -41,7 +41,7 @@ export const ENGINE_CRAWLER_PATH = `${ENGINE_PATH}/crawler`;

 export const META_ENGINE_SOURCE_ENGINES_PATH = `${ENGINE_PATH}/engines`;

-export const ENGINE_RELEVANCE_TUNING_PATH = `${ENGINE_PATH}/search-settings`;
+export const ENGINE_RELEVANCE_TUNING_PATH = `${ENGINE_PATH}/relevance-tuning`;


For the view and all view components, I am using Relevance Tuning over search settings, to match how we actually present it to a user.

+10000000000000000

Quick heads up (I'll continue reminding folks as we port over pages, no worries), URLs should be snake_cased in Kibana, not kebab-cased - I have non-migrated URLs currently in kebab-case so that they still work when generating URLs to App Search.

Suggested change

export const ENGINE_RELEVANCE_TUNING_PATH = `${ENGINE_PATH}/relevance-tuning`;

export const ENGINE_RELEVANCE_TUNING_PATH = `${ENGINE_PATH}/relevance_tuning`;

JasonStoltz · 2021-01-26T15:14:43Z

x-pack/plugins/enterprise_search/server/routes/app_search/search_settings.ts

+ * you may not use this file except in compliance with the Elastic License.
+ */
+
+import { schema } from '@kbn/config-schema';


I am treating these routes as pass through. For that reason, you will see the following 3 things:

These are still named Search Settings, NOT Relevance Tuning. This is to stay 1 to 1 with the backend search settings API.

Original routes have not been modified

I am doing no more than a high level check for keys as validation. I do not want to introduce redundant schema validation, since this is already validated on the server.

I am doing no more than a high level check for keys as validation. I do not want to introduce redundant schema validation, since this is already validated on the server.

I think this is fine, but I think we should come to a consensus on how we want to handle this sort of validation universally

I'm fine with this as part of migration speed, but I'd really like to come back and do a cleanup round this post-MVP. There are a lot of issues with our current backend APIs:

Not RESTful, inconsistent naming (e.g. queries vs query in the Analytics APIs, and I still don't know what the logic was behind /detail and /collection routes)

Inconsistent casing all over the place with both sent and returned payloads, etc.

Old/outdated naming (Search Settings vs Relevance Tuning, Reference UI vs Search UI, etc. etc.)

That being said hopefully one of our future goals is to move to our public APIs anyway and not rely so much on internal ones, so who knows what the future holds.

Re: validation, ya I think we'd need to deeper dive into that as well whenever we circle back. One thing to potentially think about is that schema errors we catch in Kibana can potentially be i18n'd, whereas errors we get back from the server are always in English. I actually don't know if there's a good solution to that and we may just have to be OK with it, but it's definitely an interesting topic at least

x-pack/plugins/enterprise_search/server/__mocks__/router.mock.ts

byronhulcher

Two small change requests, I think the typescript comment is especially worth adding.

byronhulcher · 2021-01-29T13:48:42Z

x-pack/plugins/enterprise_search/public/applications/app_search/routes.ts

@@ -41,7 +41,7 @@ export const ENGINE_CRAWLER_PATH = `${ENGINE_PATH}/crawler`;

 export const META_ENGINE_SOURCE_ENGINES_PATH = `${ENGINE_PATH}/engines`;

-export const ENGINE_RELEVANCE_TUNING_PATH = `${ENGINE_PATH}/search-settings`;
+export const ENGINE_RELEVANCE_TUNING_PATH = `${ENGINE_PATH}/relevance-tuning`;


+10000000000000000

x-pack/plugins/enterprise_search/server/__mocks__/router.mock.ts

byronhulcher · 2021-01-29T13:51:07Z

x-pack/plugins/enterprise_search/server/routes/app_search/search_settings.test.ts

+    beforeEach(() => {
+      jest.clearAllMocks();
+      mockRouter = new MockRouter({
+        method: 'get',
+        path: '/api/app_search/engines/{engineName}/search_settings/details',
+      });
+
+      registerSearchSettingsRoutes({
+        ...mockDependencies,
+        router: mockRouter.router,
+      });
+    });


Naive question, but why is this in a beforeEach rather than a beforeAll? Couldn't the mock persist since it's not being changed in between it tests?

I was definitely doing more than I needed to do here. jest.clearAllMocks was redundant as I was already doing that in a beforeAll at the root.

Instantiating mockRouter only ever needs to be done once, EXCEPT in the query tests, which I had to create a new router for because it tests query validation not body, and you can only test validate on one or the other with a mock router.

registerSearchSettingsRoutes must be done in a beforeEach, because it registers a call to a mock that is required for route specs, and since mocks get cleared before every test, it will fail every test.

dcc92e3

byronhulcher · 2021-01-29T13:52:50Z

x-pack/plugins/enterprise_search/server/routes/app_search/search_settings.ts

+ * you may not use this file except in compliance with the Elastic License.
+ */
+
+import { schema } from '@kbn/config-schema';


I am doing no more than a high level check for keys as validation. I do not want to introduce redundant schema validation, since this is already validated on the server.

I think this is fine, but I think we should come to a consensus on how we want to handle this sort of validation universally

x-pack/plugins/enterprise_search/server/routes/app_search/search_settings.ts

cee-chen · 2021-01-29T18:21:43Z

Taking a break for a late breakfast, will come back here in a bit to look at the rest of the PR

...plugins/enterprise_search/public/applications/app_search/components/engine/engine_router.tsx

x-pack/plugins/enterprise_search/server/routes/app_search/search_settings.test.ts

x-pack/plugins/enterprise_search/server/__mocks__/router.mock.ts

cee-chen · 2021-01-29T20:40:16Z

Those are my last set of comments, thanks so much for your patience Jason! All my change requests are in ```suggestion blocks, everything else is optional.

JasonStoltz

Made changes

cee-chen · 2021-02-01T20:53:27Z

@elasticmachine merge upstream

cee-chen

Awesome stuff, thanks Jason! Not sure why CI is failing, I kicked off a new build and will try to keep an eye on that here

x-pack/plugins/enterprise_search/server/__mocks__/router.mock.ts

cee-chen · 2021-02-01T20:59:53Z

x-pack/plugins/enterprise_search/server/routes/app_search/search_settings.test.ts

+      const queryRouter = new MockRouter({
+        method: 'post',
+        path: '/api/app_search/engines/{engineName}/search_settings_search',
+        payload: 'query',
+      });


Hmm I think I need to rethink how I handle payload validation if we're having to create a brand new router for validating body vs query etc. It hadn't occurred to past-me that we'd have multiple types of payloads to validate. 🤦‍♀️

Basically what I'm thinking of changing API-wise is removing the payload from MockRouter instantiation and only passing it in the actual shouldValidate/shouldThrow fn. That way we'll only need to create a single mock router per route.

I'll follow up with this PR with that refactor if that sounds good to you!

Yeah I think that sounds great Constance, ty!

cee-chen · 2021-02-01T22:35:40Z

@elasticmachine merge upstream

byronhulcher

Sorry for the delay in approval, thanks for waiting @JasonStoltz

cee-chen · 2021-02-02T17:29:50Z

kibanamachine pls 😩

cee-chen · 2021-02-02T17:29:56Z

@elasticmachine merge upstream

kibanamachine · 2021-02-02T19:10:16Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 501b360

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`enterpriseSearch`	1187	1188	+1

Async chunks

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

id	before	after	diff
`enterpriseSearch`	1.8MB	1.8MB	+1.3KB

History

💔 Build #102970 failed b43820e
💚 Build #102793 succeeded afa1665
💔 Build #102759 failed ea2c50c
💔 Build #102669 failed db7e587
💔 Build #102244 failed dcc92e3

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

)

…90154)

JasonStoltz commented Jan 26, 2021

View reviewed changes

JasonStoltz added 2 commits January 26, 2021 10:15

Added server routes

9d22cbd

Added a stub Relevance Tuning page

643c874

JasonStoltz force-pushed the relevance-tuning-1 branch from aec3666 to 643c874 Compare January 26, 2021 15:16

JasonStoltz commented Jan 26, 2021

View reviewed changes

x-pack/plugins/enterprise_search/server/__mocks__/router.mock.ts Outdated Show resolved Hide resolved

JasonStoltz added release_note:skip Skip the PR/issue when compiling release notes v7.12.0 Feature:Plugins labels Jan 26, 2021

JasonStoltz marked this pull request as ready for review January 26, 2021 15:24

JasonStoltz requested review from a team January 26, 2021 15:24

JasonStoltz mentioned this pull request Jan 26, 2021

[App Search] Relevance Tuning logic - actions and selectors only, no listeners #89313

Merged

2 tasks

byronhulcher suggested changes Jan 29, 2021

View reviewed changes

JasonStoltz added 2 commits January 29, 2021 12:44

PR feedback about beforeEach

dcc92e3

Added a comment about ts-ingore

1f9879d

JasonStoltz requested a review from byronhulcher January 29, 2021 17:57