Discuss: Ui settings ownership

[flash1293]

Lots of UI settings are currently defined in https://github.com/elastic/kibana/blob/dd7531deb4c83a2711b0d4bc4871acea425eb8cf/src/legacy/core_plugins/kibana/ui_setting_defaults.js

These should be moved to their respective owner plugins using the core.uiSettings.register interface in the server part of the plugin.

Here is a list of all settings in there - I added suggestions where to move them but I'm open to discuss this.

@elastic/kibana-platform

• defaultRoute 👉 core
• dateFormat 👉core (date format is something super central, IMHO we shouldn't split these up but put everything into core)
• dateFormat:tz 👉core
• dateFormat:scaled 👉core
• dateFormat:dow 👉core
• dateNanosFormat 👉core
• truncate:maxHeight 👉core
• theme:darkMode 👉core
• notifications:banner 👉core
• notifications:lifetime:banner 👉core
• notifications:lifetime:error 👉core
• notifications:lifetime:warning 👉core
• notifications:lifetime:info 👉core
• accessibility:disableAnimations 👉core
• state:storeInSessionStorage 👉core

Issue for platform team (not listing specific settings): #59755

@elastic/kibana-app-arch

• query:queryString:options 👉 data
• query:allowLeadingWildcards 👉 data
• search:queryLanguage 👉 data
• sort:options 👉 data
• defaultIndex 👉data
• courier:ignoreFilterIfFieldNotInIndex 👉data
• courier:setRequestPreference 👉data
• courier:customRequestPreference 👉data
• courier:maxConcurrentShardRequests 👉data
• courier:batchSearches 👉data
• search:includeFrozen 👉data
• histogram:barTarget 👉data
• histogram:maxBars 👉data
• visualize:enableLabs 👉visualizations
• csv:separator 👉share
• csv:quoteValues 👉share
• history:limit 👉data
• shortDots:enable 👉data
• format:defaultTypeMap 👉data
• format:number:defaultPattern 👉data
• format:bytes:defaultPattern 👉data
• format:percent:defaultPattern 👉data
• format:currency:defaultPattern 👉data
• format:number:defaultLocale 👉data
• timepicker:refreshIntervalDefaults 👉data
• timepicker:quickRanges 👉data
• indexPattern:placeholder 👉data
• filters:pinnedByDefault 👉data
• filterEditor:suggestValues 👉data

@elastic/kibana-app

• defaultColumns 👉discover
• metaFields 👉discover
• discover:sampleSize 👉discover
• discover:aggs:terms:size 👉discover
• discover:sort:defaultOrder 👉discover
• discover:searchOnPageLoad 👉discover
• doc_table:highlight 👉discover
• doc_table:hideTimeColumn 👉discover
• fields:popularLimit 👉discover
• visualization:colorMapping 👉charts
• visualization:loadingDelay (seems to be unused)
• visualization:dimmingOpacity 👉vis_type_vislib
• visualization:heatmap:maxBuckets 👉vis_type_vislib
• savedObjects:perPage 👉saved_objects
• savedObjects:listingLimit 👉saved_objects
• metrics:max_buckets 👉vis_type_timeseries
• context:defaultSize 👉discover
• context:step 👉discover
• context:tieBreakerFields 👉discover

@elastic/kibana-gis

• visualization:tileMap:maxPrecision 👉vis_type_tile_map
• visualization:tileMap:WMSdefaults 👉vis_type_tile_map
• visualization:regionmap:showWarnings 👉vis_type_region_map

It would be super cool if someone from each team mentioned here could review the list and give their OK/concerns about the items. When everyone is happy with the allocation, I can create separate issues for each team.

[joshdover]

The list that should move to Core mostly makes sense to me, a few notes:

• dateNanosFormat 👉core
  • I'm on fence about this one going to Core. It's only used by field formats (data plugin) and Canvas. But it probably makes sense to just keep it in Core along with the other date formatting settings.
• defaultIndex
  • Hard to search the codebase for this one, but I'm curious if anyone knows if this is used by things other than the index patterns service? If so, maybe it should go to Core instead of data.

[flash1293]

@joshdover For dateNanosFormat that was my reasoning as well. For defaultIndex I figured that if the data plugin is owning index patterns it should also own advanced settings concerned with index patterns. Is it even possible to use it in a meaningful way without the data plugin?

[joshdover]

For defaultIndex I figured that if the data plugin is owning index patterns it should also own advanced settings concerned with index patterns. Is it even possible to use it in a meaningful way without the data plugin?

I agree, I just wasn't sure if this setting was used by any plugins that aren't using index patterns directly?

[flash1293]

Despite its name, defaultIndex will not hold an index(pattern) string, but a saved object id:

So I think you have to work with index patterns to use this setting somehow, but maybe I'm missing a case.

[lukeelmers]

At first glance App Arch list makes sense to me.

Despite its name, defaultIndex will not hold an index(pattern) string, but a saved object id:

Correct, this is confusingly named but it is referring to the ID of the default index pattern, not the pattern string itself.

While we are on the topic of confusing names, I think metrics:max_buckets should have its description updated or be moved to another section of advanced settings. Currently it is under General which is confusing, and there's no indication that this is specific to TSVB other than the word metrics.

[joshdover]

@flash1293 does platform need to prioritize #59755 for 7.8? Is it a hard blocker for any migration work, or just needs to get done before we can delete the legacy kibana plugin?

[flash1293]

@joshdover No blocker for us, can happen anytime :)

