[RFC] persistable state service

[ppisljar]

Summary

PersistableStateService RFC

We need a formal way for authors of registry implementations to add migrations for this data. Since they may not own the saved object that their implementation ends up in, we can't rely on saved object migrations as it exists today (not to mention data that ends up in URLs that may need to be migrated).

We also need to make sure that all the persisted state containing references to saved objects extracts those before being saved and injects them later.

related:
stacey's presentation
peter's presentation
#63358

[elasticmachine]

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

[stacey-gammon]

I'm not a fan of this naming scheme because in many other places we use:

MyThingDefinition -> what gets put on the registry.
MyThing -> what gets retrieved from the registry.

Here though, PersistableStateDefinition takes in a PersistableState shape.

I admit I don't have a great suggestion though. Maybe PersistableStateHandler (hahaha, handler again!) or PersistableStateMigrator (eh, but doesn't cover inject/extract). 🤔

[stacey-gammon]

I'd be curious to see how the conversion from deserialized state to EmbeddableInput will go, and whether we can continue to assume users of Embeddables can still access things like:

const input: VisualizeEmbeddableInput = visEmbeddable.getInput();
console.log(input.vis.type);

vs

const input: VisualizeEmbeddableInput = visEmbeddable.getInput();
console.log(input.specializedState.visualize.type);

If we were able to have the serialized representation different from the deserialized representation, we could do this more formally.

const input: VisualizeEmbeddableInput = visEmbeddable.getInput();
console.log(input.vis.type);

const serializedState = visEmbeddablePlugin.beforeSave(input);
console.log(serializedState.specializedState.visualize.type);

// save to disk
// load from disk

const deserializedState = visEmbeddablePlugin.afterLoad(serializedState);
console.log(deserializedState.vis.type);

But the way the types are right now, deserialized and serialized state are the same. Maybe that layer doesn't have to be part of this system, but if not, I'm not sure where it'd go. 🤔

[ppisljar]

there is no serialization/deserialization happening inside this service

we are only working with Serializable state (which means a state that can be serialized, so no functions, circular references, .....)

this is kind of in between the StateInstance (EmbeddableInput instance) and SerializedState (for example json stored in database, or url string)

i dont know enough about embeddables, but for visualize state this goes something like this:

vis --> instance of Vis class, represents the state of each visualization
vis.serialize() --> returns VisSerializable
persistableStateService.beforeSave('visualization', vis.serialize()) --> returns VisSerializable and references array (the actual type of the object should not change, we just extracted the references)
savedVisLoader.save(id, persistableStateService.beforeSave('visualization', vis.serialize())) --> saves to a saved object (i think the save method actually doesn't exist (yet, or in this form))

so with embeddable i could see something like EmbeddableInput, SerializableEmbeddableInput (we need some utility to convert between those) and the later is the one PersistableStatePlugin is interested in (and can be later serialized and stored)

[stacey-gammon]

I think we should still have an answer for this for Embeddables to make sure the system supports our needs. Maybe @Dosant or @streamich can work through how this would work with Embeddables.

I think we should touch on what to do when your state is extended like SpecializedState extends BaseState. We can mention it in a generalized sense. Like the enhancements pattern, I think the state registrator will have to explicitly support this. I mention above, perhaps we could solve this and the enhancements pattern with the same method, using an enhancements key on the state?

[ppisljar]

@Dosant can you take a look into this ?

[stacey-gammon]

I would call this section something like "Handling enhancements".

[lukeelmers]

++ This should probably also link to the issue on enhancements in case folks are unfamiliar with what the word is referring to. (Unless Stacey and I are the only ones reviewing this, in which case I think we are good haha). 😉

[stacey-gammon]

Created enhancements issue because it didn't exist before and I never checked my PR in - #68880. There were some unresolved comments and I moved on to other things.

As for review, I think we should email out to Kibana when it's ready for external feedback, then consumers of this service (outside app arch team) will be aware and can weigh in. I suspect most Kibana devs won't know they need to be aware of it, so rather than headline it something like "Persistable state registries RFC" where no one knows what that means, you could frame it like "How to ensure your code doesn't break when public APIs change". It's more complicated than that, but if we frame it simply, we'll likely get more eyes on the RFC.

A link to one of the presentations in the introduction of this RFC might be useful to provide this background context, as well as embedding one of the images that shows the different roles: Registrator, Registrar, Persister and Enhancer.

[ppisljar]

not sure if we can link to representations as they are on gdrive and this is gonna be in public repo ?

[stacey-gammon]

Put some examples to show what you mean by stateType?

Not just to avoid clashes with unknown plugins, but so the registrator and the enhancer both know exactly what id to use.

Enhancer knows to register their enhancements with drilldowns-{EMBEDDABLE_MIGRATOR} and embeddable knows to look up state.enhancements[key]-{EMBEDDABLE_MIGRATOR}.

Above enhancements example should be updated to use the key.

[lukeelmers]

Put some examples to show what you mean by stateType?

++

Enhancer knows to register their enhancements with drilldowns-{EMBEDDABLE_MIGRATOR} and embeddable knows to look up state.enhancements[key]-{EMBEDDABLE_MIGRATOR}.

Do we want to recommend that registrants export these as consts or in an enum or something? This is implied in the examples but not explicitly stated.

[ppisljar]

i am not sure if the registrator and enhancer need to know about what id to use ?

for example lets take a look into drilldowns:

• drilldowns plugin registers a drilldowns-drilldownState persistable state definition
• 3rdparty plugin registers a 3rdparty-drilldownState persistable state definition

they both add to dashboardState.enhancements[key] ... when dashboard is doing migration it simply checks persistable state service for the key which for our drilldowns will be drilldowns-drildownState and for 3rd party drilldown it will be 3rdparty-drilldownState .

[stacey-gammon]

I would change this to say that this will only work when the registrator explicitly supports enhancements using an enhancements key in its state.

This wouldn't work if a plugin author followed the enhancements pattern without the registrator's knowledge.

This is going to be the same thing for the pattern of extending the base class. Do you think this method with enhancements could be used to also solve the problem of migrating state on SpecializedClass extends BaseClass?

[stacey-gammon]

I think we should still have an answer for this for Embeddables to make sure the system supports our needs. Maybe @Dosant or @streamich can work through how this would work with Embeddables.

I think we should touch on what to do when your state is extended like SpecializedState extends BaseState. We can mention it in a generalized sense. Like the enhancements pattern, I think the state registrator will have to explicitly support this. I mention above, perhaps we could solve this and the enhancements pattern with the same method, using an enhancements key on the state?

[lukeelmers]

Makes sense to me so far!

One overall question I have is what is our strategy for dealing with this on the server? (There's no mention of server vs client here AFAICT, so my assumption is that this is all designed with client in mind?).

And if we are going for a hybrid model on both client and server, what's the plan for that?

[lukeelmers]

Do you see this as its own domain which should be a standalone plugin, or as part of the share plugin like the previous POC? It would be good to call that out in the RFC, including what this would be named (if it's a new plugin).

[lukeelmers]

Two questions here about Partial<PersistableStateDefinition>:

1. This will allow register('myState', { id: 'whatever' }); which I assume is not desired.
2. Partial will also allow the definition to be an empty object, which is pointless (though perhaps harmless?)

Maybe better to do Migrate | Inject | Extract so that you are forced to provide at least one of those, and id is not allowed?

[lukeelmers]

Put some examples to show what you mean by stateType?

++

Enhancer knows to register their enhancements with drilldowns-{EMBEDDABLE_MIGRATOR} and embeddable knows to look up state.enhancements[key]-{EMBEDDABLE_MIGRATOR}.

Do we want to recommend that registrants export these as consts or in an enum or something? This is implied in the examples but not explicitly stated.

[stacey-gammon]

Some more thoughts/questions:

1. We agree the only reason we need a generic registry is when the id is not known at compile time. The only situation that I can think of where that happens is with enhancements and with subclassing. This is why I think it's important to work through those two situations. Otherwise we don't have a problem with contra-variance - don't use a registry at all, just use a pattern.

Example

2. Should the publicly exported name of the type be changed with version bumps or stay the same? Changing the name would require you to go update that type in every place it's used in the code, which could be a lot of places, however, changing the name seems more accurate, or historical types from other plugins might be wrong. (@timroes and I discussed this aspect before so the Lens team will likely ask this question if we don't address it)

Example (specifically see BarStateV1 type and the question of how to keep it an accurate representation of history).

In a comment above you say that this plugin doesn't have anything to do with serialization and deserialization but I wonder if it should in order to solve the problem with keeping types in sync across versions. Maybe if every plugin to exposed there persisterId and a mapping of { Version, Type } it would help.

3. Ensuring consistency across plugins.

I originally pushed the generic registry model pretty hard, partly because of the consistency it enforces. When you access things off a generic registry, even when the id is known at compile time, then plugin users get a consistent experience across the board.

Consistency enforced with a common registry:

// Foo Plugin
persisters.get('zed').migrate(state, version);

// Bar Plugin
persisters.get('foo').migrate(state, version);

Inconsistency allowed with individual plugins exporting:

// Foo Plugin
zed.migrate(state, version);

// Bar Plugin
foo.migrateFoo(version, state);

In that latter example, there are slight differences in the api. It's not the end of the world, especially since it's typed, but it can be annoying. Someone needs to migrate Bar data, they could see if it exists by checking if persisters.get('bar') is defined. If it's not, they can go to Bar and say, hey, I want to persist your state, can you please add a migration? In the latter case, the developer needs to explore Bar's API (which should ideally be documented).

I don't have an answer for this, and I don't think it's a problem that needs to be "solved", but I want to mention it in case anyone thinks of a way to improve this.

[lukeelmers]

I don't have an answer for this, and I don't think it's a problem that needs to be "solved", but I want to mention it in case anyone thinks of a way to improve this.

Honestly I think a shared interface that we ask plugins to implement would solve the majority of these issues.

const myPersistableStateFns: SharedPersistableStateFnInterface = {
  // TS yells at you if it isn't called `migrate`, or if `state` and `version` types are wrong
  migrate: (state, version) => ({...}), 
  extract: ...,
  inject: ...,
  // or they share `beforeSave` and `afterLoad` per my question above
};

[ppisljar]

why not ask plugins to export persistableStateDefinition on their contract, rather than individual functions. This way we can be sure that every definition looks the same. (i guess that's what Luke is suggesting as well). added this to RFC.

[joshdover]

At this point my main questions/concerns are related to how and when the schema migrations are executed.

Last that I remember discussing (and I have not followed this discussion closely), we were considering a pattern where each plugin that had a SavedObject type that stored data from registrants would aggregate those registrants migrations into it's own set of migrations that would be registered with Core. This pattern would allow all the data migrations to happen at upgrade time, once, without needing to litter migrate calls throughout the codebase.

What were the reasons we decided not to go down that path? I vaguely remember some concerns with URLs and not being able to detect that nested data was out of date, but I don't really remember why those were problems.

Let me know if what I'm talking about isn't ringing any bells or needs more explanation.

[timroes]

Disclaimer: I haven't read all comments in this PR, so please feel free to point me towards them if I am repeating stuff here.

The further I read this RFC, the more concerned I became with having this in the share plugin. This functionality will be crucial to saved state in Kibana, and not using it will break plugins (cross-plugin) boundaries hard, as it was even stated in the RFC:

teaching impact: everyone storing state from belonging to another plugin will need to remember to use this service when saving and loading state.

I have the feeling (and let's leave aside, who implements it right now), that this functionality should rather be in the Kibana Core, than in the share plugin. It's from it's purpose really close to the saved object migrations. I'd say even slightly more generic, since it applies to any kind of state, that is stored anywhere (though most of it should be stored in saved objects). If we think there's something too specific about this, that would not justify having it in core, I would still vote for having it in it's own plugin, so we have 0 dependencies to other stuff when requiring to use this.

Content wise the proposal looks good to me (also not the first time we've talked about it :D). Besides having direct plugin dependencies importing a more specifc API:

  import { extract } from 'src/plugins/pluginOwningState';
  const stateForSaving = extract(myState);

was also meant to have better typing for this, without requiring to fallback to generics? Maybe worth mentioning this in the RFC, though not sure if it's really that important for most people.

[streamich]

+1, for @timroes suggestion for this service being part of core, it feels like a natural extension of migrations offered by Saved Objects service.

From the RFC:

Use a {pluginId}+{stateType} pattern for your ID

if this service was part of the core it would know pluginId automatically without us needing this convention.

[joshdover]

* dashboard stores a dashboard saved object, than one consists of:
  -- extensions (any plugin can add to those, we have no idea who the plugins might be at compile time)
  -- panels (any embeddable might end up in here)
  --- visualize embeddable (visualize embeddable input persistable state)
  ---- consists of vis state or saved vis id, if its visstate, then that one consists of:-
  ------ saved search starte
  ------ index pattern start

This does sound very complex. A global registry allows the Dashboard plugin to handle all of this in a single place, yes, but now the Dashboard plugin has to manage all of this complexity. That seems worse than Dashboard handling a single layer of complexity (panels and embeddables) and letting those layers handle their underlying complexity. It makes Dashboard much more complex to extend since every extension would need to edit Dashboard's code. Or maybe I'm misunderstanding, and the migrate functions for Panels and Embeddables would handle calling migrate for their underlying components (eg. visualizations)? Without an example, I have a hard time understanding how this type of complex case is supposed to use the PerisistentStateRegistry.

also your proposal would require us to change a dozen of apis. embeddables would need getMigrations function, so would visualizations, vis_types, expression functions, expression types, ........

I think we should consider doing more work now if it saves us time later by keeping things simple.

lets not forget that this is not supposed to solve issues with saved object migrations only but that we need to migrate state coming from different places as well (like local storage, bookmarked urls etc)

I think this part is where I'm still missing some context. In those cases, we don't have a version correct? Is that the reason my proposal wouldn't work? I think the pattern I proposed above could be adapted for this still by providing both migrations by version and a single migrate function that can handle older versions. But I'm unclear on what is different about data stored in localStorage and URLs that makes them different.

also, this is not limited to migrations only but handling of persistable state in general, which currently on top of migrations involves extracting and injecting references, so i am wondering are we theese additional functions to all our apis as well ?

Yes I think it could be part of the same pattern. Plugins could expose the same methods that are being proposed as part of this global registry.

I think, unfortunately, we do need a global registry because of the enhancements pattern we use in the Embeddable use case.

@stacey-gammon This is one case I had not considered and I need to think through it more. Where's the best place to read (in code or docs) about how enhancement state works?

[stacey-gammon]

@joshdover - I have a PR that explains the concept with a couple example plugins here: https://github.com/elastic/kibana/pull/62998/files. It was never checked in, I wanted to see how it played out before officially recommending something (especially because I think it will change with this consideration for persistable state).

I have this gist with some code snippets at the end that attempt to walk through the embeddable use case.

This is how it works in actual code now, for embeddables to support dynamic actions that is an x-pack only feature: https://github.com/elastic/kibana/blob/master/x-pack/plugins/embeddable_enhanced/public/plugin.ts#L98

The idea was to create a pattern for one plugin to extend an OSS thing, without having to have unused functionality in OSS. It's similar to how security "decorates" the saved object client, enhancing it with security features. It's only ever been used by a single plugin being the "enhancer/decorator", but it should be adaptable to support multiple plugins "decorating" another plugin's state & functionality. Long term, I think that support would come in handy.

lmk if you have more questions or want to sync about it!

[pgayvallet]

I unfortunately lack functional knowledge of the problematic here, and I have to admit the RFC is quite abstract, so excuse any potentially stupid questions.

[pgayvallet]

How will the service be able to extract and return the version string from an arbitrary state? I don't see any delegate method returning this information on PersistableStateDefinition.

Also minor, but return type inconsistency

// obj
extract: (state: P) => { state: P, references: SavedObjectReference[] }
// array
beforeSave: (id: string, state: PersistableState) => [PersistableState, SavedObjectReference[], string],

[ppisljar]

extract is taking in latest state and should extract references from it (no version involved)

[pgayvallet]

So the consumer is in charge of passing down the associated SavedObjectReferences? If I see how this would work with states contained inside a saved objects, It's harder for things that will not be directly coming from SO, such as localStorage?

[ppisljar]

extract and inject (and thus afterLoad and beforeSave) are actually only relevant when using with saved objects. when storing to url or local storage there is no need to extract the references

[pgayvallet]

Why the static import here, can't this be achieved using setup contract instead?

Also:

If state id is known at compile time we should rather import the correct utilities directly from the registrator plugin

As it's supposed to be working using the global registry anyway, what about 'prefer one way to do things' ?

[ppisljar]

using global registry when you know the id of the state you are working with at compile time is not desired as introduces indirect dependencies (you define dependency of persistable state service, but in reality you depend on pluginOwningState, so you should rather work with plugin owning state directly.

wheter the methods are exposed on the contract or staticly is imo implementation detail of plugin exposing them, i think static exports are prefered when possible but i don't think that should be defined here.

[pgayvallet]

As this is under the registrator's responsibility to handle these 'enhancements' anyway (and not the service/registry), Isn't that a specific implementation from an existing need/structure from our code more than a generic solution that have to be applied everywhere?

[ppisljar]

i agree, this is not something that should be applied everywhere, but its listed here under the examples as Stacey had questions about how that would work, so i just wanted to demonstrate that this is not an issue for persistable state registry

[pgayvallet]

Not sure to really follow here. From what I understood, this persistable state service / registry was not directly linked to SO migrations (see # Server or Client ? part), and actual migration of the underlying data would only be performed during object access? Why would we need to fetch/migrate everything during SO migration then?

[ppisljar]

if we were to use this for saved object migrations: with saved object migrations we want to know if there is a migration to perform or not. however this is getting harder and harder because of deep dependency trees:

• do we have a migration for dashboard saved objects for 7.9.1 ?:
  • do we have migration for base embeddable input for 7.9.1 ?
    • do we need to migrate filters ?
    • do we need to migrate query ?
    • do we need to migrate timerange ?
  • do we have migration for any of the embeddable types for 7.9.1 ?
    • lens embeddable:
      • do we need to migrate lens state ?
      • do we need to migrate expression ?
        • does any expression function need to be migrated ?
    • visualize embeddable:
      • do we have migration for search source ?
      • do we have migration for index pattern ?
      • do we have migration for any vis type ?
        • once any vis type starts storing expression:
          • is there a migration required for expression?
            • is there migration required for any expression function ?
  • do we have migration for any enhancement ?
    • drilldowns:
      • do we need to migrate filters ?

you can see how our state is deeply nested with various plugins owning various parts of it. Its also not showing everything (its oversimplified). so figuring out if there is a migration required for specific version (7.9.1) is a hard problem.

also important to note: this is a hard problem with or without global registry. actually without global registry this becomes even harder problem to solve imo, as we don't have registries for a lot of parts of the above dashboard state.

[pgayvallet]

Going back on the

A plugin could have a single stateType, for example visualizations plugin has a savedVis state, so it use visualizations-savedVis as id. Other plugins might have multiple state types they want to register. Lets say that we introduce a new state type to visualizations plugin called visState we would use visualizations-visState as id.

part here. I guess that's not the case atm (types are not prefixed with the pluginId, are they). How will 'premigration' of the existing persisted data be performed?

[ppisljar]

very good point, i think we need to allow migrate function to return different state id. so when i would be migrating the current dashboard enhancements (enhancement.drilldown) i can return new state id and change that to enhancements.drilldowns_drilldownState

[joshdover]

A global registry allows the Dashboard plugin to handle all of this in a single place, yes, but now the Dashboard plugin has to manage all of this complexity. That seems worse than Dashboard handling a single layer of complexity (panels and embeddables) and letting those layers handle their underlying complexity. It makes Dashboard much more complex to extend since every extension would need to edit Dashboard's code. Or maybe I'm misunderstanding, and the migrate functions for Panels and Embeddables would handle calling migrate for their underlying components (eg. visualizations)? Without an example, I have a hard time understanding how this type of complex case is supposed to use the PerisistentStateRegistry.

Sync'd with Peter on this earlier today. I missed the changes to the RFC that demonstrate the encapsulation and how these migrations could reference other migrations.

At this point, I'm still not convinced that one global registry that blurs the plugin dependency lines is better than each registry implementing this pattern. I need to spend some time to write down my thoughts and an example of why I think a global registry will be problematic. With feature freeze and planning going on right now, I just need more time. A clear example of why a global registry is necessary would be helpful. So far, I've only seen examples where we need a global registry because there isn't already an existing registry for a particular concept (eg. dashboard extensions). My main goal is just making sure we can keep things as simple as possible and avoid introducing new unnecessary dependencies into the graph.

[kobelb]

I'm generally in agreement with the approach that @joshdover recommended here. It's also not clear to me why a global registry is necessary. I get why we'd need to have a registry with all of the extensions/enhancements for a particular subject, but I don't see the benefits that we get by having a truly global registry for everything that we consider to be "persistable state".

There's also the potential for us to not do these migrations during saved-object migration, and to do so at runtime. Using Dashboards as an example and ignoring "enhancements". Any plugin can register an Embeddable which can then be included in a Dashboard panel. When an Embeddable is registered, it should conform to a specific interface that all Embeddables support. As such, Dashboards shouldn't need to know about the internal state of the Embeddable and should be treating it as a "blackbox". It's reasonable for the Dashboard to persist the internal state of the Embeddable for it, and pass back the internal state later, but it shouldn't know the shape of the data that it's persisting.

One of the primary reasons why we do saved-object migrations the way that we do at startup is because of SavedObjectsClient::find. The SavedObjectsClient::find method queries the .kibana index to find saved-object based on certain criteria. If we didn't migrate the actual documents before performing the SavedObjectsClient::find, we'd potentially return inaccurate or no results. However, since Dashboards should be treating the internal state of the Embeddables as a "blackbox" it shouldn't ever be querying for specific fields in its state. Therefore, we could potentially delay this transformation until the Embeddable is receiving the state which the Dashboard passes to it.

[stacey-gammon]

All these questions highlight why it's so important to fully work through the Embeddable use case, so we have a concrete example everyone can see and work through to understand if it is necessary to have a global registry. After spending some thought on it this morning, I think I have a way to get around it.

Imagine I have three plugins built by three separate developers: Embed, Vis & Drill. Embed is the registrar, who exposes a registry of persistable state.

interface EmbeddableState {
  title: string
}

Vis is a registrator. This particular registry allows registrators to add additional state to be persisted on registrations of the given Embeddable type:

interface VisEmbeddableState extends EmbeddableState {
  visType: string;
}

Drill is an enhancer and extends the state on all registered items.

interface DrilldownEmbeddableState extends EmbeddableState {
  drilldownIds: string[];
}

• Embed wants to rename title to defaultTitle.
• Vis wants to rename visType to subType.
• Drill wants to change drilldownIds to drillDowns and add more information.

Dashboard knows it contains EmbeddableState but it doesn't know about VisEmbeddableState or DrilldownEmbeddableState. It calls EmbeddablePlugin.migrate(embeddableState). How do the migrations for VisEmbeddableState and DrilldownEmbeddableState get called?

One way to solve this without a global registry for VisEmbeddableState, is if the Embeddable system asks for migrations during the registration process:

registerEmbeddableFactory(type, factory, migrator, injector, extractor) {
  this.factories[type] = factory;
  this.migrators[type] = migrator;
}

migrate(state, version) {
  // Migrate state Embed owns.
  if (version === xxx) {
    state.defaultTitle = state.title;
    state.title = undefined;
  }

  // Call specific migrator for the given type
  state = this.migrators[type](state, version); 

  return state;
}

Similarly for enhancements:

setEmbeddableProvider(provider, migrator, injector, extractor) {
  this.factoryProvider = provider;
  this.providerMigrator = migrator;
}

migrate(state, version) {
  // Migrate state Embed owns.
  if (version === xxx) {
    state.defaultTitle = state.title;
    state.title = undefined;
  }

  // Call specific migrator for the given type
  state = this.migrators[type](state, version); 

  // Call provider migrator to migrate `drilldowns`
  state = this.providerMigrator(state, version);

  return state;
}

One issue with that above code is that the migrators have access to the entire state and this can cause issues. Embeddables should have clear boundaries about which migrator has access to which parts of the state.

migrate(state, version) {
  // Migrate state Embed owns.
  if (version === xxx) {
    state.defaultTitle = state.title;
    state.title = undefined;
  }

  // Call specific migrator for the given type
  state.extendedState = this.migrators[type](state.extendedState, version); 

  // Call provider migrator to migrate `drilldowns`
  state.providerEnhancementState = this.providerMigrator(state.providerEnhancementState, version);

  return state;
}

If we do it that way, I think we can avoid a global registry. We could still have a common Registry, or PersistableRegistry interface, to help create consistency and awareness of considerations when supporting state that is persistable (but don't even need that in the first pass). This also avoids any issue with trying to come up with a global id naming scheme for the migrator look ups (e.g. persistableStateMigrators.get('embeddable-enhancers-${enhancementKey}')).

[ppisljar]

what I would like to understand is why is the approach with global registry bad ? can somebody show some concrete examples of what will go wrong or explain the downsides to me ?

It feels the discussion is if we should add persistable state registry and handle all persistable state in the same manner, or rather adding multiple specific registries (for example we know we would need to add embeddable enhancements registry) with multiple specific implementations and mutliple ways (depending on what you work with) to handle persistable state.

[kobelb]

what I would like to understand is why is the approach with global registry bad ? can somebody show some concrete examples of what will go wrong or explain the downsides to me ?

One of the downsides, which Stacey previously mentioned, is we have to come up with some type of "id scheme" which allows the consumer of persistable state to find the correct PersistableStateDefinition. For example, different plugins will be registering embeddables than the plugins which register "enhancements". We end up having to create an "id scheme" to essentially partition the global registry into sub-registries

I'm also hesitant for us to treat all "persistable state" the same. For example, Dashboards have Embeddables, which have references to saved-objects. When creating the Dashboard saved-object, we need all of the Embeddables to communicate the saved-objects that they're referencing to the Dashboard, so that the Dashboard's saved-object's references are persisted properly. However, when we store some state in the URL, we don't need to extract these saved-object references in the same manner.

It feels the discussion is if we should add persistable state registry and handle all persistable state in the same manner, or rather adding multiple specific registries (for example we know we would need to add embeddable enhancements registry) with multiple specific implementations and mutliple ways (depending on what you work with) to handle persistable state.

There's potential that we'll see various types of persistable state share qualities and concerns with others, so some generalization might be beneficial in the future, but I don't think we should jump to treating all "persistable state" the same right from the start.

[ppisljar]

One of the downsides, which Stacey previously mentioned, is we have to come up with some type of "id scheme" which allows the consumer of persistable state to find the correct PersistableStateDefinition. For example, different plugins will be registering embeddables than the plugins which register "enhancements". We end up having to create an "id scheme" to essentially partition the global registry into sub-registries

hmm, i am not sure if we really need to do that. I think for embeddable usecase example the keys under enchantments property should be direct ids by which we can look up persistable state registry.

Object.getKeys(state.enhancements).forEach(key => {
  state.enhancements[key] = persistableStateRegistry.get(key).migrate(state.enhancements[key]);
});

but yeah, probably there are usecases where we will need to do that however (come up with id schemes)

[joshdover]

what I would like to understand is why is the approach with global registry bad ? can somebody show some concrete examples of what will go wrong or explain the downsides to me ?

My main problem with the global registry is that we're effectively introducing a set of 'global variables' (the registry items) and all of the traditional baggage that come with global variables.

Using these registry items requires that the consumer knows which registry item corresponds to which concept in another plugin (essentially the id scheme problem noted above). If we need an id scheme to segment this registry, consuming this registry is more complicated. Simply moving the migrations to the actual registry items (eg. Embeddables) they are related to seems much simpler.

My other main concern is that it's possible we're hiding existing or could create new circular dependencies with this pattern. For example, it's been mentioned that there is a concept of "dashboard extensions" which the dashboard has no knowledge of. If the dashboard migrations must depend on extensions registering a migration function with this registry, then how will this behave if extensions are disabled? Another example would be when one persistable state migration depends on another one that may or may not be registered by another plugin that this plugin does not explicitly depend on. These situations seem very easy to create accidentally.

[ppisljar]

from my understanding embeddable registry is also global, with all the garbage that goes with it. so we would be introducing multiple global registries in this case.

consumer doesn't need to know that as mentioned in one of my previous comments, consumer should (at least in most cases) store the non-owned-state with its stateId and then lookup the registry by this stateId.

with dashboard extensions i don't see any special problem, it would behave exactly as dashboard does with embeddables (with persistable state registry or with migrations on embeddable registry):

• we had some plugins (embeddables, extensions) enabled, and we created a dashboard (and saved it)
• we disabled some of those plugins
• loading this dashboard now will not show the disabled embeddables nor disabled extentions
• migrating this dashboard with disabled extensions or embeddables will not migrate those parts of state (no matter persistablestateregistry or not)
• opening that dashboard later will still be no issue
• enabling those plugins back becomes an issue as we already migrated the dashboard those migrations will not be executed if i am not wrong ? (which would mean we always need to run those migratons on-the-fly as well?)
• now we upgrade again and we get into a big problem, if we are only running migrations for specific version (if we would ignore the versioned migrations and do the type checking to figure out what version of state we have that would not be an issue) we will try to run migration for this embeddable that was disabled (lets say from 7.9 to 7.10) but as embeddable state is not of last version (its still 7.8 for example) that will (could) fail.

[ppisljar]

so looking back i think there were two main concerns raised about using global registry to address this:

1. problem with id collisions:

problem with id collisions is not new, there is a chance of collision with 3rd party plugins (and our internal ones) with every registry we own. we handle that in a similar manner everywhere: registering an item to a registry with the same id that is already registered will fail. The same would be the case with persistable state registry.

Its true however that with persistable state registry we increase the likeliness of collision (how much is hard to answer) but just to avoid that in RFC its suggested to use pluginName-stateName for your state ids, which i think fully avoids collisions with a trade off being longer state ids. As we are anyway suggesting not to type state id strings but to rather export a constant and use that i think this trade off is minimal.

2. problem with figuring out when a migration is necessary:

as our state is deeply nested its hard to figure out when a migration is necessary for specific top level state (see comment #67931 (comment)). But this is a problem if we do not use global registries as well. Even if we put additional logic to specific registries it will still be very hard to figure this out, probably even harder as we won't have one place and one way to check for migration functions.

there were some other concerns with using the global regisry:

3. introducing a set of 'global variables' (the registry items)

We are introducing global variables even when we are using specific registries (embeddables, expressions, visualizations, ....) so we would not be introducing bigger amount of variables or any other thing that comes with it.

4. introducing new hidden or circular dependencies

Using one registry does not increase the likeliness of introducing hidden or circular dependencies. That is just as likely to happen if we are using multiple registries. In fact i would argue its easier to educate people to use this correctly when we are using one registry as opposed to many.

5. treating all state the same:

I think we should treat all state the same. I think the alternative to this approach being suggested (using interface that should be implemented on specific registries) would also try to enforce that all the state is handled in the same manner. Extracting the references however is something specific to saved objects and i agree should not be done when storing state to url or localstorage.

[joshdover]

I think I have had a hard time getting my concerns across, so I wanted to take some time today to fully explain:

1. What are we actually talking about when we speak about registries?
2. Why is this specific global registry different than others we have in Kibana?
3. Why does that difference makes this registry obsolete?
4. Some of the reasons we may feel compelled to have a global registry anyways, but still shouldn't.

To make this as clear as possible, I started from first principles to ensure that we're all discussing the same things.

---

The primary purpose of the registry pattern is for service location. It allows consumers of the registry to access the items of the registry without knowledge of where any of the items reside in the system.

The primary purpose of interfaces in an object oriented language is to allow consumers of an interface to use different implementations of the interface, interchangeably and completely transparent to the consumer. In other words, a consumer of a registry should be able to use any implementation of an interface without any knowledge of how that specific implementation works under-the-hood. This concept is also known as the substitutionality and underpins much of OO programming.

When you combine these two concepts, you have a registry where all the items in the registry implement the same interface. This makes such a registry very useful in a wide-range of use cases. We might call such a registry a generic registry. In Kibana, this is a powerful concept because it allows plugins to interoperate with one another without plugins needing to have knowledge of how another plugin works and without needing to have a direct dependency on it. As long as the interface is respected then, in general, both consumers of the registry and providers of registry items should be compatible.

In practice, the "in general" aspect turns out to be an important distinction. It is possible to create a generic registry that does not have many of these "generic" benefits. Consider this example:

interface Calculator {
  multiply(input: any): string;
}

interface CalculatorRegistry {
  register(id: string, calculator: Calculator): void;
}

While an extreme example, it demonstrates an important point. That is, without more specific input and output types defined by the interface, it's virtually impossible to use any items in this registry in any interesting or generic way. Implementations of the Calculator interface have to make a number of decisions, and not every implementation may decide the same question the same way. For example:

• What is a valid input? Should this function be able to complete for literally any input type?
• If not, how should invalid inputs be handled? Should exceptions be thrown? Should a special error string be returned?
• What format should the output be? Does it matter?

In order for a consumer of this registry to use the items in a truly generic way, very few assumptions could be made about how any Calculator implementation may behave. No further processing could be done on the output of multiply. Consmers of the Calculator interface may also need to handle different implementations of this interface differently, depending on how they decided to handle the above questions. When consumers cannot use all implementations of an interface in the same way, the value of having such an interface is virtually zero. We lose all ability to use this interface (and the registry) in a truly generic way. This creates a number of integration problems that are nearly impossible to test for. Every consumer would need to be tested against every implementation, creating a cartesian product testing matrix.

In such a situation, our next question should be: why do we want such a registry? If not all items in the registry have the same input and output types, then it follows that the things in this registry are not related. If the consumer must have knowledge of individual registry items in order to use them, then those items are not truly compatible. Even if all the registry items can hide behind a seemigly generic interface, it's likely that this interface is simply too loose to actually provide benefits.

What's left is a registry that is simply being used for service location, not a generic registry. That in of itself is not a problem, but the next question is: why are consumers of this registry unable to locate these items? If they must already have baked-in knowledge of how to consume these items, they functionally have a dependency on these items. It follows that they should also already have a way of locating these items, whether directly via a plugin contract or indirectly via an already existing registry. I have yet to see a justification for having a registry in Kibana that is being used purely for service location. If consumers have a functional dependency on item that they do not have a way to locate, there is most likely another architectural problem lurking in the shadows.

I have drafted new documentation that demonstrates leveraging the already existing relationships between plugins to solve the same problem: rudolf#1. This is essentially the same thing I brought up in #67931 (comment) and @stacey-gammon applied more directly to the Embeddables use case in #67931 (comment).

There are situations where it has been mentioned that this pattern won't work because there is a service location problem (eg. dashboard extensions). I believe these point to architectural problems that should be solved outside of this solution, rather than leak into it. If we were to add a global registry for all such items when there may only be a couple cases where we have a service location problem, I think we'll be taking on a lot of tech debt to clean up later:

• Consuming a registry is harder, since it requires specific knowledge of how use each type of item in the registry. This creates a high potential for bugs.
• Changing any of the items in the registry is riskier since how and where these items are consumed is not immediately obvious.
• The testing matrix of such a registry is potentially very large and unlikely to have good coverage.
• This would likely become a significant support burden on the team that owns this registry.

Given that migrations are in such a critical path (upgrading Kibana) for our users, I don't believe these risks are worth the short-term benefits.

There may be temporary, practical steps we need to take along the way to get to the ideal architecture. However, I don't believe polluting the global namespace is the right first step. I believe we should use our existing relationships between plugins in the majority of cases, and use a temporary workaround (which may be a registry!) only for the cases that need it while we iron out the service location problem.

[ppisljar]

@joshdover but the suggested registry is general (so every item it has follows the same interface, provides same set of functions with same input and output types). Consumers of the registry don't need to care which registry item they are using as they are all implementing the same interface. This also means we don't need the testing matrix, each registry item can be tested individually and each registry consumption can be tested individually.

so i don't think you answered points 2. 3. and 4. that you mention above.

[joshdover]

the suggested registry is general (so every item it has follows the same interface, provides same set of functions with same input and output types)

My point is that this interface is so generic that it is not a useful abstraction

migrate: (state: unknown, version: string) => P,

As a consumer, you have to know which shape of object is allowed as an input. The registry provides no information about this. It would slightly better if at least the input type could be a generic of SavedObject, however this registry does not apply even to just top-level SavedObjects, but also other nested shapes, like embeddables, visualizations, extensions, expressions, etc.

The interface "allows" any input type but the implementations of this interface do not actually handle any input type. They only apply to a single type, and those types are not even of the same taxonomy. From an integration perspective, this creates a whole host of problems that I outlined above. An interface that does not provide any useful guarantees to consumers requires that consumers have knowledge of their implementation. That negates the usefulness of having an interface, which means we're now just using the registry for service location.

There is one point about adding a service location mechanism that I did not elaborate enough on. Kibana is a complex system and it's complexity can easily be measured by the relationships between components in the system. If you've ever seen an attempt at creating an architecture diagram for Kibana you can attest to this.

By adding additional relationships within the system we increase this complexity, especially when we're talking about potentially very cross-cutting relationships. More relationships results in more cognitive load on developers, maintenance overhead, and more communication required between teams. In general, I think this is something we want to avoid unless there is a very good reason to do so. I believe our software is simply too large to be adding more complex relationships without serious consideration. I have yet to see a compelling reason to do this for this solution at this time, but happy to be prove wrong.

so i don't think you answered points 2. 3. and 4. that you mention above.

2. problem with figuring out when a migration is necessary:
as our state is deeply nested its hard to figure out when a migration is necessary for specific top level state (see comment #67931 (comment)). But this is a problem if we do not use global registries as well. Even if we put additional logic to specific registries it will still be very hard to figure this out, probably even harder as we won't have one place and one way to check for migration functions.

Could you elaborate why this is difficult, other than "there are a lot of things"? Maybe using my example in rudolf#1 as a starting point would be helpful. Why couldn't this pattern be applied at each layer?

3. introducing a set of 'global variables' (the registry items)

The other global registries are different in that the contain items that can actually be used in the same way. This registry could not be used by consumers transparently.

4. introducing new hidden or circular dependencies

I've had a hard time coming up with a realistic example of this that isn't contrived. I think you may be right in that this isn't a significant problem. It is possible for a plugin to depend on a specific migration being registered by another plugin that they do not specifically depend on. In the case, the default no-op migration probably works fine. I suspect there may be a case where causes a bug, but it may be such a hard situation to get into that this reason alone doesn't warrant caution right now.

[ppisljar]

i updated the RFC with some type changes, which hopefully address your concert about unknown ....:
migrate function now takes in an OldState which extends PersistableState. Before this change plugins would not share type information about what an old state looks like as we assumed input state could be anything (due to corruptions etc). We were assuming same interface would be used even when using multiple registries, so i guess we can discuss how the interface of migrate function would look like independently from the question of one or multiple registries.

this means that actual migrate function definitions can now be fully typed, yet we still need to figure out the correct version: (that would be the same if we had per registry implementation):

type MyStateOld = MyStateV76 | MyStateV77 | MyState;

const migrate = (state: MyStateOld, version: string) => {
  let v76 = version === '7.6' ? state as MyStateV76 : undefined;
  let v77 = version === '7.7' ? state as MyStateV77 : undefined;
  let v78 = version === '7.8' ? state as MyState : undefined;
  if (v76) {
    v77 = { objectId: v76.object, val: v76.val };
  }
  if (v77) {
    v78 = { objectId: v77.objectId, value: v77.val };
  }
  
  return v78;
}

regarding consuming the registry: as a consumer you are required to know shape of the input:

• i guess about type changes address that
• how is this different when we use multiple registries ? so lets say embeddables, calling embeddables.migrate(...) has exactly the same problems, you are passing in either visualize state, search embeddable state, lens embeddable state .... registry provides no information about it. And that is actually the point i think, as a consumer i am NOT interested in what i am passing in. what i am passing in is not owned by me, i was just storing in and now i want to pass it to the plugin that will actually know how to make use of it.

with inject and extract function that is even more the case. i am not owning the state and i don't care what how it looks, all i know is that i want to extract the references to it, so i pass the state to the migrate function and get updated version back with array of references.

when you DO know the type of state you are dealing with at compile time (so actual typings are usefull) we are suggesting not to use the registry, but for the plugins to expose their PersistableStateDefinition on the contract and for consumer to import it from there directly. this way the consumer gets full typings and all the guarantees that come with it. Using multiple registries doesn't address this problem either as if i am using the registry (being a single one or multiple) i always loose the full typings and introduce an indirect dependency (so if i was using embeddable registry to get to my migrate function when i am working with visualize embeddable i am introducing an indirect dependency on visualize). So again, this question can be addressed separately from the one or multiple registry dicussion.

2. i think its a hard problem because of the deep nesting of our states. i don't think its hard to figure out how to do it, but implementing it will take time. i also think we will end up still loading most of saved objects a lot of times: if any embeddable, if any agg type, if any expression function, if anything in index patterns, filters, queries, searchsource, ... changed, we need to load every saved dashboard, as we can't know what exactly is it using (but it could use any of those). also this discussion can be done separately from the question of one or multiple registries as the problem exists in both scenarios and addressing it can be done in the same manner in both.

i checked rudolfs code and it makes sense to me. but all that could just as well be implemented as a single registry, just item interface is a bit different, mainly having separate migrate functions per version (and exposing them to the consumer, together with type information about previous state versions)

the downside of his approach (which could probably be eliminated by a simple wrapper function) is that the consumer needs to know exact version of the state he has, which might be easy with saved objects, but is currently impossible with our other storages (url, localstorage) .... which could also be changed if we decide that is a necessity.

In my original proposal i decided to only expose latest state type as anyone outside of the owning plugin should not need to care about old versions, they should always call migrate on the state and thus always work with latest version in code, not needing to know what version the state they got from storage was.

[joshdover]

For anyone catching up here, there are two main questions still under discussion:

• Should we have a global registry for migrations for different classes of state or should each class of state expose a single migration that encapsulates the specific implementations?
• Should migrations be exposed per-schema version or as a single function that can handle state of any schema version?

I do think the back and forth here warrants an in-person discussion and that we should try to make a decision sooner than later.

---

Adding generics does not change how the consumer would use this and it still requires that the consumer manually cast each migrate function to the appropriate types in order to have safety. It also does not change the fact that the inputs are not of the same taxonomy or class, which means these registry items are not interchangeable. The consumer has to know exactly which specific input type is allowed for each migration. They also have to know how to lookup the migration for each type. For example, to migrate a maps embeddable, you have to know how to look up that embeddable migration (eg. persistableStateRegistry.get('embeddable.maps')), rather than simply calling embeddables.migrate(mapsEmbeddableState)

In general, I think casting registry items to a more specific type is an anti-pattern we should be avoiding.

so lets say embeddables, calling embeddables.migrate(...) has exactly the same problems, you are passing in either visualize state, search embeddable state, lens embeddable state .... registry provides no information about it. And that is actually the point i think, as a consumer i am NOT interested in what i am passing in. what i am passing in is not owned by me, i was just storing in and now i want to pass it to the plugin that will actually know how to make use of it.

I'm not sure I follow here. If a embeddables.migrate(...) method existed, then the consumer can call that function with any type of embeddable state and the embeddables plugin should delegate to the correct embeddable implementation based on the type of state it received. This is more generic and requires less knowledge on the consumer side since they do not have to look up the specific embeddable type's migration.

So yes, with my proposal in rudolf#1, we would have migrations associated with multiple registries, however, this enables each registry to expose a single migration that can handle state from any type of its registry items. This does require that the state contains some sort of identifier that describes which type of item it is, but I believe we already have this identifier for rendering.

when you DO know the type of state you are dealing with at compile time (so actual typings are usefull) we are suggesting not to use the registry, but for the plugins to expose their PersistableStateDefinition on the contract and for consumer to import it from there directly.

Correct me if I am wrong, but I believe the primary case we are trying to solve with this RFC is the situation where we do not know the specific type at compile time.

For example, we will know that an object is an embeddable, but we will not know which type of embeddable it is, or if we do, we don't have a direct dependency on that embeddable type (this is the service location problem). I believe in all of these cases, having a generic migration provided by the registry that performs migrations on the base state and then delegates migrations on the type-specific state solves that problem more elegantly. This generic function can also used in cases where we have direct knowledge of the specific type of embeddable an object is.

this way the consumer gets full typings and all the guarantees that come with it. Using multiple registries doesn't address this problem either as if i am using the registry (being a single one or multiple) i always loose the full typings and introduce an indirect dependency (so if i was using embeddable registry to get to my migrate function when i am working with visualize embeddable i am introducing an indirect dependency on visualize). So again, this question can be addressed separately from the one or multiple registry dicussion.

I think losing the full typings is a feature of rudolf#1 not a bug. The consumer shouldn't have knowledge of internal state from components it does not depend on. In Dashboard's case, all the dashboard migration should care about is that the panels contain embeddables, not that the panels contain embeddables of any specific type. This allows embeddables to encapsulate the implementation details of its state which makes refactoring embeddables easier. If done this way, the embeddables plugin should be able to change the schema of its base state without affecting any embeddable implementations or any consumers.

i checked rudolfs code and it makes sense to me. but all that could just as well be implemented as a single registry, just item interface is a bit different, mainly having separate migrate functions per version (and exposing them to the consumer, together with type information about previous state versions)

the downside of his approach (which could probably be eliminated by a simple wrapper function) is that the consumer needs to know exact version of the state he has, which might be easy with saved objects, but is currently impossible with our other storages (url, localstorage) .... which could also be changed if we decide that is a necessity.

In my original proposal i decided to only expose latest state type as anyone outside of the owning plugin should not need to care about old versions, they should always call migrate on the state and thus always work with latest version in code, not needing to know what version the state they got from storage was.

This aspect is a bit separate from the global registry vs. existing registries discussion. However, I do think the versioned migrations are important because it allows us to avoid bugs that could arise from trying to detect which version an object is. If we need that aspect for other types of state for now, we could introduce a separate API like detectVersion(state: unknown) that is used to then call the correct migration function(s).

But I think ideally, we'd start storing a version in all state that we know we will need to migrate in the future (I think we had some agreement over Zoom on this before). Once that is in place, we can easily remove the detectVersion pattern without having to change the rest of the interface.

[ppisljar]

closing as we decided to imlement this on each service that needs it