refactor: remove slice level label_colors from dashboard init load

[graceguo-supercat]

SUMMARY

When dashboard is loaded, it loaded bootstrap data for render dashboard. it includes: datasources, slice metadata, and dashboard metadata.

When dashboard has set color_scheme, its metadata will have color_labels section and set color for each label in the dashboard. This attribute will overwrite slice level color_label in its form_data. So when used in dashboard, slice's own color_label has no use. But this section could be pretty big and repeated by all the slices in the dashboard, which make the dashboard initial bootstrap data size very big.

This PR is to remove slice level color_label attribute (in its form_data) in the dashboard bootstrap data.

TEST PLAN

CI and manually load some dashboards.

@etr2460

[mistercrunch]

I think that chart-level colors were a mistake and that we should revisit this. @nytai got tangled into this in the past and in some cases really makes the chart payload gigantic for no super good reason.

I think Kim's intent originally was to preserve the dashboard-assigned colors when exploring the chart, but it really bloats the chart's properties. Plus it doesn't really play well with the many-to-many aspect of charts/dashboards, what if the chart is in two dashboards using different color schemes?

Even if we wanted to preserve that context, we may want for the chart to only have a reference to the dashboard it's associated with (in that particular context), and use the metadata / color scheme from that dashboard.

[graceguo-supercat]

yes, i agree this label_colors props is inappropriately populated into slice.
If i run db migration and remove all of them from form_data, dashboard will still generate new entries.
I think we should find a better solution for allowing user customize label color in dashboard. Maybe a good project when working/refactoring dashboard UI toward SIP-34.

[mistercrunch]

If the intention was to keep the same colors when going from the dashboard to explore, I think we should pass the context to explore of which dashboard the user came from, and explore can retrieve whatever dashboard metadata (ie colors) that can affect the context of the chart itself.

[graceguo-supercat]

I want to use this PR as a short term solution, to see how big impact those redundant label_colors in slices' form_data to dashboard performance (mostly for large dashboard used in airbnb).

If the impact is big, we should do some refactoring for label_colors logic (long term solution):

1. instead of persist label_colors in form_data, we should pass the context to explore of which dashboard the user came from, and explore can retrieve whatever dashboard metadata (ie colors) that can affect the context of the chart itself.
2. do not save label_colors passed from dashboard when user save / copy slices (this is current behavior).
3. db migration to remove label_colors from form_data. -- not sure we should do this...what if user edit slice and want to set fixed color scheme for a slice?

for both long term and short term solution, dashboard should not carry slice level color_labels.

[graceguo-supercat]

ping @mistercrunch @etr2460

[etr2460]

Sorry, one clarification i'm not 100% sure of:

Does this change affect the current user experience in any way? If a dashboard doesn't have colors defined, do we fall back to slice colors which will now be removed?

I'd say, if this could change the user experience, then it might be safer to put this in a feature flag. If not, then i'm good to stamp this as an awesome optimization

[graceguo-supercat]

if dashboard don't have label_colors setting, after this PR, the label color will be assigned randomly, because slice-level label_color is not carried by dashboard. So I wrap this change into a feature flag, by default is not enable it.

This is a short term solution. if we see perf impact for large dashboard, should go with a long term solution.

[codecov-commenter]

Codecov Report

Merging #10603 into master will decrease coverage by 3.39%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #10603      +/-   ##
==========================================
- Coverage   64.06%   60.66%   -3.40%     
==========================================
  Files         771      418     -353     
  Lines       36391    13650   -22741     
  Branches     3429     3482      +53     
==========================================
- Hits        23314     8281   -15033     
+ Misses      12964     5180    -7784     
- Partials      113      189      +76     

Flag	Coverage Δ
#cypress	?
#javascript	60.66% <ø> (+0.78%)	⬆️
#python	?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
superset-frontend/src/SqlLab/App.jsx	0.00% <0.00%> (-100.00%)	⬇️
superset-frontend/src/explore/App.jsx	0.00% <0.00%> (-100.00%)	⬇️
superset-frontend/src/dashboard/App.jsx	0.00% <0.00%> (-100.00%)	⬇️
superset-frontend/src/explore/index.jsx	0.00% <0.00%> (-100.00%)	⬇️
superset-frontend/src/dashboard/index.jsx	0.00% <0.00%> (-100.00%)	⬇️
superset-frontend/src/setup/setupColors.js	0.00% <0.00%> (-100.00%)	⬇️
superset-frontend/src/chart/ChartContainer.jsx	0.00% <0.00%> (-100.00%)	⬇️
superset-frontend/src/setup/setupFormatters.js	0.00% <0.00%> (-100.00%)	⬇️
superset-frontend/src/explore/reducers/index.js	0.00% <0.00%> (-100.00%)	⬇️
superset-frontend/src/setup/setupPluginsExtra.js	0.00% <0.00%> (-100.00%)	⬇️
... and 523 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a3fd2b4...8367da4. Read the comment docs.

[etr2460]

one comment, otherwise lgtm!

[etr2460]

naming nit, REMOVE instead of REDUCE probably

[graceguo-supercat]

thanks. fixed!