[App Search] Fixed 2 relevance tuning bugs #94312

JasonStoltz · 2021-03-10T15:35:00Z

Summary

Bug 1: Selecting a "Functional" boost for a "Date" field will result in a "Functional boosts are not supported for Date fields..." error message.

The fix for that is here: ea0a3af

To Test:

Create an engine that has a "Date" field in its schema
Navigate to Your engine > Relevance Tuning > your date field > "Add Boost" > you should not see "Functional" listed as an option, only "Value" and "Proximity"

Bug 2: Selecting a "Functional" or "Proximity" boost and pressing save, without selecting an option, was causing an error.

The root cause was that when creating new Functional and Proximity boosts in state, we were not initializing all required fields of the boost. So when we attempt to post it to the server, the server was rejecting it as invalid

As it turns out, the reason we missed this on the initial implementation, is because that initialization was actually happening in the view, rather than Kea in the old repository. I.e., when the view was initialized, it was actually mutating the newly created boosts in state directly, rather than using a kea action. This is a pretty clear anti-pattern. My fix for this is to initialize the missing values as part of the "addBoost" action in Kea: 05ccd18. https://github.com/elastic/kibana/pull/94312/files#diff-8f932314a95e85fb9e58bdfb28af437b2d3be46a00caa15052d09619f96bf26eR373

Note that as part of this, I refactored the Boost types a bit to make the schema more clear. There is now a parent Boost type as well as child FunctionalBoost, ValueBoost, and ProximityBoost types, to make it clear which Boost fields are used for which boost types.

To test:

Create an engine that has a "Number" field in its schema
Navigate to Your engine > Relevance Tuning > your number field > "Add Boost" > "Functional"
Observe that the Functional boost is created in the UI with the dropdowns pre-populated.
Use the "Save" button to save the new boost.
You should not see an error.
You should see that the Functional boost is still on the page and was persisted.
Repeat the previous steps for a Proximity boost (note that you will need to enter a "center" value in order to save this one properly). The center value is not pre-populated.

Checklist

Delete any items that are not applicable to this PR.

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

For maintainers

This was checked for breaking API changes and was labeled appropriately

kibanamachine · 2021-03-10T17:34:42Z

💚 Build Succeeded

Metrics [docs]

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.1MB	2.1MB	+739.0B

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

cee-chen

Thanks so much for catching these bugs Jason, and partial apologies on my end for not QAing more thoroughly! I can confirm the date select option & the Functional boost bugs being fixed.

As a heads up I can't quite confirm the Proximity bug though - it appears to be working for me as expected on master. If I create a Proximity boost and don't enter or change anything and just hit saves, it errors that it's missing a center (which I would expect). If I create a new Proximity boost & enter a center but don't click anything else, it saves without errors.

That behavior is the same before/after this branch and seems correct to me so I'm not concerned / approving in any case, but just wanted to confirm my understanding of that behavior with you.

cee-chen · 2021-03-10T18:58:20Z

...lugins/enterprise_search/public/applications/app_search/components/relevance_tuning/types.ts

