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

[index pattern field editor] Runtime field delete in kibana apps #94381

Closed
mattkime opened this issue Mar 10, 2021 · 16 comments
Closed

[index pattern field editor] Runtime field delete in kibana apps #94381

mattkime opened this issue Mar 10, 2021 · 16 comments
Labels
discuss Feature:Data Views Data Views code and UI - index patterns before 8.0 Project:RuntimeFields Team:Visualizations Visualization editors, elastic-charts and infrastructure

Comments

@mattkime
Copy link
Contributor

mattkime commented Mar 10, 2021

Starting 7.13 development the plan was to integrate runtime field delete functionality into kibana apps (lens and discover) in much the same way its integrated into Index Pattern Management. For a variety of reasons we're reexamining this.

Some things to consider

  • Removing (or renaming or changing the type of) a runtime field may break things that depend upon that field
  • Ideally we'd like to say whats impacted, we know which index patterns have dependencies except for a couple of cases (Timelion, TSVB, Vega ?)
    • Knowing this on a field level is a large project that hasn't even entered planning
  • Providing undo functionality in some form would reduce the risk but would be a big effort
  • We could have the user type CONFIRM as part of the delete modal
  • @VijayDoshi Mentioned keeping delete out of kibana apps in line with a long term vision which includes changes that are scoped to the user.
  • We need a plan for 7.13 and may have plans for what we want to do later
@mattkime mattkime added Feature:Data Views Data Views code and UI - index patterns before 8.0 Project:RuntimeFields labels Mar 10, 2021
@elastic-jb
Copy link

I am assuming these runtime fields could still be deleted via the developer console? (with all the same risks) That may be the most reasonable solution while we sort out our plan for how to deal with the dependencies, undo, etc. Do I understand that correct? The baseline proposal would be that you can delete them via the API but not in Kibana for 7.13?

If so, that seems like a pretty safe move to me.

@mattkime
Copy link
Contributor Author

mattkime commented Mar 10, 2021

The proposal from the Kibana App team was to provide delete functionality only from Index Pattern Management.

@elastic-jb
Copy link

My biggest concern is breaking visualizations, regardless of where we put the button. From the recording I watched earlier it sounded like we don't think we can identify downstream objects, and that on a poorly permissioned system, people could delete fields and cause visualizations to fail.

I'm comfortable with the delete button only being in index pattern management, but only with some way to mitigate it. I personally like the undo idea. I am not confident the person typing confirm would understand the consequences. Allowing some way to restore it would make me much more comfortable than relying on random users to either understand or care about the consequences.

@mattkime
Copy link
Contributor Author

The delete button is necessary because runtime fields can cause performance problems. I need to check and see if runtime script errors can break queries or if there is some sort of graceful failure. I think slow queries would be enough of a problem to require the delete button.

My biggest concern is breaking visualizations, regardless of where we put the button.

There's the potential for this with any kind of index pattern defined runtime field, even if the editor isn't exposed in Lens and Discover. Making undo functionality a prerequisite would be a large reshuffling of priorities.

@timroes timroes added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label Mar 11, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-services (Team:AppServices)

@timductive
Copy link
Member

A valid point brought up by @mukeshelastic and @giladgal was to consider the happy path as well for Delete. If a user is creating runtime fields in Lens and wants to quickly delete them, having to go to the management page to delete would be quite cumbersome.

If we choose to keep the Delete functionality out of the apps, I would propose it be temporary while we collect telemetry on behavior and solve the most egregious breakages across the app. I would hate to have the opposite problem that @mattkime mentions, which is we have so many unneeded runtime fields that it affects performance.

@flash1293 @timroes @VijayDoshi

@stacey-gammon
Copy link
Contributor

My preference is to expose delete and rename functionality everywhere, but require the user to type in CONFIRM to commit the action, with a generic warning.

@flash1293
Copy link
Contributor

flash1293 commented Mar 15, 2021

I think we discussed this enough in meetings to have a good idea of the available design space (at least for the 7.13 time frame). IMHO there is no obvious absolute best choice in this design space, so for me I think the solution closest to our targeted long-term solution wins. This way we can keep the interface as stable as possible.

Taking this into account, I agree with Stacey - let's go with a generic warning and collect feedback, then iterate in the area where people are running into issues in 7.14 and 8.0 (if we want to allow people to delete from within apps, which is our long term goal as far as I understood)

@rayafratkina
Copy link
Contributor

That makes sense - let's make sure we have telemetry on how often people delete fields and when visualizations fail to load (not sure if this is something we capture already)

@flash1293
Copy link
Contributor

We are capturing telemetry on the CRUD operations of fields already, but not on loading failures. I will create an issue for this for Lens specifically.

@mattkime
Copy link
Contributor Author

We seem to have general consensus around @stacey-gammon's proposal.

@VijayDoshi
Copy link

I'm in agreement that a generic warning is sufficient for now for both delete and rename. Is renaming just changing the alias though, does it need the same warning/does renaming break existing visualizations?

@mattkime
Copy link
Contributor Author

mattkime commented Mar 16, 2021

@VijayDoshi Renaming will cause the same breakage as deletion. We don't currently have the notion of an alias at this level for working with fields.

@flash1293
Copy link
Contributor

I think the same confirmation makes sense for renaming because of this scenario:

  • User creates runtime field
  • User builds visualization on top of it and is happy with them
  • User edits the runtime field to solve a different problem
  • User changes the name, thinking it will create a new runtime field (instead of overwriting the existing one)
  • User saves
  • All other visualizations break (because the original field is gone)

@mattkime
Copy link
Contributor Author

resolved by #95237

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Feature:Data Views Data Views code and UI - index patterns before 8.0 Project:RuntimeFields Team:Visualizations Visualization editors, elastic-charts and infrastructure
Projects
None yet
Development

No branches or pull requests

9 participants