feat(metrics): Provide override for disabling ad-hoc metrics #17202

john-bodley · 2021-10-22T18:48:29Z

SUMMARY

There could be some instances where ad-hoc metrics for specific datasets should be disabled, i.e., the dataset owner specifically does not want users to create ad-hoc non-sanctioned metrics. This PR adds—at the dataset level—a custom override for disabling ad-hoc metrics, i.e., removing the SIMPLE and CUSTOM SQL tabs. The default behavior preserves the existing workflow.

Note if this override is enabled, any existing ad-hoc metric will persist and be deemed editable even though the tab is officially marked as disabled.

For consistency, for the native Druid connector I also disabled the CUSTOM SQL tab (which housed a message as to why this was invalid—this proposed way seems cleaner as well) and added the same treatment for ad-hoc filters.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Disallowed

Disallowed (pre-existing)

Interestingly the tab is disabled but the canvas remains operational.

Allowed (default)

TESTING INSTRUCTIONS

Added unit tests and manually tested for a number of visualization types and ad-hoc metric components: metric, metrics, percentage metric, sort-by metric, etc.

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

etr2460 · 2021-10-22T20:03:07Z

...rset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover/index.jsx

-                Custom SQL Metrics are not available on druid datasources
-              </div>
+          {extra.disallow_adhoc_metrics !== true && (
+            <Tabs.TabPane key={EXPRESSION_TYPES.SIMPLE} tab={t('Simple')}>


how do you feel about keeping the tabs visible, but instead setting disabled={extra.disallow_adhoc_metrics}?

Either way works, though I feel we need consistency with the ad-hoc filters.

what does the ad-hoc filters popover do? the approach you had here?

@etr2460 yes it removed the tab, but I agree that disablement is fine for consistency. Also the Druid NoSQL connector is rarely used/supported and slated for deprecation in Superset 2.0.

codecov · 2021-10-25T17:29:41Z

Codecov Report

Merging #17202 (4e2903f) into master (fa44325) will decrease coverage by 0.02%.
The diff coverage is 73.20%.

❗ Current head 4e2903f differs from pull request most recent head da996e0. Consider uploading reports for the commit da996e0 to get more accurate results

@@            Coverage Diff             @@
##           master   #17202      +/-   ##
==========================================
- Coverage   77.09%   77.07%   -0.03%     
==========================================
  Files        1037     1036       -1     
  Lines       55650    55756     +106     
  Branches     7603     7630      +27     
==========================================
+ Hits        42903    42973      +70     
- Misses      12497    12528      +31     
- Partials      250      255       +5

Flag	Coverage Δ
javascript	`71.43% <79.14%> (+0.17%)`	⬆️
presto	`?`

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

Impacted Files	Coverage Δ
superset-frontend/src/chart/Chart.jsx	`43.33% <ø> (ø)`
superset-frontend/src/chart/ChartRenderer.jsx	`36.70% <0.00%> (-0.48%)`	⬇️
...t-frontend/src/components/AsyncAceEditor/index.tsx	`88.00% <ø> (-0.47%)`	⬇️
superset-frontend/src/dashboard/actions/hydrate.js	`1.70% <0.00%> (ø)`
.../src/dashboard/components/gridComponents/Chart.jsx	`80.37% <ø> (ø)`
...perset-frontend/src/explore/components/Control.tsx	`72.22% <0.00%> (-4.25%)`	⬇️
...ntend/src/explore/components/ExploreChartPanel.jsx	`14.28% <ø> (ø)`
...d/src/explore/components/ExploreContentPopover.tsx	`100.00% <ø> (ø)`
...nd/src/explore/components/ExploreViewContainer.jsx	`2.11% <ø> (ø)`
...ontrols/DndColumnSelectControl/DndMetricSelect.tsx	`61.76% <ø> (ø)`
... and 88 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 fa44325...da996e0. Read the comment docs.

john-bodley · 2021-10-27T00:32:03Z

...rset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopover/index.jsx

    const stateIsValid = adhocFilter.isValid();
    const hasUnsavedChanges = !adhocFilter.equals(propsAdhocFilter);

-    const sectionRenders = {};
-
-    sectionRenders.CUSTOM_SQL = (


Moved to be inline.

john-bodley · 2021-11-02T05:10:34Z

@etr2460 this is ready for rearview. Note the state of the components within disabled tabs is somewhat atypical, i.e., non disabled, however this will only occur if a dataset is disabled after the fact. Note existing ad-hoc metrics will be grandfathered in, i.e., the chart will remain functional, which is desirable.

etr2460 · 2021-11-03T03:34:52Z

...rset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover/index.jsx

-          <Tabs.TabPane key={EXPRESSION_TYPES.SIMPLE} tab={t('Simple')}>
+          <Tabs.TabPane
+            key={EXPRESSION_TYPES.SIMPLE}
+            tab={t('Simple')}


should we add a tooltip on the tab with details on why it's disabled?

@etr2460 could you be more explicit? I'm not sure how we display a tooltip on a tab which is disabled and thus not accessible.

Meaning a tooltip that is displayed when you hover over the disabled tab selector. something like this? maybe with different content depending on why it's disabled?

tab={<Tooltip content={t('You may not define ad hoc metrics because it is disabled by the dataset owner.')}>{t('Simple')}</Tooltip>

etr2460 · 2021-11-03T03:36:06Z

...rset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover/index.jsx

@@ -50,7 +50,7 @@ const propTypes = {
  columns: PropTypes.arrayOf(columnType),
  savedMetricsOptions: PropTypes.arrayOf(savedMetricType),
  savedMetric: savedMetricType,
-  datasourceType: PropTypes.string,
+  datasource: PropTypes.object.isRequired,


if datasource is marked as isRequired then you shouldn't need the datasource?.type below and can just do datasource.type. But if datasource isn't passed in sometimes, then you should remove the isRequired part

etr2460 · 2021-11-03T03:37:54Z

...rset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover/index.jsx

@@ -397,30 +411,26 @@ export default class AdhocMetricEditPopover extends React.PureComponent {
            key={EXPRESSION_TYPES.SQL}
            tab={t('Custom SQL')}


same comment here

etr2460 · 2021-11-03T03:38:11Z

superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricOption.jsx

@@ -32,7 +32,7 @@ const propTypes = {
  columns: PropTypes.arrayOf(columnType),
  savedMetricsOptions: PropTypes.arrayOf(savedMetricType),
  savedMetric: savedMetricType,
-  datasourceType: PropTypes.string,
+  datasource: PropTypes.object.isRequired,


same comment about if datasource is always here

etr2460 · 2021-11-03T03:38:42Z

superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricPopoverTrigger.tsx

@@ -33,7 +33,7 @@ export type AdhocMetricPopoverTriggerProps = {
  columns: { column_name: string; type: string }[];
  savedMetricsOptions: savedMetricType[];
  savedMetric: savedMetricType;
-  datasourceType: string;
+  datasource: Datasource;


same comment about if this should be datasource: Datasource or datasource?: Datasource

etr2460 · 2021-11-03T03:39:07Z

superset-frontend/src/explore/components/controls/MetricControl/MetricDefinitionValue.jsx

@@ -34,7 +34,7 @@ const propTypes = {
  savedMetrics: PropTypes.arrayOf(savedMetricType),
  savedMetricsOptions: PropTypes.arrayOf(savedMetricType),
  multi: PropTypes.bool,
-  datasourceType: PropTypes.string,
+  datasource: PropTypes.object.isRequired,


same comment

etr2460 · 2021-11-03T03:39:11Z

superset-frontend/src/explore/components/controls/MetricControl/MetricsControl.jsx

@@ -48,7 +48,7 @@ const propTypes = {
  isLoading: PropTypes.bool,
  multi: PropTypes.bool,
  clearable: PropTypes.bool,
-  datasourceType: PropTypes.string,
+  datasource: PropTypes.object.isRequired,


same comment

john-bodley · 2021-11-05T18:58:06Z

Thanks @etr2460 for the help with this PR. I believe it is ready for review.

ktmud

two non-blocking suggestions

ktmud · 2021-11-05T19:35:16Z

...rset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover/index.jsx

+      try {
+        extra = JSON.parse(datasource.extra);
+      } catch {} // eslint-disable-line no-empty
+    }


Can the API return extra as a JSON object itself? Even if the API doesn't, this feels like something should be handled earlier than in the metric popover.

I'm unsure, though it is a pattern used elsewhere.

The point was exactly that having the API return a JSON object so this "pattern" does not have to be repeated everywhere extra is needed.

ktmud · 2021-11-05T20:33:22Z

...rset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover/index.jsx

+              extra.disallow_adhoc_metrics ? (
+                <Tooltip
+                  title={t(
+                    'Simple ad-hoc metrics are not enabled for this dataset',


How about use the same message for both "SIMPLE" and "CUSTOM SQL":

Ad-hoc metrics are not supported by this dataset, use saved metrics instead.

Having different messages kind of implies one can disallow SIMPLE and CUSTOM SQL separately.

@ktmud the reason for the slightly different messages is the Druid NoSQL connector also disallows the CUSTOM SQL tab and thus I wanted to ensure consistency between those two experiences.

I think these are two different scenarios. You can still use a different message for native Druid. IMO clarity is more important than consistency. If we have only one config value disallow_ahoc_metrics, then the message should corresponds to what this config value does----which isn't disabling SIMPLE and CUSTOM SQL separately.

Co-authored-by: John Bodley <john.bodley@airbnb.com>

pull-request-size bot added the size/L label Oct 22, 2021

john-bodley requested a review from etr2460 October 22, 2021 18:48

john-bodley force-pushed the john-bodley--dataset-allow-adhoc-metrics branch 2 times, most recently from b4a886a to 519bb33 Compare October 22, 2021 19:59

etr2460 reviewed Oct 22, 2021

View reviewed changes

john-bodley force-pushed the john-bodley--dataset-allow-adhoc-metrics branch 4 times, most recently from c7c4bc6 to 777936f Compare October 25, 2021 16:48

john-bodley commented Oct 27, 2021

View reviewed changes

john-bodley mentioned this pull request Oct 27, 2021

chore: pass datasource to metrics control apache-superset/superset-ui#1438

Merged

john-bodley force-pushed the john-bodley--dataset-allow-adhoc-metrics branch 2 times, most recently from b1f5698 to c5f1a86 Compare October 28, 2021 21:51

john-bodley requested review from ktmud and michellethomas October 28, 2021 21:51

john-bodley force-pushed the john-bodley--dataset-allow-adhoc-metrics branch 4 times, most recently from c85f8af to 1a5be9a Compare October 29, 2021 18:38

john-bodley requested a review from etr2460 October 29, 2021 21:49

etr2460 reviewed Nov 3, 2021

View reviewed changes

john-bodley force-pushed the john-bodley--dataset-allow-adhoc-metrics branch from 1a5be9a to a3df424 Compare November 4, 2021 19:00

feat(metrics): Provide override for disabling ad-hoc metrics

da996e0

john-bodley force-pushed the john-bodley--dataset-allow-adhoc-metrics branch from a3df424 to da996e0 Compare November 5, 2021 18:35

ktmud approved these changes Nov 5, 2021

View reviewed changes

john-bodley merged commit 561d1ac into apache:master Nov 8, 2021

AAfghahi pushed a commit that referenced this pull request Jan 10, 2022

feat(metrics): Provide override for disabling ad-hoc metrics (#17202)

2982bac

Co-authored-by: John Bodley <john.bodley@airbnb.com>

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.5.0 labels Mar 13, 2024

justinpark mentioned this pull request Mar 15, 2024

fix(explore): Allow only saved metrics and columns #27539

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(metrics): Provide override for disabling ad-hoc metrics #17202

feat(metrics): Provide override for disabling ad-hoc metrics #17202

john-bodley commented Oct 22, 2021 •

edited

Loading

etr2460 Oct 22, 2021

john-bodley Oct 22, 2021

etr2460 Oct 22, 2021

john-bodley Oct 22, 2021

codecov bot commented Oct 25, 2021 •

edited

Loading

john-bodley Oct 27, 2021

john-bodley commented Nov 2, 2021

etr2460 Nov 3, 2021

john-bodley Nov 4, 2021

etr2460 Nov 5, 2021

etr2460 Nov 3, 2021

etr2460 Nov 3, 2021

etr2460 Nov 3, 2021

etr2460 Nov 3, 2021

etr2460 Nov 3, 2021

etr2460 Nov 3, 2021

john-bodley commented Nov 5, 2021

ktmud left a comment

ktmud Nov 5, 2021

john-bodley Nov 8, 2021

ktmud Nov 8, 2021 •

edited

Loading

ktmud Nov 5, 2021 •

edited

Loading

john-bodley Nov 8, 2021

ktmud Nov 8, 2021

		@@ -397,30 +411,26 @@ export default class AdhocMetricEditPopover extends React.PureComponent {
		key={EXPRESSION_TYPES.SQL}
		tab={t('Custom SQL')}

feat(metrics): Provide override for disabling ad-hoc metrics #17202

feat(metrics): Provide override for disabling ad-hoc metrics #17202

Conversation

john-bodley commented Oct 22, 2021 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Disallowed

Disallowed (pre-existing)

Allowed (default)

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 25, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

john-bodley commented Nov 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

john-bodley commented Nov 5, 2021

ktmud left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktmud Nov 8, 2021 • edited Loading

Choose a reason for hiding this comment

ktmud Nov 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

john-bodley commented Oct 22, 2021 •

edited

Loading

codecov bot commented Oct 25, 2021 •

edited

Loading

ktmud Nov 8, 2021 •

edited

Loading

ktmud Nov 5, 2021 •

edited

Loading