-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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: add hook for dataset health check #11970
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,7 +19,7 @@ | |
import React from 'react'; | ||
import PropTypes from 'prop-types'; | ||
import { Col, Collapse, Row, Well } from 'react-bootstrap'; | ||
import { t, styled } from '@superset-ui/core'; | ||
import { t, styled, supersetTheme } from '@superset-ui/core'; | ||
import { ColumnOption, MetricOption } from '@superset-ui/chart-controls'; | ||
|
||
import { Dropdown, Menu } from 'src/common/components'; | ||
|
@@ -213,10 +213,13 @@ class DatasourceControl extends React.PureComponent { | |
</Menu> | ||
); | ||
|
||
// eslint-disable-next-line camelcase | ||
const { health_check_message: healthCheckMessage } = datasource; | ||
|
||
return ( | ||
<Styles className="DatasourceControl"> | ||
<ControlHeader {...this.props} /> | ||
<div> | ||
<div style={{ display: 'flex' }}> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. instead of an inline style, should we use a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed. |
||
<Tooltip title={t('Expand/collapse dataset configuration')}> | ||
<Label | ||
style={{ textTransform: 'none' }} | ||
|
@@ -230,6 +233,14 @@ class DatasourceControl extends React.PureComponent { | |
/> | ||
</Label> | ||
</Tooltip> | ||
{healthCheckMessage && ( | ||
<Tooltip title={healthCheckMessage}> | ||
<Icon | ||
name="alert-solid" | ||
color={supersetTheme.colors.warning.base} | ||
/> | ||
</Tooltip> | ||
)} | ||
<Dropdown | ||
overlay={datasourceMenu} | ||
trigger={['click']} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -991,3 +991,8 @@ class CeleryConfig: # pylint: disable=too-few-public-methods | |
except Exception: | ||
logger.exception("Found but failed to import local superset_config") | ||
raise | ||
|
||
|
||
# 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 | ||
Comment on lines
+996
to
+998
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See Jesse's comment above, if the dataset passes, it will still add a |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -58,6 +58,7 @@ | |
from superset.db_engine_specs.base import TimestampExpression | ||
from superset.errors import ErrorLevel, SupersetError, SupersetErrorType | ||
from superset.exceptions import QueryObjectValidationError, SupersetSecurityException | ||
from superset.extensions import event_logger | ||
from superset.jinja_context import ( | ||
BaseTemplateProcessor, | ||
ExtraCache, | ||
|
@@ -685,6 +686,10 @@ def select_star(self) -> Optional[str]: | |
self.table_name, schema=self.schema, show_cols=False, latest_partition=False | ||
) | ||
|
||
@property | ||
def health_check_message(self) -> Optional[str]: | ||
return self.extra_dict.get("health_check", {}).get("message") | ||
|
||
@property | ||
def data(self) -> Dict[str, Any]: | ||
data_ = super().data | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. instead of output whole "extra", i saw certified data only offer flattened information. I think it's easier for frontend usage. |
||
return data_ | ||
|
||
@property | ||
|
@@ -1467,6 +1473,29 @@ class and any keys added via `ExtraCache`. | |
extra_cache_keys += sqla_query.extra_cache_keys | ||
return extra_cache_keys | ||
|
||
def health_check(self, commit: bool = False, force: bool = False) -> None: | ||
check = config["DATASET_HEALTH_CHECK"] | ||
if check is None: | ||
return | ||
|
||
extra = self.extra_dict | ||
# force re-run health check, or health check is updated | ||
if force or extra.get("health_check", {}).get("version") != check.version: | ||
with event_logger.log_context(action="dataset_health_check"): | ||
message = check(self) | ||
if message: | ||
extra["health_check"] = { | ||
"version": check.version, | ||
"message": message, | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You might want to set the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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!) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
This I must humbly disagree. Both are actually a subclass of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
else: | ||
extra.pop("health_check", None) | ||
self.extra = json.dumps(extra) | ||
|
||
db.session.merge(self) | ||
if commit: | ||
db.session.commit() | ||
|
||
|
||
sa.event.listen(SqlaTable, "after_insert", security_manager.set_perm) | ||
sa.event.listen(SqlaTable, "after_update", security_manager.set_perm) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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"]: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you can skip the conf check here. It will be immediately checked by |
||
datasource.health_check() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @graceguo-supercat why in this instance do we not require to force a check, i.e., There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
viz_type = form_data.get("viz_type") | ||
if not viz_type and datasource.default_endpoint: | ||
return redirect(datasource.default_endpoint) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we also make sure the warning message is shown here?