[data views] DataViewLazy implementation phase 1 #173948

mattkime · 2023-12-25T05:14:54Z

Summary

Initial implementation of DataViewLazy class - an alternative DataView implementation that doesn't load the full field list on initial creation.

Implements DataViewLazy (variations from DataView class)
- getFields ({ mapped = true, scripted = true, runtime = true, fieldName, forceRefresh = false, unmapped, indexFilter, metaFields = true }) => 
    Promise<{ getFieldMap: () => Record<string, DataViewField>; getFieldMapSorted: () => Record<string, DataViewField> }>
- getRuntimeFields({ fieldName = ['*'] }) => Record<string, DataViewField>
- getScriptedFields({ fieldName = ['*'] }) => Record<string, DataViewField>
- getFieldByName (string) => Promise<DataViewField>
- toSpec (same params as getFields) => Promise<DataViewSpec>

other notable changes using DataViewLazy over DataView
- won't throw if index pattern fails to match
- by default toSpec doesn't return fields, must pass in parameters for requested fields

---

DataViews service additions
- toDataView: (toDataView: DataViewLazy) => Promise<DataView>;
- toDataViewLazy: (dataView: DataView) => Promise<DataViewLazy>;
- getAllDataViewLazy: () => Promise<DataViewLazy[]>;
- getDataViewLazy: (id: string) => Promise<DataViewLazy>;
- createDataViewLazy: (spec: DataViewSpec) => Promise<DataViewLazy>;
- createAndSaveDataViewLazy: (spec: DataViewSpec, override?: boolean) => Promise<DataViewLazy>;
- getDefaultDataViewLazy: () => Promise<DataViewLazy | null>;

Closes #176426 and #167750

…nto data_view_async_fields

…c_fields_new_class

jughosta

Great job, Matt!

src/plugins/data_views/common/data_views/abstract_data_views.ts

src/plugins/data_views/server/rest_api_routes/public/create_data_view.ts

src/plugins/data_views/server/rest_api_routes/public/scripted_fields/update_scripted_field.ts

...lugins/observability_solution/logs_shared/server/services/log_views/log_views_client.test.ts

jughosta

LGTM 👍

Relying on tests here.

davismcphee

I haven't made it all the way through yet and will finish up my review tomorrow, but it's looking good overall! Just a few comments so far.

src/plugins/data_views/common/types.ts

src/plugins/data_views/server/rest_api_routes/public/create_data_view.ts

src/plugins/data_views/server/rest_api_routes/public/runtime_fields/create_runtime_field.ts

src/plugins/data_views/server/rest_api_routes/public/runtime_fields/get_runtime_field.ts

jughosta · 2024-03-27T12:21:02Z

Sorry, merge conflicts are because of #178983 I introduced a new param to toMinimalSpec.

…me/kibana into data_views_async_fields_new_class

davismcphee

I made it through the rest of the code, and it's looking good overall! Left some feedback, mostly questions and a few minor suggestions.

davismcphee · 2024-03-31T19:03:09Z

src/plugins/data_views/common/data_views/data_views.ts

@@ -1078,11 +1206,11 @@ export class DataViewsService {
      overwrite,
    })) as SavedObject<DataViewAttributes>;

-    const createdIndexPattern = await this.initFromSavedObject(response, displayErrors);


Just curious why we no longer need to call this anymore?

We were recreating the dataview on save. instead, we're modifying the one we've been handed.

src/plugins/data_views/common/data_views/data_view_lazy.ts

davismcphee · 2024-04-01T01:34:53Z

src/plugins/data_views/common/data_views/data_view_lazy.ts

+        if (cachedField) {
+          cachedField.runtimeField = field.runtimeField;
+          cachedField.spec.type = castEsToKbnFieldTypeName(field.type);
+          cachedField.spec.esTypes = [field.type];
+        } else {


Just curious, why do we update cachedField from the spec when we get runtime fields?

I think you're right, this is unneeded.

davismcphee · 2024-04-01T01:36:23Z

src/plugins/data_views/common/data_views/data_view_lazy.ts

+      }
+    );
+
+    return [field];


Why do we return an array from this method?