[mshustov]

I don't think it's a blocker for plugin migration. It should be done to make LP deletion possible, so we didn't set a high priority on #59755

[timroes]

I have a general concern or something we should keep in mind here and I need some clarification on:

Once we move an advanced setting to a specific plugin and you disable that plugin I assume this advanced setting will no longer be registered? Meaning, that if you want to consume an advanced setting that is not defined within your plugin, you must have that other plugin as a dependency and/or handle that that setting might not be present at all?

Are we sure that for those settings not going to core, we're not using them across plugins that might not yet have a dependency on each other?

cc @joshdover @flash1293

[mshustov]

Meaning, that if you want to consume an advanced setting that is not defined within your plugin, you must have that other plugin as a dependency and/or handle that that setting might not be present at all?

Yes. That would make clear that settings are part of your public contract, which would prevent accidental breaking changes when you need to update this setting.

Are we sure that for those settings not going to core

The platform team is just not the right owner for all of them. We don't know about use-cases for every setting. When someone wants to update a setting and pings the platform team, we won't be able to verify whether changes are correct and do not affect all the consumers.

we're not using them across plugins that might not yet have a dependency on each other?

It means that either a parameter has several responsibilities or a plugin access value that doesn't belong to it. Even if we have such a problem, we can duplicate a setting in both plugins and add a migration for the current value.

[flash1293]

@timroes This is a good point, it should be part of the migration to audit this (I'm 83% sure we don't have that case right now though :) ). We should probably provide guidelines as well how to consume uisettings in a save way.

[kindsun]

Looks like it's just a small chunk of the overall list, but wrt the legacy maps settings, no objections from me to move registration to the maps_legacy plugin. To the best of my knowledge, none of those settings should have effects outside of the maps apps.

[Dosant]

@elastic/kibana-platform,

What do you think of

state:storeInSessionStorage 👉kibana_utils

moving into core ?

Why I think kibana_utils doesn't look like a best place for this now:

1. kibana_utils is technically not a plugin (yet), so it can't register this setting.
2. I think this option also related to global url_overflow handling and popup which suggests to enable this option. I believe that code is part of core?
3. state syncing utils from kibana_utils are not pulling this option from uiSettings directly, but each app passes it as param when setting up state syncing

const stateSync = createStateSync({useHash: uiSettings.get('state:storeInSessionStorage')})

In theory this condition ⤴️ could be different for specific use cases.

cc @lukeelmers

[joshdover]

What do you think of

state:storeInSessionStorage 👉kibana_utils

moving into core ?

Came here to actually say this too 😄

With the changes in #67899, Core will be referencing this setting in the UI. Makes sense to move to Core itself, even if we're not consuming it, it acts as a global option that affects many apps.

EDIT: updated the list, but if anyone objects we can change it again

[joshdover]

Platform team is planning to remove the legacy kibana plugin in 7.10. We still need to migrate our settings to Core, but these also need still to be migrated by other teams:

@elastic/kibana-gis

• visualization:tileMap:maxPrecision
• visualization:tileMap:WMSdefaults
• visualization:regionmap:showWarnings

@elastic/kibana-app-arch

• timepicker:timeDefaults

I suspect moving these is not difficult, but we will need to do this soon in the 7.10 development cycle in order to unblock #56205

[lukeelmers]

I have a PR open for the timepicker:timeDefaults setting which was missed: #73750

[flash1293]

As this issue was intended for the discussion of ownership, I will close now - Maps team ( @elastic/kibana-gis ), it seems like you are missing a few migrations.

[nreese]

Map migrations completed in #75887