Index patterns - Server API #69105

mattkime · 2020-06-13T02:35:09Z

Summary

Use async interface for uiSettings - provide public and server shims to use common interface. Pre-cache values for sync calls such as field formatters.
IndexPatternsApiClient - shim, passing in as argument - mocked on server since client implementation makes http calls. Will throw error if used on server. used for updating field list _get_fields_for_wildcard Will fully implement later.
SavedObjects client shim - Works with the server's SavedObject rather than SimpleSavedObject
plugin functional test verifies functionality

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

This reverts commit 5de1b21.

This reverts commit ebc56d8.

elasticmachine · 2020-06-27T01:54:39Z

Pinging @elastic/kibana-app-arch (Team:AppArch)

mattkime · 2020-06-29T04:12:51Z

@elasticmachine merge upstream

ppisljar

code LGTM

ppisljar · 2020-06-29T12:04:31Z

src/plugins/data/common/index_patterns/index_patterns/index_patterns.ts

+  async specToIndexPattern(spec: IndexPatternSpec) {
+    const shortDotsEnable = await this.config.get(UI_SETTINGS.SHORT_DOTS_ENABLE);
+    const metaFields = await this.config.get(UI_SETTINGS.META_FIELDS);
+    const uiSettingsValues = await this.config.getAll();


i guess shortDotsEnable and metaFields are also part of getAll ?
i would prefer not doing getAll at all and rather identify the specific settings that are required if possible ?

Currently FieldFormatters can access any uiSetting. getAll is used to support them. We should look at refactoring FieldFormatters so they're not dependent on being passed a dependency like this.

i would prefer not doing getAll at all and rather identify the specific settings that are required if possible ?

We should look at refactoring FieldFormatters so they're not dependent on being passed a dependency like this.

++ To both of these ideas. Ideally we would only include the subset of uiSettings which are actually needed.

One thing I'm still not sure about is whether we are going to run into issues caching the uiSettingsValues like this. What happens if someone updates a uiSetting (on the client) after we have already cached the settings with getAll? Now they will be getting an outdated value. Whereas in the previous implementation (with getConfig) we were calling uiSettings.get each time, which guaranteed you would always be getting the latest value.

Looking at index_pattern.ts it seems getConfig is only called once in the constructor which means this wouldn't be an issue anywhere except in deserializeFieldFormatMap where the cached values are passed to a new FieldFormat instance -- so does this mean that the field formats will all now have a cached version of the ui settings which could potentially be stale?

I'm wondering if instead this cache should be handled internally in the ui settings client/public wrapper, and then subscribe to the uiSettings.getSaved$ observable which (I believe) will broadcast an update any time a setting is updated. This could then be used to update the cached retrieved with getAll:

class UiSettingsPublicToCommon { constructor() { this.cache = core.uiSettings.getAll(); this.subscription = core.uiSettings.getSaved$.subscribe(({ key, newValue, oldValue }) => { this.cache[key] = newValue; }); } get() { return Promise.resolve(this.cache[val]); } stop() { // cleanup which would need to be called in the plugin's `stop` this.subscription.unsubscribe(); } }

ppisljar · 2020-06-29T12:05:46Z

src/plugins/data/public/index_patterns/saved_objects_client_wrapper.ts

+  ...omit(simpleSavedObject, '_version'),
+});
+
+export class SavedObjectsClientPublicToCommon implements SavedObjectsClientCommon {


have you checked with platform team about this ?

Yes, they've confirmed that its the correct strategy and that more complete parity between APIs would take a fair amount more work.

mattkime · 2020-06-29T14:16:44Z

.../plugins/maps/public/classes/layers/solution_layers/security/security_index_pattern_utils.ts


  const indexPatternCache = await getIndexPatternService().getCache();
  return indexPatternCache!
-    .filter((savedObject: SimpleSavedObject<IndexPatternSavedObjectAttrs>) => {


now uses SavedObject instead of SimpleSavedObject - type info is available without specifying.

mattkime · 2020-06-29T14:16:51Z

...tics/pages/analytics_management/hooks/use_create_analytics_form/use_create_analytics_form.ts

      const indexPatternsMap: SourceIndexMap = {};
      const savedObjects = (await mlContext.indexPatterns.getCache()) || [];
-      savedObjects.forEach((obj: SimpleSavedObject<Record<string, any>>) => {
+      savedObjects.forEach((obj) => {


now uses SavedObject instead of SimpleSavedObject - type info is available without specifying.

nreese

maps changes LGTM
code review, tested in chrome

…bana into index_pattern_server_exposed

darnautov

ML changes LGTM

kibanamachine · 2020-06-29T18:05:37Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: dd711d2

Build metrics

@kbn/optimizer bundle module count

id	value	diff	baseline
data	208	-1	209

History

💚 Build #57007 succeeded 0fb5f64
💚 Build #56923 succeeded f85f991

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

lukeelmers

Overall code LGTM! I just have one question about caching the uiSettings on the client that we should probably sort out, as I'm not sure how big of an issue it might be.

Plugin functional tests were a nice touch, thanks for adding those! 🆒

lukeelmers · 2020-06-30T02:03:25Z

src/plugins/data/common/index_patterns/types.ts

 */

 import { ToastInputFields, ErrorToastOptions } from 'src/core/public/notifications';
+// eslint-disable-next-line


Is this disabled because you are importing from server into common and eslint doesn't realize this is a type which will be compiled away?

lukeelmers · 2020-06-30T02:20:55Z

src/plugins/data/common/index_patterns/index_patterns/index_patterns.ts

+  async specToIndexPattern(spec: IndexPatternSpec) {
+    const shortDotsEnable = await this.config.get(UI_SETTINGS.SHORT_DOTS_ENABLE);
+    const metaFields = await this.config.get(UI_SETTINGS.META_FIELDS);
+    const uiSettingsValues = await this.config.getAll();


i would prefer not doing getAll at all and rather identify the specific settings that are required if possible ?

We should look at refactoring FieldFormatters so they're not dependent on being passed a dependency like this.

++ To both of these ideas. Ideally we would only include the subset of uiSettings which are actually needed.

One thing I'm still not sure about is whether we are going to run into issues caching the uiSettingsValues like this. What happens if someone updates a uiSetting (on the client) after we have already cached the settings with getAll? Now they will be getting an outdated value. Whereas in the previous implementation (with getConfig) we were calling uiSettings.get each time, which guaranteed you would always be getting the latest value.

Looking at index_pattern.ts it seems getConfig is only called once in the constructor which means this wouldn't be an issue anywhere except in deserializeFieldFormatMap where the cached values are passed to a new FieldFormat instance -- so does this mean that the field formats will all now have a cached version of the ui settings which could potentially be stale?

I'm wondering if instead this cache should be handled internally in the ui settings client/public wrapper, and then subscribe to the uiSettings.getSaved$ observable which (I believe) will broadcast an update any time a setting is updated. This could then be used to update the cached retrieved with getAll:

class UiSettingsPublicToCommon { constructor() { this.cache = core.uiSettings.getAll(); this.subscription = core.uiSettings.getSaved$.subscribe(({ key, newValue, oldValue }) => { this.cache[key] = newValue; }); } get() { return Promise.resolve(this.cache[val]); } stop() { // cleanup which would need to be called in the plugin's `stop` this.subscription.unsubscribe(); } }

mattkime · 2020-06-30T03:19:24Z

@lukeelmers

Now they will be getting an outdated value. Whereas in the previous implementation (with getConfig) we were calling uiSettings.get each time, which guaranteed you would always be getting the latest value.

This is determined by the lifecycle of uiSettings and the indexPattern class. Calling getConfig in the browser doesn't necessarily get the latest value, it gets the value currently in the browser. Also, I'm not sure its possible to change the value in the browser without reloading the index pattern which would flush the cached uiSettings copy.

so does this mean that the field formats will all now have a cached version of the ui settings which could potentially be stale?

I think it would be good to come up with a case where that could happen. I can't think of one but you might.

Might be worth talking to platform about the lifecycle of uiSettings.

lukeelmers

I think it would be good to come up with a case where that could happen. I can't think of one but you might.

I'm going to look into this further, but I don't see a reason why it needs to block merging this PR, especially because it may be a non-issue when #69379 is done anyway.

If I find a concrete test case for a regression, I can create a separate issue to track it, or possibly resolve the issue in the course of the aggs migration work I'm doing, which will also need a shared uiSettings implementation.

mattkime · 2020-06-30T15:13:37Z

fwiw the platform team confirmed my understanding of uiSettings value changes.

lukeelmers · 2020-06-30T16:41:47Z

I'm not sure its possible to change the value in the browser without reloading the index pattern which would flush the cached uiSettings copy.

I've looked into this further, and so far I haven't found a case where index patterns aren't reloaded upon navigating away from advanced settings. So even if the cache is technically stale, it won't matter since it gets flushed before users see anything, so folks are not likely to ever see this inconsistency.

* index patterns on the server # Conflicts: # docs/development/plugins/data/public/kibana-plugin-plugins-data-public.indexpattern._constructor_.md # docs/development/plugins/data/public/kibana-plugin-plugins-data-public.indexpattern.md # src/plugins/data/common/index_patterns/index_patterns/index_pattern.test.ts # src/plugins/data/common/index_patterns/index_patterns/index_pattern.ts # src/plugins/data/common/index_patterns/index_patterns/index_patterns.ts # src/plugins/data/common/index_patterns/types.ts # src/plugins/data/public/index_patterns/index_patterns/index.ts # src/plugins/data/public/plugin.ts # src/plugins/data/public/public.api.md

* index patterns on the server

mattkime and others added 30 commits June 4, 2020 11:54

partial progress

a2da8d3

front end code should be working again

3a40208

restore unknown field toast

cb9e775

fix jest test mock

5de1b21

Revert "fix jest test mock"

2683b7a

This reverts commit 5de1b21.

fix jest test mock

2913835

add onError callback, remove comments

5cfaee5

Merge branch 'master' into index_pattern_server_api_2

f963496

better define fieldFormats common start service

ec2f0c6

dependency cleanup

ebc56d8

Revert "dependency cleanup"

c994a1f

This reverts commit ebc56d8.

update api docs

f097b31

remove commented lines

6998499

typefix

01e6e2f

refactor core usage

f75873f

Merge branch 'master' into index_pattern_server_api_2

49a06d7

Merge branch 'master' into index_pattern_server_api_2

ba4d131

more specific dependencies

30bf1e0

Merge branch 'master' into index_pattern_server_api_2

8e0f3b8

partial progress

5f38261

serialize and deserialize index patterns

a63107a

serialize and deserialize index patterns

75f6b12

Merge branch 'master' into index_pattern_serialize

3ad8c2f

just a comment

4eecca4

Merge branch 'master' into index_pattern_serialize

37a4b7f

remove underscores in front of vars

ca88133

add tests

907e21b

complete tests

9f6e464

update docs

508625f

update docs

fec59e1

mattkime requested review from a team as code owners June 27, 2020 01:54

mattkime added the release_note:skip Skip the PR/issue when compiling release notes label Jun 27, 2020

mattkime added 3 commits June 26, 2020 20:55

Merge branch 'master' into index_pattern_server_exposed

69cdd8f

minor fixes

e0349e6

more flexible tests

f85f991

elastic deleted a comment from kibanamachine Jun 27, 2020

Merge branch 'master' into index_pattern_server_exposed

0fb5f64

ppisljar approved these changes Jun 29, 2020

View reviewed changes

mattkime commented Jun 29, 2020

View reviewed changes

nreese approved these changes Jun 29, 2020

View reviewed changes

mattkime added 2 commits June 29, 2020 11:09

Merge branch 'master' into index_pattern_server_exposed

5e83d25

Merge branch 'index_pattern_server_exposed' of github.com:mattkime/ki…

dd711d2

…bana into index_pattern_server_exposed

darnautov approved these changes Jun 29, 2020

View reviewed changes

lukeelmers reviewed Jun 30, 2020

View reviewed changes

lukeelmers approved these changes Jun 30, 2020

View reviewed changes

mattkime merged commit 2fe0051 into elastic:master Jun 30, 2020

mattkime mentioned this pull request Jun 30, 2020

[7.x] Index patterns - Server API (#69105) #70367

Merged

mattkime added a commit that referenced this pull request Jul 1, 2020

Index patterns - Server API (#69105) (#70367)

9484fb7

* index patterns on the server

Bamieh pushed a commit to Bamieh/kibana that referenced this pull request Jul 1, 2020

Index patterns - Server API (elastic#69105)

2395bba

* index patterns on the server

mattkime mentioned this pull request Jul 13, 2020

[Meta] Index Patterns #67574

Closed

12 tasks

mattkime mentioned this pull request Sep 24, 2020

[Meta] Index Patterns Roadmap #78376

Closed

63 tasks

Index patterns - Server API #69105

Index patterns - Server API #69105

Uh oh!

Conversation

mattkime commented Jun 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

elasticmachine commented Jun 27, 2020

Uh oh!

mattkime commented Jun 29, 2020

Uh oh!

ppisljar left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nreese left a comment

Choose a reason for hiding this comment

Uh oh!

darnautov left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Jun 29, 2020

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

History

Uh oh!

lukeelmers left a comment

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!

Choose a reason for hiding this comment

Uh oh!

mattkime commented Jun 30, 2020

Uh oh!

lukeelmers left a comment

Choose a reason for hiding this comment

Uh oh!

mattkime commented Jun 30, 2020

Uh oh!

lukeelmers commented Jun 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

mattkime commented Jun 13, 2020 •

edited

Loading

lukeelmers commented Jun 30, 2020 •

edited

Loading