From 9971e177161818380a2dddc37d3e1d3b9af5617e Mon Sep 17 00:00:00 2001 From: Mathieu Leplatre Date: Mon, 7 Oct 2024 14:01:40 +0200 Subject: [PATCH 01/14] Move statsd out of core --- kinto/core/initialization.py | 53 ++---------- kinto/core/statsd.py | 64 +------------- kinto/core/testing.py | 3 +- kinto/plugins/statsd/__init__.py | 115 +++++++++++++++++++++++++ tests/core/resource/test_events.py | 6 +- tests/core/test_initialization.py | 97 ++++----------------- tests/plugins/test_history.py | 2 +- tests/plugins/test_quotas.py | 2 +- tests/{core => plugins}/test_statsd.py | 102 +++++++++++++++++++++- 9 files changed, 246 insertions(+), 198 deletions(-) create mode 100644 kinto/plugins/statsd/__init__.py rename tests/{core => plugins}/test_statsd.py (50%) diff --git a/kinto/core/initialization.py b/kinto/core/initialization.py index 561ebfc1d..1c82169ad 100644 --- a/kinto/core/initialization.py +++ b/kinto/core/initialization.py @@ -14,12 +14,11 @@ HTTPMethodNotAllowed, HTTPTemporaryRedirect, ) -from pyramid.interfaces import IAuthenticationPolicy from pyramid.renderers import JSON as JSONRenderer from pyramid.response import Response from pyramid.security import NO_PERMISSION_REQUIRED from pyramid.settings import asbool, aslist -from pyramid_multiauth import MultiAuthenticationPolicy, MultiAuthPolicySelected +from pyramid_multiauth import MultiAuthPolicySelected from kinto.core import cache, errors, permission, storage, utils from kinto.core.events import ACTIONS, ResourceChanged, ResourceRead @@ -334,51 +333,11 @@ def on_app_created(event): def setup_statsd(config): - settings = config.get_settings() - config.registry.statsd = None - - if settings["statsd_url"]: - statsd_mod = settings["statsd_backend"] - statsd_mod = config.maybe_dotted(statsd_mod) - client = statsd_mod.load_from_config(config) - - config.registry.statsd = client - - client.watch_execution_time(config.registry.cache, prefix="backend") - client.watch_execution_time(config.registry.storage, prefix="backend") - client.watch_execution_time(config.registry.permission, prefix="backend") - - # Commit so that configured policy can be queried. - config.commit() - policy = config.registry.queryUtility(IAuthenticationPolicy) - if isinstance(policy, MultiAuthenticationPolicy): - for name, subpolicy in policy.get_policies(): - client.watch_execution_time(subpolicy, prefix="authentication", classname=name) - else: - client.watch_execution_time(policy, prefix="authentication") - - def on_new_response(event): - request = event.request - - # Count unique users. - user_id = request.prefixed_userid - if user_id: - # Get rid of colons in metric packet (see #1282). - user_id = user_id.replace(":", ".") - client.count("users", unique=user_id) - - # Count authentication verifications. - if hasattr(request, "authn_type"): - client.count(f"authn_type.{request.authn_type}") - - # Count view calls. - service = request.current_service - if service: - client.count(f"view.{service.name}.{request.method}") - - config.add_subscriber(on_new_response, NewResponse) - - return client + warnings.warn( + "``setup_statsd()`` is now deprecated. Check release notes.", + DeprecationWarning, + ) + config.include("kinto.plugins.statsd") def install_middlewares(app, settings): diff --git a/kinto/core/statsd.py b/kinto/core/statsd.py index aea62ecd8..2c6b1ecad 100644 --- a/kinto/core/statsd.py +++ b/kinto/core/statsd.py @@ -1,63 +1 @@ -import types -from urllib.parse import urlparse - -from pyramid.exceptions import ConfigurationError - -from kinto.core import utils - - -try: - import statsd as statsd_module -except ImportError: # pragma: no cover - statsd_module = None - - -class Client: - def __init__(self, host, port, prefix): - self._client = statsd_module.StatsClient(host, port, prefix=prefix) - - def watch_execution_time(self, obj, prefix="", classname=None): - classname = classname or utils.classname(obj) - members = dir(obj) - for name in members: - value = getattr(obj, name) - is_method = isinstance(value, types.MethodType) - if not name.startswith("_") and is_method: - statsd_key = f"{prefix}.{classname}.{name}" - decorated_method = self.timer(statsd_key)(value) - setattr(obj, name, decorated_method) - - def timer(self, key): - return self._client.timer(key) - - def count(self, key, count=1, unique=None): - if unique is None: - return self._client.incr(key, count=count) - else: - return self._client.set(key, unique) - - -def statsd_count(request, count_key): - statsd = request.registry.statsd - if statsd: - statsd.count(count_key) - - -def load_from_config(config): - # If this is called, it means that a ``statsd_url`` was specified in settings. - # (see ``kinto.core.initialization``) - # Raise a proper error if the ``statsd`` module is not installed. - if statsd_module is None: - error_msg = "Please install Kinto with monitoring dependencies (e.g. statsd package)" - raise ConfigurationError(error_msg) - - settings = config.get_settings() - uri = settings["statsd_url"] - uri = urlparse(uri) - - if settings["project_name"] != "": - prefix = settings["project_name"] - else: - prefix = settings["statsd_prefix"] - - return Client(uri.hostname, uri.port, prefix) +from kinto.plugins.statsd import load_from_config # noqa: F401 diff --git a/kinto/core/testing.py b/kinto/core/testing.py index 01225ef36..9a779fb08 100644 --- a/kinto/core/testing.py +++ b/kinto/core/testing.py @@ -8,9 +8,10 @@ from cornice import errors as cornice_errors from pyramid.url import parse_url_overrides -from kinto.core import DEFAULT_SETTINGS, statsd +from kinto.core import DEFAULT_SETTINGS from kinto.core.storage import generators from kinto.core.utils import encode64, follow_subrequest, memcache, sqlalchemy +from kinto.plugins import statsd skip_if_ci = unittest.skipIf("CI" in os.environ, "ci") diff --git a/kinto/plugins/statsd/__init__.py b/kinto/plugins/statsd/__init__.py new file mode 100644 index 000000000..8fbafd436 --- /dev/null +++ b/kinto/plugins/statsd/__init__.py @@ -0,0 +1,115 @@ +import types +import warnings +from urllib.parse import urlparse + +from pyramid.events import NewResponse +from pyramid.exceptions import ConfigurationError +from pyramid.interfaces import IAuthenticationPolicy +from pyramid_multiauth import MultiAuthenticationPolicy + +from kinto.core import utils + + +try: + import statsd as statsd_module +except ImportError: # pragma: no cover + statsd_module = None + + +class Client: + def __init__(self, host, port, prefix): + self._client = statsd_module.StatsClient(host, port, prefix=prefix) + + def watch_execution_time(self, obj, prefix="", classname=None): + classname = classname or utils.classname(obj) + members = dir(obj) + for name in members: + value = getattr(obj, name) + is_method = isinstance(value, types.MethodType) + if not name.startswith("_") and is_method: + statsd_key = f"{prefix}.{classname}.{name}" + decorated_method = self.timer(statsd_key)(value) + setattr(obj, name, decorated_method) + + def timer(self, key): + return self._client.timer(key) + + def count(self, key, count=1, unique=None): + if unique is None: + return self._client.incr(key, count=count) + else: + return self._client.set(key, unique) + + +def statsd_count(request, count_key): + statsd = request.registry.statsd + if statsd: + statsd.count(count_key) + + +def load_from_config(config): + # If this is called, it means that a ``statsd_url`` was specified in settings. + # (see ``kinto.core.initialization``) + # Raise a proper error if the ``statsd`` module is not installed. + if statsd_module is None: + error_msg = "Please install Kinto with monitoring dependencies (e.g. statsd package)" + raise ConfigurationError(error_msg) + + settings = config.get_settings() + uri = settings["statsd_url"] + uri = urlparse(uri) + + if settings["project_name"] != "": + prefix = settings["project_name"] + else: + prefix = settings["statsd_prefix"] + + return Client(uri.hostname, uri.port, prefix) + + +def includeme(config): + settings = config.get_settings() + config.registry.statsd = None + + if not settings["statsd_url"]: + return + + statsd_mod = settings["statsd_backend"] + statsd_mod = config.maybe_dotted(statsd_mod) + client = statsd_mod.load_from_config(config) + + config.registry.statsd = client + + client.watch_execution_time(config.registry.cache, prefix="backend") + client.watch_execution_time(config.registry.storage, prefix="backend") + client.watch_execution_time(config.registry.permission, prefix="backend") + + # Commit so that configured policy can be queried. + config.commit() + policy = config.registry.queryUtility(IAuthenticationPolicy) + if isinstance(policy, MultiAuthenticationPolicy): + for name, subpolicy in policy.get_policies(): + client.watch_execution_time(subpolicy, prefix="authentication", classname=name) + else: + client.watch_execution_time(policy, prefix="authentication") + + def on_new_response(event): + request = event.request + + # Count unique users. + user_id = request.prefixed_userid + if user_id: + # Get rid of colons in metric packet (see #1282). + user_id = user_id.replace(":", ".") + client.count("users", unique=user_id) + + # Count authentication verifications. + if hasattr(request, "authn_type"): + client.count(f"authn_type.{request.authn_type}") + + # Count view calls. + service = request.current_service + if service: + client.count(f"view.{service.name}.{request.method}") + + config.add_subscriber(on_new_response, NewResponse) diff --git a/tests/core/resource/test_events.py b/tests/core/resource/test_events.py index cbb9c8b66..11cacf6d3 100644 --- a/tests/core/resource/test_events.py +++ b/tests/core/resource/test_events.py @@ -4,7 +4,6 @@ from pyramid.config import Configurator -from kinto.core import statsd from kinto.core.events import ( ACTIONS, AfterResourceChanged, @@ -14,7 +13,8 @@ notify_resource_event, ) from kinto.core.storage.exceptions import BackendError -from kinto.core.testing import unittest +from kinto.core.testing import skip_if_no_statsd, unittest +from kinto.plugins import statsd from ..support import BaseWebTest @@ -491,7 +491,7 @@ def __call__(self, event): return ClassListener() -@unittest.skipIf(not statsd.statsd_module, "statsd is not installed.") +@skip_if_no_statsd class StatsDTest(BaseWebTest, unittest.TestCase): @classmethod def get_app_settings(cls, *args, **kwargs): diff --git a/tests/core/test_initialization.py b/tests/core/test_initialization.py index caa072507..d5616c140 100644 --- a/tests/core/test_initialization.py +++ b/tests/core/test_initialization.py @@ -293,89 +293,30 @@ def unexpected_exceptions_are_reported(self): class StatsDConfigurationTest(unittest.TestCase): + settings = { + **kinto.core.DEFAULT_SETTINGS, + "statsd_url": "udp://host:8080", + "multiauth.policies": "basicauth", + } + def setUp(self): - settings = { - **kinto.core.DEFAULT_SETTINGS, - "statsd_url": "udp://host:8080", - "multiauth.policies": "basicauth", - } - self.config = Configurator(settings=settings) + patch = mock.patch("kinto.plugins.statsd.Client") + self.mocked = patch.start() + self.addCleanup(patch.stop) + + self.config = Configurator(settings=self.settings) self.config.registry.storage = {} self.config.registry.cache = {} self.config.registry.permission = {} - patch = mock.patch("kinto.core.statsd.load_from_config") - self.mocked = patch.start() - self.addCleanup(patch.stop) - - def test_statsd_isnt_called_if_statsd_url_is_not_set(self): - self.config.add_settings({"statsd_url": None}) - initialization.setup_statsd(self.config) - self.mocked.assert_not_called() - - def test_statsd_is_set_to_none_if_statsd_url_not_set(self): - self.config.add_settings({"statsd_url": None}) - initialization.setup_statsd(self.config) - self.assertEqual(self.config.registry.statsd, None) - - def test_statsd_is_called_if_statsd_url_is_set(self): - initialization.setup_statsd(self.config) - self.mocked.assert_called_with(self.config) - # See `tests/core/test_statsd.py` for instantiation tests. - - def test_statsd_is_expose_in_the_registry_if_url_is_set(self): - initialization.setup_statsd(self.config) - self.assertEqual(self.config.registry.statsd, self.mocked.return_value) - - def test_statsd_is_set_on_cache(self): - c = initialization.setup_statsd(self.config) - c.watch_execution_time.assert_any_call({}, prefix="backend") - - def test_statsd_is_set_on_storage(self): - c = initialization.setup_statsd(self.config) - c.watch_execution_time.assert_any_call({}, prefix="backend") - - def test_statsd_is_set_on_permission(self): - c = initialization.setup_statsd(self.config) - c.watch_execution_time.assert_any_call({}, prefix="backend") - - def test_statsd_is_set_on_authentication(self): - c = initialization.setup_statsd(self.config) - c.watch_execution_time.assert_any_call(None, prefix="authentication") - - def test_statsd_counts_nothing_on_anonymous_requests(self): - kinto.core.initialize(self.config, "0.0.1", "settings_prefix") - app = webtest.TestApp(self.config.make_wsgi_app()) - app.get("/") - self.assertFalse(self.mocked.count.called) - - def test_statsd_counts_views_and_methods(self): - kinto.core.initialize(self.config, "0.0.1", "settings_prefix") - app = webtest.TestApp(self.config.make_wsgi_app()) - app.get("/v0/__heartbeat__") - self.mocked().count.assert_any_call("view.heartbeat.GET") - - def test_statsd_counts_unknown_urls(self): - kinto.core.initialize(self.config, "0.0.1", "settings_prefix") - app = webtest.TestApp(self.config.make_wsgi_app()) - app.get("/v0/coucou", status=404) - self.assertFalse(self.mocked.count.called) - - @mock.patch("kinto.core.utils.hmac_digest") - def test_statsd_counts_unique_users(self, digest_mocked): - digest_mocked.return_value = "mat" - kinto.core.initialize(self.config, "0.0.1", "settings_prefix") - app = webtest.TestApp(self.config.make_wsgi_app()) - headers = {"Authorization": "Basic bWF0Og=="} - app.get("/v0/", headers=headers) - self.mocked().count.assert_any_call("users", unique="basicauth.mat") - - def test_statsd_counts_authentication_types(self): - kinto.core.initialize(self.config, "0.0.1", "settings_prefix") - app = webtest.TestApp(self.config.make_wsgi_app()) - headers = {"Authorization": "Basic bWF0Og=="} - app.get("/v0/", headers=headers) - self.mocked().count.assert_any_call("authn_type.basicauth") + def test_statsd_is_instanciated_from_plugin(self): + with mock.patch("warnings.warn") as mocked_warnings: + initialization.setup_statsd(self.config) + mocked_warnings.assert_called_with( + "``setup_statsd()`` is now deprecated. Check release notes.", + DeprecationWarning, + ) + self.mocked.assert_called_with("host", 8080, "kinto.core") class RequestsConfigurationTest(unittest.TestCase): diff --git a/tests/plugins/test_history.py b/tests/plugins/test_history.py index 720136151..f20a7a49f 100644 --- a/tests/plugins/test_history.py +++ b/tests/plugins/test_history.py @@ -22,7 +22,7 @@ def test_a_statsd_timer_is_used_for_history_if_configured(self): "includes": "kinto.plugins.history", } config = testing.setUp(settings=settings) - with mock.patch("kinto.core.statsd.Client.timer") as mocked: + with mock.patch("kinto.plugins.statsd.Client.timer") as mocked: kinto_main(None, config=config) mocked.assert_called_with("plugins.history") diff --git a/tests/plugins/test_quotas.py b/tests/plugins/test_quotas.py index c31408eb7..e01046fdf 100644 --- a/tests/plugins/test_quotas.py +++ b/tests/plugins/test_quotas.py @@ -29,7 +29,7 @@ def test_a_statsd_timer_is_used_for_quotas_if_configured(self): "includes": "kinto.plugins.quotas", } config = testing.setUp(settings=settings) - with mock.patch("kinto.core.statsd.Client.timer") as mocked: + with mock.patch("kinto.plugins.statsd.Client.timer") as mocked: kinto_main(None, config=config) mocked.assert_called_with("plugins.quotas") diff --git a/tests/core/test_statsd.py b/tests/plugins/test_statsd.py similarity index 50% rename from tests/core/test_statsd.py rename to tests/plugins/test_statsd.py index 748b9ce06..07b63154c 100644 --- a/tests/core/test_statsd.py +++ b/tests/plugins/test_statsd.py @@ -1,12 +1,106 @@ from unittest import mock +import webtest from pyramid import testing +from pyramid.config import Configurator from pyramid.exceptions import ConfigurationError -from kinto.core import statsd +import kinto from kinto.core.testing import skip_if_no_statsd, unittest +from kinto.plugins import statsd -from .support import BaseWebTest +from ..support import BaseWebTest + + +class StatsDConfigurationTest(unittest.TestCase): + def setUp(self): + settings = { + **kinto.core.DEFAULT_SETTINGS, + "statsd_url": "udp://host:8080", + "multiauth.policies": "basicauth", + } + self.config = Configurator(settings=settings) + self.config.registry.storage = {} + self.config.registry.cache = {} + self.config.registry.permission = {} + + patch = mock.patch("kinto.plugins.statsd.load_from_config") + self.mocked = patch.start() + self.addCleanup(patch.stop) + + def test_statsd_isnt_called_if_statsd_url_is_not_set(self): + self.config.add_settings({"statsd_url": None}) + self.config.include("kinto.plugins.statsd") + self.mocked.assert_not_called() + + def test_statsd_is_set_to_none_if_statsd_url_not_set(self): + self.config.add_settings({"statsd_url": None}) + self.config.include("kinto.plugins.statsd") + self.assertEqual(self.config.registry.statsd, None) + + def test_statsd_is_called_if_statsd_url_is_set(self): + # For some reasons, when using ``self.config.include("kinto.plugins.statsd")`` + # the config object is recreated breaks ``assert_called_with(self.config)``. + statsd.includeme(self.config) + self.mocked.assert_called_with(self.config) + + def test_statsd_is_expose_in_the_registry_if_url_is_set(self): + self.config.include("kinto.plugins.statsd") + self.assertEqual(self.config.registry.statsd, self.mocked.return_value) + + def test_statsd_is_set_on_cache(self): + self.config.include("kinto.plugins.statsd") + c = self.config.registry.statsd + c.watch_execution_time.assert_any_call({}, prefix="backend") + + def test_statsd_is_set_on_storage(self): + self.config.include("kinto.plugins.statsd") + c = self.config.registry.statsd + c.watch_execution_time.assert_any_call({}, prefix="backend") + + def test_statsd_is_set_on_permission(self): + self.config.include("kinto.plugins.statsd") + c = self.config.registry.statsd + c.watch_execution_time.assert_any_call({}, prefix="backend") + + def test_statsd_is_set_on_authentication(self): + self.config.include("kinto.plugins.statsd") + c = self.config.registry.statsd + c.watch_execution_time.assert_any_call(None, prefix="authentication") + + def test_statsd_counts_nothing_on_anonymous_requests(self): + kinto.core.initialize(self.config, "0.0.1", "settings_prefix") + app = webtest.TestApp(self.config.make_wsgi_app()) + app.get("/") + self.assertFalse(self.mocked.count.called) + + def test_statsd_counts_views_and_methods(self): + kinto.core.initialize(self.config, "0.0.1", "settings_prefix") + app = webtest.TestApp(self.config.make_wsgi_app()) + app.get("/v0/__heartbeat__") + self.mocked().count.assert_any_call("view.heartbeat.GET") + + def test_statsd_counts_unknown_urls(self): + kinto.core.initialize(self.config, "0.0.1", "settings_prefix") + app = webtest.TestApp(self.config.make_wsgi_app()) + app.get("/v0/coucou", status=404) + self.assertFalse(self.mocked.count.called) + + @mock.patch("kinto.core.utils.hmac_digest") + def test_statsd_counts_unique_users(self, digest_mocked): + digest_mocked.return_value = "mat" + kinto.core.initialize(self.config, "0.0.1", "settings_prefix") + app = webtest.TestApp(self.config.make_wsgi_app()) + headers = {"Authorization": "Basic bWF0Og=="} + app.get("/v0/", headers=headers) + self.mocked().count.assert_any_call("users", unique="basicauth.mat") + + def test_statsd_counts_authentication_types(self): + kinto.core.initialize(self.config, "0.0.1", "settings_prefix") + app = webtest.TestApp(self.config.make_wsgi_app()) + headers = {"Authorization": "Basic bWF0Og=="} + app.get("/v0/", headers=headers) + self.mocked().count.assert_any_call("authn_type.basicauth") class TestedClass: @@ -69,14 +163,14 @@ def test_count_with_unique_uses_sets_for_key(self): self.client.count("click", unique="menu") mocked_client.set.assert_called_with("click", "menu") - @mock.patch("kinto.core.statsd.statsd_module") + @mock.patch("kinto.plugins.statsd.statsd_module") def test_load_from_config(self, module_mock): config = testing.setUp() config.registry.settings = self.settings statsd.load_from_config(config) module_mock.StatsClient.assert_called_with("foo", 1234, prefix="prefix") - @mock.patch("kinto.core.statsd.statsd_module") + @mock.patch("kinto.plugins.statsd.statsd_module") def test_load_from_config_uses_project_name_if_defined(self, module_mock): config = testing.setUp() config.registry.settings = {**self.settings, "project_name": "projectname"} From 51b2ef0286e21887e4cf12c648d2f08188de156f Mon Sep 17 00:00:00 2001 From: Mathieu Leplatre Date: Mon, 7 Oct 2024 14:07:02 +0200 Subject: [PATCH 02/14] Include statsd plugin by default --- kinto/core/__init__.py | 1 - kinto/core/initialization.py | 3 +++ tests/core/test_initialization.py | 5 +++++ 3 files changed, 8 insertions(+), 1 deletion(-) diff --git a/kinto/core/__init__.py b/kinto/core/__init__.py index 464abe1f2..51b47c31c 100644 --- a/kinto/core/__init__.py +++ b/kinto/core/__init__.py @@ -66,7 +66,6 @@ "kinto.core.initialization.setup_authentication", "kinto.core.initialization.setup_backoff", "kinto.core.initialization.setup_sentry", - "kinto.core.initialization.setup_statsd", "kinto.core.initialization.setup_listeners", "kinto.core.events.setup_transaction_hook", ), diff --git a/kinto/core/initialization.py b/kinto/core/initialization.py index 1c82169ad..ece04e4c6 100644 --- a/kinto/core/initialization.py +++ b/kinto/core/initialization.py @@ -613,3 +613,6 @@ def initialize(config, version=None, settings_prefix="", default_settings=None): # Include kinto.core views with the correct api version prefix. config.include("kinto.core", route_prefix=api_version) config.route_prefix = api_version + + # While statsd is deprecated, we include its plugin by default. + config.include("kinto.plugins.statsd") diff --git a/tests/core/test_initialization.py b/tests/core/test_initialization.py index d5616c140..52745dd50 100644 --- a/tests/core/test_initialization.py +++ b/tests/core/test_initialization.py @@ -318,6 +318,11 @@ def test_statsd_is_instanciated_from_plugin(self): ) self.mocked.assert_called_with("host", 8080, "kinto.core") + def test_statsd_is_included_by_default(self): + kinto.core.initialize(self.config, "0.0.1", "name") + + self.mocked.assert_called_with("host", 8080, "kinto.core") + class RequestsConfigurationTest(unittest.TestCase): def _get_app(self, settings={}): From f93b4969c1f8cd1b997ba3f088f2eb882f67eddf Mon Sep 17 00:00:00 2001 From: Mathieu Leplatre Date: Mon, 7 Oct 2024 14:26:13 +0200 Subject: [PATCH 03/14] Rename config.registry.statsd to config.registry.metrics --- kinto/core/initialization.py | 6 +++--- kinto/plugins/history/__init__.py | 6 +++--- kinto/plugins/quotas/__init__.py | 6 +++--- kinto/plugins/statsd/__init__.py | 19 ++++++++++++----- tests/core/resource/test_events.py | 5 +++-- tests/plugins/test_statsd.py | 33 +++++++++++++++++++----------- 6 files changed, 47 insertions(+), 28 deletions(-) diff --git a/kinto/core/initialization.py b/kinto/core/initialization.py index ece04e4c6..6f4b89a56 100644 --- a/kinto/core/initialization.py +++ b/kinto/core/initialization.py @@ -478,10 +478,10 @@ def setup_listeners(config): listener = listener_mod.load_from_config(config, prefix) # If StatsD is enabled, monitor execution time of listeners. - if getattr(config.registry, "statsd", None): - statsd_client = config.registry.statsd + if getattr(config.registry, "metrics", None): + metrics = config.registry.metrics key = f"listeners.{name}" - listener = statsd_client.timer(key)(listener.__call__) + listener = metrics.timer(key)(listener.__call__) # Optional filter by event action. actions_setting = prefix + "actions" diff --git a/kinto/plugins/history/__init__.py b/kinto/plugins/history/__init__.py index 7551ea127..48ee71fb2 100644 --- a/kinto/plugins/history/__init__.py +++ b/kinto/plugins/history/__init__.py @@ -14,11 +14,11 @@ def includeme(config): # Activate end-points. config.scan("kinto.plugins.history.views") - # If StatsD is enabled, monitor execution time of listener. + # If metrics are enabled, monitor execution time of listener. listener = on_resource_changed - if config.registry.statsd: + if config.registry.metrics: key = "plugins.history" - listener = config.registry.statsd.timer(key)(on_resource_changed) + listener = config.registry.metrics.timer(key)(on_resource_changed) # Listen to every resources (except history) config.add_subscriber( diff --git a/kinto/plugins/quotas/__init__.py b/kinto/plugins/quotas/__init__.py index 144fe151b..e3d40d70d 100644 --- a/kinto/plugins/quotas/__init__.py +++ b/kinto/plugins/quotas/__init__.py @@ -10,11 +10,11 @@ def includeme(config): url="https://kinto.readthedocs.io", ) - # If StatsD is enabled, monitor execution time of listener. + # If metrics are enabled, monitor execution time of listener. listener = on_resource_changed - if config.registry.statsd: + if config.registry.metrics: key = "plugins.quotas" - listener = config.registry.statsd.timer(key)(on_resource_changed) + listener = config.registry.metrics.timer(key)(on_resource_changed) # Listen to every resources (except history) config.add_subscriber( diff --git a/kinto/plugins/statsd/__init__.py b/kinto/plugins/statsd/__init__.py index 8fbafd436..61f451484 100644 --- a/kinto/plugins/statsd/__init__.py +++ b/kinto/plugins/statsd/__init__.py @@ -42,9 +42,9 @@ def count(self, key, count=1, unique=None): def statsd_count(request, count_key): - statsd = request.registry.statsd - if statsd: - statsd.count(count_key) + metrics = request.registry.metrics + if metrics: + metrics.count(count_key) def load_from_config(config): @@ -67,9 +67,17 @@ def load_from_config(config): return Client(uri.hostname, uri.port, prefix) +def deprecated_registry(self): + warnings.warn( + "``config.registry.statsd`` is now deprecated. Check release notes.", + DeprecationWarning, + ) + return self.metrics + + def includeme(config): settings = config.get_settings() - config.registry.statsd = None + config.registry.metrics = None if not settings["statsd_url"]: return @@ -77,8 +85,9 @@ def includeme(config): statsd_mod = settings["statsd_backend"] statsd_mod = config.maybe_dotted(statsd_mod) client = statsd_mod.load_from_config(config) + config.registry.metrics = client - config.registry.statsd = client + config.registry.__class__.statsd = property(deprecated_registry) client.watch_execution_time(config.registry.cache, prefix="backend") client.watch_execution_time(config.registry.storage, prefix="backend") diff --git a/tests/core/resource/test_events.py b/tests/core/resource/test_events.py index 11cacf6d3..6de66c201 100644 --- a/tests/core/resource/test_events.py +++ b/tests/core/resource/test_events.py @@ -506,8 +506,9 @@ def get_app_settings(cls, *args, **kwargs): return settings def test_statds_tracks_listeners_execution_duration(self): - statsd_client = self.app.app.registry.statsd._client - with mock.patch.object(statsd_client, "timing") as mocked: + # This test may break when introducing a generic interface for Prometheus. + metrics_client = self.app.app.registry.metrics._client + with mock.patch.object(metrics_client, "timing") as mocked: self.app.post_json(self.plural_url, {"data": {"name": "pouet"}}, headers=self.headers) timers = set(c[0][0] for c in mocked.call_args_list) self.assertIn("listeners.test", timers) diff --git a/tests/plugins/test_statsd.py b/tests/plugins/test_statsd.py index 07b63154c..060d6a778 100644 --- a/tests/plugins/test_statsd.py +++ b/tests/plugins/test_statsd.py @@ -36,36 +36,45 @@ def test_statsd_isnt_called_if_statsd_url_is_not_set(self): def test_statsd_is_set_to_none_if_statsd_url_not_set(self): self.config.add_settings({"statsd_url": None}) self.config.include("kinto.plugins.statsd") - self.assertEqual(self.config.registry.statsd, None) + self.assertEqual(self.config.registry.metrics, None) def test_statsd_is_called_if_statsd_url_is_set(self): # For some reasons, when using ``self.config.include("kinto.plugins.statsd")`` - # the config object is recreated breaks ``assert_called_with(self.config)``. + # the config object is recreated and breaks ``assert_called_with(self.config)``. statsd.includeme(self.config) self.mocked.assert_called_with(self.config) - def test_statsd_is_expose_in_the_registry_if_url_is_set(self): + def test_metrics_attr_is_exposed_in_the_registry_if_url_is_set(self): self.config.include("kinto.plugins.statsd") - self.assertEqual(self.config.registry.statsd, self.mocked.return_value) + self.assertEqual(self.config.registry.metrics, self.mocked.return_value) + + def test_statsd_attr_is_exposed_in_the_registry_if_url_is_set(self): + self.config.include("kinto.plugins.statsd") + with mock.patch("warnings.warn") as mocked_warnings: + self.config.registry.statsd.count("key") + mocked_warnings.assert_called_with( + "``config.registry.statsd`` is now deprecated. Check release notes.", + DeprecationWarning, + ) def test_statsd_is_set_on_cache(self): self.config.include("kinto.plugins.statsd") - c = self.config.registry.statsd + c = self.config.registry.metrics c.watch_execution_time.assert_any_call({}, prefix="backend") def test_statsd_is_set_on_storage(self): self.config.include("kinto.plugins.statsd") - c = self.config.registry.statsd + c = self.config.registry.metrics c.watch_execution_time.assert_any_call({}, prefix="backend") def test_statsd_is_set_on_permission(self): self.config.include("kinto.plugins.statsd") - c = self.config.registry.statsd + c = self.config.registry.metrics c.watch_execution_time.assert_any_call({}, prefix="backend") def test_statsd_is_set_on_authentication(self): self.config.include("kinto.plugins.statsd") - c = self.config.registry.statsd + c = self.config.registry.metrics c.watch_execution_time.assert_any_call(None, prefix="authentication") def test_statsd_counts_nothing_on_anonymous_requests(self): @@ -179,12 +188,12 @@ def test_load_from_config_uses_project_name_if_defined(self, module_mock): def test_statsd_count_handle_unconfigured_statsd_client(self): request = mock.MagicMock() - request.registry.statsd = None + request.registry.metrics = None statsd.statsd_count(request, "toto") # Doesn't raise def test_statsd_count_call_the_client_if_configured(self): request = mock.MagicMock() - request.registry.statsd = self.mocked_client + request.registry.metrics = self.mocked_client statsd.statsd_count(request, "toto") self.mocked_client.count.assert_called_with("toto") @@ -201,13 +210,13 @@ def get_app_settings(cls, *args, **kwargs): return settings def test_statds_tracks_listeners_execution_duration(self): - statsd_client = self.app.app.registry.statsd._client + statsd_client = self.app.app.registry.metrics._client with mock.patch.object(statsd_client, "timing") as mocked: self.app.get("/", headers=self.headers) self.assertTrue(mocked.called) def test_statds_tracks_authentication_policies(self): - statsd_client = self.app.app.registry.statsd._client + statsd_client = self.app.app.registry.metrics._client with mock.patch.object(statsd_client, "timing") as mocked: self.app.get("/", headers=self.headers) timers = set(c[0][0] for c in mocked.call_args_list) From eb03f718ab61a1baa756d7422e01a4b25bcdcf88 Mon Sep 17 00:00:00 2001 From: Mathieu Leplatre Date: Tue, 8 Oct 2024 14:03:57 +0200 Subject: [PATCH 04/14] Mock client instead of dynamically loaded function --- tests/plugins/test_statsd.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/tests/plugins/test_statsd.py b/tests/plugins/test_statsd.py index 060d6a778..17030548f 100644 --- a/tests/plugins/test_statsd.py +++ b/tests/plugins/test_statsd.py @@ -23,8 +23,10 @@ def setUp(self): self.config.registry.storage = {} self.config.registry.cache = {} self.config.registry.permission = {} + self.assertFalse(hasattr(self.config.registry, "statsd")) + self.assertFalse(hasattr(self.config.registry, "metrics")) - patch = mock.patch("kinto.plugins.statsd.load_from_config") + patch = mock.patch("kinto.plugins.statsd.Client") self.mocked = patch.start() self.addCleanup(patch.stop) @@ -41,8 +43,9 @@ def test_statsd_is_set_to_none_if_statsd_url_not_set(self): def test_statsd_is_called_if_statsd_url_is_set(self): # For some reasons, when using ``self.config.include("kinto.plugins.statsd")`` # the config object is recreated and breaks ``assert_called_with(self.config)``. - statsd.includeme(self.config) - self.mocked.assert_called_with(self.config) + self.config.include("kinto.plugins.statsd") + self.mocked.assert_called_with("host", 8080, "kinto.core") + self.assertIsNotNone(self.config.registry.metrics) def test_metrics_attr_is_exposed_in_the_registry_if_url_is_set(self): self.config.include("kinto.plugins.statsd") From 13a36492cfe361d97d6a6a2562060921539c90ed Mon Sep 17 00:00:00 2001 From: Mathieu Leplatre Date: Tue, 8 Oct 2024 17:55:48 +0200 Subject: [PATCH 05/14] Introduce the IMetricsInterface --- kinto/core/__init__.py | 1 + kinto/core/initialization.py | 30 +++++++++++++++--- kinto/core/metrics.py | 26 ++++++++++++++++ kinto/plugins/statsd/__init__.py | 25 +++++---------- tests/core/test_initialization.py | 52 +++++++++++++++++++++++++++++-- tests/plugins/test_statsd.py | 38 ++-------------------- 6 files changed, 112 insertions(+), 60 deletions(-) create mode 100644 kinto/core/metrics.py diff --git a/kinto/core/__init__.py b/kinto/core/__init__.py index 51b47c31c..0665b58a3 100644 --- a/kinto/core/__init__.py +++ b/kinto/core/__init__.py @@ -67,6 +67,7 @@ "kinto.core.initialization.setup_backoff", "kinto.core.initialization.setup_sentry", "kinto.core.initialization.setup_listeners", + "kinto.core.initialization.setup_metrics", "kinto.core.events.setup_transaction_hook", ), "event_listeners": "", diff --git a/kinto/core/initialization.py b/kinto/core/initialization.py index 6f4b89a56..c9574bc7c 100644 --- a/kinto/core/initialization.py +++ b/kinto/core/initialization.py @@ -20,7 +20,7 @@ from pyramid.settings import asbool, aslist from pyramid_multiauth import MultiAuthPolicySelected -from kinto.core import cache, errors, permission, storage, utils +from kinto.core import cache, errors, metrics, permission, storage, utils from kinto.core.events import ACTIONS, ResourceChanged, ResourceRead @@ -337,7 +337,7 @@ def setup_statsd(config): "``setup_statsd()`` is now deprecated. Check release notes.", DeprecationWarning, ) - config.include("kinto.plugins.statsd") + setup_metrics(config) def install_middlewares(app, settings): @@ -425,6 +425,29 @@ def on_new_response(event): config.add_subscriber(on_new_response, NewResponse) +def setup_metrics(config): + settings = config.get_settings() + + # This does not fully respect the Pyramid/ZCA patterns, but the rest of Kinto uses + # `registry.storage`, `registry.cache`, etc. Consistency seems more important. + config.registry.__class__.metrics = property( + lambda reg: reg.queryUtility(metrics.IMetricsService) + ) + + def deprecated_registry(self): + warnings.warn( + "``config.registry.statsd`` is now deprecated. Check release notes.", + DeprecationWarning, + ) + return self.metrics + + config.registry.__class__.statsd = property(deprecated_registry) + + # While statsd is deprecated, we include its plugin by default for retro-compability. + if settings["statsd_url"]: + config.include("kinto.plugins.statsd") + + class EventActionFilter: def __init__(self, actions, config): actions = ACTIONS.from_string_list(actions) @@ -613,6 +636,3 @@ def initialize(config, version=None, settings_prefix="", default_settings=None): # Include kinto.core views with the correct api version prefix. config.include("kinto.core", route_prefix=api_version) config.route_prefix = api_version - - # While statsd is deprecated, we include its plugin by default. - config.include("kinto.plugins.statsd") diff --git a/kinto/core/metrics.py b/kinto/core/metrics.py new file mode 100644 index 000000000..ce27ad433 --- /dev/null +++ b/kinto/core/metrics.py @@ -0,0 +1,26 @@ +from zope.interface import Interface + + +class IMetricsService(Interface): + """ + An interface that defines the metrics service contract. + Any class implementing this must provide all its methods. + """ + + def watch_execution_time(obj, prefix="", classname=None): + """ + TODO: move this elsewhere since it's not specific by implementer. + Decorate all methods of an object in order to watch their execution time. + Metrics will be named `{prefix}.{classname}.{method}`. + """ + + def timer(key): + """ + Watch execution time. + """ + + def count(key, count=1, unique=None): + """ + Count occurrences. If `unique` is set, overwrites the counter value + on each call. + """ diff --git a/kinto/plugins/statsd/__init__.py b/kinto/plugins/statsd/__init__.py index 61f451484..d0e85323b 100644 --- a/kinto/plugins/statsd/__init__.py +++ b/kinto/plugins/statsd/__init__.py @@ -1,13 +1,13 @@ import types -import warnings from urllib.parse import urlparse from pyramid.events import NewResponse from pyramid.exceptions import ConfigurationError from pyramid.interfaces import IAuthenticationPolicy from pyramid_multiauth import MultiAuthenticationPolicy +from zope.interface import implementer -from kinto.core import utils +from kinto.core import metrics, utils try: @@ -16,7 +16,8 @@ statsd_module = None -class Client: +@implementer(metrics.IMetricsService) +class StatsDService: def __init__(self, host, port, prefix): self._client = statsd_module.StatsClient(host, port, prefix=prefix) @@ -64,31 +65,19 @@ def load_from_config(config): else: prefix = settings["statsd_prefix"] - return Client(uri.hostname, uri.port, prefix) - - -def deprecated_registry(self): - warnings.warn( - "``config.registry.statsd`` is now deprecated. Check release notes.", - DeprecationWarning, - ) - return self.metrics + return StatsDService(uri.hostname, uri.port, prefix) def includeme(config): settings = config.get_settings() - config.registry.metrics = None - - if not settings["statsd_url"]: - return statsd_mod = settings["statsd_backend"] statsd_mod = config.maybe_dotted(statsd_mod) client = statsd_mod.load_from_config(config) - config.registry.metrics = client - config.registry.__class__.statsd = property(deprecated_registry) + config.registry.registerUtility(client, metrics.IMetricsService) + # TODO: move this elsewhere since it's not specific to StatsD. client.watch_execution_time(config.registry.cache, prefix="backend") client.watch_execution_time(config.registry.storage, prefix="backend") client.watch_execution_time(config.registry.permission, prefix="backend") diff --git a/tests/core/test_initialization.py b/tests/core/test_initialization.py index 52745dd50..5fe4f719b 100644 --- a/tests/core/test_initialization.py +++ b/tests/core/test_initialization.py @@ -300,7 +300,7 @@ class StatsDConfigurationTest(unittest.TestCase): } def setUp(self): - patch = mock.patch("kinto.plugins.statsd.Client") + patch = mock.patch("kinto.plugins.statsd.StatsDService") self.mocked = patch.start() self.addCleanup(patch.stop) @@ -309,7 +309,7 @@ def setUp(self): self.config.registry.cache = {} self.config.registry.permission = {} - def test_statsd_is_instanciated_from_plugin(self): + def test_setup_statsd_step_is_still_supported(self): with mock.patch("warnings.warn") as mocked_warnings: initialization.setup_statsd(self.config) mocked_warnings.assert_called_with( @@ -323,6 +323,54 @@ def test_statsd_is_included_by_default(self): self.mocked.assert_called_with("host", 8080, "kinto.core") + def test_statsd_isnt_included_if_statsd_url_is_not_set(self): + self.config.add_settings({"statsd_url": None}) + kinto.core.initialize(self.config, "0.0.1", "name") + + self.mocked.assert_not_called() + + +class MetricsRegistryConfigurationTest(unittest.TestCase): + settings = kinto.core.DEFAULT_SETTINGS + + def setUp(self): + self.config = Configurator(settings=self.settings) + self.config.registry.storage = {} + self.config.registry.cache = {} + self.config.registry.permission = {} + + patch = mock.patch("kinto.plugins.statsd.StatsDService") + self.mocked = patch.start() + self.addCleanup(patch.stop) + + def test_metrics_and_statsd_are_none_if_statsd_url_not_set(self): + self.config.add_settings({"statsd_url": None}) + + initialization.setup_metrics(self.config) + + self.assertIsNone(self.config.registry.statsd) + self.assertIsNone(self.config.registry.metrics) + + def test_metrics_attr_is_set_if_statsd_url_is_set(self): + self.config.add_settings({"statsd_url": "udp://host:8080"}) + + initialization.setup_metrics(self.config) + + self.assertIsNotNone(self.config.registry.statsd) + self.assertIsNotNone(self.config.registry.metrics) + + def test_statsd_attr_is_exposed_in_the_registry_if_url_is_set(self): + self.config.add_settings({"statsd_url": "udp://host:8080"}) + + initialization.setup_metrics(self.config) + + with mock.patch("warnings.warn") as mocked_warnings: + self.config.registry.statsd.count("key") + mocked_warnings.assert_called_with( + "``config.registry.statsd`` is now deprecated. Check release notes.", + DeprecationWarning, + ) + class RequestsConfigurationTest(unittest.TestCase): def _get_app(self, settings={}): diff --git a/tests/plugins/test_statsd.py b/tests/plugins/test_statsd.py index 17030548f..42122254d 100644 --- a/tests/plugins/test_statsd.py +++ b/tests/plugins/test_statsd.py @@ -12,7 +12,7 @@ from ..support import BaseWebTest -class StatsDConfigurationTest(unittest.TestCase): +class StatsDIncludeTest(unittest.TestCase): def setUp(self): settings = { **kinto.core.DEFAULT_SETTINGS, @@ -23,43 +23,11 @@ def setUp(self): self.config.registry.storage = {} self.config.registry.cache = {} self.config.registry.permission = {} - self.assertFalse(hasattr(self.config.registry, "statsd")) - self.assertFalse(hasattr(self.config.registry, "metrics")) - patch = mock.patch("kinto.plugins.statsd.Client") + patch = mock.patch("kinto.plugins.statsd.StatsDService") self.mocked = patch.start() self.addCleanup(patch.stop) - def test_statsd_isnt_called_if_statsd_url_is_not_set(self): - self.config.add_settings({"statsd_url": None}) - self.config.include("kinto.plugins.statsd") - self.mocked.assert_not_called() - - def test_statsd_is_set_to_none_if_statsd_url_not_set(self): - self.config.add_settings({"statsd_url": None}) - self.config.include("kinto.plugins.statsd") - self.assertEqual(self.config.registry.metrics, None) - - def test_statsd_is_called_if_statsd_url_is_set(self): - # For some reasons, when using ``self.config.include("kinto.plugins.statsd")`` - # the config object is recreated and breaks ``assert_called_with(self.config)``. - self.config.include("kinto.plugins.statsd") - self.mocked.assert_called_with("host", 8080, "kinto.core") - self.assertIsNotNone(self.config.registry.metrics) - - def test_metrics_attr_is_exposed_in_the_registry_if_url_is_set(self): - self.config.include("kinto.plugins.statsd") - self.assertEqual(self.config.registry.metrics, self.mocked.return_value) - - def test_statsd_attr_is_exposed_in_the_registry_if_url_is_set(self): - self.config.include("kinto.plugins.statsd") - with mock.patch("warnings.warn") as mocked_warnings: - self.config.registry.statsd.count("key") - mocked_warnings.assert_called_with( - "``config.registry.statsd`` is now deprecated. Check release notes.", - DeprecationWarning, - ) - def test_statsd_is_set_on_cache(self): self.config.include("kinto.plugins.statsd") c = self.config.registry.metrics @@ -143,7 +111,7 @@ class StatsdClientTest(unittest.TestCase): settings = {"statsd_url": "udp://foo:1234", "statsd_prefix": "prefix", "project_name": ""} def setUp(self): - self.client = statsd.Client("localhost", 1234, "prefix") + self.client = statsd.StatsDService("localhost", 1234, "prefix") self.test_object = TestedClass() with mock.patch.object(self.client, "_client") as mocked_client: From a9ea629c89b9eefef23fcf00a1df4b27aabc66ca Mon Sep 17 00:00:00 2001 From: Mathieu Leplatre Date: Tue, 8 Oct 2024 17:57:54 +0200 Subject: [PATCH 06/14] Remove unused helper --- kinto/plugins/statsd/__init__.py | 6 ------ tests/plugins/test_statsd.py | 11 ----------- 2 files changed, 17 deletions(-) diff --git a/kinto/plugins/statsd/__init__.py b/kinto/plugins/statsd/__init__.py index d0e85323b..a491c871a 100644 --- a/kinto/plugins/statsd/__init__.py +++ b/kinto/plugins/statsd/__init__.py @@ -42,12 +42,6 @@ def count(self, key, count=1, unique=None): return self._client.set(key, unique) -def statsd_count(request, count_key): - metrics = request.registry.metrics - if metrics: - metrics.count(count_key) - - def load_from_config(config): # If this is called, it means that a ``statsd_url`` was specified in settings. # (see ``kinto.core.initialization``) diff --git a/tests/plugins/test_statsd.py b/tests/plugins/test_statsd.py index 42122254d..ceb60f27c 100644 --- a/tests/plugins/test_statsd.py +++ b/tests/plugins/test_statsd.py @@ -157,17 +157,6 @@ def test_load_from_config_uses_project_name_if_defined(self, module_mock): statsd.load_from_config(config) module_mock.StatsClient.assert_called_with("foo", 1234, prefix="projectname") - def test_statsd_count_handle_unconfigured_statsd_client(self): - request = mock.MagicMock() - request.registry.metrics = None - statsd.statsd_count(request, "toto") # Doesn't raise - - def test_statsd_count_call_the_client_if_configured(self): - request = mock.MagicMock() - request.registry.metrics = self.mocked_client - statsd.statsd_count(request, "toto") - self.mocked_client.count.assert_called_with("toto") - @skip_if_no_statsd class TimingTest(BaseWebTest, unittest.TestCase): From 137026fa40f1896bc06ab6eae1f5aa273bc81349 Mon Sep 17 00:00:00 2001 From: Mathieu Leplatre Date: Tue, 8 Oct 2024 18:58:17 +0200 Subject: [PATCH 07/14] Move instrumentation of backends and authentication out of kinto.core.statsd --- kinto/core/initialization.py | 49 ++++++++++++++++++++++++++++- kinto/core/metrics.py | 27 +++++++++++----- kinto/plugins/statsd/__init__.py | 53 ++------------------------------ tests/plugins/test_statsd.py | 36 ++++++++++++++-------- 4 files changed, 93 insertions(+), 72 deletions(-) diff --git a/kinto/core/initialization.py b/kinto/core/initialization.py index c9574bc7c..dbcbaf967 100644 --- a/kinto/core/initialization.py +++ b/kinto/core/initialization.py @@ -14,11 +14,12 @@ HTTPMethodNotAllowed, HTTPTemporaryRedirect, ) +from pyramid.interfaces import IAuthenticationPolicy from pyramid.renderers import JSON as JSONRenderer from pyramid.response import Response from pyramid.security import NO_PERMISSION_REQUIRED from pyramid.settings import asbool, aslist -from pyramid_multiauth import MultiAuthPolicySelected +from pyramid_multiauth import MultiAuthenticationPolicy, MultiAuthPolicySelected from kinto.core import cache, errors, metrics, permission, storage, utils from kinto.core.events import ACTIONS, ResourceChanged, ResourceRead @@ -443,6 +444,52 @@ def deprecated_registry(self): config.registry.__class__.statsd = property(deprecated_registry) + def on_app_created(event): + config = event.app + metrics_service = config.registry.metrics + if not metrics_service: + logger.warning("No metrics service registered.") + return + + metrics.watch_execution_time(metrics_service, config.registry.cache, prefix="backend") + metrics.watch_execution_time(metrics_service, config.registry.storage, prefix="backend") + metrics.watch_execution_time(metrics_service, config.registry.permission, prefix="backend") + + policy = config.registry.queryUtility(IAuthenticationPolicy) + if isinstance(policy, MultiAuthenticationPolicy): + for name, subpolicy in policy.get_policies(): + metrics.watch_execution_time( + metrics_service, subpolicy, prefix="authentication", classname=name + ) + else: + metrics.watch_execution_time(metrics_service, policy, prefix="authentication") + + config.add_subscriber(on_app_created, ApplicationCreated) + + def on_new_response(event): + request = event.request + metrics_service = config.registry.metrics + if not metrics_service: + return + + # Count unique users. + user_id = request.prefixed_userid + if user_id: + # Get rid of colons in metric packet (see #1282). + user_id = user_id.replace(":", ".") + metrics_service.count("users", unique=user_id) + + # Count authentication verifications. + if hasattr(request, "authn_type"): + metrics_service.count(f"authn_type.{request.authn_type}") + + # Count view calls. + service = request.current_service + if service: + metrics_service.count(f"view.{service.name}.{request.method}") + + config.add_subscriber(on_new_response, NewResponse) + # While statsd is deprecated, we include its plugin by default for retro-compability. if settings["statsd_url"]: config.include("kinto.plugins.statsd") diff --git a/kinto/core/metrics.py b/kinto/core/metrics.py index ce27ad433..5b3ff858f 100644 --- a/kinto/core/metrics.py +++ b/kinto/core/metrics.py @@ -1,5 +1,9 @@ +import types + from zope.interface import Interface +from kinto.core import utils + class IMetricsService(Interface): """ @@ -7,13 +11,6 @@ class IMetricsService(Interface): Any class implementing this must provide all its methods. """ - def watch_execution_time(obj, prefix="", classname=None): - """ - TODO: move this elsewhere since it's not specific by implementer. - Decorate all methods of an object in order to watch their execution time. - Metrics will be named `{prefix}.{classname}.{method}`. - """ - def timer(key): """ Watch execution time. @@ -24,3 +21,19 @@ def count(key, count=1, unique=None): Count occurrences. If `unique` is set, overwrites the counter value on each call. """ + + +def watch_execution_time(metrics_service, obj, prefix="", classname=None): + """ + Decorate all methods of an object in order to watch their execution time. + Metrics will be named `{prefix}.{classname}.{method}`. + """ + classname = classname or utils.classname(obj) + members = dir(obj) + for name in members: + value = getattr(obj, name) + is_method = isinstance(value, types.MethodType) + if not name.startswith("_") and is_method: + statsd_key = f"{prefix}.{classname}.{name}" + decorated_method = metrics_service.timer(statsd_key)(value) + setattr(obj, name, decorated_method) diff --git a/kinto/plugins/statsd/__init__.py b/kinto/plugins/statsd/__init__.py index a491c871a..4cc77165f 100644 --- a/kinto/plugins/statsd/__init__.py +++ b/kinto/plugins/statsd/__init__.py @@ -1,13 +1,9 @@ -import types from urllib.parse import urlparse -from pyramid.events import NewResponse from pyramid.exceptions import ConfigurationError -from pyramid.interfaces import IAuthenticationPolicy -from pyramid_multiauth import MultiAuthenticationPolicy from zope.interface import implementer -from kinto.core import metrics, utils +from kinto.core import metrics try: @@ -21,17 +17,6 @@ class StatsDService: def __init__(self, host, port, prefix): self._client = statsd_module.StatsClient(host, port, prefix=prefix) - def watch_execution_time(self, obj, prefix="", classname=None): - classname = classname or utils.classname(obj) - members = dir(obj) - for name in members: - value = getattr(obj, name) - is_method = isinstance(value, types.MethodType) - if not name.startswith("_") and is_method: - statsd_key = f"{prefix}.{classname}.{name}" - decorated_method = self.timer(statsd_key)(value) - setattr(obj, name, decorated_method) - def timer(self, key): return self._client.timer(key) @@ -65,43 +50,9 @@ def load_from_config(config): def includeme(config): settings = config.get_settings() + # TODO: this backend abstraction may not be required anymore. statsd_mod = settings["statsd_backend"] statsd_mod = config.maybe_dotted(statsd_mod) client = statsd_mod.load_from_config(config) config.registry.registerUtility(client, metrics.IMetricsService) - - # TODO: move this elsewhere since it's not specific to StatsD. - client.watch_execution_time(config.registry.cache, prefix="backend") - client.watch_execution_time(config.registry.storage, prefix="backend") - client.watch_execution_time(config.registry.permission, prefix="backend") - - # Commit so that configured policy can be queried. - config.commit() - policy = config.registry.queryUtility(IAuthenticationPolicy) - if isinstance(policy, MultiAuthenticationPolicy): - for name, subpolicy in policy.get_policies(): - client.watch_execution_time(subpolicy, prefix="authentication", classname=name) - else: - client.watch_execution_time(policy, prefix="authentication") - - def on_new_response(event): - request = event.request - - # Count unique users. - user_id = request.prefixed_userid - if user_id: - # Get rid of colons in metric packet (see #1282). - user_id = user_id.replace(":", ".") - client.count("users", unique=user_id) - - # Count authentication verifications. - if hasattr(request, "authn_type"): - client.count(f"authn_type.{request.authn_type}") - - # Count view calls. - service = request.current_service - if service: - client.count(f"view.{service.name}.{request.method}") - - config.add_subscriber(on_new_response, NewResponse) diff --git a/tests/plugins/test_statsd.py b/tests/plugins/test_statsd.py index ceb60f27c..9f936688f 100644 --- a/tests/plugins/test_statsd.py +++ b/tests/plugins/test_statsd.py @@ -28,25 +28,35 @@ def setUp(self): self.mocked = patch.start() self.addCleanup(patch.stop) + patch_watch = mock.patch("kinto.core.metrics.watch_execution_time") + self.mocked_watch = patch_watch.start() + self.addCleanup(patch_watch.stop) + def test_statsd_is_set_on_cache(self): - self.config.include("kinto.plugins.statsd") - c = self.config.registry.metrics - c.watch_execution_time.assert_any_call({}, prefix="backend") + kinto.core.initialize(self.config, "0.0.1", "settings_prefix") + _app = webtest.TestApp(self.config.make_wsgi_app()) + + self.mocked_watch.assert_any_call(self.mocked(), {}, prefix="backend") def test_statsd_is_set_on_storage(self): - self.config.include("kinto.plugins.statsd") - c = self.config.registry.metrics - c.watch_execution_time.assert_any_call({}, prefix="backend") + kinto.core.initialize(self.config, "0.0.1", "settings_prefix") + _app = webtest.TestApp(self.config.make_wsgi_app()) + + self.mocked_watch.assert_any_call(self.mocked(), {}, prefix="backend") def test_statsd_is_set_on_permission(self): - self.config.include("kinto.plugins.statsd") - c = self.config.registry.metrics - c.watch_execution_time.assert_any_call({}, prefix="backend") + kinto.core.initialize(self.config, "0.0.1", "settings_prefix") + _app = webtest.TestApp(self.config.make_wsgi_app()) + + self.mocked_watch.assert_any_call(self.mocked(), {}, prefix="backend") def test_statsd_is_set_on_authentication(self): - self.config.include("kinto.plugins.statsd") - c = self.config.registry.metrics - c.watch_execution_time.assert_any_call(None, prefix="authentication") + kinto.core.initialize(self.config, "0.0.1", "settings_prefix") + _app = webtest.TestApp(self.config.make_wsgi_app()) + + self.mocked_watch.assert_any_call( + self.mocked(), mock.ANY, prefix="authentication", classname="basicauth" + ) def test_statsd_counts_nothing_on_anonymous_requests(self): kinto.core.initialize(self.config, "0.0.1", "settings_prefix") @@ -115,7 +125,7 @@ def setUp(self): self.test_object = TestedClass() with mock.patch.object(self.client, "_client") as mocked_client: - self.client.watch_execution_time(self.test_object, prefix="test") + kinto.core.metrics.watch_execution_time(self.client, self.test_object, prefix="test") self.mocked_client = mocked_client def test_public_methods_generates_statsd_calls(self): From 507bf64d3b004146ef99bae43d3ec21fd48824e7 Mon Sep 17 00:00:00 2001 From: Mathieu Leplatre Date: Wed, 9 Oct 2024 12:16:54 +0200 Subject: [PATCH 08/14] Wrap event listeners to delay evaluation of config.registry.metrics after setup --- kinto/core/initialization.py | 12 +++++------ kinto/core/metrics.py | 18 ++++++++++++++++ kinto/plugins/history/__init__.py | 11 +++++----- kinto/plugins/quotas/__init__.py | 11 +++++----- tests/plugins/test_history.py | 34 ++++++++++++++++--------------- tests/plugins/test_quotas.py | 33 ++++++++++++++++-------------- 6 files changed, 69 insertions(+), 50 deletions(-) diff --git a/kinto/core/initialization.py b/kinto/core/initialization.py index dbcbaf967..134c816e4 100644 --- a/kinto/core/initialization.py +++ b/kinto/core/initialization.py @@ -547,11 +547,9 @@ def setup_listeners(config): listener_mod = config.maybe_dotted(module_value) listener = listener_mod.load_from_config(config, prefix) - # If StatsD is enabled, monitor execution time of listeners. - if getattr(config.registry, "metrics", None): - metrics = config.registry.metrics - key = f"listeners.{name}" - listener = metrics.timer(key)(listener.__call__) + wrapped_listener = metrics.listener_with_timer( + config, f"listeners.{name}", listener.__call__ + ) # Optional filter by event action. actions_setting = prefix + "actions" @@ -577,11 +575,11 @@ def setup_listeners(config): options = dict(for_actions=actions, for_resources=resource_names) if ACTIONS.READ in actions: - config.add_subscriber(listener, ResourceRead, **options) + config.add_subscriber(wrapped_listener, ResourceRead, **options) actions = [a for a in actions if a != ACTIONS.READ] if len(actions) > 0: - config.add_subscriber(listener, ResourceChanged, **options) + config.add_subscriber(wrapped_listener, ResourceChanged, **options) def load_default_settings(config, default_settings): diff --git a/kinto/core/metrics.py b/kinto/core/metrics.py index 5b3ff858f..dbb02128a 100644 --- a/kinto/core/metrics.py +++ b/kinto/core/metrics.py @@ -37,3 +37,21 @@ def watch_execution_time(metrics_service, obj, prefix="", classname=None): statsd_key = f"{prefix}.{classname}.{name}" decorated_method = metrics_service.timer(statsd_key)(value) setattr(obj, name, decorated_method) + + +def listener_with_timer(config, key, func): + """ + Add a timer with the specified `key` on the specified `func`. + This is used to avoid evaluating `config.registry.metrics` during setup time + to avoid having to deal with initialization order and configuration committing. + """ + + def wrapped(*args, **kwargs): + metrics_service = config.registry.metrics + if not metrics_service: + return func(*args, **kwargs) + # If metrics are enabled, monitor execution time of listeners. + with metrics_service.timer(key): + return func(*args, **kwargs) + + return wrapped diff --git a/kinto/plugins/history/__init__.py b/kinto/plugins/history/__init__.py index 48ee71fb2..dbb6fc7e7 100644 --- a/kinto/plugins/history/__init__.py +++ b/kinto/plugins/history/__init__.py @@ -1,4 +1,5 @@ from kinto.authorization import PERMISSIONS_INHERITANCE_TREE +from kinto.core import metrics from kinto.core.events import ResourceChanged from .listener import on_resource_changed @@ -14,15 +15,13 @@ def includeme(config): # Activate end-points. config.scan("kinto.plugins.history.views") - # If metrics are enabled, monitor execution time of listener. - listener = on_resource_changed - if config.registry.metrics: - key = "plugins.history" - listener = config.registry.metrics.timer(key)(on_resource_changed) + wrapped_listener = metrics.listener_with_timer(config, "plugins.history", on_resource_changed) # Listen to every resources (except history) config.add_subscriber( - listener, ResourceChanged, for_resources=("bucket", "group", "collection", "record") + wrapped_listener, + ResourceChanged, + for_resources=("bucket", "group", "collection", "record"), ) # Register the permission inheritance for history entries. diff --git a/kinto/plugins/quotas/__init__.py b/kinto/plugins/quotas/__init__.py index e3d40d70d..119e9394a 100644 --- a/kinto/plugins/quotas/__init__.py +++ b/kinto/plugins/quotas/__init__.py @@ -1,3 +1,4 @@ +from kinto.core import metrics from kinto.core.events import ResourceChanged from .listener import on_resource_changed @@ -10,13 +11,11 @@ def includeme(config): url="https://kinto.readthedocs.io", ) - # If metrics are enabled, monitor execution time of listener. - listener = on_resource_changed - if config.registry.metrics: - key = "plugins.quotas" - listener = config.registry.metrics.timer(key)(on_resource_changed) + wrapped_listener = metrics.listener_with_timer(config, "plugins.quotas", on_resource_changed) # Listen to every resources (except history) config.add_subscriber( - listener, ResourceChanged, for_resources=("bucket", "group", "collection", "record") + wrapped_listener, + ResourceChanged, + for_resources=("bucket", "group", "collection", "record"), ) diff --git a/tests/plugins/test_history.py b/tests/plugins/test_history.py index f20a7a49f..f21b34106 100644 --- a/tests/plugins/test_history.py +++ b/tests/plugins/test_history.py @@ -3,9 +3,6 @@ import unittest from unittest import mock -from pyramid import testing - -from kinto import main as kinto_main from kinto.core.testing import get_user_headers, skip_if_no_statsd from .. import support @@ -14,19 +11,6 @@ DATETIME_REGEX = r"^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d{6}\+\d{2}:\d{2}$" -class PluginSetup(unittest.TestCase): - @skip_if_no_statsd - def test_a_statsd_timer_is_used_for_history_if_configured(self): - settings = { - "statsd_url": "udp://127.0.0.1:8125", - "includes": "kinto.plugins.history", - } - config = testing.setUp(settings=settings) - with mock.patch("kinto.plugins.statsd.Client.timer") as mocked: - kinto_main(None, config=config) - mocked.assert_called_with("plugins.history") - - class HistoryWebTest(support.BaseWebTest, unittest.TestCase): @classmethod def get_app_settings(cls, extras=None): @@ -42,6 +26,24 @@ def test_history_capability_if_enabled(self): self.assertIn("history", capabilities) +class MetricsTest(HistoryWebTest): + @classmethod + def get_app_settings(cls, extras=None): + settings = super().get_app_settings(extras) + settings.update( + **{ + "statsd_url": "udp://127.0.0.1:8125", + } + ) + return settings + + @skip_if_no_statsd + def test_a_statsd_timer_is_used_for_history_if_configured(self): + with mock.patch("kinto.plugins.statsd.StatsDService.timer") as mocked: + self.app.put("/buckets/test", headers=self.headers) + mocked.assert_any_call("plugins.history") + + class HistoryViewTest(HistoryWebTest): def setUp(self): self.bucket_uri = "/buckets/test" diff --git a/tests/plugins/test_quotas.py b/tests/plugins/test_quotas.py index e01046fdf..e4f2865ab 100644 --- a/tests/plugins/test_quotas.py +++ b/tests/plugins/test_quotas.py @@ -3,9 +3,7 @@ import pytest import transaction -from pyramid import testing -from kinto import main as kinto_main from kinto.core.errors import ERRORS from kinto.core.storage import Sort from kinto.core.storage.exceptions import ObjectNotFoundError @@ -21,19 +19,6 @@ from .. import support -class PluginSetup(unittest.TestCase): - @skip_if_no_statsd - def test_a_statsd_timer_is_used_for_quotas_if_configured(self): - settings = { - "statsd_url": "udp://127.0.0.1:8125", - "includes": "kinto.plugins.quotas", - } - config = testing.setUp(settings=settings) - with mock.patch("kinto.plugins.statsd.Client.timer") as mocked: - kinto_main(None, config=config) - mocked.assert_called_with("plugins.quotas") - - class QuotaWebTest(support.BaseWebTest, unittest.TestCase): bucket_uri = "/buckets/test" collection_uri = "/buckets/test/collections/col" @@ -91,6 +76,24 @@ def test_quota_capability_if_enabled(self): self.assertIn("quotas", capabilities) +class MetricsTest(QuotaWebTest): + @classmethod + def get_app_settings(cls, extras=None): + settings = super().get_app_settings(extras) + settings.update( + **{ + "statsd_url": "udp://127.0.0.1:8125", + } + ) + return settings + + @skip_if_no_statsd + def test_a_statsd_timer_is_used_for_quotas_if_configured(self): + with mock.patch("kinto.plugins.statsd.StatsDService.timer") as mocked: + self.app.put("/buckets/test", headers=self.headers) + mocked.assert_any_call("plugins.quotas") + + class QuotaListenerTest(QuotaWebTest): # # Bucket From 3bb567c8781a82bf4277dae7a1d1c4e8467fce05 Mon Sep 17 00:00:00 2001 From: Mathieu Leplatre Date: Wed, 9 Oct 2024 15:43:31 +0200 Subject: [PATCH 09/14] Skip StatsD test with raw install --- tests/core/test_initialization.py | 3 ++- tests/plugins/test_statsd.py | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/core/test_initialization.py b/tests/core/test_initialization.py index 5fe4f719b..ca1dd8c16 100644 --- a/tests/core/test_initialization.py +++ b/tests/core/test_initialization.py @@ -8,7 +8,7 @@ import kinto.core from kinto.core import initialization -from kinto.core.testing import unittest +from kinto.core.testing import skip_if_no_statsd, unittest class InitializationTest(unittest.TestCase): @@ -292,6 +292,7 @@ def unexpected_exceptions_are_reported(self): assert len(mocked.call_args_list) > 0 +@skip_if_no_statsd class StatsDConfigurationTest(unittest.TestCase): settings = { **kinto.core.DEFAULT_SETTINGS, diff --git a/tests/plugins/test_statsd.py b/tests/plugins/test_statsd.py index 9f936688f..e6065c4bf 100644 --- a/tests/plugins/test_statsd.py +++ b/tests/plugins/test_statsd.py @@ -12,6 +12,7 @@ from ..support import BaseWebTest +@skip_if_no_statsd class StatsDIncludeTest(unittest.TestCase): def setUp(self): settings = { From 1275c63e2a84e450f6b93727cea672ab0219bf3c Mon Sep 17 00:00:00 2001 From: Mathieu Leplatre Date: Wed, 9 Oct 2024 16:26:50 +0200 Subject: [PATCH 10/14] Move tests and assertions where they belong --- tests/core/test_initialization.py | 84 +++++++++++++++--- tests/core/test_metrics.py | 49 +++++++++++ tests/plugins/test_history.py | 2 +- tests/plugins/test_quotas.py | 2 +- tests/plugins/test_statsd.py | 139 +----------------------------- 5 files changed, 126 insertions(+), 150 deletions(-) create mode 100644 tests/core/test_metrics.py diff --git a/tests/core/test_initialization.py b/tests/core/test_initialization.py index ca1dd8c16..6d52d75a7 100644 --- a/tests/core/test_initialization.py +++ b/tests/core/test_initialization.py @@ -8,7 +8,7 @@ import kinto.core from kinto.core import initialization -from kinto.core.testing import skip_if_no_statsd, unittest +from kinto.core.testing import get_user_headers, skip_if_no_statsd, unittest class InitializationTest(unittest.TestCase): @@ -293,7 +293,7 @@ def unexpected_exceptions_are_reported(self): @skip_if_no_statsd -class StatsDConfigurationTest(unittest.TestCase): +class MetricsConfigurationTest(unittest.TestCase): settings = { **kinto.core.DEFAULT_SETTINGS, "statsd_url": "udp://host:8080", @@ -305,6 +305,10 @@ def setUp(self): self.mocked = patch.start() self.addCleanup(patch.stop) + patch_watch = mock.patch("kinto.core.metrics.watch_execution_time") + self.mocked_watch = patch_watch.start() + self.addCleanup(patch_watch.stop) + self.config = Configurator(settings=self.settings) self.config.registry.storage = {} self.config.registry.cache = {} @@ -330,19 +334,75 @@ def test_statsd_isnt_included_if_statsd_url_is_not_set(self): self.mocked.assert_not_called() + # + # Backends. + # -class MetricsRegistryConfigurationTest(unittest.TestCase): - settings = kinto.core.DEFAULT_SETTINGS + def test_statsd_is_set_on_cache(self): + kinto.core.initialize(self.config, "0.0.1", "settings_prefix") + _app = webtest.TestApp(self.config.make_wsgi_app()) - def setUp(self): - self.config = Configurator(settings=self.settings) - self.config.registry.storage = {} - self.config.registry.cache = {} - self.config.registry.permission = {} + self.mocked_watch.assert_any_call(self.mocked(), {}, prefix="backend") - patch = mock.patch("kinto.plugins.statsd.StatsDService") - self.mocked = patch.start() - self.addCleanup(patch.stop) + def test_statsd_is_set_on_storage(self): + kinto.core.initialize(self.config, "0.0.1", "settings_prefix") + _app = webtest.TestApp(self.config.make_wsgi_app()) + + self.mocked_watch.assert_any_call(self.mocked(), {}, prefix="backend") + + def test_statsd_is_set_on_permission(self): + kinto.core.initialize(self.config, "0.0.1", "settings_prefix") + _app = webtest.TestApp(self.config.make_wsgi_app()) + + self.mocked_watch.assert_any_call(self.mocked(), {}, prefix="backend") + + # + # Authentication. + # + + def test_statsd_is_set_on_authentication(self): + kinto.core.initialize(self.config, "0.0.1", "settings_prefix") + _app = webtest.TestApp(self.config.make_wsgi_app()) + + self.mocked_watch.assert_any_call( + self.mocked(), mock.ANY, prefix="authentication", classname="basicauth" + ) + + @mock.patch("kinto.core.utils.hmac_digest") + def test_statsd_counts_unique_users(self, digest_mocked): + digest_mocked.return_value = "mat" + kinto.core.initialize(self.config, "0.0.1", "settings_prefix") + app = webtest.TestApp(self.config.make_wsgi_app()) + app.get("/v0/", headers=get_user_headers("bob")) + self.mocked().count.assert_any_call("users", unique="basicauth.mat") + + def test_statsd_counts_authentication_types(self): + kinto.core.initialize(self.config, "0.0.1", "settings_prefix") + app = webtest.TestApp(self.config.make_wsgi_app()) + app.get("/v0/", headers=get_user_headers("bob")) + self.mocked().count.assert_any_call("authn_type.basicauth") + + # + # Endpoints. + # + + def test_statsd_counts_nothing_on_anonymous_requests(self): + kinto.core.initialize(self.config, "0.0.1", "settings_prefix") + app = webtest.TestApp(self.config.make_wsgi_app()) + app.get("/") + self.assertFalse(self.mocked.count.called) + + def test_statsd_counts_views_and_methods(self): + kinto.core.initialize(self.config, "0.0.1", "settings_prefix") + app = webtest.TestApp(self.config.make_wsgi_app()) + app.get("/v0/__heartbeat__") + self.mocked().count.assert_any_call("view.heartbeat.GET") + + def test_statsd_counts_unknown_urls(self): + kinto.core.initialize(self.config, "0.0.1", "settings_prefix") + app = webtest.TestApp(self.config.make_wsgi_app()) + app.get("/v0/coucou", status=404) + self.assertFalse(self.mocked.count.called) def test_metrics_and_statsd_are_none_if_statsd_url_not_set(self): self.config.add_settings({"statsd_url": None}) diff --git a/tests/core/test_metrics.py b/tests/core/test_metrics.py new file mode 100644 index 000000000..c6ff06a22 --- /dev/null +++ b/tests/core/test_metrics.py @@ -0,0 +1,49 @@ +import unittest +from unittest import mock + +from pyramid.config import Configurator + +from kinto.core import metrics + + +class TestedClass: + attribute = 3.14 + + def test_method(self): + pass + + def _private_method(self): + pass + + +class WatchExecutionTimeTest(unittest.TestCase): + def setUp(self): + self.test_object = TestedClass() + self.mocked = mock.MagicMock() + metrics.watch_execution_time(self.mocked, self.test_object, prefix="test") + + def test_public_methods_generates_statsd_calls(self): + self.test_object.test_method() + self.mocked.timer.assert_called_with("test.testedclass.test_method") + + def test_private_methods_does_not_generates_statsd_calls(self): + self.test_object._private_method() + self.assertFalse(self.mocked().timer.called) + + +class ListenerWithTimerTest(unittest.TestCase): + def setUp(self): + self.config = Configurator() + self.func = lambda x: x # noqa: E731 + + def test_without_metrics_service(self): + wrapped = metrics.listener_with_timer(self.config, "key", self.func) + + self.assertEqual(wrapped(42), 42) # does not raise + + def test_with_metrics_service(self): + self.config.registry.registerUtility(mock.MagicMock(), metrics.IMetricsService) + wrapped = metrics.listener_with_timer(self.config, "key", self.func) + + self.assertEqual(wrapped(42), 42) + self.config.registry.metrics.timer.assert_called_with("key") diff --git a/tests/plugins/test_history.py b/tests/plugins/test_history.py index f21b34106..36088fc25 100644 --- a/tests/plugins/test_history.py +++ b/tests/plugins/test_history.py @@ -26,6 +26,7 @@ def test_history_capability_if_enabled(self): self.assertIn("history", capabilities) +@skip_if_no_statsd class MetricsTest(HistoryWebTest): @classmethod def get_app_settings(cls, extras=None): @@ -37,7 +38,6 @@ def get_app_settings(cls, extras=None): ) return settings - @skip_if_no_statsd def test_a_statsd_timer_is_used_for_history_if_configured(self): with mock.patch("kinto.plugins.statsd.StatsDService.timer") as mocked: self.app.put("/buckets/test", headers=self.headers) diff --git a/tests/plugins/test_quotas.py b/tests/plugins/test_quotas.py index e4f2865ab..27ec5c736 100644 --- a/tests/plugins/test_quotas.py +++ b/tests/plugins/test_quotas.py @@ -76,6 +76,7 @@ def test_quota_capability_if_enabled(self): self.assertIn("quotas", capabilities) +@skip_if_no_statsd class MetricsTest(QuotaWebTest): @classmethod def get_app_settings(cls, extras=None): @@ -87,7 +88,6 @@ def get_app_settings(cls, extras=None): ) return settings - @skip_if_no_statsd def test_a_statsd_timer_is_used_for_quotas_if_configured(self): with mock.patch("kinto.plugins.statsd.StatsDService.timer") as mocked: self.app.put("/buckets/test", headers=self.headers) diff --git a/tests/plugins/test_statsd.py b/tests/plugins/test_statsd.py index e6065c4bf..975941f2a 100644 --- a/tests/plugins/test_statsd.py +++ b/tests/plugins/test_statsd.py @@ -1,108 +1,11 @@ from unittest import mock -import webtest from pyramid import testing -from pyramid.config import Configurator from pyramid.exceptions import ConfigurationError -import kinto from kinto.core.testing import skip_if_no_statsd, unittest from kinto.plugins import statsd -from ..support import BaseWebTest - - -@skip_if_no_statsd -class StatsDIncludeTest(unittest.TestCase): - def setUp(self): - settings = { - **kinto.core.DEFAULT_SETTINGS, - "statsd_url": "udp://host:8080", - "multiauth.policies": "basicauth", - } - self.config = Configurator(settings=settings) - self.config.registry.storage = {} - self.config.registry.cache = {} - self.config.registry.permission = {} - - patch = mock.patch("kinto.plugins.statsd.StatsDService") - self.mocked = patch.start() - self.addCleanup(patch.stop) - - patch_watch = mock.patch("kinto.core.metrics.watch_execution_time") - self.mocked_watch = patch_watch.start() - self.addCleanup(patch_watch.stop) - - def test_statsd_is_set_on_cache(self): - kinto.core.initialize(self.config, "0.0.1", "settings_prefix") - _app = webtest.TestApp(self.config.make_wsgi_app()) - - self.mocked_watch.assert_any_call(self.mocked(), {}, prefix="backend") - - def test_statsd_is_set_on_storage(self): - kinto.core.initialize(self.config, "0.0.1", "settings_prefix") - _app = webtest.TestApp(self.config.make_wsgi_app()) - - self.mocked_watch.assert_any_call(self.mocked(), {}, prefix="backend") - - def test_statsd_is_set_on_permission(self): - kinto.core.initialize(self.config, "0.0.1", "settings_prefix") - _app = webtest.TestApp(self.config.make_wsgi_app()) - - self.mocked_watch.assert_any_call(self.mocked(), {}, prefix="backend") - - def test_statsd_is_set_on_authentication(self): - kinto.core.initialize(self.config, "0.0.1", "settings_prefix") - _app = webtest.TestApp(self.config.make_wsgi_app()) - - self.mocked_watch.assert_any_call( - self.mocked(), mock.ANY, prefix="authentication", classname="basicauth" - ) - - def test_statsd_counts_nothing_on_anonymous_requests(self): - kinto.core.initialize(self.config, "0.0.1", "settings_prefix") - app = webtest.TestApp(self.config.make_wsgi_app()) - app.get("/") - self.assertFalse(self.mocked.count.called) - - def test_statsd_counts_views_and_methods(self): - kinto.core.initialize(self.config, "0.0.1", "settings_prefix") - app = webtest.TestApp(self.config.make_wsgi_app()) - app.get("/v0/__heartbeat__") - self.mocked().count.assert_any_call("view.heartbeat.GET") - - def test_statsd_counts_unknown_urls(self): - kinto.core.initialize(self.config, "0.0.1", "settings_prefix") - app = webtest.TestApp(self.config.make_wsgi_app()) - app.get("/v0/coucou", status=404) - self.assertFalse(self.mocked.count.called) - - @mock.patch("kinto.core.utils.hmac_digest") - def test_statsd_counts_unique_users(self, digest_mocked): - digest_mocked.return_value = "mat" - kinto.core.initialize(self.config, "0.0.1", "settings_prefix") - app = webtest.TestApp(self.config.make_wsgi_app()) - headers = {"Authorization": "Basic bWF0Og=="} - app.get("/v0/", headers=headers) - self.mocked().count.assert_any_call("users", unique="basicauth.mat") - - def test_statsd_counts_authentication_types(self): - kinto.core.initialize(self.config, "0.0.1", "settings_prefix") - app = webtest.TestApp(self.config.make_wsgi_app()) - headers = {"Authorization": "Basic bWF0Og=="} - app.get("/v0/", headers=headers) - self.mocked().count.assert_any_call("authn_type.basicauth") - - -class TestedClass: - attribute = 3.14 - - def test_method(self): - pass - - def _private_method(self): - pass - class StatsDMissing(unittest.TestCase): def setUp(self): @@ -123,21 +26,10 @@ class StatsdClientTest(unittest.TestCase): def setUp(self): self.client = statsd.StatsDService("localhost", 1234, "prefix") - self.test_object = TestedClass() - - with mock.patch.object(self.client, "_client") as mocked_client: - kinto.core.metrics.watch_execution_time(self.client, self.test_object, prefix="test") - self.mocked_client = mocked_client - def test_public_methods_generates_statsd_calls(self): - self.test_object.test_method() - - self.mocked_client.timer.assert_called_with("test.testedclass.test_method") - - def test_private_methods_does_not_generates_statsd_calls(self): - self.mocked_client.reset_mock() - self.test_object._private_method() - self.assertFalse(self.mocked_client.timer.called) + patch = mock.patch.object(self.client, "_client") + self.mocked_client = patch.start() + self.addCleanup(patch.stop) def test_count_increments_the_counter_for_key(self): with mock.patch.object(self.client, "_client") as mocked_client: @@ -167,28 +59,3 @@ def test_load_from_config_uses_project_name_if_defined(self, module_mock): config.registry.settings = {**self.settings, "project_name": "projectname"} statsd.load_from_config(config) module_mock.StatsClient.assert_called_with("foo", 1234, prefix="projectname") - - -@skip_if_no_statsd -class TimingTest(BaseWebTest, unittest.TestCase): - @classmethod - def get_app_settings(cls, *args, **kwargs): - settings = super().get_app_settings(*args, **kwargs) - if not statsd.statsd_module: - return settings - - settings["statsd_url"] = "udp://localhost:8125" - return settings - - def test_statds_tracks_listeners_execution_duration(self): - statsd_client = self.app.app.registry.metrics._client - with mock.patch.object(statsd_client, "timing") as mocked: - self.app.get("/", headers=self.headers) - self.assertTrue(mocked.called) - - def test_statds_tracks_authentication_policies(self): - statsd_client = self.app.app.registry.metrics._client - with mock.patch.object(statsd_client, "timing") as mocked: - self.app.get("/", headers=self.headers) - timers = set(c[0][0] for c in mocked.call_args_list) - self.assertIn("authentication.basicauth.unauthenticated_userid", timers) From 839b40c49a2272adf06fb90b6db0cf8e8bfe5a73 Mon Sep 17 00:00:00 2001 From: Mathieu Leplatre Date: Wed, 9 Oct 2024 17:27:55 +0200 Subject: [PATCH 11/14] Fix coverage to 100% --- tests/core/test_initialization.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/tests/core/test_initialization.py b/tests/core/test_initialization.py index 6d52d75a7..c82dd660f 100644 --- a/tests/core/test_initialization.py +++ b/tests/core/test_initialization.py @@ -2,6 +2,7 @@ from unittest import mock import webtest +from pyramid.authentication import SessionAuthenticationPolicy from pyramid.config import Configurator from pyramid.events import NewRequest from pyramid.exceptions import ConfigurationError @@ -360,7 +361,7 @@ def test_statsd_is_set_on_permission(self): # Authentication. # - def test_statsd_is_set_on_authentication(self): + def test_statsd_is_set_on_authentication_multiauth(self): kinto.core.initialize(self.config, "0.0.1", "settings_prefix") _app = webtest.TestApp(self.config.make_wsgi_app()) @@ -368,6 +369,15 @@ def test_statsd_is_set_on_authentication(self): self.mocked(), mock.ANY, prefix="authentication", classname="basicauth" ) + def test_statsd_is_set_on_authentication_raw_auth(self): + authn_policy = SessionAuthenticationPolicy() + self.config.set_authentication_policy(authn_policy) + + kinto.core.initialize(self.config, "0.0.1", "settings_prefix") + _app = webtest.TestApp(self.config.make_wsgi_app()) + + self.mocked_watch.assert_any_call(self.mocked(), mock.ANY, prefix="authentication") + @mock.patch("kinto.core.utils.hmac_digest") def test_statsd_counts_unique_users(self, digest_mocked): digest_mocked.return_value = "mat" From 3cba8d0c6861d669a3f5eb6a6a9e077f8fbddfa0 Mon Sep 17 00:00:00 2001 From: Mathieu Leplatre Date: Wed, 9 Oct 2024 17:36:53 +0200 Subject: [PATCH 12/14] Adjust docs and deprecation warnings --- docs/configuration/settings.rst | 32 ++++++++++++++++++------------- kinto/core/initialization.py | 6 ++++-- tests/core/test_initialization.py | 4 ++-- 3 files changed, 25 insertions(+), 17 deletions(-) diff --git a/docs/configuration/settings.rst b/docs/configuration/settings.rst index 786b3929b..3e106b06b 100644 --- a/docs/configuration/settings.rst +++ b/docs/configuration/settings.rst @@ -304,18 +304,6 @@ hello view `. Logging and Monitoring ====================== -+------------------------+----------------------------------------+--------------------------------------------------------------------------+ -| Setting name | Default | What does it do? | -+========================+========================================+==========================================================================+ -| kinto.statsd_backend | ``kinto.core.statsd`` | The Python **dotted** location of the StatsD module that should be used | -| | | for monitoring. Useful to plug custom implementations like Datadog™. | -+------------------------+----------------------------------------+--------------------------------------------------------------------------+ -| kinto.statsd_prefix | ``kinto`` | The prefix to use when sending data to statsd. | -+------------------------+----------------------------------------+--------------------------------------------------------------------------+ -| kinto.statsd_url | ``None`` | The fully qualified URL to use to connect to the statsd host. e.g. | -| | | ``udp://localhost:8125`` | -+------------------------+----------------------------------------+--------------------------------------------------------------------------+ - Standard Logging :::::::::::::::: @@ -425,12 +413,27 @@ Or the equivalent environment variables: The application sends an event on startup (mainly for setup check). +.. _monitoring-with-statsd: + Monitoring with StatsD :::::::::::::::::::::: Requires the ``statsd`` package. -StatsD metrics can be enabled (disabled by default): ++------------------------+----------------------------------------+--------------------------------------------------------------------------+ +| Setting name | Default | What does it do? | ++========================+========================================+==========================================================================+ +| kinto.statsd_backend | ``kinto.core.statsd`` | The Python **dotted** location of the StatsD module that should be used | +| | | for monitoring. Useful to plug custom implementations like Datadog™. | ++------------------------+----------------------------------------+--------------------------------------------------------------------------+ +| kinto.statsd_prefix | ``kinto`` | The prefix to use when sending data to statsd. | ++------------------------+----------------------------------------+--------------------------------------------------------------------------+ +| kinto.statsd_url | ``None`` | The fully qualified URL to use to connect to the statsd host. e.g. | +| | | ``udp://localhost:8125`` | ++------------------------+----------------------------------------+--------------------------------------------------------------------------+ + + +StatsD metrics can be enabled with (disabled by default): .. code-block:: ini @@ -501,6 +504,9 @@ list of Python modules: | ``kinto.plugins.quotas`` | It allows to limit storage per collection size, number of records, etc. | | | (:ref:`more details `). | +---------------------------------------+--------------------------------------------------------------------------+ +| ``kinto.plugins.statsd`` | Send metrics about backend duration, authentication, endpoints hits, .. | +| | (:ref:`more details `). | ++---------------------------------------+--------------------------------------------------------------------------+ There are `many available packages`_ in Pyramid ecosystem, and it is straightforward to build one, diff --git a/kinto/core/initialization.py b/kinto/core/initialization.py index 134c816e4..023e939a3 100644 --- a/kinto/core/initialization.py +++ b/kinto/core/initialization.py @@ -334,8 +334,10 @@ def on_app_created(event): def setup_statsd(config): + # It would be pretty rare to find users that have a custom ``kinto.initialization_sequence`` setting. + # But just in case, warn that it will be removed in next major. warnings.warn( - "``setup_statsd()`` is now deprecated. Check release notes.", + "``setup_statsd()`` is now deprecated. Use ``kinto.core.initialization.setup_metrics()`` instead.", DeprecationWarning, ) setup_metrics(config) @@ -437,7 +439,7 @@ def setup_metrics(config): def deprecated_registry(self): warnings.warn( - "``config.registry.statsd`` is now deprecated. Check release notes.", + "``config.registry.statsd`` is now deprecated. Use ``config.registry.metrics`` instead.", DeprecationWarning, ) return self.metrics diff --git a/tests/core/test_initialization.py b/tests/core/test_initialization.py index c82dd660f..e999d3628 100644 --- a/tests/core/test_initialization.py +++ b/tests/core/test_initialization.py @@ -319,7 +319,7 @@ def test_setup_statsd_step_is_still_supported(self): with mock.patch("warnings.warn") as mocked_warnings: initialization.setup_statsd(self.config) mocked_warnings.assert_called_with( - "``setup_statsd()`` is now deprecated. Check release notes.", + "``setup_statsd()`` is now deprecated. Use ``kinto.core.initialization.setup_metrics()`` instead.", DeprecationWarning, ) self.mocked.assert_called_with("host", 8080, "kinto.core") @@ -438,7 +438,7 @@ def test_statsd_attr_is_exposed_in_the_registry_if_url_is_set(self): with mock.patch("warnings.warn") as mocked_warnings: self.config.registry.statsd.count("key") mocked_warnings.assert_called_with( - "``config.registry.statsd`` is now deprecated. Check release notes.", + "``config.registry.statsd`` is now deprecated. Use ``config.registry.metrics`` instead.", DeprecationWarning, ) From 4d1ff773333a28eef25ccaffeca1762d0e54ea57 Mon Sep 17 00:00:00 2001 From: Mathieu Leplatre Date: Thu, 10 Oct 2024 12:44:26 +0200 Subject: [PATCH 13/14] Use single file instead of folder (like flush.py) --- kinto/plugins/{statsd/__init__.py => statsd.py} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename kinto/plugins/{statsd/__init__.py => statsd.py} (100%) diff --git a/kinto/plugins/statsd/__init__.py b/kinto/plugins/statsd.py similarity index 100% rename from kinto/plugins/statsd/__init__.py rename to kinto/plugins/statsd.py From 6ed8ac516dc0ac01b65ddd05fb8d879d83cdf414 Mon Sep 17 00:00:00 2001 From: Mathieu Leplatre Date: Thu, 10 Oct 2024 12:50:15 +0200 Subject: [PATCH 14/14] Mention statsd with uWsgi --- docs/configuration/settings.rst | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/docs/configuration/settings.rst b/docs/configuration/settings.rst index 3e106b06b..f23196ff7 100644 --- a/docs/configuration/settings.rst +++ b/docs/configuration/settings.rst @@ -429,7 +429,7 @@ Requires the ``statsd`` package. | kinto.statsd_prefix | ``kinto`` | The prefix to use when sending data to statsd. | +------------------------+----------------------------------------+--------------------------------------------------------------------------+ | kinto.statsd_url | ``None`` | The fully qualified URL to use to connect to the statsd host. e.g. | -| | | ``udp://localhost:8125`` | +| | | ``udp://host:8125`` | +------------------------+----------------------------------------+--------------------------------------------------------------------------+ @@ -437,10 +437,23 @@ StatsD metrics can be enabled with (disabled by default): .. code-block:: ini - kinto.statsd_url = udp://localhost:8125 + kinto.statsd_url = udp://host:8125 # kinto.statsd_prefix = kinto-prod +StatsD can also be enabled at the *uWSGI* level: + +.. code-block:: ini + + [uwsgi] + + # ... + + enable-metrics = true + plugin = dogstatsd + stats-push = dogstatsd:host:8125,kinto.{{ $deployment }} + + Monitoring with New Relic :::::::::::::::::::::::::