feat: add hook for dataset health check #11970

graceguo-supercat · 2020-12-09T05:52:01Z

SUMMARY

Inside airbnb, we spent a lot of time on Superset dashboard and query performance issues. We create some guidelines for airbnb users, to help them write better queries to create virtual dataset. So we want to add a dataset health check logic when user explore a chart, and show some warning message if health check returns any message.

To decide whether a dataset is good or bad, this logic is probably very specific to each corporation. Some rules applied in airbnb may totally not relevant for Dropbox. So in the open source codebase, we only add an entry point,
DATASET_HEALTH_CHECK = _your function to check dataset quality_
which allow each company to implement and hook their own rules.

For example, after implements a rule that checks if the chart is using a virtual dataset:

def my_check(datasource: SqlaTable) -> str:
    message = None # pass the check no warning
    if datasource.type == "table" and datasource.sql:
        message = "You should replace this virtual datasource with physical table."
    return message

DATASET_HEALTH_CHECK = my_check
DATASET_HEALTH_CHECK.version = 0.1

I will see a warning message like this:

Note
The health_check result will be saved with datasource record extra column. When user update datasource, or DATASET_HEALTH_CHECK.version is changed, health_check function will be triggered again and update the record in the database. Please see how we discussed different ideas for this feature.

TEST PLAN

CI

ADDITIONAL INFORMATION

cc @ktmud @etr2460

graceguo-supercat · 2020-12-09T05:54:53Z

superset-frontend/src/explore/components/ControlPanelsContainer.jsx

@@ -139,6 +139,10 @@ class ControlPanelsContainer extends React.Component {
          controls[s].validationErrors.length > 0,
      ),
    );
