Remove Dashboard Import/Export API

rudolf commented 5 years ago

The dashboard import/export API has been superseded by the saved objects import/export API

Since this change introduces a new API endpoint and file format (json -> ndjson), dashboards will have to be re-exported using the new API.

This issue tracks the deprecation and removal of the API as well as the migration of any consumers.

[x] [7.2] Display UI warning and log deprecation message on server when importing legacy json files using the UI https://github.com/elastic/kibana/pull/33513
[x] [7.15] Add deprecation docs https://github.com/elastic/kibana/issues/102223
[x] [7.16] Add deprecation warning log and usage data when calling the API https://github.com/elastic/kibana/pull/111283
[ ] Remove API
[ ] Remove documentation /docs/api/dashboard/import-dashboard.asciidoc and /docs/api/dashboard/export-dashboard.asciidoc

Pending consumer migrations:

[x] Beats https://github.com/elastic/beats/issues/20672

Note: this issue was previously conflating the Dashboard Import/Export API with the legacy json import files which are two separate features. This issue only tracks the Dashboard import/export API removal, the legacy import feature is being tracked in #103921

elasticmachine commented 5 years ago

Pinging @elastic/kibana-platform

rudolf commented 5 years ago

@urso With which release would beats be able to migrate over to the new API, i.e. which release can we target for the deprecation?

joshdover commented 5 years ago

Beats is tentatively planning to remove this feature completely. We should ensure this API continues to exist in all of 7.x, but can move forward with deprecating now.

rudolf commented 4 years ago

We didn't want to mark this API as deprecated before 8.0 because that would mean that users using a completely up to date stack would receive deprecation warnings they can't do anything about.

pgayvallet commented 4 years ago

Makes sense. Should we just remove it in 8.0 as a 'breaking' change then, or do we want to support it for a time after 8.0 is out?

lukeelmers commented 3 years ago

Beats has moved away from this API in https://github.com/elastic/beats/pull/27220, which looks like it will make the cut for 7.15 🤞

Should we just remove it in 8.0 as a 'breaking' change then, or do we want to support it for a time after 8.0 is out?

After further discussion, we've decided we will not remove this in 8.0, but rather we will:

Mark the API as deprecated
Add telemetry so we can track remaining usage of it now that Beats is no longer relying on it

We are okay with this tradeoff because of the relatively low cost of maintenance for the API. I will update the issue description here accordingly; feel free to chime in with any corrections @Rudolf @pgayvallet @kobelb

pgayvallet commented 3 years ago

Add telemetry so we can track remaining usage of it now that Beats is no longer relying on it

What kind of telemetry we ideally want to add here? Should we use an existing (or new) collector somehow? Do we want to use a usage counter?

rudolf commented 3 years ago

I've added usage data in https://github.com/elastic/kibana/pull/111283

But thinking more about this, I think we need to keep pushing to remove it in 8.0.

Keeping the code around has a relatively low cost, but supporting it won't:

the legacy import API doesn't have any of the conflict detection of the new API https://github.com/elastic/kibana/blob/master/src/core/server/saved_objects/import/import_saved_objects.ts#L97-L117 which can lead to cryptic 500 errors when we introduce sharing to multiple spaces in 8.0
It doesn't set originId on imports
It doesn't execute import hooks (maybe unlikely that someone uses this API with types that use hooks, but still a possibility)

In this case, we won't be helping any users by keeping this API around but it's behaviour is badly tested and definitely not compatible with a multi namespace world.

So if usage data shows significant amount of users using this, we will have to invest time into improving this API if we don't remove it.

rudolf commented 3 years ago

Or alternatively, we need to clearly document the impact/shortcomings of using this API

pgayvallet commented 3 years ago

Keeping the code around has a relatively low cost, but supporting it won't:

Imho supporting the code is the same as keeping the code, so that means that keeping the code comes at a heavy cost.

In this case, we won't be helping any users by keeping this API around but it's behaviour is badly tested and definitely not compatible with a multi namespace world.

++

Or alternatively, we need to clearly document the impact/shortcomings of using this API

We need to at least do that. But it would feel safer to be allowed to just drop the API imho.

lukeelmers commented 3 years ago

Keeping the code around has a relatively low cost, but supporting it won't

These are all good points, and I don't think the lack of support for these features was considered in the original discussion around not moving forward with the break.

@kobelb What are your thoughts on this?

The API doesn't require any active "maintenance", however it isn't as feature complete or well-tested as the regular import/export API, which could lead to unexpected bugs or confusion for users
We only just added telemetry so we don't yet have a good idea of how heavily it is used (we know that Beats, our main internal consumer, has now moved off it)

If we proceed with the plan of not making the breaking change, our only option for preventing possible confusion will be to document the caveats.

kobelb commented 3 years ago

Keeping the code around has a relatively low cost, but supporting it won't:

Imho supporting the code is the same as keeping the code

Agreed.

Is my understanding correct that this API is undocumented? If so given the fact that there is a high-cost to keeping around the compatibility layer, I'm alright with us removing it in 8.0. However, I do think that we need to integrate it with the upgrade assistant deprecations in 7.16 so the user is aware that it's being removed and what HTTP API they should use instead.

kobelb commented 3 years ago

I'm checking with Mark to ensure he's comfortable adding this to the list of 8.0 breaking changes, and I'll reply here with his response.

kobelb commented 3 years ago

Mark is alright with us removing the HTTP API in 8.0. However, he'd like for us to add telemetry in 7.16. This will give us a window of time to measure whether our assumptions are correct that this HTTP API isn't used widely and potentially reverse coarse and reintroduce this HTTP API.

rudolf commented 3 years ago

Re-opening since https://github.com/elastic/kibana/pull/111283 only deprecated the API but didn't remove it

pgayvallet commented 2 years ago

@rudolf do you know if we're safe to now remove the API in 8.1 or 8.2? Do we still have any consumers of the dashboard import/export in 8.x?

rudolf commented 2 years ago

Percentage of clusters using this API:

7.17.0 0.6%
7.16.3 1.5%
7.16.2 1.2%
7.16.1 2.5%
7.16.0 0.2%

I'm not sure what exactly our cutt-off would be, but these usage numbers seem pretty low to me. Given the cost and risks outlined above I don't think these justify keeping the API around.

lukeelmers commented 2 years ago

I'm not sure what exactly our cutt-off would be

I think our rough rule of thumb is < 1%, but based on our earlier discussions on this thread, it sounds like we were already comfortable removing it. I'm also unclear on whether the fact that this didn't merge for 8.0 means we need to stick with the newer, longer deprecation timelines, or if simply meeting the telemetry thresholds is sufficient. cc @stacey-gammon WDYT?

stacey-gammon commented 2 years ago

Unfortunately, since it didn't make it into 8.0, I think we need to stick with the new deprecation policy which is 18 months and getting breaking change approval. I think the process for getting approval is still TBD. Let me check and I'll loop back.

TinaHeiligers commented 3 months ago

elastic / kibana

Remove Dashboard Import/Export API #41439