[Runtime field editor] Composite runtime in Kibana Data Views by sebelga · Pull Request #110226 · elastic/kibana

sebelga · 2021-08-26T13:06:42Z

This PR adds support in Kibana Data views (former "Index pattern") for composite runtime fields.

It is the implementation of the #99462 discussion.

The problem

The new "composite" runtime fields allows a user to emit multiple values from a script. This means that when creating one runtime field of type: "composite" what the user is actually doing is creating one or multiple runtime fields behind a namespace (the name of the runtime field).

When we attach a composite runtime field to a search request:

GET my-index-000001/_search
{
  "runtime_mappings": {
    "myComposite" : {
        "type" : "composite",
        "script" : "emit('day_of_week', 'someValue'); emit('anotherField',  1234);",
        "fields" : {
            "day_of_week" : {
                "type" : "keyword"
            },
            "anotherField" : {
                "type" : "long"
            }
        }
      }
  },
  "aggs": {
    "day_of_week": {
      "terms": {
        "field": "myComposite.day_of_week"
      }
    }
  }
}

we are creating two runtime fields for the user: myComposite.day_of_week and myComposite.anotherField. The former of type keyword and the second of type long.

We need a way to store in the Kibana Data view both the composite runtime field (which is not a field but a holder of fields) and the fields themselves.

How to test

Navigate to Discover
Create a new field on the index pattern
Give a name (e.g. "log") and set the type to composite
Toggle the "Set value" and add a script

emit('field1', 'hello');
emit('field2', 'world');