-  type: BoostType;
-  newBoost?: boolean;
-  center?: string | number;
+export interface RawBoost extends Omit<Boost, 'value'> {


💯 Awesome use of omit!

cee-chen · 2021-03-10T19:11:35Z

...lugins/enterprise_search/public/applications/app_search/components/relevance_tuning/types.ts

  operation?: BoostOperation;
  function?: BoostFunction;
+  newBoost?: boolean;
+  center?: string | number;
+  factor: number;
+  value?: string[];


(just curious, feel free to stop me if I'm missing something)

Do we really need this base Boost type any longer if the more specific Boost types below are overriding value, function, and operation in any case? 🤷

I think the reason it's nice to have that is for when we're passing a boost around in the general sense, and don't really care what type of boost it is.

Like in a reducer I can accept any Boost as a prop and check its newBoost prop. I don't really care what type of Boost it is, I just care that it's a Boost, and all Boosts have a newBoost prop. If I need to do something FunctionalBoost specific, I can cast it to FunctionalBoost.

It's kind of like a classic inheritance thing, right?

Gotcha, totally makes sense! Thanks for taking the time to explain!

cee-chen · 2021-03-10T19:13:20Z

..._search/public/applications/app_search/components/relevance_tuning/relevance_tuning_logic.ts

    value: BoostOperation | BoostFunction
  ): {
    name: string;
    boostIndex: number;
-    optionType: keyof BaseBoost;
+    optionType: keyof Pick<Boost, 'operation' | 'function'>;


Do you mind walking me through what the output of this actually looks like? I think I'd get both keyof and Pick separately, but that + | is tripping me up. Just want to make sure I understand what this compiles to!

Boost:

{ operation?: BoostOperation; function?: BoostFunction; type: BoostType; newBoost?: boolean; center?: string | number; factor: number; ... }

Pick<Boost, 'operation' | 'function'>:

{ operation?: BoostOperation; function?: BoostFunction; }

keyof Pick<Boost, 'operation' | 'function'>

'operation' | 'function'

So it's a stupid long way of writing 'operation' | 'function'

Whatever is passed in optionType is ultimately used to update the boost by that key...

optionsType = 'operation' boost[optionType] = whatever

So I wanted to make it clear that there is an association between optionType and keys within a boost.

Writing them as follows just makes them feel a bit like magic strings...

optionType: 'operation' | 'function'

But for simplicity maybe that would be preferable.

Sorry, I realize this is confusing.

What do you think, should I simplify this in my next PR?

Ahhhh whoa I was way off in my guess on what that compiled to lol. I think I'd opt for simplicity if that's OK with you - it doesn't feel too magical to me / feels like a more straightforward string enum, but maybe I'm missing something 🤔

cee-chen · 2021-03-10T19:14:33Z

..._search/public/applications/app_search/components/relevance_tuning/relevance_tuning_logic.ts

@@ -376,7 +370,7 @@ export const RelevanceTuningLogic = kea<
    addBoost: ({ name, type }) => {
      const { searchSettings } = values;
      const { boosts } = searchSettings;
-      const emptyBoost = { type, factor: 1, newBoost: true };
+      const emptyBoost = BOOST_TYPE_TO_EMPTY_BOOST[type];


Just to confirm my understanding: this is essentially the fix, correct? (well, + the correct empty const definitions above)

Yes, sorry, there is a busy PR.

kibanamachine · 2021-03-10T19:39:31Z

💚 Backport successful

✅ 7.x / #94357

Successful backport PRs will be merged automatically after passing CI.

Co-authored-by: Jason Stoltzfus <[email protected]>

…-action * 'master' of github.com:elastic/kibana: (43 commits) [Console] Update copy when showing warnings in response headers (#94270) [TSVB] Type public code. Step 1 (#93231) [ML] Functional tests - stabilize slider value selection (#94313) skip another suite blocking es promotion (#94367) [Security Solution] Eliminates a redundant external link icon (#94194) skip another suite blocking es promotion (#94367) [App Search] Role mappings migration part 1 (#94346) [Security Solution][Detections] Fix flaky indicator enrichment tests (#94241) [Workplace Search] Deduplicate icons (#94359) [ML] Add latest transform to intro text (#94039) skip test failing es promotion (#94367) [Maps] convert elasticsearch_utils to TS (#93984) [Security_Solution][Telemetry] - Update endpoint usage to use agentService (#93829) [Security Solution][Exceptions] Fixes OS adding method for exception enrichment (#94343) [ILM] Add support for frozen phase (#93068) [App Search] Fixed 2 relevance tuning bugs (#94312) remove `try` auth mode (#94287) Removing resolver functional tests (#94331) migrate warning mixin to core (#94273) [App Search] Add routes for Role Mappings (#94221) ... # Conflicts: # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/cold_phase/cold_phase.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/phase/phase.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/shared_fields/searchable_snapshot_field/searchable_snapshot_field.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/edit_policy.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/form/configuration_issues_context.tsx

JasonStoltz added 2 commits March 10, 2021 10:21

Hide Functional boost for Date types

ea0a3af

Fixed bug and reworked types

05ccd18

JasonStoltz requested a review from a team March 10, 2021 15:35

JasonStoltz changed the title ~~[App Search] Fix 2 relevance tuning bugs~~ [App Search] Fixed 2 relevance tuning bugs Mar 10, 2021

JasonStoltz added v7.13.0 release_note:skip Skip the PR/issue when compiling release notes auto-backport Deprecated - use backport:version if exact versions are needed labels Mar 10, 2021

JasonStoltz enabled auto-merge (squash) March 10, 2021 15:37

cee-chen approved these changes Mar 10, 2021

View reviewed changes

JasonStoltz merged commit a198744 into elastic:master Mar 10, 2021

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Mar 10, 2021

[App Search] Fixed 2 relevance tuning bugs (elastic#94312)

07b2eeb

kibanamachine mentioned this pull request Mar 10, 2021

[7.x] [App Search] Fixed 2 relevance tuning bugs (#94312) #94357

Merged

kibanamachine added a commit that referenced this pull request Mar 10, 2021

[App Search] Fixed 2 relevance tuning bugs (#94312) (#94357)

f37eafd

Co-authored-by: Jason Stoltzfus <[email protected]>

JasonStoltz deleted the filter-invalid-boost branch March 11, 2021 15:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[App Search] Fixed 2 relevance tuning bugs #94312

[App Search] Fixed 2 relevance tuning bugs #94312

JasonStoltz commented Mar 10, 2021 •

edited

Loading

kibanamachine commented Mar 10, 2021

cee-chen left a comment

cee-chen Mar 10, 2021

cee-chen Mar 10, 2021

JasonStoltz Mar 10, 2021 •

edited

Loading

cee-chen Mar 10, 2021

cee-chen Mar 10, 2021

JasonStoltz Mar 10, 2021 •

edited

Loading

JasonStoltz Mar 10, 2021

cee-chen Mar 10, 2021

cee-chen Mar 10, 2021

JasonStoltz Mar 10, 2021

kibanamachine commented Mar 10, 2021

[App Search] Fixed 2 relevance tuning bugs #94312

[App Search] Fixed 2 relevance tuning bugs #94312

Conversation

JasonStoltz commented Mar 10, 2021 • edited Loading

Summary

Checklist

For maintainers

kibanamachine commented Mar 10, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

cee-chen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JasonStoltz Mar 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JasonStoltz Mar 10, 2021 • 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

kibanamachine commented Mar 10, 2021

💚 Backport successful

JasonStoltz commented Mar 10, 2021 •

edited

Loading

JasonStoltz Mar 10, 2021 •

edited

Loading

JasonStoltz Mar 10, 2021 •

edited

Loading