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

[Mappings editor] Add missing max_shingle_size parameter to search_as_you_type #55161

Merged
merged 4 commits into from
Jan 21, 2020
Merged

[Mappings editor] Add missing max_shingle_size parameter to search_as_you_type #55161

merged 4 commits into from
Jan 21, 2020

Conversation

sebelga
Copy link
Contributor

@sebelga sebelga commented Jan 17, 2020

This PR adds the missing max_shingle_size parameter to the search_as_you_type type.

Fix #54803

Screen Shot 2020-01-17 at 13 08 06

@sebelga sebelga added 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 v7.6.0 v8.0.0 labels Jan 17, 2020
@elasticmachine
Copy link
Contributor

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

@sebelga sebelga added the release_note:skip Skip the PR/issue when compiling release notes label Jan 17, 2020
alisonelizabeth
alisonelizabeth approved these changes Jan 17, 2020
View reviewed changes
Copy link
Contributor

@alisonelizabeth alisonelizabeth left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this @sebelga! I left one comment, but otherwise LGTM. Tested locally.

...s/index_management/public/app/components/mappings_editor/constants/parameters_definition.tsx
@@ -894,4 +894,14 @@ export const PARAMETERS_DEFINITION = {
},
schema: t.string,
},
max_shingle_size: {
Copy link
Contributor

Choose a reason for hiding this comment

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

should we define a schema here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great catch, yes indeed we should 👍

cjcenizal
cjcenizal reviewed Jan 17, 2020
View reviewed changes
...s/mappings_editor/components/document_fields/field_parameters/max_shingle_size_parameter.tsx Outdated
})}
description={i18n.translate('xpack.idxMgmt.mappingsEditor.maxShingleSizeFieldDescription', {
defaultMessage:
'The largest shingle size to index the input with and create subfields for, creating one subfield for each shingle size between 2 and the max value.',
Copy link
Contributor

Choose a reason for hiding this comment

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

I had a tricky time parsing this. What do you think of this copy Gail and I came up with?

The default is three shingle subfields. More subfields enables more specific queries, but increases index size.

Copy link
Contributor Author

@sebelga sebelga Jan 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I think it is great to strive for improving the UI help texts, but we should align them with our docs. So one is not "better" (easier to understand) than the other. This is the current text for the parameter

Screen Shot 2020-01-20 at 12 18 55

Also, I am not sure we should add the default (hardcoded) value in the translation. That would be one more place to look for if it ever changes. If we think it adds value, then better to write it in parenthesis (Default: 3) and read the value from the parameter definition constant.

[EDIT] Just to be clear, I think it is great to improve the texts to explain a complex configuration parameter. But we should also make sure to port that effort to our docs if we've found an easier way to explain how a parameter behaves.

Copy link
Contributor

Choose a reason for hiding this comment

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

I love the idea of updating our docs. What do you propose? Would you like to incorporate the change we suggested, and we'll create an issue to update the docs accordingly?

I hear you in terms of keeping the text maintainable. We have hardcoded values in our help text in various places, so this isn't the only place we'll have this problem. How about we hardcode it for now, and create an issue for universally implementing a more robust solution like the one you suggest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I love the idea of updating our docs. What do you propose? Would you like to incorporate the change we suggested, and we'll create an issue to update the docs accordingly?

Yes let's do that 👍

Regarding the default value, I haven't looked in details where/when we started using default values in text description but I think we should try as much as possible to look how we did in previous places/forms and repeat the pattern (unless we decided that was a wrong idea).

This is how we did with follower indices and I think it was a great call

Screen Shot 2020-01-21 at 10 57 15

After we change the default:

Screen Shot 2020-01-21 at 10 57 35

@sebelga
Copy link
Contributor Author

sebelga commented Jan 20, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@sebelga
Copy link
Contributor Author

sebelga commented Jan 21, 2020

Thanks for the review @alisonelizabeth and @cjcenizal !

I added the missing schema for max_shingle_size and updated the description text as suggested by CJ.

I also created an issue to update our docs with the enhance description text (https://github.com/elastic/kibana/issues/55360) and I created another issue to remove the default values hardcoded in the i18n texts (#55364)

@sebelga sebelga merged commit a20ac0d into elastic:master Jan 21, 2020
@sebelga sebelga deleted the fix/add-missing-max_shingle_size-parameter branch January 21, 2020 08:57
@sebelga sebelga mentioned this pull request Jan 21, 2020
Merged
sebelga added a commit to sebelga/kibana that referenced this pull request Jan 21, 2020
@sebelga
[Mappings editor] Add missing max_shingle_size parameter to search_as…
ec6c6ea 
…_you_type (elastic#55161)
@sebelga sebelga mentioned this pull request Jan 21, 2020
Merged
sebelga added a commit that referenced this pull request Jan 21, 2020
@sebelga
[Mappings editor] Add missing max_shingle_size parameter to search_as…
5122ac3 
…_you_type (#55161) (#55366)
sebelga added a commit that referenced this pull request Jan 21, 2020
@sebelga
[Mappings editor] Add missing max_shingle_size parameter to search_as…
7f09726 
…_you_type (#55161) (#55367)
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jan 21, 2020
@gmmorris
Merge branch 'master' into actions/platform
78e0253 
* master:
  [State Management] remove AppState from Dashboard app (elastic#54105)
  Expose fatalErrors API from the Start contract (elastic#55300)
  [BUG] Data fetching twice on discover timefilter change  (elastic#55279)
  [Mappings editor] Add missing max_shingle_size parameter to search_as_you_type (elastic#55161)
  [Logs UI] Fix z-index of logs page toolbar (elastic#54469)
  removes CTA from Task Manager info message (elastic#55334)
@sebelga sebelga mentioned this pull request Jan 31, 2020
Closed
@jrodewig jrodewig mentioned this pull request Mar 12, 2020
Merged
jrodewig added a commit to elastic/elasticsearch that referenced this pull request Mar 13, 2020
@jrodewig
[DOCS] Clarify max_shingle_size parm def (#53480)
febe7af 
Rewrites the `search_as_you_type` field datatype's `max_shingle_size`
mapping parameter to improve clarity and better communicate trade-offs
regarding index size.

Relates to [elastic/kibana#55161][0].

Closes #51774.

[0]: elastic/kibana#55161 (comment)
jrodewig added a commit to elastic/elasticsearch that referenced this pull request Mar 13, 2020
@jrodewig
[DOCS] Clarify max_shingle_size parm def (#53480)
1e31031 
Rewrites the `search_as_you_type` field datatype's `max_shingle_size`
mapping parameter to improve clarity and better communicate trade-offs
regarding index size.

Relates to [elastic/kibana#55161][0].

Closes #51774.

[0]: elastic/kibana#55161 (comment)
jrodewig added a commit to elastic/elasticsearch that referenced this pull request Mar 13, 2020
@jrodewig
[DOCS] Clarify max_shingle_size parm def (#53480)
0e495a3 
Rewrites the `search_as_you_type` field datatype's `max_shingle_size`
mapping parameter to improve clarity and better communicate trade-offs
regarding index size.

Relates to [elastic/kibana#55161][0].

Closes #51774.

[0]: elastic/kibana#55161 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjcenizal cjcenizal cjcenizal left review comments

@alisonelizabeth alisonelizabeth alisonelizabeth approved these changes

Assignees
No one assigned
Labels
bug Fixes for quality problems that affect the customer experience Feature:Mappings Editor Index mappings editor UI release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.6.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Mappings editor] Search-as-you-type missing max_shingle_size parameter
5 participants
@sebelga @elasticmachine @kibanamachine @cjcenizal @alisonelizabeth