Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 10 commits into from
Feb 4, 2021
Merged

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

merged 10 commits into from
Feb 4, 2021

Conversation

JasonStoltz
Copy link
Member

@JasonStoltz JasonStoltz commented Jan 26, 2021 •
edited
Loading

Summary

This is Part 2 of Relevance Tuning.

This is the actions and selectors of the Logic file from ent-search, no listeners have been implemented yet. That will be Part 3.

To move fast and reduce friction in the migration, this code is almost an exact replica of what was in ent-search. I have not changed the logic in any significant way other than to match the current patterns of our code. Please consider reviewing this as just that, a migration. There is much opportunity for refactor, but I don't think we need to take that on right now.

For maintainers

@JasonStoltz JasonStoltz force-pushed the relevance-tuning-2 branch 2 times, most recently from 17da8e0 to 936083d Compare January 26, 2021 16:12
JasonStoltz
JasonStoltz commented Jan 26, 2021
View reviewed changes
...ch/public/applications/app_search/components/relevance_tuning/relevance_tuning_logic.test.ts
import { RelevanceTuningLogic } from './relevance_tuning_logic';

describe('RelevanceTuningLogic', () => {
const searchSettings = {
Copy link
Member Author

@JasonStoltz JasonStoltz Jan 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Generally speaking, I changed the name and overall theme of this file to match its place it the web application, which is "Relevance Tuning", not "Search Settings".

The underlying model that gets updated and persisted is still named SearchSettings to match what we call it on the server. So you will still see the "Search Settings" name used in this file.

@JasonStoltz JasonStoltz force-pushed the relevance-tuning-2 branch 2 times, most recently from 97e3877 to e966ec9 Compare January 26, 2021 17:12
@JasonStoltz JasonStoltz added v7.12.0 release_note:skip Skip the PR/issue when compiling release notes Feature:Plugins labels Jan 26, 2021
@JasonStoltz JasonStoltz marked this pull request as ready for review January 26, 2021 17:24
@JasonStoltz JasonStoltz requested review from a team January 26, 2021 17:24
@JasonStoltz JasonStoltz mentioned this pull request Jan 27, 2021
Merged
3 tasks
byronhulcher
byronhulcher reviewed Jan 29, 2021
View reviewed changes
Copy link
Contributor

@byronhulcher byronhulcher left a comment

Choose a reason for hiding this comment

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

Generally looks good, will hold off on final approval until the parent PR is merged

...ch/public/applications/app_search/components/relevance_tuning/relevance_tuning_logic.test.ts Outdated
Comment on lines 195 to 196
// I believe this is because schemas *always* have an ID, and we don't consider that
// a tunable field
Copy link
Contributor

Choose a reason for hiding this comment

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

We should validate this with another team member and remove the ambiguity from this comment

Copy link
Member Author

Choose a reason for hiding this comment

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

True. I updated the comment: 62e9598

@byronhulcher
Copy link
Contributor

Looking forward to approving this after #89461 is merged

@JasonStoltz
Copy link
Member Author

@byronhulcher Merged #89461, this is all set

cee-chen
cee-chen reviewed Feb 3, 2021
View reviewed changes
...ch/public/applications/app_search/components/relevance_tuning/relevance_tuning_logic.test.ts
Comment on lines +195 to +196
// This is because if a schema only has a single field, it is "id", which we do not
// consider a tunable field.
Copy link
Member

Choose a reason for hiding this comment

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

💯 thanks for the explanation!

cee-chen
cee-chen approved these changes Feb 3, 2021
View reviewed changes
Copy link
Member

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

Mostly optional comments & some discussion, awesome work on this. Also I had to go back to the UI to remember when on earth we filter schema fields haha 🤦‍♀️ Would definitely super appreciate FIELD_FILTER_CUTOFF being in a constants file if/when we port that over

cee-chen
cee-chen approved these changes Feb 4, 2021
View reviewed changes
Copy link
Member

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

Changes look great! Thanks Jason!

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
enterpriseSearch 1189 1190 +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 +2.1KB

History

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

@JasonStoltz JasonStoltz merged commit 82df009 into elastic:master Feb 4, 2021
@JasonStoltz JasonStoltz deleted the relevance-tuning-2 branch February 4, 2021 19:40
gmmorris added a commit to gmmorris/kibana that referenced this pull request Feb 4, 2021
@gmmorris
Merge branch 'master' into task-manager/support-limited-concurrency
5190abf 
* master: (244 commits)
  [maps] Top hits per entity--change to title to use recent, minor edits (elastic#89254)
  [DOCS] Update installation details (elastic#90354)
  RFC for automatically generated typescript API documentation for every plugins public services, types, and functionality (elastic#86704)
  Elastic Maps Server config is `host` not `hostname` (elastic#90234)
  Use doc link services in index pattern management (elastic#89937)
  [Fleet] Managed Agent Policy (elastic#88688)
  [Workplace Search] Fix Source Settings bug  (elastic#90242)
  [Enterprise Search] Refactor MockRouter test helper to not store payload (elastic#90206)
  Use doc link service in more Stack Monitoring pages (elastic#89050)
  [App Search] Relevance Tuning logic - actions and selectors only, no listeners (elastic#89313)
  Remove UI filters from UI (elastic#89793)
  Use newfeed.service config for all newsfeeds (elastic#90252)
  skip flaky suite (elastic#85086)
  Add readme to geo containment alert covering test alert setup (elastic#89625)
  [APM] Enabling yesterday option when 24 hours is selected (elastic#90017)
  Test user for maps tests under import geoJSON tests (elastic#86015)
  [Lens] Hide column in table (elastic#88680)
  [Security Solution][Detections] Reduce detection engine reliance on _source (elastic#89371)
  [Discover] Minor cleanup (elastic#90260)
  [Search Session][Management] Rename "cancel" button and delete "Reload" button (elastic#90015)
  ...
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 89313 or prevent reminders by adding the backport:skip label.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Feb 8, 2021
@JasonStoltz JasonStoltz mentioned this pull request Feb 9, 2021
Merged
JasonStoltz added a commit to JasonStoltz/kibana that referenced this pull request Feb 9, 2021
@JasonStoltz
[App Search] Relevance Tuning logic - actions and selectors only, no …
6ef7e13 
…listeners (elastic#89313)
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Feb 9, 2021
JasonStoltz added a commit that referenced this pull request Feb 9, 2021
@JasonStoltz
[App Search] Relevance Tuning logic - actions and selectors only, no …
f4c2710 
…listeners (#89313) (#90769)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cee-chen cee-chen cee-chen approved these changes

@byronhulcher byronhulcher Awaiting requested review from byronhulcher

Assignees
No one assigned
Labels
Feature:Plugins release_note:skip Skip the PR/issue when compiling release notes v7.12.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@JasonStoltz @byronhulcher @kibanamachine @cee-chen