[maps] saved object migration to migrate tile_map and region_map visualizations to maps #105115
Comments
nreese
added
the
[Deprecated-Use Team:Presentation]Team:Geo
|
Pinging @elastic/kibana-gis (Team:Geo) |
|
ping @pgayvallet |
nreese
added
the
Team:Core
|
Pinging @elastic/kibana-core (Team:Core) |
pgayvallet
added
discuss
project:ResilientSavedObjectMigrations
|
note: if you don't need all the context, you can jump to We already have an issue about allowing type renaming (here: #91143). However in the existing issue, the only need was to rename the type of ALL SOs of a given type to another in a given release, without further changes. In theory, the SO migrator does already allow to change the type of a document during its migration. E.g during a However the need here is more complex, due to the simple fact that we only want to migrate a subset of the The SO Note that when thinking about the correct approach to potentially solve the issue at stake here, we also need to take SO import/export into account, as due to its amazing design, it's effectively possible to import objects without their references
So we basically have to be able to handle both
I see two possible directions if we want to do that conversion, neither of them being perfect unfortunately. Collect and expose a migration contextAs we need to know which subset of visualizations will need to be converted to maps (for the references rewrite), we could (in addition to #91143), add the concept of 'migration context' that would be passed down to the migration functions, and add an API for type owners to register context gathering hooks. We're already providing a I think the easiest way would be to expose a handler that would have access to a (subset of the) SO client configured to hit the core.savedObjects.registerMigrationContextCollector = <T>(domain: string, collector: SavedObjectContextCollector<T>): void
type SavedObjectContextCollector<T> = (info: ContextCollectorInfo): Promise<T>;
type ContextCollectorInfo = {
client: CollectorClient;
}
type CollectorClient = Pick<EsClient, 'whatever' | 'we' | 'want' | 'to' | 'expose'>and then we'll update the type SavedObjectMigrationContext {
// already present
readonly log: SavedObjectsMigrationLogger;
readonly migrationVersion: string;
readonly convertToMultiNamespaceTypeVersion?: string;
// added
readonly migrationContext: Record<string, unknown>; // I don't think we can easily do better than unknown
}If that is achieved, in addition to allow type owners to register The biggest problem here is the interactions with the import/export API: I'm not sure how we would be able to make that work with the import API, as we're using 'on-create' migration for this, meaning that the context will not have knowledge of the documents that were present in the import file. E.g
Also note that as the documents are migrated atomically (we apply all migration for a given doc then move forward to the next one), the logic that determine which visualizations gonna need to be migrated to maps will have to work for any initial version (e.g from cc @jportner in particular: I also see we're currently not calling the Add a new post migration hook/APIA solution that could sounds simpler (probably due to the fact that's it is uglier) would be to expose a new API to register 'post-migration' hooks, where consumers would register a function to execute an arbitrary migration 'script' that would have access to the SO client. The API could look like core.savedObjects.registerPostMigration = (hook: SavedObjectPostMigrationHook): void;
type SavedObjectPostMigrationHook = (context: SavedObjectPostMigrationHookContext): Promise<SavedObjectPostMigrationHookResult>
interface SavedObjectPostMigrationHookContext {
client: SavedObjectClientContract;
// other things?
}
interface SavedObjectPostMigrationHookResult {
// TBD
}The pro is that is could easily be plugged into the import/export system, as we could just invoke the same hook after a import against the cluster with the imported objects. The main con is that this would be performed after the whole migration, meaning that it would need to support manually applying the migration that may have been skipped. To explain more in details:
In that case, the post hook will effectively only be called after the complete migration of all the documents up to Given that there is no reliable way to know which version the migration was initiated from (the EDIT : I need to stop thinking about SO migration during my week-ends, but: Option 3: detect id renames automatically and perform reference rewrite in a second passJust though of it, but: This goes in-between the two previous options. Given that, in theory, the doc migration does support type renaming, I'm wondering if we couldn't just try to automatically detect id/type renaming during the migration, and trigger a references rename automatically as a second pass. I think it may actually be the easiest way. The idea is the following:
`7.x.x`: (doc) => {
if(shouldChangeToMap(doc)) {
return {
...doc,
attributes: convertToMap(doc.attributes),
type: 'map',
id: doc.id // I don't think we need to do anything in the id
}
}
}
This could even totally replace the One of the edge case for the migration would be if it fails between before the second-pass is applied, as in that case, if the docs are already in the target index, we would loose the information to allow references rewrite during the second attempt, so I think we need to do that in the temporary index to avoid such risk. It would invalidate #104080 btw. cc @elastic/kibana-core @jportner @kobelb Please tell me if you see any other option, or if I said anything wrong (or forget anything) @jportner especially, as our official honorary expert of the SO migration, what do you think about this third option? Did I miss anything that would make it not work? |
|
If we can make it work, I strongly prefer going with a solution that is handled directly by the Saved Object migration infrastructure over one that requires type owners to implement any custom interactions with Elasticsearch or SavedObjectsClient. I fear that opening that box would only increase the fragility of this system and make auditing our migration code for issues much harder. The more centralized, well-tested migration features we can provide at the Core level, the better IMO. That said, I also want to be sure that we choose a path forward that will be coherent with any other migration features we need to add and support in the future. With that in mind, of these options, I only think the 3rd one is going to be safe 'enough' to be considered. An open ended post-migration hook is likely the wrong long-term direction for the migration framework and IMO the migration context option seems unlikely to scale. Wouldn't you need to preserve the context for the entire migration, meaning that each batch of objects would make the context grow?
I'm curious if we should still be doing things this way. It does seem to me that if we have any migrations that have interactions across type boundaries that we're going to need to upgrade the documents version-by-version rather than type-by-type. This way the behavior is deterministic regardless of which version you're upgrading from. Tangential to this topic, I'd like to have a dedicated API for anything like renaming types or splitting types over special behavior based on the return value. The purpose of a dedicated API would be to make these actions very explicitly chosen by the developer rather than something that is happening 'by accident' because of a return value. It also helps with discoverability of the features provided by the migration framework. IMO an ideal "migration-on-rails" API might look something like this: core.savedObjects.registerType({
name: 'visualization',
mappings: {},
migrations: {
'8.0.0': core.savedObjects
.buildMigration()
.renameField('fieldA', 'fieldB', 'default-value')
.convertToType({
newTypeName: 'map',
filter: (doc) => isLegacyMap(doc),
transform: (doc) => transformLegacyMapToNewMap(doc)
})
.customTransform((doc) => myCustomTransform(doc))
}
});The function that is returned by this |
@pgayvallet Yeah that's intentional. We only use the If you have exported an object from 7.14, and that is converted in 8.0, and then you want to import it in 8.1: the import code will check for conflicts based on object ID and origin ID fields
I think so!* See (1) below
Agree.
Two things come to mind: 1) "Changed Types/IDs" map must be cumulative and space-awareFor example, in a 7.14 instance, there are two visualizations and two dashboards (using raw document IDs for clarity): { type: 'visualization', id: '123' }
{ type: 'dashboard', id: '456', references: [{ type: 'visualization', id: '123' }] }
{ namespace: 'foo', type: 'visualization', id: '123' }
{ namespace: 'foo', type: 'dashboard', id: '456', references: [{ type: 'visualization', id: '123' }] }These are single-namespace types. (Note: The plan is to convert them to "share-capable" The user upgrades their instance from 7.14 to 8.2. { namespaces: ['default'], type: 'visualization', id: 'ffcee6e4' }
{ namespaces: ['default'], type: 'dashboard', id: '7d598c64', references: [{ type: 'visualization', id: '123' }] }
{ namespaces: ['foo'], type: 'visualization', id: 'dadccda1' }
{ namespaces: ['foo'], type: 'dashboard', id: '1fc8a162', references: [{ type: 'visualization', id: '123' }] }In 8.2 we change the visualizations to maps: { namespaces: ['default'], type: 'map', id: 'ffcee6e4' }
{ namespaces: ['default'], type: 'dashboard', id: '7d598c64', references: [{ type: 'visualization', id: '123' }] }
{ namespaces: ['foo'], type: 'map', id: 'dadccda1' }
{ namespaces: ['foo'], type: 'dashboard', id: '1fc8a162', references: [{ type: 'visualization', id: '123' }] }After all of that is complete, then in the second-pass we need to change any reference to visualization '123' in the default space to map 'ffcee6e4', and we need to change any reference to visualization '123' in the foo space to map 'dadccda1'. (We should also account for the "*" space.) Result: { namespaces: ['default'], type: 'map', id: 'ffcee6e4' }
{ namespaces: ['default'], type: 'dashboard', id: '7d598c64', references: [{ type: 'map', id: 'ffcee6e4' }] }
{ namespaces: ['foo'], type: 'map', id: 'dadccda1' }
{ namespaces: ['foo'], type: 'dashboard', id: '1fc8a162', references: [{ type: 'map', id: 'dadccda1' }] }2) Consumers can no longer reliably write their own migrations that leverage reference types or IDsThis may be a minor point, but: the current algorithm for If use this "second-pass", then consumers can run into problems when trying to manipulate references on their own. In the example above, if the consumer added a dashboard migration for 8.1 to, say, update the In general I really don't like this fact -- the second-pass means that updating from 7.14 to 8.2 is not the same as upgrading from 7.14 to 8.0, then upgrading from 8.0 to 8.2. We can't avoid this by doing a "second-pass" between each version, because all of the migrations are applied to the saved object on the client side before it is written to the temporary index.
@joshdover I agree |
|
Another route would be to avoid the SO migration. #105326 is a proof of concept of creating a tile_map visualization implementation that is just a thin wrapper around the MapEmbeddable. If we go this route, then the saved objects can stay as-is. Thoughts? In this screen shot, the visualization is rendered using MapEmbeddable. There is no way to edit the visualization unless you open in maps. In this screen shot, the tile_map visualization saved object is rendered using MapEmbeddable. |
Thanks joe for the detailed example. Following type/id rename in-between version doesn't seem that complicated. However I agree with you both that this is not ideal given the constraints it would create and the effective limitations that this will apply on the future migration APIs we would be able to expose to type owners. Thinking it thought, it I had to choose one approach, I would still go with option core.savedObjects
.buildMigration()
.renameField('fieldA', 'fieldB', 'default-value')
.convertToType({
newTypeName: 'map',
filter: (doc) => isLegacyMap(doc),
transform: (doc) => transformLegacyMapToNewMap(doc)
})
.customTransform((doc) => myCustomTransform(doc))This would be some work, but could be a possibility (saying only could here because every time we want to touch the document migrator, we discover another limitation or problem that this would introduce)
If we do that, we would still have to limit what the current migration functions are allowed to do. Atm you can rename an id and/or a type from a custom migration function (even if effectively never used i believe/hope). If we were to add a fluent API and especially specific methods to changeId/renamedoc, I'm not sure we could still be able to support it from custom transforms, as those specific transformations would trigger additional actions from the migration system (e.g references handling when changing an id). I don't even understand why migration functions are allowed to migrate more than just Overall, the more I'm thinking about it, the more I think the migration should be handled on a per-version basis internally. When migrating to The point is, even if we were to agree that this would be the correct approach (which I'm not even sure) designing and implementing it would realistically never be done for 8.0 I kinda hate it to be honest. id/type rename, document splitting, documents merging, all of these feels like valid migration needs to me, but our system simply can not handle them, and we don't have ANY correct solution to implement them (also, there is the import/export interactions that don't help a bit) In short, if you do have an alternative that doesn't rely on a migration for this conversion, I think it would probably be our best shot. I really don't know the actual implication of your adapter/wrapper pattern, but in a pure design point of view, it sounds good to me, and if you don't foresee any future problem this could cause later, I'd say go with it. A question though: did you think about future map/vis migrations? How would you handle |
|
Sounds like we're generally in agreement that option 3 is likely the most viable path forward to supporting this feature before 8.0 given the current design of the system. In order to make a decision here we're going to need answers to:
|
I agree 100%. We need to be performing migrations in the most deterministic way possible so that we can be sure that they continue working regardless of the upgrade path a cluster takes. As it is, we already have too many variables (eg. disabled plugins) that can impact what operations will happen during a migration. I think this change will need to be a focus for the team during 8.x so that we can support these "table stakes" migration features. While performance is a concern, I think if we can make upgrades rock-solid and seamless, there are other potential ways to work around a slower migration process (eg. auto-scheduled upgrades during off-peak hours). |
I will keep on pushing it to make sure there are no unseen implementation problems.
The tile_map visualization saved object is converted to maps configuration in code when run. We will add functional tests make sure changes to either visualization saved objects or maps configurations do not break things. Then we can just update the logic that converts tile_map visualization saved object is converted to maps configuration if needed. |
I think the UI concerns are limited. For users consuming tile_map and region_map visualizations in Dashboard and Canvas, there will be no UI problems as the panel will just render as a normal map embeddable. I agree there may be some confusion when users edit tile_map and region_map visualizations, but, again I think this is limited. Since 7.10, tile_map and region_map have displayed a large ugly deprecation message encouraging them to migrate to Maps. @kmartastic What are your thoughts on UI issues with the proposed solution of making tile_map visualizations wrappers around map embeddable? |
|
@pgayvallet @joshdover @jportner I wanted to give an update on moving tile_map and region_map visualization implementations to MapEmbeddable. The PR has progressed well and it has been verified that the implementation will work for both tile_map and region_map. The only UI wrinkle is that editing tile_map and region_map visualizations will require users to manually save the tile_map and region_map visualization as a Map saved object and then manually replace the tile_map and region_map panel in Dashboard or canvas with the new Map saved object. @elastic/kibana-gis discussed this at todays team sync and is ok moving forward with the solution proposed in #105326. This means, that for the 8.0 time frame, from @elastic/kibana-gis's perspective, saved object migration work needed to support migrating tile_map and region_map visualization saved objects to maps saved objects can be pushed down in priority. |
jb1b84
added
Feature:Maps
Team:Presentation
|
Pinging @elastic/kibana-presentation (Team:Presentation) |
nreese
added
the
impact:medium
|
With serverless / ZDT, we now know for sure that type renames are technically not doable. Given that and the fact that 8.0 is way far behind us now, I'll assume we no longer need this and will close it. |
tile_map and region_map visualizations are getting replaced with maps in 8.0. Saved object migration required to migrate tile_map and region_map visualizations to maps
download saved object export
- tile_map visualization saved object
- tile_map visualization saved object as map saved object
- region_map visualization saved object
- region_map visualization saved object as map saved object
- dashboard saved object with by reference tile_map and region_map panels
- dashboard saved object with by reference map panels
- dashboard saved object with by value tile_map and region_map panels
- dashboard saved object with by reference map panels
The text was updated successfully, but these errors were encountered: