diff --git a/superset/dashboards/api.py b/superset/dashboards/api.py index 993795142b2de..c1d6590591ffc 100644 --- a/superset/dashboards/api.py +++ b/superset/dashboards/api.py @@ -51,7 +51,11 @@ from superset.tasks.thumbnails import cache_dashboard_thumbnail from superset.utils.screenshots import DashboardScreenshot from superset.views.base import generate_download_headers -from superset.views.base_api import BaseSupersetModelRestApi, RelatedFieldFilter +from superset.views.base_api import ( + BaseSupersetModelRestApi, + RelatedFieldFilter, + statsd_metrics, +) from superset.views.filters import FilterRelatedOwners logger = logging.getLogger(__name__) @@ -139,6 +143,7 @@ def __init__(self) -> None: @expose("/", methods=["POST"]) @protect() @safe + @statsd_metrics def post(self) -> Response: """Creates a new Dashboard --- @@ -193,6 +198,7 @@ def post(self) -> Response: @expose("/", methods=["PUT"]) @protect() @safe + @statsd_metrics def put( # pylint: disable=too-many-return-statements, arguments-differ self, pk: int ) -> Response: @@ -260,6 +266,7 @@ def put( # pylint: disable=too-many-return-statements, arguments-differ @expose("/", methods=["DELETE"]) @protect() @safe + @statsd_metrics def delete(self, pk: int) -> Response: # pylint: disable=arguments-differ """Deletes a Dashboard --- @@ -306,6 +313,7 @@ def delete(self, pk: int) -> Response: # pylint: disable=arguments-differ @expose("/", methods=["DELETE"]) @protect() @safe + @statsd_metrics @rison(get_delete_ids_schema) def bulk_delete( self, **kwargs: Any @@ -366,6 +374,7 @@ def bulk_delete( @expose("/export/", methods=["GET"]) @protect() @safe + @statsd_metrics @rison(get_export_ids_schema) def export(self, **kwargs: Any) -> Response: """Export dashboards diff --git a/superset/utils/core.py b/superset/utils/core.py index ca49989a5f2eb..89582da2ce69a 100644 --- a/superset/utils/core.py +++ b/superset/utils/core.py @@ -37,8 +37,10 @@ from email.utils import formatdate from enum import Enum from time import struct_time +from timeit import default_timer from typing import ( Any, + Callable, Dict, Iterator, List, @@ -1229,6 +1231,21 @@ def create_ssl_cert_file(certificate: str) -> str: return path +def time_function(func: Callable, *args, **kwargs) -> Tuple[float, Any]: + """ + Measures the amount of time a function takes to execute in ms + + :param func: The function execution time to measure + :param args: args to be passed to the function + :param kwargs: kwargs to be passed to the function + :return: A tuple with the duration and response from the function + """ + start = default_timer() + response = func(*args, **kwargs) + stop = default_timer() + return stop - start, response + + def MediumText() -> Variant: return Text().with_variant(MEDIUMTEXT(), "mysql") diff --git a/superset/views/base_api.py b/superset/views/base_api.py index 4afe13bf3d400..3f426178d6979 100644 --- a/superset/views/base_api.py +++ b/superset/views/base_api.py @@ -14,15 +14,18 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. +import functools import logging -from typing import cast, Dict, Set, Tuple, Type, Union +from typing import Any, cast, Dict, Optional, Set, Tuple, Type, Union +from flask import Response from flask_appbuilder import ModelRestApi from flask_appbuilder.api import expose, protect, rison, safe from flask_appbuilder.models.filters import BaseFilter, Filters from flask_appbuilder.models.sqla.filters import FilterStartsWith from superset.stats_logger import BaseStatsLogger +from superset.utils.core import time_function logger = logging.getLogger(__name__) get_related_schema = { @@ -35,6 +38,19 @@ } +def statsd_metrics(f): + """ + Handle sending all statsd metrics from the REST API + """ + + def wraps(self, *args: Any, **kwargs: Any) -> Response: + duration, response = time_function(f, self, *args, **kwargs) + self.send_stats_metrics(response, f.__name__, duration) + return response + + return functools.update_wrapper(wraps, f) + + class RelatedFieldFilter: # data class to specify what filter to use on a /related endpoint # pylint: disable=too-few-public-methods @@ -128,11 +144,71 @@ def _get_related_filter(self, datamodel, column_name: str, value: str) -> Filter return filters def incr_stats(self, action: str, func_name: str) -> None: + """ + Proxy function for statsd.incr to impose a key structure for REST API's + + :param action: String with an action name eg: error, success + :param func_name: The function name + """ self.stats_logger.incr(f"{self.__class__.__name__}.{func_name}.{action}") + def timing_stats(self, action: str, func_name: str, value: float) -> None: + """ + Proxy function for statsd.incr to impose a key structure for REST API's + + :param action: String with an action name eg: error, success + :param func_name: The function name + :param value: A float with the time it took for the endpoint to execute + """ + self.stats_logger.timing( + f"{self.__class__.__name__}.{func_name}.{action}", value + ) + + def send_stats_metrics( + self, response: Response, key: str, time_delta: Optional[float] = None + ) -> None: + """ + Helper function to handle sending statsd metrics + + :param response: flask response object, will evaluate if it was an error + :param key: The function name + :param time_delta: Optional time it took for the endpoint to execute + """ + if 200 <= response.status_code < 400: + self.incr_stats("success", key) + else: + self.incr_stats("error", key) + if time_delta: + self.timing_stats("time", key, time_delta) + + def info_headless(self, **kwargs) -> Response: + """ + Add statsd metrics to builtin FAB _info endpoint + """ + duration, response = time_function(super().info_headless, **kwargs) + self.send_stats_metrics(response, self.info.__name__, duration) + return response + + def get_headless(self, pk, **kwargs) -> Response: + """ + Add statsd metrics to builtin FAB GET endpoint + """ + duration, response = time_function(super().get_headless, pk, **kwargs) + self.send_stats_metrics(response, self.get.__name__, duration) + return response + + def get_list_headless(self, **kwargs) -> Response: + """ + Add statsd metrics to builtin FAB GET list endpoint + """ + duration, response = time_function(super().get_list_headless, **kwargs) + self.send_stats_metrics(response, self.get_list.__name__, duration) + return response + @expose("/related/", methods=["GET"]) @protect() @safe + @statsd_metrics @rison(get_related_schema) def related(self, column_name: str, **kwargs): """Get related fields data @@ -185,6 +261,7 @@ def related(self, column_name: str, **kwargs): $ref: '#/components/responses/500' """ if column_name not in self.allowed_rel_fields: + self.incr_stats("error", self.related.__name__) return self.response_404() args = kwargs.get("rison", {}) # handle pagination diff --git a/tests/base_tests.py b/tests/base_tests.py index 97c69f3778714..370adf306775f 100644 --- a/tests/base_tests.py +++ b/tests/base_tests.py @@ -18,10 +18,11 @@ """Unit tests for Superset""" import imp import json -from typing import Union -from unittest.mock import Mock +from typing import Union, Dict +from unittest.mock import Mock, patch import pandas as pd +from flask import Response from flask_appbuilder.security.sqla import models as ab_models from flask_testing import TestCase @@ -35,6 +36,7 @@ from superset.models.dashboard import Dashboard from superset.models.datasource_access_request import DatasourceAccessRequest from superset.utils.core import get_example_database +from superset.views.base_api import BaseSupersetModelRestApi FAKE_DB_NAME = "fake_db_100" @@ -328,3 +330,81 @@ def validate_sql( def get_dash_by_slug(self, dash_slug): sesh = db.session() return sesh.query(Dashboard).filter_by(slug=dash_slug).first() + + def get_assert_metric(self, uri: str, func_name: str) -> Response: + """ + Simple client get with an extra assertion for statsd metrics + + :param uri: The URI to use for the HTTP GET + :param func_name: The function name that the HTTP GET triggers + for the statsd metric assertion + :return: HTTP Response + """ + with patch.object( + BaseSupersetModelRestApi, "incr_stats", return_value=None + ) as mock_method: + rv = self.client.get(uri) + if 200 <= rv.status_code < 400: + mock_method.assert_called_once_with("success", func_name) + else: + mock_method.assert_called_once_with("error", func_name) + return rv + + def delete_assert_metric(self, uri: str, func_name: str) -> Response: + """ + Simple client delete with an extra assertion for statsd metrics + + :param uri: The URI to use for the HTTP DELETE + :param func_name: The function name that the HTTP DELETE triggers + for the statsd metric assertion + :return: HTTP Response + """ + with patch.object( + BaseSupersetModelRestApi, "incr_stats", return_value=None + ) as mock_method: + rv = self.client.delete(uri) + if 200 <= rv.status_code < 400: + mock_method.assert_called_once_with("success", func_name) + else: + mock_method.assert_called_once_with("error", func_name) + return rv + + def post_assert_metric(self, uri: str, data: Dict, func_name: str) -> Response: + """ + Simple client post with an extra assertion for statsd metrics + + :param uri: The URI to use for the HTTP POST + :param data: The JSON data payload to be posted + :param func_name: The function name that the HTTP POST triggers + for the statsd metric assertion + :return: HTTP Response + """ + with patch.object( + BaseSupersetModelRestApi, "incr_stats", return_value=None + ) as mock_method: + rv = self.client.post(uri, json=data) + if 200 <= rv.status_code < 400: + mock_method.assert_called_once_with("success", func_name) + else: + mock_method.assert_called_once_with("error", func_name) + return rv + + def put_assert_metric(self, uri: str, data: Dict, func_name: str) -> Response: + """ + Simple client put with an extra assertion for statsd metrics + + :param uri: The URI to use for the HTTP PUT + :param data: The JSON data payload to be posted + :param func_name: The function name that the HTTP PUT triggers + for the statsd metric assertion + :return: HTTP Response + """ + with patch.object( + BaseSupersetModelRestApi, "incr_stats", return_value=None + ) as mock_method: + rv = self.client.put(uri, json=data) + if 200 <= rv.status_code < 400: + mock_method.assert_called_once_with("success", func_name) + else: + mock_method.assert_called_once_with("error", func_name) + return rv diff --git a/tests/dashboards/api_tests.py b/tests/dashboards/api_tests.py index 03405b79e1e4f..f8119f55592d7 100644 --- a/tests/dashboards/api_tests.py +++ b/tests/dashboards/api_tests.py @@ -79,13 +79,13 @@ def insert_dashboard( def test_get_dashboard(self): """ - Dashboard API: Test get dashboard + Dashboard API: Test get dashboard """ admin = self.get_user("admin") dashboard = self.insert_dashboard("title", "slug1", [admin.id]) self.login(username="admin") uri = f"api/v1/dashboard/{dashboard.id}" - rv = self.client.get(uri) + rv = self.get_assert_metric(uri, "get") self.assertEqual(rv.status_code, 200) expected_result = { "changed_by": None, @@ -121,19 +121,28 @@ def test_get_dashboard(self): db.session.delete(dashboard) db.session.commit() + def test_info_dashboard(self): + """ + Dashboard API: Test info + """ + self.login(username="admin") + uri = f"api/v1/dashboard/_info" + rv = self.get_assert_metric(uri, "info") + self.assertEqual(rv.status_code, 200) + def test_get_dashboard_not_found(self): """ - Dashboard API: Test get dashboard not found + Dashboard API: Test get dashboard not found """ max_id = db.session.query(func.max(Dashboard.id)).scalar() self.login(username="admin") uri = f"api/v1/dashboard/{max_id + 1}" - rv = self.client.get(uri) + rv = self.get_assert_metric(uri, "get") self.assertEqual(rv.status_code, 404) def test_get_dashboard_no_data_access(self): """ - Dashboard API: Test get dashboard without data access + Dashboard API: Test get dashboard without data access """ admin = self.get_user("admin") dashboard = self.insert_dashboard("title", "slug1", [admin.id]) @@ -148,7 +157,7 @@ def test_get_dashboard_no_data_access(self): def test_get_dashboards_filter(self): """ - Dashboard API: Test get dashboards filter + Dashboard API: Test get dashboards filter """ admin = self.get_user("admin") gamma = self.get_user("gamma") @@ -160,7 +169,8 @@ def test_get_dashboards_filter(self): "filters": [{"col": "dashboard_title", "opr": "sw", "value": "ti"}] } uri = f"api/v1/dashboard/?q={prison.dumps(arguments)}" - rv = self.client.get(uri) + + rv = self.get_assert_metric(uri, "get_list") self.assertEqual(rv.status_code, 200) data = json.loads(rv.data.decode("utf-8")) self.assertEqual(data["count"], 1) @@ -182,7 +192,7 @@ def test_get_dashboards_filter(self): def test_get_dashboards_custom_filter(self): """ - Dashboard API: Test get dashboards custom filter + Dashboard API: Test get dashboards custom filter """ admin = self.get_user("admin") dashboard1 = self.insert_dashboard("foo", "ZY_bar", [admin.id]) @@ -232,7 +242,7 @@ def test_get_dashboards_custom_filter(self): def test_get_dashboards_no_data_access(self): """ - Dashboard API: Test get dashboards no data access + Dashboard API: Test get dashboards no data access """ admin = self.get_user("admin") dashboard = self.insert_dashboard("title", "slug1", [admin.id]) @@ -253,20 +263,20 @@ def test_get_dashboards_no_data_access(self): def test_delete_dashboard(self): """ - Dashboard API: Test delete + Dashboard API: Test delete """ admin_id = self.get_user("admin").id dashboard_id = self.insert_dashboard("title", "slug1", [admin_id]).id self.login(username="admin") uri = f"api/v1/dashboard/{dashboard_id}" - rv = self.client.delete(uri) + rv = self.delete_assert_metric(uri, "delete") self.assertEqual(rv.status_code, 200) model = db.session.query(Dashboard).get(dashboard_id) self.assertEqual(model, None) def test_delete_bulk_dashboards(self): """ - Dashboard API: Test delete bulk + Dashboard API: Test delete bulk """ admin_id = self.get_user("admin").id dashboard_count = 4 @@ -282,7 +292,7 @@ def test_delete_bulk_dashboards(self): self.login(username="admin") argument = dashboard_ids uri = f"api/v1/dashboard/?q={prison.dumps(argument)}" - rv = self.client.delete(uri) + rv = self.delete_assert_metric(uri, "bulk_delete") self.assertEqual(rv.status_code, 200) response = json.loads(rv.data.decode("utf-8")) expected_response = {"message": f"Deleted {dashboard_count} dashboards"} @@ -293,7 +303,7 @@ def test_delete_bulk_dashboards(self): def test_delete_bulk_dashboards_bad_request(self): """ - Dashboard API: Test delete bulk bad request + Dashboard API: Test delete bulk bad request """ dashboard_ids = [1, "a"] self.login(username="admin") @@ -304,7 +314,7 @@ def test_delete_bulk_dashboards_bad_request(self): def test_delete_not_found_dashboard(self): """ - Dashboard API: Test not found delete + Dashboard API: Test not found delete """ self.login(username="admin") dashboard_id = 1000 @@ -314,7 +324,7 @@ def test_delete_not_found_dashboard(self): def test_delete_bulk_dashboards_not_found(self): """ - Dashboard API: Test delete bulk not found + Dashboard API: Test delete bulk not found """ dashboard_ids = [1001, 1002] self.login(username="admin") @@ -325,7 +335,7 @@ def test_delete_bulk_dashboards_not_found(self): def test_delete_dashboard_admin_not_owned(self): """ - Dashboard API: Test admin delete not owned + Dashboard API: Test admin delete not owned """ gamma_id = self.get_user("gamma").id dashboard_id = self.insert_dashboard("title", "slug1", [gamma_id]).id @@ -339,7 +349,7 @@ def test_delete_dashboard_admin_not_owned(self): def test_delete_bulk_dashboard_admin_not_owned(self): """ - Dashboard API: Test admin delete bulk not owned + Dashboard API: Test admin delete bulk not owned """ gamma_id = self.get_user("gamma").id dashboard_count = 4 @@ -368,7 +378,7 @@ def test_delete_bulk_dashboard_admin_not_owned(self): def test_delete_dashboard_not_owned(self): """ - Dashboard API: Test delete try not owned + Dashboard API: Test delete try not owned """ user_alpha1 = self.create_user( "alpha1", "password", "Alpha", email="alpha1@superset.org" @@ -393,7 +403,7 @@ def test_delete_dashboard_not_owned(self): def test_delete_bulk_dashboard_not_owned(self): """ - Dashboard API: Test delete bulk try not owned + Dashboard API: Test delete bulk try not owned """ user_alpha1 = self.create_user( "alpha1", "password", "Alpha", email="alpha1@superset.org" @@ -455,7 +465,7 @@ def test_delete_bulk_dashboard_not_owned(self): def test_create_dashboard(self): """ - Dashboard API: Test create dashboard + Dashboard API: Test create dashboard """ admin_id = self.get_user("admin").id dashboard_data = { @@ -469,7 +479,7 @@ def test_create_dashboard(self): } self.login(username="admin") uri = "api/v1/dashboard/" - rv = self.client.post(uri, json=dashboard_data) + rv = self.post_assert_metric(uri, dashboard_data, "post") self.assertEqual(rv.status_code, 201) data = json.loads(rv.data.decode("utf-8")) model = db.session.query(Dashboard).get(data.get("id")) @@ -478,7 +488,7 @@ def test_create_dashboard(self): def test_create_simple_dashboard(self): """ - Dashboard API: Test create simple dashboard + Dashboard API: Test create simple dashboard """ dashboard_data = {"dashboard_title": "title1"} self.login(username="admin") @@ -492,7 +502,7 @@ def test_create_simple_dashboard(self): def test_create_dashboard_empty(self): """ - Dashboard API: Test create empty + Dashboard API: Test create empty """ dashboard_data = {} self.login(username="admin") @@ -516,12 +526,12 @@ def test_create_dashboard_empty(self): def test_create_dashboard_validate_title(self): """ - Dashboard API: Test create dashboard validate title + Dashboard API: Test create dashboard validate title """ dashboard_data = {"dashboard_title": "a" * 600} self.login(username="admin") uri = "api/v1/dashboard/" - rv = self.client.post(uri, json=dashboard_data) + rv = self.post_assert_metric(uri, dashboard_data, "post") self.assertEqual(rv.status_code, 400) response = json.loads(rv.data.decode("utf-8")) expected_response = { @@ -531,7 +541,7 @@ def test_create_dashboard_validate_title(self): def test_create_dashboard_validate_slug(self): """ - Dashboard API: Test create validate slug + Dashboard API: Test create validate slug """ admin_id = self.get_user("admin").id dashboard = self.insert_dashboard("title1", "slug1", [admin_id]) @@ -560,7 +570,7 @@ def test_create_dashboard_validate_slug(self): def test_create_dashboard_validate_owners(self): """ - Dashboard API: Test create validate owners + Dashboard API: Test create validate owners """ dashboard_data = {"dashboard_title": "title1", "owners": [1000]} self.login(username="admin") @@ -573,7 +583,7 @@ def test_create_dashboard_validate_owners(self): def test_create_dashboard_validate_json(self): """ - Dashboard API: Test create validate json + Dashboard API: Test create validate json """ dashboard_data = {"dashboard_title": "title1", "position_json": '{"A:"a"}'} self.login(username="admin") @@ -598,13 +608,13 @@ def test_create_dashboard_validate_json(self): def test_update_dashboard(self): """ - Dashboard API: Test update + Dashboard API: Test update """ admin = self.get_user("admin") dashboard_id = self.insert_dashboard("title1", "slug1", [admin.id]).id self.login(username="admin") uri = f"api/v1/dashboard/{dashboard_id}" - rv = self.client.put(uri, json=self.dashboard_data) + rv = self.put_assert_metric(uri, self.dashboard_data, "put") self.assertEqual(rv.status_code, 200) model = db.session.query(Dashboard).get(dashboard_id) self.assertEqual(model.dashboard_title, self.dashboard_data["dashboard_title"]) @@ -620,7 +630,7 @@ def test_update_dashboard(self): def test_update_dashboard_chart_owners(self): """ - Dashboard API: Test update chart owners + Dashboard API: Test update chart owners """ user_alpha1 = self.create_user( "alpha1", "password", "Alpha", email="alpha1@superset.org" @@ -663,7 +673,7 @@ def test_update_dashboard_chart_owners(self): def test_update_partial_dashboard(self): """ - Dashboard API: Test update partial + Dashboard API: Test update partial """ admin_id = self.get_user("admin").id dashboard_id = self.insert_dashboard("title1", "slug1", [admin_id]).id @@ -692,7 +702,7 @@ def test_update_partial_dashboard(self): def test_update_dashboard_new_owner(self): """ - Dashboard API: Test update set new owner to current user + Dashboard API: Test update set new owner to current user """ gamma_id = self.get_user("gamma").id admin = self.get_user("admin") @@ -711,7 +721,7 @@ def test_update_dashboard_new_owner(self): def test_update_dashboard_slug_formatting(self): """ - Dashboard API: Test update slug formatting + Dashboard API: Test update slug formatting """ admin_id = self.get_user("admin").id dashboard_id = self.insert_dashboard("title1", "slug1", [admin_id]).id @@ -728,7 +738,7 @@ def test_update_dashboard_slug_formatting(self): def test_update_dashboard_validate_slug(self): """ - Dashboard API: Test update validate slug + Dashboard API: Test update validate slug """ admin_id = self.get_user("admin").id dashboard1 = self.insert_dashboard("title1", "slug-1", [admin_id]) @@ -763,7 +773,7 @@ def test_update_dashboard_validate_slug(self): def test_update_published(self): """ - Dashboard API: Test update published patch + Dashboard API: Test update published patch """ admin = self.get_user("admin") gamma = self.get_user("gamma") @@ -785,7 +795,7 @@ def test_update_published(self): def test_update_dashboard_not_owned(self): """ - Dashboard API: Test update dashboard not owned + Dashboard API: Test update dashboard not owned """ user_alpha1 = self.create_user( "alpha1", "password", "Alpha", email="alpha1@superset.org" @@ -802,7 +812,7 @@ def test_update_dashboard_not_owned(self): self.login(username="alpha2", password="password") dashboard_data = {"dashboard_title": "title1_changed", "slug": "slug1 changed"} uri = f"api/v1/dashboard/{dashboard.id}" - rv = self.client.put(uri, json=dashboard_data) + rv = self.put_assert_metric(uri, dashboard_data, "put") self.assertEqual(rv.status_code, 403) db.session.delete(dashboard) db.session.delete(user_alpha1) @@ -811,13 +821,12 @@ def test_update_dashboard_not_owned(self): def test_export(self): """ - Dashboard API: Test dashboard export + Dashboard API: Test dashboard export """ self.login(username="admin") argument = [1, 2] uri = f"api/v1/dashboard/export/?q={prison.dumps(argument)}" - - rv = self.client.get(uri) + rv = self.get_assert_metric(uri, "export") self.assertEqual(rv.status_code, 200) self.assertEqual( rv.headers["Content-Disposition"], @@ -826,7 +835,7 @@ def test_export(self): def test_export_not_found(self): """ - Dashboard API: Test dashboard export not found + Dashboard API: Test dashboard export not found """ self.login(username="admin") argument = [1000] @@ -836,7 +845,7 @@ def test_export_not_found(self): def test_export_not_allowed(self): """ - Dashboard API: Test dashboard export not allowed + Dashboard API: Test dashboard export not allowed """ admin_id = self.get_user("admin").id dashboard = self.insert_dashboard("title", "slug1", [admin_id], published=False)