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.

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

[Mappings editor] Missing "positive_score_impact" parameter in rank feature field #76258

Closed
sebelga opened this issue Aug 31, 2020 · 4 comments · Fixed by #76824
Closed

[Mappings editor] Missing "positive_score_impact" parameter in rank feature field #76258

sebelga opened this issue Aug 31, 2020 · 4 comments · Fixed by #76824
Labels
bug Fixes for quality problems that affect the customer experience Feature:Mappings Editor Index mappings editor UI Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more

Comments

@sebelga
Copy link
Contributor

sebelga commented Aug 31, 2020

As per the documentation: https://www.elastic.co/guide/en/elasticsearch/reference/master/rank-feature.html, the rank_feature field type accepts a positive_score_impact parameter that we don't surface in the UI

Screenshot 2020-08-31 at 10 17 38

@sebelga sebelga added the Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more label Aug 31, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/es-ui (Team:Elasticsearch UI)

@sebelga sebelga added bug Fixes for quality problems that affect the customer experience Feature:Mappings Editor Index mappings editor UI labels Aug 31, 2020
@cjcenizal
Copy link
Contributor

@sebelga Great find! Do you know if we create an index template with this parameter in Console, and then edit and save it with the UI, is the parameter stripped out or preserved? If it's stripped out, I would consider that a deeper bug that we should fix in order to future-proof the UI against new parameters that might get added to the API.

@sebelga
Copy link
Contributor Author

sebelga commented Sep 1, 2020

It will be stripped out if you edit the field and click the "Update" button.

The current behavior is:

  1. Read the field object with its parameters (here the positive_score_impact appears)
  2. Form fields in the JSX try to access those parameters. (here no field tries to access the positive_score_impact parameters)
  3. When we switch type, old fields get removed, and new ones appear in the DOM. They all try to access the parameters from point 1.

We can not simply keep unknown parameters and merge them at the end as the user might have switched field type and those parameters are invalid. Or add them blindly even with the same type as we don't know exactly what the user wants to do.

So the fix here is to add the missing number field in the form.

in order to future-proof the UI against new parameters that might get added to the API

This is the problem I raised multiple times when building the mappings editor.

With what I said above I don't think it is the UI that needs (can) to be future-proof. I think we need a system in place (tests?) that break the build when new parameters are added to a field from ES. Maybe the new TS typings will help us do that. Currently, the easiest "system" is: make sure we are aware (being pinged by ES engineers) of new parameters being added or removed (or allowing new values in selects for ex.).

@sebelga
Copy link
Contributor Author

sebelga commented Sep 2, 2020

From the last email from Adrien, it seems that the rank_feature type has only 0.1% of adoption. So this is a very low-risk issue (who knows what it has been rounded to 0.1 from?).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:Mappings Editor Index mappings editor UI Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants