Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Persistable state services] Support migrations, exporting references, and telemetry for visualizations that aren't part of the visualization library #62950

Closed
stacey-gammon opened this issue Apr 8, 2020 · 11 comments
Labels
discuss loe:needs-research This issue requires some research before it can be worked on or estimated Meta NeededFor:Security Project:ShareToSpace Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@stacey-gammon
Copy link
Contributor

stacey-gammon commented Apr 8, 2020

What

A generic solution for the problems that come with registries that contain state that may end up persisted somewhere, usually in a saved object (although could also be in a URL bookmark).

Any time someone registers something that includes state that may be persisted they need to handle:

**1. Saved object references (inject/extract) **

Consider a dashboard which supports dynamically adding embeddables and storing this state. This state may contain references to other saved objects. If the user wants to export this dashboard, or copy it to another space, the dashboard needs to know if any of the state it has pulled off a registry contains a reference to a saved object.

2. Migrations

Similar to the above example, any persisted state needs to know how it will migrate old state to maintain BWC. We don't want the author of an expression function to change the state they support and accidentally break all existing saved objects that contain older state.

**3. Telemetry **

Since the container state doesn't know details for the nested state, it needs to provide telemetry information so we can support the same level of telemetry we have today. For example, a visualization nested "by value" inside a dashboard should still be counted towards the telemetry that counts the types of visualizations that are by reference.

Implementation plan

Rather than implement a global solution, we are implementing this on a per registry basis. Once we have a few concrete end to end examples, we can determine if a global plugin is necessary, or if we can make do with helper utilities.

Current registries which would need to switch over to this

  • Url Generators
  • Expressions
  • Embeddables
  • UI Actions

Related

@joelgriffith
Copy link
Contributor

@stacey-gammon this popped up in unlabelled issues. I can definitely try my best to label this, but you'll probably do better

@joelgriffith joelgriffith added the loe:needs-research This issue requires some research before it can be worked on or estimated label Apr 8, 2020
@stacey-gammon stacey-gammon added discuss Team:AppArch Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc labels Apr 8, 2020
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform (Team:Platform)

@streamich
Copy link
Contributor

What would be an example, for example, in Expressions? When you say "registries that contain state that may end up persisted somewhere" do you mean that for example an alert can be created using kibana | kibana_context | esaggs .... expression and persisted in database and then basically all functions (kibana, kibana_context, esaggs) cannot change their names, nor parameters in the future anymore?

@stacey-gammon
Copy link
Contributor Author

stacey-gammon commented Apr 9, 2020

Expressions are tricky because the string is user visible. I had a conversation with @timroes about this as well. The options we have are:

  • Use the name of the expression function as the id
    or
  • Include a version number somewhere in the expression string

Neither are good options for expressions, but I prefer the first.

We can also offer some additional options specific to expressions, like introducing the concept of deprecated arguments. A dev could add a new one, e.g.

Version 1

esaggs indexPattern="123"

version 2

esaggs indexPatternRefs={...}

The function itself would need handle on the fly migration from the deprecated argument to the new one. Auto complete would only show the new version.

Still, there could be times where that alone wouldn't be sufficient. Providing these capabilities would give us an out for those, hopefully very rare, situations.

I'm open to other options, I just really don't like the idea of storing a version number in the expression string. I think it's cleaner to have a migration from an old expression function to a new one, even though that creates complications with choosing a good name.

do you mean that for example an alert can be created using kibana | kibana_context | esaggs .... expression and persisted in database and then basically all functions (kibana, kibana_context, esaggs) cannot change their names, nor parameters in the future anymore?

Not quite, the out for migrating is to change the expression functions name. We have these expression strings stored in saved objects already and no formal way to migrate them. Like I said earlier, I think we would first try to stick with the same name and do some magic inside the expression function itself to convert to a new version. Another temporary solution we could opt for now is to migrate the saved objects that we know about. There is just no guarantee that we know all the saved objects expression strings are stored in. There is also no guarantee that we know about all the expression function migrations that need to take place (what if one third party developer wants to migrate their expression function, and another third party developer is storing it in a saved object - what could we do to help the developer write a migration without breaking all the other developers saved objects?)

@ppisljar
Copy link
Member

ppisljar commented May 6, 2020

i think the key here is the difference between expression and persisted expression.

if our persistable state plugin exposes a registry, where every plugin can register its persistable state id together with:

  • migration function migrate(state: unknown): MyPersistableState
  • reference extraction function extract(state: MyPersistableState): [state: MyPersistableState, references: SavedObjectReference[]]
  • reference injection function inject(state: MyPersistableState, references: []): MyPeristableState

then the persister (consumer of persistable state who wants to persist it) can lookup the persistable state in this registry and:

  • run extract() when persisting
  • run migrate(inject()) when loading

for the expressions, expression plugin would register its state expression'. Until you are persisting the expression (or any other state) you don't need to worry about it, but when you do you call the extract function on the expression (naming should probably be different, maybe something like prepareForSave`).

in case of expressions this function would:

  • attach kibana version to expression string
  • walk the AST tree and check the persistable state registry for any expr_fn_{function_id} and run its prepareForSave if its found.

If expression function name would change, we would add an entry to persistable state registyr with the old function name and the migrate function would just return the new function name (with new parameters)

@mattkime
Copy link
Contributor

After getting up to speed on this I have two questions - do we have agreement on the problem definition? If so, can we start expressing this in code? I think the problem has enough complexity that we might need some discussion regarding the optimal code expression of the problem even before we work on a solution.

@stacey-gammon
Copy link
Contributor Author

stacey-gammon commented May 12, 2020

Yes, this should go through an RFC with code examples, and an arch review. @ppisljar has started a presentation with some code examples. We talked this morning regarding this gist: https://gist.github.com/stacey-gammon/ad6e60637404e779bdaba8502e54ac03 and related questions.

  1. Should registrators register a single migration function to cover all versions, or a migration function per version?

  2. How do we handle situations where there is shared state like how EmbeddableInput is the base interface for specializations.

For 1. we decided going with a single migration function is better because it eliminates the possibility of a nested migration function being skipped. If you had it per version, you could get into this situation:

// EmbeddableInput gets migrated from 7.1.0 to 7.3.0. There were no migrations registered
// for 7.2.0.
persistableStateMigrations.register('embeddable', '7.1.0', (state) => {
  return {
    newVersion: '7.3.0',
    newState: {
      // Migrate `EmbeddableInput` state
      ...migrateEmbeddableInput710To730(state.input),
      enhancements: {
        ...state.input.enhancements.map(
            key => persistableStateMigrations.migrate(`embeddableEnhancements-${key}`, '7.1.0').newState)
       }
     }
  };
}

// However, an enhancement registered a migration for 7.2.0. It never gets called!
persistableStateMigrations.register(`embeddableEnhancements-${key}`, '7.2.0', (state) => {
  return { newState: migratedStateTo730(state ),  newVersion: '7.3.0' };
});

So we have to do two things:

  1. Always call a migration function for nested state.
  2. Maintain state boundaries along plugin boundaries. Do not allow VisualizeEmbeddableInput to migrate data on EmbeddableInput. Don't even give it access to this state at all.

As long as state ownership is isolated, we can do something like this:

const embeddableMigrator: PersistableStateMigrationFn (state: { type: string; input: unknown }, version: string): State {
  return {
    ...migrateInputToLatest(state.input),
    enhancements: {
       ...state.input.enhancements.map(enhancement => 
          persistableStateMigrations.get(enhancement).migrate(state.input.enhancements[extension], version)),
    },
    specializedState: persistableStateMigrations.get(state.type).migrate(state.input.specializedState, version)),
  }
}

The key here is that we have to separate out the state owned by VisualizeEmbeddableInput and that owned by EmbeddableInput. If we allow them both access to the full input (VisualizeEmbeddableInput extends EmbeddableInput), there is the possibility for key clashes. For instance:

// We start out with the base class having an attribute of `title`. 
interface EmbeddableInputV1 {
  title: string;
}

// `Title` is taken by the base class, so Visualize plugin adds `defaultTitle`.
interface VisualizeEmbeddableInputV1 implements EmbeddableInputV1 {
  defaultTitle: string;
}

// In a future version, EmbeddableInput author decides to rename `title` => `defaultTitle`.
// They don't know about `VisualizeEmbeddableInput` so don't realize there is going to be
// a key collision.
interface EmbeddableInputV2 {
  defaultTitle: string;
}

It's probably a rare occurrence, but it's also very risky. EmbeddableInput thinks it owns defaultTitle. Let's say it decides to change the name yet again, so in V3 changes it to customPanelTitle and removes defaultTitle. Let's also say VisualizeEmbeddable author doesn't add a migration for V2 or V3. A user migrates to V3, VisualizeEmbeddable just had its defaultTitle state wiped out unknowingly (types will still say it's a required parameters). This has the potential to permanently break saved objects with no way to undo the migration.

tl;dr: We need to avoid shared persistable state. We can't do things like have base interfaces that other plugins extend, if that state is going to be serialized. We should fix embeddables. We might want to think of not using input as the serialized state, but a function that deserializes/serializes so we can continue to operate on runtime code as if it's VisualizeEmbeddable extends EmbeddableInput.

cc @streamich @Dosant @peterschretlen

@stacey-gammon stacey-gammon changed the title Persistable State Plugin [Persistable state services] Support migrations, exporting references, and telemetry for visualizations that aren't part of the visualization library Sep 10, 2020
@ppisljar
Copy link
Member

closing this due to inactivity, please re-open if its still relevant

@timroes
Copy link
Contributor

timroes commented May 10, 2021

@ThomThomson do we have a separate issue for catching proper telemetry from embedded visualizations (so we can collect the same telemetry that we do on visualization library) besides this one? Or was this the one used for tracking that effort?

@ThomThomson
Copy link
Contributor

@timroes, I believe this issue is where we're tracking that: #86396

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss loe:needs-research This issue requires some research before it can be worked on or estimated Meta NeededFor:Security Project:ShareToSpace Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

No branches or pull requests

8 participants