+    if (exploreState.dataset_health_check_message) {


i can populate warning message from here, or from superset-ui-chart-controls:
https://github.com/apache-superset/superset-ui/blob/1f8a700b953efbed81d2c9970b514d7fe031ad2e/packages/superset-ui-chart-controls/src/shared-controls/index.tsx#L166

but it will need to create another PR in another code base.@ktmud do you have preference?

This component ControlPanelsContainer is generic and should stay generic. It should not contain logics related to any specific control field.

ktmud · 2020-12-09T05:58:32Z

What if users change datasource or update datasource to point to a physical datasource in the Edit Datasource modal?

codecov-io · 2020-12-09T05:59:57Z

Codecov Report

Merging #11970 (9800606) into master (cc44a2c) will decrease coverage by 6.01%.
The diff coverage is 81.81%.

@@            Coverage Diff             @@
##           master   #11970      +/-   ##
==========================================
- Coverage   66.26%   60.24%   -6.02%     
==========================================
  Files         941      912      -29     
  Lines       45715    45009     -706     
  Branches     4395     4061     -334     
==========================================
- Hits        30293    27116    -3177     
- Misses      15287    17893    +2606     
+ Partials      135        0     -135

Flag	Coverage Δ
cypress	`53.10% <66.66%> (+9.29%)`	⬆️
javascript	`?`
python	`64.07% <83.33%> (-0.19%)`	⬇️

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

Impacted Files	Coverage Δ
superset/config.py	`90.47% <0.00%> (+0.39%)`	⬆️
.../explore/components/controls/DatasourceControl.jsx	`72.46% <66.66%> (+16.40%)`	⬆️
superset/connectors/sqla/models.py	`91.24% <100.00%> (+0.46%)`	⬆️
superset/datasets/dao.py	`87.28% <100.00%> (+0.21%)`	⬆️
superset/views/core.py	`72.76% <100.00%> (-1.95%)`	⬇️
superset/views/datasource.py	`95.00% <100.00%> (+0.17%)`	⬆️
...uperset-frontend/src/dashboard/util/dnd-reorder.js	`0.00% <0.00%> (-100.00%)`	⬇️
...rset-frontend/src/dashboard/util/getEmptyLayout.js	`0.00% <0.00%> (-100.00%)`	⬇️
...dashboard/components/resizable/ResizableHandle.jsx	`0.00% <0.00%> (-100.00%)`	⬇️
.../src/dashboard/util/getFilterScopeFromNodesTree.js	`0.00% <0.00%> (-93.48%)`	⬇️
... and 458 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 cc44a2c...9800606. Read the comment docs.

ktmud · 2020-12-09T06:01:06Z

superset-frontend/src/explore/controls.jsx

@@ -197,6 +197,7 @@ export const controls = {
    label: t('Dataset'),
    default: null,
    description: null,
+    healthCheckMessage: null,


I believe most if not all control configs in this file are not in use any more. We should clean them up.

yes. this line has no use :(

graceguo-supercat · 2020-12-09T06:01:55Z

What if users change datasource or update datasource to point to a physical datasource in the Edit Datasource modal?

I don't offer any rule in the PR. the goal for this PR is to offer a hook in the codebase. Each company define their own rules.

ktmud · 2020-12-09T06:39:20Z

I mean does the error message update when users switch datasources? It seems the check only happens at the first page load.

graceguo-supercat · 2020-12-09T17:57:09Z

I mean does the error message update when users switch datasources? It seems the check only happens at the first page load.

No. save dataset data is on a different api. This health check will only happen when opening explore view.

graceguo-supercat · 2020-12-15T04:14:32Z

superset/connectors/sqla/models.py

@@ -1467,6 +1469,26 @@ class and any keys added via `ExtraCache`.
            extra_cache_keys += sqla_query.extra_cache_keys
        return extra_cache_keys

+    @event_logger.log_this


can i add event log this way? want to monitor the duration for health check logic, in case it may affect explore view perf.

What you care most is the actual check function, the part where we check version and skip the health check (happens for most page views) is known to be pretty trivial. So maybe use this code instead:

with event_logger.log_context(action="datasource_health_check") as log: message = check(self) ...

graceguo-supercat · 2020-12-15T21:03:24Z

superset/connectors/sqla/models.py

@@ -698,6 +703,7 @@ def data(self) -> Dict[str, Any]:
            data_["fetch_values_predicate"] = self.fetch_values_predicate
            data_["template_params"] = self.template_params
            data_["is_sqllab_view"] = self.is_sqllab_view
+            data_["health_check_message"] = self.health_check_message


instead of output whole "extra", i saw certified data only offer flattened information. I think it's easier for frontend usage.

ktmud

Thanks for the iterations! One blocking comment (you should set health_check even when check returns None), otherwise LGTM.

I'm also curious to hear how others think about this feature, though.

@junlincc @mistercrunch do you think this align well with overall Superset roadmap?

ktmud · 2020-12-15T22:24:10Z

superset/views/core.py

@@ -650,6 +650,10 @@ def explore(  # pylint: disable=too-many-locals,too-many-return-statements
                f"datasource_id={datasource_id}&"
            )

+        # if feature enabled, run some health check rules for sqla datasource
+        if hasattr(datasource, "health_check") and conf["DATASET_HEALTH_CHECK"]:


I think you can skip the conf check here. It will be immediately checked by datasource.health_check() anyway.

ktmud · 2020-12-15T22:30:37Z

superset/connectors/sqla/models.py

+                    extra["health_check"] = {
+                        "version": check.version,
+                        "message": message,
+                    }


You might want to set the health_check field even if check message returns None or empty. Because otherwise this will run for every "good" datasources that didn't have any health check warnings.

I would also recommend making the message more structured for scalability (i.e. let DATASET_HEALTH_CHECK return a dict instead of a string). Because in the future, you may need to localize the warning message, then it would be better to store an ERROR_CODE + some metadata instead of just a message string. This also opens the door for more actionable warning message where users may click on the warning icon and open a popup for more details.

Maybe we can even define it as a new SupersetError type and use @etr2460 's error message registry to render it.

But of course, this could also be reserved for future refactor/extension. (We can always update the health check version when that happens!)

It's not as strong as SupesetError. Error and Warning should be treated differently.

Let's keep this PR as simple as it is, and we can reiterate when it is really needed. The impact of this feature is still unknown to me, because it depends a lot on the rule implementation. Let's run the core function with some real examples before putting too much efforts on extras.

Yes, agreed. We don't need to go that route if it's too much work. Just laying out my thoughts here in case we want to double down on this feature. Even if the message depends on specific rule implementation, we can enforce a basic structure so that it's possible the override the message renders.

Error and Warning should be treated differently.

This I must humbly disagree. Both are actually a subclass of Alert in most UI libraries, and we see in a lot of places the concept of "error levels" (error, warning, info). Most of the time, you'd see warnings and errors being rendered by the same component, just different colors or icons. And in case we want to render them differently, the shared infrastructure of custom renderer registry comes handy.

thanks. i will keep this work in mind. If we see this feature is useful, I will polish it with localization and other re-usable UI component.

etr2460 · 2020-12-16T00:11:53Z

superset-frontend/spec/javascripts/explore/components/DatasourceControl_spec.jsx

+  it('should render health check message', () => {
+    const wrapper = setup();
+    const icons = wrapper.find(Icon);
+    expect(icons.first().prop('name')).toBe('alert-solid');


should we also make sure the warning message is shown here?

etr2460 · 2020-12-16T00:12:23Z

superset-frontend/src/explore/components/controls/DatasourceControl.jsx

    return (
      <Styles className="DatasourceControl">
        <ControlHeader {...this.props} />
-        <div>
+        <div style={{ display: 'flex' }}>


instead of an inline style, should we use a styled.div to apply this style?

etr2460 · 2020-12-16T00:16:55Z

superset/config.py

+# It's possible to add a dataset health check logic which is specific to your system.
+# It will get executed each time when user open a chart's explore view.
+DATASET_HEALTH_CHECK = None


can you add the signature about the health check to the comment here?

Or maybe even better, include a default function that always passes. It's actually a bit unclear to me how this should be used (should I return True if the dataset passes? None?)

See Jesse's comment above, if the dataset passes, it will still add a health_check section into db record (so that not running check again).

john-bodley · 2021-04-06T18:38:29Z

superset/views/core.py

@@ -650,6 +650,10 @@ def explore(  # pylint: disable=too-many-locals,too-many-return-statements
                f"datasource_id={datasource_id}&"
            )

+        # if feature enabled, run some health check rules for sqla datasource
+        if hasattr(datasource, "health_check"):
+            datasource.health_check()


@graceguo-supercat why in this instance do we not require to force a check, i.e., datasource.health_check(force=True)?

We should run check again when user updates dataset, or when health check rule's version is changed. Otherwise the healthy status will not be changed.
This function is triggered at the time explore view is opened from browser, so it doesn't need to force run the check.

pull-request-size bot added the size/M label Dec 9, 2020

graceguo-supercat requested a review from ktmud December 9, 2020 05:52

graceguo-supercat commented Dec 9, 2020

View reviewed changes

ktmud reviewed Dec 9, 2020

View reviewed changes

graceguo-supercat mentioned this pull request Dec 9, 2020

chore: add extra field for Datasource control config apache-superset/superset-ui#856

Closed

graceguo-supercat force-pushed the gg-DatasetHealthCheck branch from 5f0a0bf to 5f4f2cc Compare December 15, 2020 01:37

feat: add hook for dataset health check

88a96b3

graceguo-supercat force-pushed the gg-DatasetHealthCheck branch from 5f4f2cc to 88a96b3 Compare December 15, 2020 02:00

graceguo-supercat commented Dec 15, 2020

View reviewed changes

graceguo-supercat force-pushed the gg-DatasetHealthCheck branch from 6b5315f to 04309c5 Compare December 15, 2020 05:58

add event log

420adcc

graceguo-supercat force-pushed the gg-DatasetHealthCheck branch from 04309c5 to 420adcc Compare December 15, 2020 06:03

optimize datasource json data like certified data

c17f86a

graceguo-supercat commented Dec 15, 2020

View reviewed changes

ktmud reviewed Dec 15, 2020

View reviewed changes

add unit test

5146afa

graceguo-supercat force-pushed the gg-DatasetHealthCheck branch from ea3b23b to 5146afa Compare December 15, 2020 23:31

fix review comments

975d275

ktmud approved these changes Dec 16, 2020

View reviewed changes

etr2460 reviewed Dec 16, 2020

View reviewed changes

extra code review comments

9800606

graceguo-supercat merged commit 8da1900 into apache:master Dec 16, 2020

mayurnewase mentioned this pull request Dec 16, 2020

[time-series]Echarts time-series contribution mode not working #11792

Closed

graceguo-supercat mentioned this pull request Feb 3, 2021

fix: Config for dataset health check #12906

Merged

6 tasks

john-bodley reviewed Apr 6, 2021

View reviewed changes

john-bodley mentioned this pull request Apr 7, 2021

fix: Issue 13956 #13980

Merged

8 tasks

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add hook for dataset health check #11970

feat: add hook for dataset health check #11970

graceguo-supercat commented Dec 9, 2020 •

edited

Loading

graceguo-supercat Dec 9, 2020 •

edited

Loading

ktmud Dec 9, 2020

ktmud commented Dec 9, 2020

codecov-io commented Dec 9, 2020 •

edited

Loading

ktmud Dec 9, 2020

graceguo-supercat Dec 9, 2020

graceguo-supercat commented Dec 9, 2020 •

edited

Loading

ktmud commented Dec 9, 2020

graceguo-supercat commented Dec 9, 2020

graceguo-supercat Dec 15, 2020 •

edited

Loading

ktmud Dec 15, 2020

graceguo-supercat Dec 15, 2020 •

edited

Loading

ktmud left a comment

ktmud Dec 15, 2020 •

edited

Loading

ktmud Dec 15, 2020

ktmud Dec 15, 2020 •

edited

Loading

graceguo-supercat Dec 15, 2020 •

edited

Loading

ktmud Dec 16, 2020

graceguo-supercat Dec 16, 2020 •

edited

Loading

etr2460 Dec 16, 2020

etr2460 Dec 16, 2020

graceguo-supercat Dec 16, 2020

etr2460 Dec 16, 2020

graceguo-supercat Dec 16, 2020 •

edited

Loading

john-bodley Apr 6, 2021

graceguo-supercat Apr 6, 2021 •

edited

Loading

feat: add hook for dataset health check #11970

feat: add hook for dataset health check #11970

Conversation

graceguo-supercat commented Dec 9, 2020 • edited Loading

SUMMARY

TEST PLAN

ADDITIONAL INFORMATION

graceguo-supercat Dec 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktmud commented Dec 9, 2020

codecov-io commented Dec 9, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graceguo-supercat commented Dec 9, 2020 • edited Loading

ktmud commented Dec 9, 2020

graceguo-supercat commented Dec 9, 2020

graceguo-supercat Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graceguo-supercat Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

ktmud left a comment

Choose a reason for hiding this comment

ktmud Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktmud Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

graceguo-supercat Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graceguo-supercat Dec 16, 2020 • edited Loading

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

graceguo-supercat Dec 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graceguo-supercat Apr 6, 2021 • edited Loading

Choose a reason for hiding this comment

graceguo-supercat commented Dec 9, 2020 •

edited

Loading

graceguo-supercat Dec 9, 2020 •

edited

Loading

codecov-io commented Dec 9, 2020 •

edited

Loading

graceguo-supercat commented Dec 9, 2020 •

edited

Loading

graceguo-supercat Dec 15, 2020 •

edited

Loading

graceguo-supercat Dec 15, 2020 •

edited

Loading

ktmud Dec 15, 2020 •

edited

Loading

ktmud Dec 15, 2020 •

edited

Loading

graceguo-supercat Dec 15, 2020 •

edited

Loading

graceguo-supercat Dec 16, 2020 •

edited

Loading

graceguo-supercat Dec 16, 2020 •

edited

Loading

graceguo-supercat Apr 6, 2021 •

edited

Loading