Its consistent with adding a composite runtime field which can return more than one field.

davismcphee · 2024-04-01T02:01:12Z

src/plugins/data_views/common/data_views/data_view_lazy.ts

+      if (fld) {
+        this.fieldCache.delete(field.name);
+      }


Also curious why we delete and re-add scripted fields to the cache when we get them?

Scripted fields override mapped fields. Added a comment to the code.

src/plugins/data_views/common/data_views/data_view_lazy.ts

davismcphee · 2024-04-01T02:35:17Z

src/plugins/data_views/common/data_views/data_view_lazy.ts

+    return field ? field.type === 'date_nanos' : false;
+  }
+
+  getTimeField = () => (this.timeFieldName ? this.getFieldByName(this.timeFieldName) : undefined);


Suggested change

getTimeField = () => (this.timeFieldName ? this.getFieldByName(this.timeFieldName) : undefined);

getTimeField = async () => (this.timeFieldName ? this.getFieldByName(this.timeFieldName) : undefined);

It might be worth making this method async to return Promise<DataViewField | undefined> instead of Promise<DataViewField> | undefined which seems trickier for consumers.

They're equivalent constructs. Awaiting non promises simply returns the item being awaited.

I was thinking more if a consumer is using promises since they'd have to conditionally call .then(), but it's a minor nit and also not breaking if we decided to change it later, so I think it's fine to leave as is.

Ah, good point

src/plugins/data_views/server/rest_api_routes/public/runtime_fields/get_runtime_field.ts

…-fix'

…me/kibana into data_views_async_fields_new_class

mattkime · 2024-04-02T19:25:53Z

/ci

kibana-ci · 2024-04-02T20:40:26Z

💚 Build Succeeded

Buildkite Build
Commit: 68f0692

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`dataViews`	53	55	+2

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`data`	2601	2619	+18
`dataViews`	294	370	+76
total			+94

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`dataViewEditor`	48.6KB	48.6KB	+1.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`dataViews`	49.9KB	61.5KB	+11.6KB

Unknown metric groups

API count

id	before	after	diff
`data`	3261	3283	+22
`dataViews`	976	1069	+93
total			+115

References to deprecated APIs

id	before	after	diff
`dataViews`	26	36	+10

History

💔 Build #200642 failed 2b20647
💔 Build #200564 failed 0bf272c
💚 Build #200427 succeeded 1f8d01e
💛 Build #199059 was flaky f8df70b
💛 Build #199054 was flaky 62373eb

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

cc @mattkime

davismcphee

The latest changes LGTM, I think this is ready to go 👍 Great work, and I'm excited to see this merged so we can start using it!

davismcphee · 2024-04-03T01:10:21Z

src/plugins/data_views/common/data_views/data_view_lazy.ts

+    return field ? field.type === 'date_nanos' : false;
+  }
+
+  getTimeField = () => (this.timeFieldName ? this.getFieldByName(this.timeFieldName) : undefined);


I was thinking more if a consumer is using promises since they'd have to conditionally call .then(), but it's a minor nit and also not breaking if we decided to change it later, so I think it's fine to leave as is.

mattkime added 30 commits July 10, 2023 23:06

initial creation of abstract data view class

bda2de6

Merge branch 'main' into data_view_async_fields

cdecf14

should save scripted fields now

b9b299c

fix extracting field counts from data views

7f81e89

fix serialization of fieldAttrs, fix related tests

5175cb4

attempt to fix add scripted field

faa87ee

fix jest test

8bab954

fix scripted field removal

ff14b2f

remove unneeded changes

60ef7de

fix delete scripted field api

6c99ffb

Merge branch 'main' into data_view_async_fields

883457a

fix test

02c2e3a

restore getFieldAttrs method

6e27b8e

fix tests

0051e34

fix update scripted field rest api

e9ab9d4

remove comment

ee0f880

Merge branch 'main' into data_view_async_fields

2ebf6c6

Merge branch 'main' into data_view_async_fields

107cb29

fix toSpec functions

16389e7

Merge branch 'data_view_async_fields' of github.com:mattkime/kibana i…

2b39fcc

…nto data_view_async_fields

restore removal of count from toSpec if fields aren't included

a010c17

Merge branch 'main' into data_views_async_fields_new_class

cc0a5a8

Merge branch 'main' of github.com:elastic/kibana into data_views_asyn…

fbfd2b1

…c_fields_new_class

partial progress

e208d2e

Merge branch 'main' into data_views_async_fields_new_class

210a5f4

Merge branch 'main' into data_views_async_fields_new_class

b0047ae

Merge branch 'main' into data_views_async_fields_new_class

559a407

partial progress, some tests working

a2d820b

partial progress, some tests working

e2564ab

Merge branch 'main' into data_views_async_fields_new_class

b3a7199

add field custom description support

e6133d1

jughosta reviewed Mar 20, 2024

View reviewed changes

rename scriptedFieldMap, fix custom description

c2af6be

jughosta approved these changes Mar 21, 2024

View reviewed changes

davismcphee reviewed Mar 22, 2024

View reviewed changes

mattkime added 2 commits March 24, 2024 16:17

REST api improvements, simple type improvement

62373eb

Merge branch 'main' into data_views_async_fields_new_class

f8df70b

Merge branch 'data_views_async_fields_new_class' of github.com:mattki…

cd60de6

…me/kibana into data_views_async_fields_new_class

davismcphee reviewed Apr 1, 2024

View reviewed changes

mattkime and others added 9 commits April 1, 2024 23:23

Merge branch 'main' into data_views_async_fields_new_class

586bf5a

merge

17cf5b3

Merge branch 'main' into data_views_async_fields_new_class

d66c6ea

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

1f8d01e

…-fix'

getField improvements

23a54fb

Merge branch 'data_views_async_fields_new_class' of github.com:mattki…

0bf272c

…me/kibana into data_views_async_fields_new_class

fix jest test, add mocked function

c99aca1

simplify field getters

2b20647

add mock to jest test

68f0692

davismcphee approved these changes Apr 3, 2024

View reviewed changes

mattkime merged commit e7a3e2c into elastic:main Apr 3, 2024
18 checks passed

kibanamachine added v8.14.0 backport:skip This commit does not require backporting labels Apr 3, 2024

This was referenced Jul 12, 2024

[data views] async and partial loading of field list #152159

Closed

[Meta] Field capabilities improvements #147484

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[data views] DataViewLazy implementation phase 1 #173948

[data views] DataViewLazy implementation phase 1 #173948

mattkime commented Dec 25, 2023 •

edited

Loading

jughosta left a comment

jughosta left a comment

davismcphee left a comment

jughosta commented Mar 27, 2024

davismcphee left a comment

davismcphee Mar 31, 2024

mattkime Apr 2, 2024

davismcphee Apr 1, 2024

mattkime Apr 2, 2024

davismcphee Apr 1, 2024

mattkime Apr 2, 2024

davismcphee Apr 1, 2024

mattkime Apr 2, 2024

davismcphee Apr 1, 2024

mattkime Apr 2, 2024

davismcphee Apr 3, 2024

mattkime Apr 3, 2024

mattkime commented Apr 2, 2024

kibana-ci commented Apr 2, 2024

API count

References to deprecated APIs

davismcphee left a comment

davismcphee Apr 3, 2024

	getTimeField = () => (this.timeFieldName ? this.getFieldByName(this.timeFieldName) : undefined);
	getTimeField = async () => (this.timeFieldName ? this.getFieldByName(this.timeFieldName) : undefined);

[data views] DataViewLazy implementation phase 1 #173948

[data views] DataViewLazy implementation phase 1 #173948

Conversation

mattkime commented Dec 25, 2023 • edited Loading

Summary

jughosta left a comment

Choose a reason for hiding this comment

jughosta left a comment

Choose a reason for hiding this comment

davismcphee left a comment

Choose a reason for hiding this comment

jughosta commented Mar 27, 2024

davismcphee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattkime commented Apr 2, 2024

kibana-ci commented Apr 2, 2024

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Page load bundle

API count

References to deprecated APIs

History

davismcphee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattkime commented Dec 25, 2023 •

edited

Loading