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

Add support for runtime field types to mappings editor. #77420

Merged

Conversation

cjcenizal
Copy link
Contributor

@cjcenizal cjcenizal commented Sep 14, 2020

Fixes #77110

To test, add a runtime field to an index template's mappings via the UI or via Console like so:

PUT /my-index/_mappings
{
    "properties" : {
        "day_of_week" : {
            "type" : "runtime",
            "runtime_type" : "keyword",
            "script" : {
                "source" : "emitValue(doc['timestamp'].value.dayOfWeekEnum.getDisplayName(TextStyle.FULL, Locale.ROOT))"
            }
        }
    }
}

Release note

Index templates can now be configured with runtime fields in their mappings.

image

image

- Add tests for getTypeLabelFromField util.
@cjcenizal cjcenizal added release_note:enhancement Feature:Index Management Index and index templates UI 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 14, 2020
@cjcenizal cjcenizal requested a review from a team as a code owner September 14, 2020 22:49
@elasticmachine
Copy link
Contributor

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

defaultMessage: 'Runtime',
}),
documentation: {
main: '/runtime_field.html',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that these docs don't exist yet.

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 hold off on adding a value then? Or are we confident the doc url will eventually be /runtime_field.html?

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 working on this @cjcenizal!

I left a few comments in the code.

I'm also wondering about the general workflow. For other field types that have required fields (such as alias and token_count), we require the required fields before adding it.

For example:
Screen Shot 2020-09-15 at 10 52 38 AM

I think we probably want to do the same for runtime field types. As it is, if I add a runtime field type via the UI first, I see this:

Screen Shot 2020-09-15 at 10 51 25 AM

I'm also able to proceed to the review step, but end up getting an error back from ES since runtime_type and script are both required.

For the runtime_type parameter, I think we could possibly do something similar for numeric and range types, and have a subtype dropdown.

For example:

Screen Shot 2020-09-15 at 11 16 20 AM

Let me know what you think!


export const RuntimeType = () => {
return (
<>
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: unnecessary fragment

)}
singleSelection={{ asPlainText: true }}
options={RUNTIME_FIELD_OPTIONS}
selectedOptions={value as ComboBoxOption[]}
Copy link
Contributor

Choose a reason for hiding this comment

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

It's probably more appropriate to use the EuiComboBoxOptionOption type now (and at some point remove ComboBoxOption).

defaultMessage: 'Runtime',
}),
documentation: {
main: '/runtime_field.html',
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 hold off on adding a value then? Or are we confident the doc url will eventually be /runtime_field.html?


export const getTypeLabelFromField = (field: Field) => {
// eslint-disable-next-line @typescript-eslint/naming-convention
const { type, runtime_type } = field;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just camelcase here so we don't need the eslint-disable?

const { type, runtime_type: runtimeType } = field;

</MappingsEditorProvider>
</AppContextProvider>
</Provider>
<KibanaReactContextProvider>
Copy link
Contributor

Choose a reason for hiding this comment

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

I might be missing something here, but is this necessary? I don't see it being used anywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The CodeEditor component used for editing the painless script requires it to provide uiSettings, which is really unclear from the code. I'll add a comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah. Yeah, it isn't very clear, but I see that now after looking at the code_editor code. Thanks for adding the comment!

@cjcenizal
Copy link
Contributor Author

Thanks for the great review @alisonelizabeth! I incorporated your suggestions, and added a screenshot to the PR description to show how the UI now requires you to define the type before adding a runtime field. Could you please review again?

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 making the changes @cjcenizal! This is looking great.

script also appears to be a required field. I understand the editor takes up some real estate, but I think it's important to make the user provide a value here before being allowed to submit the form. WDYT?

Also, I had a question about the copy for keyword and ip runtime types.

Screen Shot 2020-09-16 at 10 17 19 AM

Screen Shot 2020-09-16 at 10 17 37 AM

Is "...use the text data type" and "...use the IP range data type" still relevant in the context of runtime fields?

</MappingsEditorProvider>
</AppContextProvider>
</Provider>
<KibanaReactContextProvider>
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah. Yeah, it isn't very clear, but I see that now after looking at the code_editor code. Thanks for adding the comment!

@cjcenizal cjcenizal force-pushed the feature/index-templates-runtime-fields branch from 7ef7643 to 90e1445 Compare September 21, 2020 22:37
@cjcenizal
Copy link
Contributor Author

script also appears to be a required field. I understand the editor takes up some real estate, but I think it's important to make the user provide a value here before being allowed to submit the form. WDYT?

Whoops, good catch! Thanks for calling this out. I added validation. I had to surface the error message as a callout in addition to an error text, since the code editor is tall enough that it pushes the error text out of view on shorter screens. I think we need to solve this in the code editor component, e.g. by making it resize vertically to accommodate its content.

image

image

I also update a couple other places where we were using callouts similarly, to follow the patterns shown in the EUI examples.

image

image

Is "...use the text data type" and "...use the IP range data type" still relevant in the context of runtime fields?

This is a great question. It's difficult for me to answer because I have a hard time understanding the user's intention when defining runtime fields in the index template. Ostensibly, if the user is defining or editing an index template, they have some level of control over the ingestion system, which includes the source of the documents being ingested and the ingest node pipeline used to process them. If this is the case, then they might be making decisions about how best to store, say, an IP. Maybe they're still trying to figure out the right design for their ingestion system, and maybe this text helps them realize they actually do want an IP range instead of an IP? Or maybe they started with the intention to define an IP range, but they didn't realize it's not yet supported by runtime fields? 🤷 I guess I'm not entirely sure if it's helpful, but I don't see how it hurts. Do you find it confusing?

@cjcenizal
Copy link
Contributor Author

@elasticmachine merge upstream

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 making the changes @cjcenizal. Great work!

I still wonder if the script code editor should be present when initially adding the field (since it's required), but I'll leave that up to you.

(in this view:)
Screen Shot 2020-09-21 at 8 58 35 PM


This is a great question. It's difficult for me to answer because I have a hard time understanding the user's intention when defining runtime fields in the index template.

👍 I think it is fine to leave as-is. I was also wondering what the use case would look like.

@cjcenizal
Copy link
Contributor Author

@alisonelizabeth Good point. I was originally on the fence about adding the code editor to the "add field" component because I thought its height would disrupt the UX by causing content below it to jump around. But I think it's more important to be consistent with required parameters, so I added it in. I made the editor shorter, which I think helps (and also allowed me to remove the callout I had put in). I doubt these scripts will be so involved that they require as much height as I originally provided.

image

image

@cjcenizal cjcenizal force-pushed the feature/index-templates-runtime-fields branch from 00573c1 to 3ac63a6 Compare September 22, 2020 16:24
@sebelga
Copy link
Contributor

sebelga commented Sep 24, 2020

@cjcenizal Can you explain why you are not using 2 columns for the required fields? I am surprised to see a huge select occupying the whole width. I understand in smaller screens those 2 will stack on top of another but on desktop?

Or maybe have a described group with the "Emited type" on the left and the select on the right. Then the same for the script.

@cjcenizal
Copy link
Contributor Author

Great idea, @sebelga! Done.

@sebelga
Copy link
Contributor

sebelga commented Sep 25, 2020

Awesome thanks @cjcenizal !

I haven't played with it yet but maybe in the future for those scenarios, it would be nice to make use of the Resize observer and CSS (or maybe set the stack var in your case) to change the layout depending of the width of the parent.

This would allow us to create components with a layout that adapts to wherever they are thrown in.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

id value diff baseline
indexManagement 508 +5 503

async chunks size

id value diff baseline
indexManagement 1.6MB +8.6KB 1.6MB

page load bundle size

id value diff baseline
indexManagement 122.8KB +158.0B 122.6KB

History

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

@cjcenizal cjcenizal merged commit 0dc89cb into elastic:master Sep 25, 2020
@cjcenizal cjcenizal deleted the feature/index-templates-runtime-fields branch September 25, 2020 15:25
cjcenizal added a commit that referenced this pull request Sep 25, 2020
)

* Add support for runtime field types to mappings editor.
* Add tests for getTypeLabelFromField util.
* Refine callout appearance in term vector and alias parameters.
gmmorris added a commit to gmmorris/kibana that referenced this pull request Sep 28, 2020
* master: (226 commits)
  [Enterprise Search] Added Logic for the Credentials View (elastic#77626)
  [CSM] Js errors (elastic#77919)
  Add the @kbn/apm-config-loader package (elastic#77855)
  [Security Solution] Refactor useSelector (elastic#75297)
  Implement tagcloud renderer (elastic#77910)
  [APM] Alerting: Add global option to create all alert types (elastic#78151)
  [Ingest pipelines] Upload indexed document to test a pipeline (elastic#77939)
  TypeScript cleanup in visualizations plugin (elastic#78428)
  Lazy load metric & mardown visualizations (elastic#78391)
  [Detections][EQL] EQL rule execution in detection engine (elastic#77419)
  Update tutorial-full-experience.asciidoc (elastic#75836)
  Update tutorial-define-index.asciidoc (elastic#75754)
  Add support for runtime field types to mappings editor. (elastic#77420)
  [Monitoring] Usage collection (elastic#75878)
  [Docs][Actions] Add docs for Jira and IBM Resilient (elastic#78316)
  [Security Solution][Resolver] Update @timestamp formatting (elastic#78166)
  [Security Solution] Fix app layout (elastic#76668)
  [Security Solution][Resolver] 2 new functions to DAL (elastic#78477)
  Adds new elasticsearch client to telemetry plugin (elastic#78046)
  skip flaky suite (elastic#78512) (elastic#78511) (elastic#78510) (elastic#78509) (elastic#78508) (elastic#78507) (elastic#78506) (elastic#78505) (elastic#78504) (elastic#78503) (elastic#78502) (elastic#78501) (elastic#78500)
  ...
cjcenizal added a commit to cjcenizal/kibana that referenced this pull request Oct 6, 2020
cjcenizal added a commit that referenced this pull request Oct 6, 2020
cjcenizal added a commit that referenced this pull request Oct 6, 2020
cjcenizal added a commit to cjcenizal/kibana that referenced this pull request Oct 7, 2020
gmmorris added a commit that referenced this pull request Oct 8, 2020
…into feature/task_manager_429

* 'feature/task_manager_429' of github.com:elastic/kibana: (158 commits)
  Add license check to direct package upload handler. (#79653)
  [Ingest Manager] Rename API /api/ingest_manager => /api/fleet (#79193)
  [Security Solution][Resolver] Simplify CopyableField styling and add comments (#79594)
  Fine-tunes ML related text on Metrics UI (#79425)
  [ML] DF Analytics creation wizard: ensure job creation possible when model memory lower than estimate (#79229)
  Add new "Add Data" tutorials (#77237)
  Update APM telemetry docs (#79583)
  Revert "Add support for runtime field types to mappings editor. (#77420)" (#79611)
  Kibana request headers (#79218)
  ensure missing indexPattern error is bubbled up to error callout (#79378)
  Missing space fix (#79585)
  remove duplicate tab states (#79501)
  [data.ui] Lazy load UI components in data plugin. (#78889)
  Add generic type params to search dependency. (#79608)
  [Ingest Manager] Internal action for policy reassign (#78493)
  [ILM] Add index_codec to forcemerge action in hot and warm phases (#78175)
  [Ingest Manager] Update open API spec and add condition to agent upgrade endpoint (#79579)
  [ML] Hide Data Grid column options when histogram charts are enabled. (#79459)
  [Telemetry] Synchronous `setup` and `start` methods (#79457)
  [Observability] Persist time range across apps (#79258)
  ...
cjcenizal added a commit that referenced this pull request Oct 12, 2020
cjcenizal added a commit that referenced this pull request Oct 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Index Management Index and index templates UI Feature:Mappings Editor Index mappings editor UI reverted 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.

Add support for runtime fields to mappings editor
5 participants