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

[metrics control] Show error message when chart contains bad data #17238

Closed
graceguo-supercat opened this issue Oct 26, 2021 · 6 comments
Closed
Labels
explore:control Related to the controls panel of Explore nice-to-have Nice to have features or enhancements

Comments

@graceguo-supercat
Copy link

Is your feature request related to a problem? Please describe.
This feature request is related to #17201.

Describe the solution you'd like
When user creates a chart, they can pick one or more metrics from dataset. With time passing by, or same dataset shared with different groups of users, dataset itself may be changed. In airbnb we found many cases that some metrics used by certain charts, which are not aware by other users, are deleted unintentionally. Later when a user open an old chart which contains these metrics, the whole chart is kind of broken. #17201 fixed UI issue. It unblock user to see the chart with valid metrics. But the invalid metric data is still in the Slice entity.

Ideally, Superset should let user know which piece of data is not valid anymore, and ask user to take action to clean up this bad data

Describe alternatives you've considered
#17201 fixed UI issue so that unblock user to see the chart with valid metrics.

Additional context
This issue has been in Superset for a long time: #16719, #14034.

cc @kgabryje @junlincc

@graceguo-supercat graceguo-supercat added nice-to-have Nice to have features or enhancements explore:control Related to the controls panel of Explore labels Oct 27, 2021
@graceguo-supercat graceguo-supercat changed the title [metrics control] Show error message to user when chart contains bad data [metrics control] Show error message when chart contains bad data Oct 27, 2021
@kgabryje
Copy link
Member

@junlincc I feel like changing the flow requires some PM and/or designer attention. WDYT?

@villebro
Copy link
Member

villebro commented Oct 29, 2021

If I understand this correctly, the real issue is the fact that we don't check which metrics are in use when attempting to delete them. To check this, we would need to add link table linking charts to metrics (and why not also calculated columns), similar to how we do between dashboards and charts or charts and datasets. Then we'd just check if the metric is referenced by charts when trying to delete it, similar to how we do when trying to delete a dataset that's referenced by charts:
image

I suspect the reason this hasn't been implemented is due to the dual Druid/SQLA datasource model, but if we remove the Druid connector for 2.0, then this will be much easier implement.

@graceguo-supercat
Copy link
Author

@villebro thanks for the looking this issue.

there are 2 places we can prevent this issue:

  1. from source: we should check which metrics are in use when attempting to delete them. I think this is you suggested.
  2. from target: when bad data already existed in superset data, how Superset should handle the error. this is what i suggested.

I feel both places need some work to fix. From technical point of view, fix 1 is harder, given some companies (like airbnb) already had huge number of charts and dataset.

@stale
Copy link

stale bot commented Apr 17, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. For admin, please label this issue .pinned to prevent stale bot from closing the issue.

@stale stale bot added the inactive Inactive for >= 30 days label Apr 17, 2022
@rusackas
Copy link
Member

rusackas commented Feb 2, 2023

This issue has gone stale so I'll reluctantly close it, but it's also worth pointing out that @kasiazjc put a whole lot of work into surfacing relationships between charts/dashboards, and there's been some design work here on dataset relationships too. Maybe @kasiazjc can speak to what's left to implement in this effort (if anything) and we can either (a) reopen this issue, (b) open a new Discussion thread if warranted, or (c) just leave this closed and let nature take its course.

@stale stale bot removed the inactive Inactive for >= 30 days label Feb 2, 2023
@rusackas rusackas closed this as not planned Won't fix, can't repro, duplicate, stale Feb 2, 2023
@kasiazjc
Copy link
Contributor

kasiazjc commented Feb 3, 2023

We actually have implemented all of the dashboard x charts features for now, but we didn't really touch the dataset part. It would probably be a stretch to include it in the dataset flow redesign or CRUD redesign which is happening, BUT for the visibility and consideration I'm tagging @khallon

--

edit: quote didn't work, but fyi @rusackas 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
explore:control Related to the controls panel of Explore nice-to-have Nice to have features or enhancements
Projects
None yet
Development

No branches or pull requests

5 participants