Save the runtime field
The 2 fields should appear in the list of fields (under "log.field1" and "log.field2"
You should be able to select them as any other field

The 2 fields should be available in the Search bar

You should not be able to edit or delete them (work for future PR)

Future work

This PR is going to a feature branch. Future PR will add

Functional tests
Component integration tests
Server CRUD routes to Get/Create/Update/Delete composite runtime in the Data view
UI to set the format, type, custom label and popularity to sub fields
UI (modals) to indicate how to delete/edit a subfield

Fixes #99177

sebelga · 2021-08-26T13:20:26Z

src/plugins/index_pattern_field_editor/public/components/field_editor_flyout_content.tsx

+          const subField: EnhancedRuntimeField = {
+            // We hardcode "keyword" and "format" for now until the UI
+            // lets us define them for each of the sub fields
+            type: 'keyword' as const,


"type" and "format" will be defined in the UI in a following PR

sebelga · 2021-08-26T13:25:52Z

src/plugins/index_pattern_field_editor/public/open_delete_modal.tsx

+      parent === undefined ? false : options.ctx.indexPattern.getRuntimeComposite(parent) !== null;
+
+    if (doesBelongToCompositeField) {
+      console.log( // eslint-disable-line


To be done in a following PR

src/plugins/index_pattern_field_editor/public/open_editor.tsx

…ield with a parent

…o index-pattern-field-editor/composite-runtime-in-data-views

sebelga · 2021-09-06T15:07:38Z

src/plugins/data/server/index_patterns/routes/runtime_fields/create_runtime_field.ts

-      indexPattern.addRuntimeField(name, runtimeField);
-
-      const addedField = indexPattern.fields.getByName(name);
-      if (!addedField) throw new Error(`Could not create a field [name = ${name}].`);


There is no need to add this validation as adding a field will either throw (in which case the error will be displayed from that throw) or succeed.

sebelga · 2021-09-06T15:09:28Z

src/plugins/data/server/index_patterns/routes/runtime_fields/create_runtime_field.ts


      await indexPatternsService.updateSavedObject(indexPattern);

-      const savedField = indexPattern.fields.getByName(name);


Here too this validation is not necessary. We can wrap the indexPatternsService.updateSavedObject() in a try/catch but that operation should never remove the field created.

sebelga · 2021-09-06T15:17:32Z

@stacey-gammon @mattkime The PR is ready for another round of review!
I have followed Matt's suggestion and removed the need of an intermediary compositeRuntimeMap. You will also see in the types that there is just RuntimeField (no more EnhancedRuntimeField).

Why is this returning a DataViewField type?

const fields: DataViewField[] = dv.addRuntimeField(name, runtimeField);

This is to help consumers knowing which DataViewField(s) have been created when adding the runtime field. Now with composite runtime fields it could be one or a lot more.

Here you are returning a different type, you've lost the extra fields (customLabel, etc).

const rtField: RuntimeField = dv.getRuntimeField(name);

Great point, I have updated the code and we now always get back a RuntimeField (with custom label, format and popularity information)

I'd prefer to keep addRuntimeField a simple method that only appends to the runtimeFieldMap.

@mattkime That would be too much of a change on the existing architecture. Probably worth revisiting as a separate work. For now I followed what was in place, adding a runtime field will adding it to the runtimeFieldMap + creating the DataViewField(s).

sebelga · 2021-09-06T21:51:42Z

test/api_integration/apis/index_patterns/runtime_fields_crud/put_runtime_field/errors.ts

      expect(response.status).to.be(404);
    });
-
-    it('returns error on non-runtime field update attempt', async () => {


No need of this test as the lookup is in the runtimeFieldMap and not the field list, so there is no way to edit a non-runtime field through API.

sebelga · 2021-09-06T21:52:22Z

test/api_integration/apis/index_patterns/runtime_fields_crud/get_runtime_field/errors.ts

      );
    });
-
-    it('returns error when attempting to fetch a field which is not a runtime field', async () => {


No need of this test as the lookup is in the runtimeFieldMap and not the field list, so there is no way to get a non-runtime field through API.

sebelga · 2021-09-06T21:52:36Z

test/api_integration/apis/index_patterns/runtime_fields_crud/delete_runtime_field/errors.ts

      expect(response1.status).to.be(404);
    });

-    it('returns error when attempting to delete a field which is not a runtime field', async () => {


No need of this test as the lookup is in the runtimeFieldMap and not the field list, so there is no way to delete a non-runtime field through API.

mattkime · 2021-09-07T04:16:43Z

@mattkime That would be too much of a change on the existing architecture. Probably worth revisiting as a separate work. For now I followed what was in place, adding a runtime field will adding it to the runtimeFieldMap + creating the DataViewField(s).

Yes, the field list update change would be a chunk of work. What about keeping the existing functionality of addRuntimeField and placing the logic you've added into a new function?

sebelga · 2021-09-07T09:13:17Z

What about keeping the existing functionality of addRuntimeField and placing the logic you've added into a new function?

I moved the logic to create a composite runtime field to a new private method, which reduced the changes to addRuntimeField. addRuntimeField() is still the only public method to add a runtime field in the DataView for the consumer.

mattkime · 2021-09-08T03:35:43Z

I got through most of this today but want to take another look at it tomorrow after letting this sink in. Looks like some tests will need to be added, which makes sense considering some of the recent discussion we've had.

…o index-pattern-field-editor/composite-runtime-in-data-views

kibanamachine · 2021-09-08T16:53:31Z

💚 Build Succeeded

History

💚 Build #151417 succeeded 0339cdd
💚 Build #151356 succeeded f454c16
💔 Build #151338 failed dd43053
💔 Build #151336 failed 9f83024
💔 Build #151316 failed 31d735a

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

mattkime · 2021-09-08T21:15:42Z

src/plugins/data/server/index_patterns/routes/runtime_fields/get_runtime_field.ts

+          // @deprecated
+          // To avoid creating a breaking change in 7.16 we continue to support
+          // the old "field" in the response
+          field: dataViewFields[0].toSpec(),


I'm tempted to remove support for this. Its only been in 7.14 and 7.15. I'll address this separately, no reason to hold up this PR.

@mattkime - if you want to make a breaking change in 8.0, even if it's only been in 7.14 and 7.15, please reach out. With the new Make it Minor changes, we have been asked to have all breaking changes approved at team/tech lead level. REST API breaking changes are included in that (but not plugin API changes).

This is not only on 8.x, the breaking change would also ship in 7.16.

mattkime

code looks good and works well, thanks for addressing my feedback

mbondyra

@elastic/kibana-vis-editors changes LGTM, code review only.

sebelga · 2021-09-09T13:19:36Z

Thanks for the review @mattkime and @mbondyra !

* Initial commit * [Runtime field editor] Composite runtime in Kibana Data Views (#110226) * Apply updates from feature branch * Fix TS issues * Fix TS issue * Fix TS issue * Fix jest tests * fix jest tests * fix integration test * fix delete error test * partial progress * partial progress * remove mistaken change * fix import * remove unused translation * partial progress * merge * use preview api * cleanup * use specific index instead of index pattern * fix jest test * one less any * setting type on composite subfields is roughly working * partial progress * setState not working * [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix' * partial progress * working but a bit wonky * merge * [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix' * fix handing of field types, remove some console.log statements * fix initial type for subfields * fix subfield type updates, rename some vars * fix breakage from bad merge * fix types * type fixes * cleanup * i18n fix * i18n fix * i18n fix * comment cleanup * remove unused var * add code comment * remove comments * fix jest test * add start of functional test * functional test: * composite subfield preview * add functional test * functional tests * functional tests * rendering improvements * functional tests * functional tests * add jest test * add jest test * move to observables * [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix' * cleanup * better use of form lib * type fixes * cleanup * add refresh button * remove ts ignore * improve dev docs * internationalize text * type fix * delete should warn regarding subfields * typescript fix * redraws of FieldEditor would reset diff state. This fixes it. * add placeholder text to code editor * hook cleanup * add getFieldPreviewChanges jest test * add getFieldPreviewChanges jest test * keep parent name in sync with preview when changed during script update * fix test * move subfields to observables * fix jest tests * fix jest tests * fix save after field type change to composite * previewFields to behaviorSubject * fix test Co-authored-by: Sébastien Loix <sabee77@gmail.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Sébastien Loix <sebastien.loix@elastic.co>

sebelga added 20 commits August 26, 2021 13:50

Add runtimeObject to indexPattern

84ee07c

Add parent param to IndexPatternField

c136163

Block edit and delete of object subFields

a376579

Handle computedRuntimeFields and computedRuntimeObjects

14816d8

Refactor interface naming, using EnhancedRuntimeField for Kibana

ce5148b

Cleanup

6df961a

Add comment for getComputedRuntimeObjects()

94e08e7

Don't allow saving runtime objects without subfields

6209b5d

Fix TS issues

d922d41

Update jest snapshot

9b865ed

Add "composite" option and update Preview to support it

58be09d

Refactor code to use "composite" instead of "object"

f6527d4

Update IndexPattern to handle composite runtime field

3929e19

Update comments

46d907b

Update API docs

9c589e8

Fix FieldFormatConfig type

b10c61a

Update API docs

cc14ed7

Update indexPatternSpecSchema on server

85296a5

Fix component integration tests

a4e1113

Update API docs

183db29

sebelga changed the title ~~Index pattern field editor/composite runtime in data views~~ [Runtime field editor] Composite runtime in Kibana Data Views Aug 26, 2021

sebelga commented Aug 26, 2021

View reviewed changes

src/plugins/index_pattern_field_editor/public/open_editor.tsx Outdated Show resolved Hide resolved

sebelga added 3 commits August 26, 2021 14:47

Rely on types from the data plugin instead of creating new one

9c7e8d1

Rename "parent" to "parentComposite" and prevent removing a runtime f…

a3f0d32

…ield with a parent

Fix TS issue

05d13d9

sebelga mentioned this pull request Aug 26, 2021

[Index pattern] Add support for composite runtime fields #103879

Closed

Fix jest snapshot

c466d49

sebelga requested a review from mattkime August 26, 2021 14:36

sebelga added 4 commits September 6, 2021 08:25

Merge branch 'index-pattern-field-editor/composite-runtime-field' int…

94a7e37

…o index-pattern-field-editor/composite-runtime-in-data-views

Refactor to not use runtimeCompositeMap

f1f42b3

Update server side code

5462209

Fix tests

31d735a

sebelga commented Sep 6, 2021

View reviewed changes

sebelga added 3 commits September 6, 2021 16:43

Fix TS issue and API integration test

9f83024

Fix TS issue

dd43053

Fix tests

f454c16

sebelga commented Sep 6, 2021

View reviewed changes

Move logic to add composite runtime field to its own method

0339cdd

sebelga added 2 commits September 8, 2021 15:36

Remove dependency on expressions plugin

d4a75dc

Merge branch 'index-pattern-field-editor/composite-runtime-field' int…

d104bde

…o index-pattern-field-editor/composite-runtime-in-data-views

mattkime reviewed Sep 8, 2021

View reviewed changes

mattkime approved these changes Sep 9, 2021

View reviewed changes

timroes removed the request for review from a team September 9, 2021 07:42

mbondyra approved these changes Sep 9, 2021

View reviewed changes

sebelga merged commit 18ba720 into elastic:index-pattern-field-editor/composite-runtime-field Sep 9, 2021

sebelga mentioned this pull request Oct 25, 2021

[Index pattern] Add support for object runtime field "subFields" #99177

Closed

KOTungseth mentioned this pull request May 5, 2022

[DOCS] [Runtime fields] Composite runtime fields on Data Views page #131665

Closed


		await indexPatternsService.updateSavedObject(indexPattern);

		const savedField = indexPattern.fields.getByName(name);

Conversation

sebelga commented Aug 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The problem

How to test

Future work

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebelga commented Sep 6, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattkime commented Sep 7, 2021

Uh oh!

sebelga commented Sep 7, 2021

Uh oh!

mattkime commented Sep 8, 2021

Uh oh!

kibanamachine commented Sep 8, 2021

💚 Build Succeeded

History

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattkime left a comment

Choose a reason for hiding this comment

Uh oh!

mbondyra left a comment

Choose a reason for hiding this comment

Uh oh!

sebelga commented Sep 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

sebelga commented Aug 26, 2021 •

edited

Loading