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] Add support for constant_keyword field type #76564

Conversation

alisonelizabeth
Copy link
Contributor

Fixes #69751

Release note

The mappings editor in the Index Templates UI now supports configuring the constant_keyword field type.

Screen Shot 2020-09-02 at 4 01 29 PM

@alisonelizabeth alisonelizabeth added release_note:enhancement v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more Feature:Mappings Editor Index mappings editor UI v7.10.0 labels Sep 2, 2020
@alisonelizabeth alisonelizabeth requested a review from a team as a code owner September 2, 2020 20:12
@elasticmachine
Copy link
Contributor

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

Copy link
Contributor

@debadair debadair left a comment

Choose a reason for hiding this comment

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

Left a few suggestions. I didn't look closely at the other field types, so if they are inconsistent with existing UI text, I'd err on the side of consistency. (Though it looks like an editing pass of all of them might make sense at some point.)

<p>
<FormattedMessage
id="xpack.idxMgmt.mappingsEditor.dataType.constantKeywordLongDescription"
defaultMessage="Constant keyword fields support the same queries and aggregations as {keyword} fields, but take advantage of the fact that all documents have the same value per index."
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd lead with a definition of the field type.

Suggested change
defaultMessage="Constant keyword fields support the same queries and aggregations as {keyword} fields, but take advantage of the fact that all documents have the same value per index."
defaultMessage="A special type of keyword field for fields that contain the same keyword across all documents in the index. Supports the same queries and aggregations as {keyword} fields."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We've typically started all the descriptions with the name of the field type, so I've changed this to:

Constant keyword fields are a special type of keyword field for fields that contain the same keyword across all documents in the index. Supports the same queries and aggregations as {keyword} fields."

alisonelizabeth and others added 4 commits September 3, 2020 12:53
…mappings_editor/components/document_fields/fields/field_types/constant_keyword_type.tsx

Co-authored-by: debadair <[email protected]>
…mappings_editor/components/document_fields/fields/field_types/constant_keyword_type.tsx

Co-authored-by: debadair <[email protected]>
…mappings_editor/constants/parameters_definition.tsx

Co-authored-by: debadair <[email protected]>
@alisonelizabeth
Copy link
Contributor Author

Thanks @debadair for the copy review!

Left a few suggestions. I didn't look closely at the other field types, so if they are inconsistent with existing UI text, I'd err on the side of consistency. (Though it looks like an editing pass of all of them might make sense at some point.)

👍 The field type forms did go through copy review when they were initially added, but doesn't hurt to do a second pass at some point.

@alisonelizabeth
Copy link
Contributor Author

@elasticmachine merge upstream

@sebelga
Copy link
Contributor

sebelga commented Sep 7, 2020

@elasticmachine merge upstream

sebelga
sebelga previously approved these changes Sep 7, 2020
Copy link
Contributor

@sebelga sebelga left a comment

Choose a reason for hiding this comment

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

Great work @alisonelizabeth ! I tested locally and it works as expected. The issue I mentioned in #76671 around meta does not happen here.

But I would add an additional allowArray in the isJsonField validator (true by default to not create breaking change) that would return an error if an array is provided and the option is set to false.

