[Explore] Saved metrics label overrides not working

[ktmud]

Screenshots

How to reproduce the bug

The new AdhocMetric control made it possible to add a custom label for saved metrics, but the added override doesn't actually affect anything. This is confusing. We should either disable "Click to edit label" or allow the label override to propagate to the selected pills and charts. The former is probably easier.

1. Add a saved metric
2. Click on the popover title to edit the metric label

Environment

Latest master

Checklist

Make sure to follow these steps before submitting your issue - thank you!

• I have checked the superset logs for python stacktraces and included it here as text if there are any.
• I have reproduced the issue with at least the latest released version of superset.
• I have checked the issue tracker for the same issue and I haven't found one similar.

Additional context

BTW, I really think saved metrics should be the first tab (at least when the dataset has some saved metrics), because the point of having saved metrics is to have an easy access to them. We should also encourage the use of saved metrics as it: 1) helps organizations reuse the same metric definition; 2) saves the labor of reconfigure the same metric.

[junlincc]

BTW, I really think saved metrics should be the first tab (at least when the dataset has some saved metrics), because the point of having saved metrics is to have an easy access to them. We should also encourage the use of saved metrics as it: 1) helps organizations reuse the same metric definition; 2) saves the labor of reconfigure the same metric.

can we conduct user research or at least a poll to have some data point to support this proposed change?

[ktmud]

I've heard many times at Airbnb that it's beneficial to have shared metric definitions. Other company may have different needs, but the discussion can start from some simple reasoning.

What was the reasoning behind putting SIMPLE as the default tab? Is it because we don't want to show users an empty list of saved metrics? Then what do you think of conditionally change the default tab for new metrics based on whether there are saved metrics in the datasource or not?

[villebro]

@ktmud @junlincc I like the idea of supporting custom labels for saved metrics. If you agree I can fix this.

[junlincc]

@ktmud @junlincc I like the idea of supporting custom labels for saved metrics. If you agree I can fix this.

please do! but hold off from switching tabs. @ktmud your request and reasoning makes sense, please allow research and design process to take place first.

[ktmud]

@ktmud @junlincc I like the idea of supporting custom labels for saved metrics. If you agree I can fix this.

@villebro could you hold until #10270 and apache-superset/superset-ui#889 are merged? I just need to add more test cases and fix the CI. There are some (relatively large) refactoring related to QueryObject and formData, which may very likely conflict with whatever changes you will make in this area.

[villebro]

@villebro could you hold until #10270 and apache-superset/superset-ui#889 are merged? I just need to add more test cases and fix the CI. There are some (relatively large) refactoring related to QueryObject and formData, which may very likely conflict with whatever changes you will make in this area.

Sure thing 👍🏼

[geido]

I am having a look at this

[geido]

@junlincc @ktmud can't reproduce this on master. The custom name of the metric is correctly shown. Is there any other place where the custom name should be propagated?

[ktmud]

@geido The bug with the custom label not reflected in select metrics has been fixed. But unless I missed any followup PRs, this label still doesn't apply to the query results and visualizations of most charts.

[geido]

@ktmud The change is applied indeed. The problem you might be perceiving is that the change to the label is not applied on the fly. Check the video attached.

If we want to apply it on the fly, we might be discussing quite a big change as the results and the charts are built dynamically from the data received after the request.

Another possible solution is to force-run the query when the label name is changed and saved. I am not sure if that would be optimal though.

DEV.Games.mp4

[ktmud]

What you had is a adhoc metric, not a saved metric.

[junlincc]

#14595

[geido]

Sorry for the confusion there, but the reason why I was looking at the 'adhoc metrics' is because the 'saved metrics' do not have the ability to change the title. This is explicitly turned off here https://github.com/apache/superset/blob/master/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricPopoverTrigger.tsx#L116.

I tried to force-enabling it but indeed changing the title does not affect anything, not even the label itself. This seems to be leaning toward a feature request more than a bug fix.

Additionally, as the labels for the charts and the query results are returned by the backend, the implementation will require backend support.

CC @junlincc @rusackas

[ktmud]

As stated in the issue description:

We should either disable "Click to edit label" or allow the label override to propagate to the selected pills and charts. The former is probably easier.

The bug was fixed by the former approach at some point. But I believe this thread was open because we also discussed actually allowing overriding saved metrics which is more work and, as you discovered, requires backend changes. Maybe we should have opened a new issue to track this instead of keep this open.

[stale]

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.

[rusackas]

Maybe we should have opened a new issue to track this instead of keep this open.

Since this has gone silent for two years, I think it's time we close it. If any of the above details are still bothering anyone, let's open one or more (discrete) new issues, with updated/simplified context. Thanks!