elastic / kibana

Your window into the Elastic Stack
https://www.elastic.co/products/kibana
Other
19.67k stars 8.1k forks source link

Disable legacy url preflights logic when not required #112492

Open LeeDr opened 2 years ago

LeeDr commented 2 years ago

Describe the feature: There's ongoing work related to "Prevent creating saved object legacy URL conflicts". Without going into the details of that work, it seems like it might be very useful if we kept track of the "initial version" of a Kibana instance somehow. My first thought is that it would be added to the config doc in the .kibana index, but there could be better alternatives.

Describe a specific use case for the feature: The use case would be that if a new Kibana instance was created on version 8.+, the code could know that there weren't any legacy URL alias conflicts and could potentially save an Elasticsearch query every time a saved object was opened. Reference: https://www.elastic.co/guide/en/kibana/master/sharing-saved-objects.html#sharing-saved-objects-faq-resolve-outcomes

Slight complication if I create a 8+ Kibana instance and then import saved objects from a previous version. Since we never tracked this before, I think we would just have to remove the initialVersion from the config doc and fall back to the behavior when we don't know what version we started with.

We currently have some fields in the config docs like this (from 7.15.0);

coreMigrationVersion: 7.15.0 migrationVersion.config 7.13.0

We actually have a config doc per space. But there's always a default space config. Maybe that's all we need.

//cc @jportner

jportner commented 2 years ago

Describe a specific use case for the feature: The use case would be that if a new Kibana instance was created on version 8.+, the code could know that there weren't any legacy URL alias conflicts and could potentially save an Elasticsearch query every time a saved object was opened.

In this situation, we could also skip querying for conflict scenarios before create / bulkCreate (planned in a new RFC but not implemented yet).

Slight complication if I create a 8+ Kibana instance and then import saved objects from a previous version.

Actually that's not an issue, saved objects are only "converted" (with new IDs and aliases) when Kibana is upgraded. Importing old saved objects won't have that effect.


There is a risk that if we store this info in the config saved object, if that object gets deleted for any reason, then Kibana will recreate it and start behaving differently. Right now the only use case for different behavior is legacy URL aliases, but if we add other use cases this could potentially wind up having a lot of impact on a user. I wonder if it would be better if we wanted to do this to use a different hidden, space-agnostic saved object type to store this information. I'm curious to see what @elastic/kibana-core thinks.

Another option, if we didn't want to store this info in a saved object: Kibana could query Elasticsearch for any legacy URL aliases on startup, and if it doesn't find any, it could flip a switch to skip the aforementioned queries. Just spitballing 😄

rudolf commented 2 years ago

One risk I see with this is that in development we have a different performance profile from what most of our 7.x to 8.x users would experience. But we could always switch on conflict detection in development mode.

I like that this gives us a narrative to encourage users to remove aliases "To improve the performance of Kibana, consider deleting the following legacy url aliases which haven't been used in the last 90 days."

Since a legacy url alias "targets" an object in a specific namespace, I think we might be able to make this behaviour more granular and only apply conflict detection to namespaces which have legacy url aliases pointing to them?

This shouldn't be something a user can configure, and ideally it would automatically be switched on once a cluster (or namespace?) no longer has any legacy url aliases. So using a startup query sounds like the best way to achieve that, if a user deletes all url aliases and restarts Kibana conflict detection will automatically be switched off.

The only downside is that it's not so easy to inspect whether a given Kibana is using conflict detection or not, so we should probably have an info log message on startup to help us should be ever have to debug an issue with a user.

pgayvallet commented 2 years ago

the code could know that there weren't any legacy URL alias conflicts and could potentially save an Elasticsearch query every time a saved object was opened

Which exact checks do we plan to disable here if the (TBD) criteria match? I'm guessing all legacy alias related checks? so the ones performed during resolve +bulkResolve, but also (not implemented yet) for create/bulkCreate?

Kibana could query Elasticsearch for any legacy URL aliases on startup, and if it doesn't find any, it could flip a switch to skip the aforementioned queries

++, Seems more solid to me than relying on a static value stored on some doc.

Since a legacy url alias "targets" an object in a specific namespace, I think we might be able to make this behaviour more granular and only apply conflict detection to namespaces which have legacy url aliases pointing to them?

We got APIs potentially targeting multiple spaces (e.g you can bulkCreate objects with distinct initialNamespaces). If this would make the preflight check implementation too complex, I would probably avoid doing this per-namespace.

pgayvallet commented 2 years ago

As a side note, we should probably rename the issue to reflect the real problem. Disable legacy url preflights logic when not required?

LeeDr commented 2 years ago

Kibana could query Elasticsearch for any legacy URL aliases on startup, and if it doesn't find any, it could flip a switch to skip the aforementioned queries

This has the added advantage of disabling the alias checks in Kibana instances which did start on an earlier version but didn't use spaces and therefor didn't create any URL aliases.

pgayvallet commented 2 years ago

@rudolf @jportner Do you think the perf improvement is significant enough that we want to prioritize this for early 8.x?

rudolf commented 2 years ago

At this point it's just an uninformed guess, but my guess is that most plugins and most users won't see a significant performance benefit. However, heavy users of performance critical plugins like alerting/task manager might and having an escape hatch for them would reduce the potential impact.

So I think we should start by trying to quantify if it would introduce a performance regression for alerting as a kind of case study to understand how much of a difference it could make to other areas.

jportner commented 2 years ago

At this point it's just an uninformed guess, but my guess is that most plugins and most users won't see a significant performance benefit. However, heavy users of performance critical plugins like alerting/task manager might and having an escape hatch for them would reduce the potential impact.

So I think we should start by trying to quantify if it would introduce a performance regression for alerting as a kind of case study to understand how much of a difference it could make to other areas.

Agreed.

elasticmachine commented 1 month ago

Pinging @elastic/kibana-security (Team:Security)

pgayvallet commented 1 month ago

cc-ing to security, do you think this is something that would still make sense today, or shall we just close this issue?

azasypkin commented 1 month ago

cc-ing to security, do you think this is something that would still make sense today, or shall we just close this issue?

It looks like we never actually tried to quantify the performance benefits of these improvements, which means they were never prioritized due to a lack of demand. On the security side, I think it's fine to close this issue and reopen it if it ever becomes a problem worth solving.