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

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

Conversation

john-bodley
Copy link
Member

@john-bodley john-bodley commented Oct 22, 2021

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

Screen Shot 2021-11-05 at 11 33 31 AM

Screen Shot 2021-11-05 at 11 33 39 AM

Screen Shot 2021-11-05 at 11 41 45 AM

Disallowed (pre-existing)

Interestingly the tab is disabled but the canvas remains operational.

Screen Shot 2021-10-28 at 2 58 27 PM

Allowed (default)

Screen Shot 2021-10-22 at 11 30 10 AM

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

@john-bodley 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
Custom SQL Metrics are not available on druid datasources
</div>
{extra.disallow_adhoc_metrics !== true && (
<Tabs.TabPane key={EXPRESSION_TYPES.SIMPLE} tab={t('Simple')}>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

@john-bodley 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
@codecov
Copy link

codecov bot commented Oct 25, 2021

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
Impacted file tree graph

@@            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.

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

const sectionRenders = {};

sectionRenders.CUSTOM_SQL = (
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to be inline.

@john-bodley 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 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
Copy link
Member Author

@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.

<Tabs.TabPane key={EXPRESSION_TYPES.SIMPLE} tab={t('Simple')}>
<Tabs.TabPane
key={EXPRESSION_TYPES.SIMPLE}
tab={t('Simple')}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment here

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment about if datasource is always here

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment

@john-bodley john-bodley force-pushed the john-bodley--dataset-allow-adhoc-metrics branch from 1a5be9a to a3df424 Compare November 4, 2021 19:00
@john-bodley john-bodley force-pushed the john-bodley--dataset-allow-adhoc-metrics branch from a3df424 to da996e0 Compare November 5, 2021 18:35
@john-bodley
Copy link
Member Author

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

Copy link
Member

@ktmud ktmud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

two non-blocking suggestions

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@ktmud ktmud Nov 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

@ktmud ktmud Nov 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@john-bodley john-bodley merged commit 561d1ac into apache:master Nov 8, 2021
AAfghahi pushed a commit that referenced this pull request Jan 10, 2022
Co-authored-by: John Bodley <john.bodley@airbnb.com>
@mistercrunch 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/L 🚢 1.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants