-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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 all 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 |
---|---|---|
|
@@ -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,26 @@ 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.get("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) | ||
extra["health_check"] = { | ||
"version": check.version, | ||
"message": message, | ||
} | ||
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"): | ||
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.
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 comment
The 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
health_check
section into db record (so that not running check again).