return JSON.stringify(value, null, 2);
},
serializer: (value: string) => {
try {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment here as in #76671. No need anymore of the try/catch

serializer: (value: string) => {
  const parsed = JSON.parse(value);
  // If an empty object was passed, strip out this value entirely.
  if (!Object.keys(parsed).length) {
    return undefined;
  }
  return parsed;
},

@sebelga
Copy link
Contributor

sebelga commented Sep 7, 2020

[EDIT]

The isue with the meta also occurs here as per my comment #76671 (comment)

It seems to occur when we use an index pattern that could also match other template, even with a totally unique field defined on the new template. And removing the meta fixes the ES error. I think that this should be investigated further.

@sebelga
Copy link
Contributor

sebelga commented Sep 7, 2020

[EDIT 2]

Actually the index pattern does not have an effect. This request fails as well

Screenshot 2020-09-07 at 12 09 45

@sebelga sebelga self-requested a review September 7, 2020 10:11
Copy link
Contributor

@sebelga sebelga left a comment

Choose a reason for hiding this comment

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

I'd like to investigate a bit further before approving as there is a very odd behavior. Maybe it is 100% on ES side, but I'd like to confirm this.

@sebelga sebelga self-requested a review September 7, 2020 10:13
@sebelga sebelga dismissed their stale review September 7, 2020 10:15

I'd like to investigate a bit further before approving as there is a very odd behavior. Maybe it is 100% on ES side, but I'd like to confirm this.

@alisonelizabeth
Copy link
Contributor Author

@sebelga thanks for the review! I believe I addressed your feedback. Can you take another look when you get a chance?

The meta field only accepts strings as values. I believe that is why you were hitting the error.

Field metadata enforces at most 5 entries, that keys have a length that is less than or equal to 20, and that values are strings whose length is less than or equal to 50.

Do you think we should enforce these rules on the client? It is unfortunate that ES doesn't return a better error message.

@sebelga
Copy link
Contributor

sebelga commented Sep 16, 2020

I will review it later today or tm morning.

The meta field only accepts strings as values. I believe that is why you were hitting the error.

I do think it'd be great to have a validator that makes sure that each value is a string. It is easy to add on our side and it could avoid a long debugging for the user to understand why his template is failing.

Maybe the reason is better explained by ES deeply nested in the error. I opened #77165 to improve the error message surfaced in the UI.

@alisonelizabeth
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

‼️ unable to find a baseline build for [master@e5b302e]. Try merging the upstream branch and trying again.

History

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

Copy link
Contributor

@sebelga sebelga 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 making the changes and adding the validation @alisonelizabeth ! Tested locally and works as expected 👍

@alisonelizabeth alisonelizabeth merged commit 9ac2fdf into elastic:master Sep 17, 2020
@alisonelizabeth alisonelizabeth deleted the feature/mappings_editor/constant_keyword_field branch September 17, 2020 13:39
alisonelizabeth added a commit to alisonelizabeth/kibana that referenced this pull request Sep 17, 2020
jloleysens added a commit to jloleysens/kibana that referenced this pull request Sep 18, 2020
…rok/new-patterns-component-use-array

* 'master' of github.com:elastic/kibana: (140 commits)
  Add telemetry as an automatic privilege grant (elastic#77390)
  [Security Solutions][Cases] Cases Redesign (elastic#73247)
  Use Search API in TSVB (elastic#76274)
  [Mappings editor] Add support for constant_keyword field type (elastic#76564)
  [ML] Adds ML modules for Metrics UI Integration (elastic#76460)
  [Drilldowns] {{event.points}} in URL drilldown for VALUE_CLICK_TRIGGER (elastic#76771)
  Migrate status & stats APIs to KP + remove legacy status lib (elastic#76054)
  use App updater API instead of deprecated chrome.navLinks.update (elastic#77708)
  [CSM Dashboard] Remove points from line chart (elastic#77617)
  [APM] Trace timeline: Replace multi-fold function icons with new EuiIcon glyphs (elastic#77470)
  [Observability] Overview: Alerts section style improvements (elastic#77670)
  Bump the Node.js version used by Docker in CI (elastic#77714)
  Upgrade all minimist (sub)dependencies to version ^1.2.5 (elastic#60284)
  Remove unneeded forced package resolutions (elastic#77467)
  [ML] Add metrics app to check made for internal custom URLs (elastic#77627)
  Functional tests - add supertest for test_user (elastic#77584)
  [ML] Adding option to create AD jobs without starting the datafeed (elastic#77484)
  Bump node-fetch to 2.6.1 (elastic#77445)
  Bump sharkdown from v0.1.0 to v0.1.1 (elastic#77607)
  [APM]fixing y axis on transaction error rate to 100% (elastic#77609)
  ...

# Conflicts:
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/manage_processor_form.container.tsx
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/manage_processor_form.tsx
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/processor_form/field_components/drag_and_drop_text_list.scss
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/processor_form/field_components/drag_and_drop_text_list.tsx
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/processor_form/field_components/text_editor.scss
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/processor_form/processors/grok.test.tsx
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Mappings Editor Index mappings editor UI release_note:enhancement Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Mappings editor] Add UI form for constant keyword datatype
5 participants