From cfd7caf7a65c3666690404ec783920805e425e02 Mon Sep 17 00:00:00 2001
From: Kiran Jonnalagadda <kiran@hasgeek.com>
Date: Thu, 7 May 2020 03:44:15 +0530
Subject: [PATCH 01/11] Add statsd support

---
 baseframe/__init__.py |   4 ++
 baseframe/statsd.py   | 151 ++++++++++++++++++++++++++++++++++++++++++
 setup.py              |  37 ++++++-----
 3 files changed, 174 insertions(+), 18 deletions(-)
 create mode 100644 baseframe/statsd.py

diff --git a/baseframe/__init__.py b/baseframe/__init__.py
index d9032e53..5629393b 100644
--- a/baseframe/__init__.py
+++ b/baseframe/__init__.py
@@ -32,6 +32,7 @@
 from . import translations
 from ._version import __version__, __version_info__
 from .assets import Version, assets
+from .statsd import Statsd
 
 try:
     from flask_debugtoolbar import DebugToolbarExtension
@@ -61,6 +62,7 @@
 asset_cache = Cache(with_jinja2_ext=False)
 cache = Cache()
 babel = Babel()
+statsd = Statsd()
 if DebugToolbarExtension is not None:  # pragma: no cover
     toolbar = DebugToolbarExtension()
 else:  # pragma: no cover
@@ -186,6 +188,8 @@ def init_app(
                 ],
             )
 
+        statsd.init_app(app)
+
         # Since Flask 0.11, templates are no longer auto reloaded.
         # Setting the config alone doesn't seem to work, so we explicitly
         # set the jinja environment here.
diff --git a/baseframe/statsd.py b/baseframe/statsd.py
new file mode 100644
index 00000000..7f010822
--- /dev/null
+++ b/baseframe/statsd.py
@@ -0,0 +1,151 @@
+# -*- coding: utf-8 -*-
+
+from __future__ import absolute_import
+
+import time
+
+from flask import current_app, request
+
+from statsd import StatsClient
+
+__all__ = ['Statsd']
+
+START_TIME_ATTR = 'statsd_start_time'
+
+# This extension was adapted from
+# https://github.com/nylas/flask-statsd/blob/master/flask_statsd.py
+
+
+class Statsd(object):
+    """
+    Statsd extension for Flask
+
+    Offers conveniences on top of using statsd directly:
+    1. In a multi-app setup, each app can have its own statsd config
+    2. The app's name is automatically prefixed to all stats
+    3. Sampling rate can be specified in app config instead of in each call
+    4. Requests are automatically timed and counted unless STATSD_REQUEST_TIMER is False
+
+    App configuration defaults:
+
+    STATSD_HOST = '127.0.0.1'
+    STATSD_PORT = 8125
+    STATSD_PREFIX = None
+    STATSD_MAXUDPSIZE = 512
+    STATSD_IPV6 = False
+    STATSD_RATE = 1
+    STATSD_REQUEST_TIMER = True
+    """
+
+    def __init__(self, app=None):
+        if app is not None:
+            self.init_app(app)
+
+    def init_app(self, app):
+        app.config.setdefault('STATSD_RATE', 1)
+
+        app.statsd = StatsClient(
+            host=app.config.setdefault('STATSD_HOST', '127.0.0.1'),
+            port=app.config.setdefault('STATSD_PORT', 8125),
+            prefix=app.config.setdefault('STATSD_PREFIX', None),
+            maxudpsize=app.config.setdefault('STATSD_MAXUDPSIZE', 512),
+            ipv6=app.config.setdefault('STATSD_IPV6', False),
+        )
+
+        if app.config.setdefault('STATSD_REQUEST_TIMER', True):
+            app.before_request(self._before_request)
+            app.after_request(self._after_request)
+
+    def _metric_name(self, name):
+        return '%s.%s' % (current_app.name, name)
+
+    def timer(self, stat, rate=None):
+        """
+        Return a Timer object that can be used as a context manager to automatically
+        record timing for a block or function call. Use as a decorator is not supported
+        as an application context is required.
+        """
+        return current_app.statsd.timer(
+            stat, rate=rate if rate is not None else current_app.config['STATSD_RATE']
+        )
+
+    def timing(self, stat, delta, rate=None):
+        """
+        Record timer information.
+        """
+        return current_app.statsd.timing(
+            self._metric_name(stat),
+            delta,
+            rate=rate if rate is not None else current_app.config['STATSD_RATE'],
+        )
+
+    def incr(self, stat, count=1, rate=None):
+        """
+        Increment a counter.
+        """
+        return current_app.statsd.incr(
+            self._metric_name(stat),
+            count,
+            rate=rate if rate is not None else current_app.config['STATSD_RATE'],
+        )
+
+    def decr(self, stat, count=1, rate=None):
+        """
+        Decrement a counter.
+        """
+        return current_app.statsd.decr(
+            self._metric_name(stat),
+            count,
+            rate=rate if rate is not None else current_app.config['STATSD_RATE'],
+        )
+
+    def gauge(self, stat, value, rate=None, delta=False):
+        """
+        Set a gauge value.
+        """
+        return current_app.statsd.gauge(
+            self._metric_name(stat),
+            value,
+            rate=rate if rate is not None else current_app.config['STATSD_RATE'],
+            delta=delta,
+        )
+
+    def set(self, stat, value, rate=None):  # NOQA: A003
+        """
+        Increment a set value.
+
+        The statsd server does _not_ take the sample rate into account for sets. Use
+        with care.
+        """
+        return current_app.statsd.set(
+            self._metric_name(stat),
+            value,
+            rate=rate if rate is not None else current_app.config['STATSD_RATE'],
+        )
+
+    def pipeline(self):
+        return current_app.statsd.pipeline()
+
+    def _before_request(self):
+        if current_app.config['STATSD_RATE'] != 0:
+            setattr(request, START_TIME_ATTR, time.time())
+
+    def _after_request(self, response):
+        if hasattr(request, START_TIME_ATTR):
+            metrics = [
+                '.'.join(
+                    ['request_handlers', request.endpoint, str(response.status_code)]
+                ),
+                '.'.join(['request_handlers', '_overall', str(response.status_code)]),
+            ]
+
+            for metric_name in metrics:
+                # Use `timing` instead of `timer` because we record it as two metrics.
+                # Convert time from seconds:float to milliseconds:int
+                self.timing(
+                    metric_name,
+                    int((time.time() - getattr(request, START_TIME_ATTR)) * 1000),
+                )
+                self.incr(metric_name)
+
+        return response
diff --git a/setup.py b/setup.py
index e7ae7d10..0d02a1d6 100644
--- a/setup.py
+++ b/setup.py
@@ -17,35 +17,35 @@
     raise RuntimeError("Unable to find version string in baseframe/_version.py.")
 
 requires = [
-    'six>=1.13.0',
-    'semantic_version',
     'bleach',
-    'pytz',
-    'pyIsEmail',
+    'coaster',
+    'cssmin',
     'dnspython',
     'emoji',
-    'WTForms>=2.2',
-    'Flask>=1.0',
     'Flask-Assets',
-    'Flask-WTF>=0.14',
-    'Flask-Caching',
     'Flask-Babelhg',
-    'speaklater',
-    'redis',
-    'cssmin',
-    'coaster',
+    'Flask-Caching',
+    'Flask-WTF>=0.14',
+    'Flask>=1.0',
+    'furl',
     'lxml',
     'mxsniff',
-    'furl',
+    'ndg-httpsclient',
+    'pyasn1',
     'pycountry',
+    'pyIsEmail',
+    'pyOpenSSL',
+    'pytz',
+    'redis',
+    'requests',
     'rq',
+    'semantic_version',
     'sentry-sdk',
-    # For link validation with SNI SSL support
-    'requests',
-    'pyOpenSSL',
-    'ndg-httpsclient',
-    'pyasn1',
+    'six>=1.13.0',
+    'speaklater',
+    'statsd',
     'werkzeug',
+    'WTForms>=2.2',
 ]
 
 
@@ -70,6 +70,7 @@ def run(self):
         "Programming Language :: Python :: 2.7",
         "Programming Language :: Python :: 3.6",
         "Programming Language :: Python :: 3.7",
+        "Programming Language :: Python :: 3.8",
         "License :: OSI Approved :: BSD License",
         "Operating System :: OS Independent",
         "Intended Audience :: Developers",

From 58cb5f965a77521529b2018dc5abd5c5866edff1 Mon Sep 17 00:00:00 2001
From: Kiran Jonnalagadda <kiran@hasgeek.com>
Date: Fri, 8 May 2020 02:11:36 +0530
Subject: [PATCH 02/11] Use `SITE_ID` instead of app.name and log errorhandlers

---
 baseframe/errors.py |  7 +++++--
 baseframe/statsd.py | 40 ++++++++++++++++++++++++----------------
 2 files changed, 29 insertions(+), 18 deletions(-)

diff --git a/baseframe/errors.py b/baseframe/errors.py
index 578a07d0..1f6f6e2c 100644
--- a/baseframe/errors.py
+++ b/baseframe/errors.py
@@ -1,11 +1,11 @@
 # -*- coding: utf-8 -*-
 
-from flask import redirect, request
+from flask import current_app, redirect, request
 from werkzeug.routing import MethodNotAllowed, NotFound, RequestRedirect
 
 from coaster.views import render_with
 
-from . import baseframe, baseframe_translations, current_app
+from . import baseframe, baseframe_translations, statsd
 
 
 @baseframe.app_errorhandler(404)
@@ -27,6 +27,7 @@ def error404(e):
         except (NotFound, RequestRedirect, MethodNotAllowed):
             pass
     baseframe_translations.as_default()
+    statsd.log_request_timer(404)
     return {'error': "404 Not Found"}, 404
 
 
@@ -34,6 +35,7 @@ def error404(e):
 @render_with('403.html.jinja2', json=True)
 def error403(e):
     baseframe_translations.as_default()
+    statsd.log_request_timer(403)
     return {'error': "403 Forbidden"}, 403
 
 
@@ -44,4 +46,5 @@ def error500(e):
         current_app.extensions['sqlalchemy'].db.session.rollback()
 
     baseframe_translations.as_default()
+    statsd.log_request_timer(500)
     return {'error': "500 Internal Server Error"}, 500
diff --git a/baseframe/statsd.py b/baseframe/statsd.py
index 7f010822..5ee94be0 100644
--- a/baseframe/statsd.py
+++ b/baseframe/statsd.py
@@ -26,15 +26,16 @@ class Statsd(object):
     3. Sampling rate can be specified in app config instead of in each call
     4. Requests are automatically timed and counted unless STATSD_REQUEST_TIMER is False
 
-    App configuration defaults:
-
-    STATSD_HOST = '127.0.0.1'
-    STATSD_PORT = 8125
-    STATSD_PREFIX = None
-    STATSD_MAXUDPSIZE = 512
-    STATSD_IPV6 = False
-    STATSD_RATE = 1
-    STATSD_REQUEST_TIMER = True
+    App configuration defaults::
+
+        SITE_ID = app.name  # Used as a prefix in stats
+        STATSD_HOST = '127.0.0.1'
+        STATSD_PORT = 8125
+        STATSD_PREFIX = None
+        STATSD_MAXUDPSIZE = 512
+        STATSD_IPV6 = False
+        STATSD_RATE = 1
+        STATSD_REQUEST_TIMER = True
     """
 
     def __init__(self, app=None):
@@ -43,6 +44,7 @@ def __init__(self, app=None):
 
     def init_app(self, app):
         app.config.setdefault('STATSD_RATE', 1)
+        app.config.setdefault('SITE_ID', app.name)
 
         app.statsd = StatsClient(
             host=app.config.setdefault('STATSD_HOST', '127.0.0.1'),
@@ -57,7 +59,7 @@ def init_app(self, app):
             app.after_request(self._after_request)
 
     def _metric_name(self, name):
-        return '%s.%s' % (current_app.name, name)
+        return '%s.%s' % (current_app.config['SITE_ID'], name)
 
     def timer(self, stat, rate=None):
         """
@@ -126,17 +128,25 @@ def set(self, stat, value, rate=None):  # NOQA: A003
     def pipeline(self):
         return current_app.statsd.pipeline()
 
+    # before/after request don't always capture the time taken by _other_ before/after
+    # request handlers since they can run even before or after. We also miss requests
+    # that result in errors unless the error handler makes a log entry.
+    # For comprehensive logging of the entire request, we need WSGI middleware wrapping
+    # the entire app, as described here: https://steinn.org/post/flask-statsd-revisited/
+
     def _before_request(self):
         if current_app.config['STATSD_RATE'] != 0:
             setattr(request, START_TIME_ATTR, time.time())
 
     def _after_request(self, response):
+        self.log_request_timer(response.status_code)
+        return response
+
+    def log_request_timer(self, status_code):
         if hasattr(request, START_TIME_ATTR):
             metrics = [
-                '.'.join(
-                    ['request_handlers', request.endpoint, str(response.status_code)]
-                ),
-                '.'.join(['request_handlers', '_overall', str(response.status_code)]),
+                '.'.join(['request_handlers', request.endpoint, str(status_code)]),
+                '.'.join(['request_handlers', '_overall', str(status_code)]),
             ]
 
             for metric_name in metrics:
@@ -147,5 +157,3 @@ def _after_request(self, response):
                     int((time.time() - getattr(request, START_TIME_ATTR)) * 1000),
                 )
                 self.incr(metric_name)
-
-        return response

From 416ee34ff9105c5e74d3d48cfb359148e01854c1 Mon Sep 17 00:00:00 2001
From: Kiran Jonnalagadda <kiran@hasgeek.com>
Date: Fri, 8 May 2020 02:46:55 +0530
Subject: [PATCH 03/11] Update exported symbols

---
 baseframe/__init__.py | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/baseframe/__init__.py b/baseframe/__init__.py
index 5629393b..c6566bc9 100644
--- a/baseframe/__init__.py
+++ b/baseframe/__init__.py
@@ -52,9 +52,15 @@
     '__version__',
     '__version_info__',
     'assets',
+    'babel',
     'baseframe',
     'baseframe_css',
     'baseframe_js',
+    'cache',
+    'localize_timezone',
+    'localized_country_list',
+    'request_is_xhr',
+    'statsd',
     'Version',
 ]
 

From fe81acebd3d49980e1ee37167f369508702f6d80 Mon Sep 17 00:00:00 2001
From: Kiran Jonnalagadda <kiran@hasgeek.com>
Date: Fri, 8 May 2020 15:28:20 +0530
Subject: [PATCH 04/11] Add an `app` prefix and drop configurable statsd prefix

* Use `app` as a prefix to namespace app metrics away from other metrics
* Drop the configurable prefix since it's of unclear value
---
 baseframe/statsd.py | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/baseframe/statsd.py b/baseframe/statsd.py
index 5ee94be0..1fcd54dc 100644
--- a/baseframe/statsd.py
+++ b/baseframe/statsd.py
@@ -31,7 +31,6 @@ class Statsd(object):
         SITE_ID = app.name  # Used as a prefix in stats
         STATSD_HOST = '127.0.0.1'
         STATSD_PORT = 8125
-        STATSD_PREFIX = None
         STATSD_MAXUDPSIZE = 512
         STATSD_IPV6 = False
         STATSD_RATE = 1
@@ -49,7 +48,7 @@ def init_app(self, app):
         app.statsd = StatsClient(
             host=app.config.setdefault('STATSD_HOST', '127.0.0.1'),
             port=app.config.setdefault('STATSD_PORT', 8125),
-            prefix=app.config.setdefault('STATSD_PREFIX', None),
+            prefix=None,
             maxudpsize=app.config.setdefault('STATSD_MAXUDPSIZE', 512),
             ipv6=app.config.setdefault('STATSD_IPV6', False),
         )
@@ -59,7 +58,7 @@ def init_app(self, app):
             app.after_request(self._after_request)
 
     def _metric_name(self, name):
-        return '%s.%s' % (current_app.config['SITE_ID'], name)
+        return 'app.%s.%s' % (current_app.config['SITE_ID'], name)
 
     def timer(self, stat, rate=None):
         """

From 5181c0619f5567eb217f9fd25d331f9a8f4ca4f4 Mon Sep 17 00:00:00 2001
From: Kiran Jonnalagadda <kiran@hasgeek.com>
Date: Sat, 9 May 2020 00:06:14 +0530
Subject: [PATCH 05/11] Use app.extensions

---
 baseframe/statsd.py | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/baseframe/statsd.py b/baseframe/statsd.py
index 1fcd54dc..48aafa75 100644
--- a/baseframe/statsd.py
+++ b/baseframe/statsd.py
@@ -45,7 +45,8 @@ def init_app(self, app):
         app.config.setdefault('STATSD_RATE', 1)
         app.config.setdefault('SITE_ID', app.name)
 
-        app.statsd = StatsClient(
+        app.extensions['statsd'] = self
+        app.extensions['statsd_core'] = StatsClient(
             host=app.config.setdefault('STATSD_HOST', '127.0.0.1'),
             port=app.config.setdefault('STATSD_PORT', 8125),
             prefix=None,
@@ -66,7 +67,7 @@ def timer(self, stat, rate=None):
         record timing for a block or function call. Use as a decorator is not supported
         as an application context is required.
         """
-        return current_app.statsd.timer(
+        return current_app.extensions['statsd_core'].timer(
             stat, rate=rate if rate is not None else current_app.config['STATSD_RATE']
         )
 
@@ -74,7 +75,7 @@ def timing(self, stat, delta, rate=None):
         """
         Record timer information.
         """
-        return current_app.statsd.timing(
+        return current_app.extensions['statsd_core'].timing(
             self._metric_name(stat),
             delta,
             rate=rate if rate is not None else current_app.config['STATSD_RATE'],
@@ -84,7 +85,7 @@ def incr(self, stat, count=1, rate=None):
         """
         Increment a counter.
         """
-        return current_app.statsd.incr(
+        return current_app.extensions['statsd_core'].incr(
             self._metric_name(stat),
             count,
             rate=rate if rate is not None else current_app.config['STATSD_RATE'],
@@ -94,7 +95,7 @@ def decr(self, stat, count=1, rate=None):
         """
         Decrement a counter.
         """
-        return current_app.statsd.decr(
+        return current_app.extensions['statsd_core'].decr(
             self._metric_name(stat),
             count,
             rate=rate if rate is not None else current_app.config['STATSD_RATE'],
@@ -104,7 +105,7 @@ def gauge(self, stat, value, rate=None, delta=False):
         """
         Set a gauge value.
         """
-        return current_app.statsd.gauge(
+        return current_app.extensions['statsd_core'].gauge(
             self._metric_name(stat),
             value,
             rate=rate if rate is not None else current_app.config['STATSD_RATE'],
@@ -118,14 +119,14 @@ def set(self, stat, value, rate=None):  # NOQA: A003
         The statsd server does _not_ take the sample rate into account for sets. Use
         with care.
         """
-        return current_app.statsd.set(
+        return current_app.extensions['statsd_core'].set(
             self._metric_name(stat),
             value,
             rate=rate if rate is not None else current_app.config['STATSD_RATE'],
         )
 
     def pipeline(self):
-        return current_app.statsd.pipeline()
+        return current_app.extensions['statsd_core'].pipeline()
 
     # before/after request don't always capture the time taken by _other_ before/after
     # request handlers since they can run even before or after. We also miss requests

From 42407061875b2df151dd6aab45055a7e7685c86a Mon Sep 17 00:00:00 2001
From: Kiran Jonnalagadda <kiran@hasgeek.com>
Date: Sun, 10 May 2020 01:10:14 +0530
Subject: [PATCH 06/11] Use request signals for more reliable timing capture

---
 baseframe/errors.py |  5 +----
 baseframe/statsd.py | 30 ++++++++++++------------------
 2 files changed, 13 insertions(+), 22 deletions(-)

diff --git a/baseframe/errors.py b/baseframe/errors.py
index 1f6f6e2c..d693ce62 100644
--- a/baseframe/errors.py
+++ b/baseframe/errors.py
@@ -5,7 +5,7 @@
 
 from coaster.views import render_with
 
-from . import baseframe, baseframe_translations, statsd
+from . import baseframe, baseframe_translations
 
 
 @baseframe.app_errorhandler(404)
@@ -27,7 +27,6 @@ def error404(e):
         except (NotFound, RequestRedirect, MethodNotAllowed):
             pass
     baseframe_translations.as_default()
-    statsd.log_request_timer(404)
     return {'error': "404 Not Found"}, 404
 
 
@@ -35,7 +34,6 @@ def error404(e):
 @render_with('403.html.jinja2', json=True)
 def error403(e):
     baseframe_translations.as_default()
-    statsd.log_request_timer(403)
     return {'error': "403 Forbidden"}, 403
 
 
@@ -46,5 +44,4 @@ def error500(e):
         current_app.extensions['sqlalchemy'].db.session.rollback()
 
     baseframe_translations.as_default()
-    statsd.log_request_timer(500)
     return {'error': "500 Internal Server Error"}, 500
diff --git a/baseframe/statsd.py b/baseframe/statsd.py
index 48aafa75..16f84414 100644
--- a/baseframe/statsd.py
+++ b/baseframe/statsd.py
@@ -4,7 +4,7 @@
 
 import time
 
-from flask import current_app, request
+from flask import current_app, request, request_finished, request_started
 
 from statsd import StatsClient
 
@@ -55,8 +55,10 @@ def init_app(self, app):
         )
 
         if app.config.setdefault('STATSD_REQUEST_TIMER', True):
-            app.before_request(self._before_request)
-            app.after_request(self._after_request)
+            # Use signals because they are called before and after all other request
+            # processors, allowing us to capture (nearly) all time taken for processing
+            request_started.connect(self._request_started, app)
+            request_finished.connect(self._request_finished, app)
 
     def _metric_name(self, name):
         return 'app.%s.%s' % (current_app.config['SITE_ID'], name)
@@ -128,25 +130,17 @@ def set(self, stat, value, rate=None):  # NOQA: A003
     def pipeline(self):
         return current_app.extensions['statsd_core'].pipeline()
 
-    # before/after request don't always capture the time taken by _other_ before/after
-    # request handlers since they can run even before or after. We also miss requests
-    # that result in errors unless the error handler makes a log entry.
-    # For comprehensive logging of the entire request, we need WSGI middleware wrapping
-    # the entire app, as described here: https://steinn.org/post/flask-statsd-revisited/
-
-    def _before_request(self):
-        if current_app.config['STATSD_RATE'] != 0:
+    def _request_started(self, app):
+        if app.config['STATSD_RATE'] != 0:
             setattr(request, START_TIME_ATTR, time.time())
 
-    def _after_request(self, response):
-        self.log_request_timer(response.status_code)
-        return response
-
-    def log_request_timer(self, status_code):
+    def _request_finished(self, app, response):
         if hasattr(request, START_TIME_ATTR):
             metrics = [
-                '.'.join(['request_handlers', request.endpoint, str(status_code)]),
-                '.'.join(['request_handlers', '_overall', str(status_code)]),
+                '.'.join(
+                    ['request_handlers', request.endpoint, str(response.status_code)]
+                ),
+                '.'.join(['request_handlers', '_overall', str(response.status_code)]),
             ]
 
             for metric_name in metrics:

From ab0b542dce749355c286bf55a9b2e3e9044b56a2 Mon Sep 17 00:00:00 2001
From: Kiran Jonnalagadda <kiran@hasgeek.com>
Date: Sun, 10 May 2020 05:09:51 +0530
Subject: [PATCH 07/11] Update docs so statsd is included

---
 docs/index.rst | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/docs/index.rst b/docs/index.rst
index f23092ed..c55c60c7 100644
--- a/docs/index.rst
+++ b/docs/index.rst
@@ -17,10 +17,28 @@ Reusable styles and templates for HasGeek projects.
     :maxdepth: 2
 
 .. automodule:: baseframe
-	:members:
+    :members:
 
 .. automodule:: baseframe.forms
-   :members:
+    :members:
+
+.. automodule:: baseframe.errors
+    :members:
+
+.. automodule:: baseframe.filters
+    :members:
+
+.. automodule:: baseframe.signals
+    :members:
+
+.. automodule:: baseframe.statsd
+    :members:
+
+.. automodule:: baseframe.utils
+    :members:
+
+.. automodule:: baseframe.views
+    :members:
 
 
 Indices and tables

From be98606c6045ebd082fbaecd0cf8606341790e3c Mon Sep 17 00:00:00 2001
From: Kiran Jonnalagadda <kiran@hasgeek.com>
Date: Sun, 10 May 2020 05:10:51 +0530
Subject: [PATCH 08/11] Support tags, simplify wrappers, and stub statsd tests

---
 baseframe/statsd.py  | 125 +++++++++++++++++++++----------------------
 setup.py             |   2 +-
 tests/test_statsd.py |  44 +++++++++++++++
 3 files changed, 105 insertions(+), 66 deletions(-)
 create mode 100644 tests/test_statsd.py

diff --git a/baseframe/statsd.py b/baseframe/statsd.py
index 16f84414..afd21a40 100644
--- a/baseframe/statsd.py
+++ b/baseframe/statsd.py
@@ -2,6 +2,7 @@
 
 from __future__ import absolute_import
 
+from functools import partial
 import time
 
 from flask import current_app, request, request_finished, request_started
@@ -20,10 +21,10 @@ class Statsd(object):
     """
     Statsd extension for Flask
 
-    Offers conveniences on top of using statsd directly:
+    Offers conveniences on top of using py-statsd directly:
     1. In a multi-app setup, each app can have its own statsd config
     2. The app's name is automatically prefixed to all stats
-    3. Sampling rate can be specified in app config instead of in each call
+    3. Sampling rate can be specified in app config
     4. Requests are automatically timed and counted unless STATSD_REQUEST_TIMER is False
 
     App configuration defaults::
@@ -34,16 +35,55 @@ class Statsd(object):
         STATSD_MAXUDPSIZE = 512
         STATSD_IPV6 = False
         STATSD_RATE = 1
+        STATSD_TAGS = None
         STATSD_REQUEST_TIMER = True
+
+    If the statsd server supports tags, the ``STATSD_TAGS`` parameter may contain a
+    separator character per the server's syntax::
+
+        # Influxdb:
+        ',' == 'metric_name,tag1=value1,tag2=value2'
+        # Carbon/Graphite:
+        ';' == 'metric_name;tag1=value;tag2=value2'
+
+    Tags will be discarded when ``STATSD_TAGS`` is unset. Servers have varying
+    limitations on the allowed content in tags and values. Alphanumeric values are
+    generally safe.
+
+    From Carbon/Graphite's documentation:
+
+    > Tag names must have a length >= 1 and may contain any ascii characters except
+    > ``;!^=``. Tag values must also have a length >= 1, they may contain any ascii
+    > characters except ``;`` and the first character must not be ``~``. UTF-8
+    > characters may work for names and values, but they are not well tested and it is
+    > not recommended to use non-ascii characters in metric names or tags.
+
+    The Datadog and SignalFx tag formats are not supported at this time.
     """
 
     def __init__(self, app=None):
         if app is not None:
             self.init_app(app)
 
+        # TODO: When Baseframe drops Python 2.7 support, replace `partial` here with
+        # `partialmethod` (Py 3.4+) and set these on the class definition
+
+        for method in ('timer', 'timing', 'incr', 'decr', 'gauge', 'set'):
+            func = partial(self._wrapper, method)
+            func.__name__ = method
+            func.__doc__ = getattr(StatsClient, method).__doc__
+            setattr(self, method, func)
+
+        self.timer.__doc__ = """
+        Return a Timer object that can be used as a context manager to automatically
+        record timing for a block or function call. Use as a decorator is not supported
+        as an application context is required.
+        """
+
     def init_app(self, app):
         app.config.setdefault('STATSD_RATE', 1)
         app.config.setdefault('SITE_ID', app.name)
+        app.config.setdefault('STATSD_TAGS', None)
 
         app.extensions['statsd'] = self
         app.extensions['statsd_core'] = StatsClient(
@@ -60,72 +100,24 @@ def init_app(self, app):
             request_started.connect(self._request_started, app)
             request_finished.connect(self._request_finished, app)
 
-    def _metric_name(self, name):
+    def _metric_name(self, name, tags):
+        # In Py 3.8, the following two lines can be combined with a walrus operator:
+        # if tags and (tag_sep := current_app.config['STATSD_TAGS']):
+        if tags and current_app.config['STATSD_TAGS']:
+            tag_sep = current_app.config['STATSD_TAGS']
+            name += tag_sep + tag_sep.join(
+                '='.join((str(t), str(v))) if v is not None else str(t)
+                for t, v in tags.items()
+            )
         return 'app.%s.%s' % (current_app.config['SITE_ID'], name)
 
-    def timer(self, stat, rate=None):
-        """
-        Return a Timer object that can be used as a context manager to automatically
-        record timing for a block or function call. Use as a decorator is not supported
-        as an application context is required.
-        """
-        return current_app.extensions['statsd_core'].timer(
-            stat, rate=rate if rate is not None else current_app.config['STATSD_RATE']
-        )
+    def _wrapper(self, metric, stat, *args, **kwargs):
+        tags = kwargs.pop('tags', None)
+        if kwargs.setdefault('rate', None) is None:
+            kwargs['rate'] = current_app.config['STATSD_RATE']
+        stat = self._metric_name(stat, tags)
 
-    def timing(self, stat, delta, rate=None):
-        """
-        Record timer information.
-        """
-        return current_app.extensions['statsd_core'].timing(
-            self._metric_name(stat),
-            delta,
-            rate=rate if rate is not None else current_app.config['STATSD_RATE'],
-        )
-
-    def incr(self, stat, count=1, rate=None):
-        """
-        Increment a counter.
-        """
-        return current_app.extensions['statsd_core'].incr(
-            self._metric_name(stat),
-            count,
-            rate=rate if rate is not None else current_app.config['STATSD_RATE'],
-        )
-
-    def decr(self, stat, count=1, rate=None):
-        """
-        Decrement a counter.
-        """
-        return current_app.extensions['statsd_core'].decr(
-            self._metric_name(stat),
-            count,
-            rate=rate if rate is not None else current_app.config['STATSD_RATE'],
-        )
-
-    def gauge(self, stat, value, rate=None, delta=False):
-        """
-        Set a gauge value.
-        """
-        return current_app.extensions['statsd_core'].gauge(
-            self._metric_name(stat),
-            value,
-            rate=rate if rate is not None else current_app.config['STATSD_RATE'],
-            delta=delta,
-        )
-
-    def set(self, stat, value, rate=None):  # NOQA: A003
-        """
-        Increment a set value.
-
-        The statsd server does _not_ take the sample rate into account for sets. Use
-        with care.
-        """
-        return current_app.extensions['statsd_core'].set(
-            self._metric_name(stat),
-            value,
-            rate=rate if rate is not None else current_app.config['STATSD_RATE'],
-        )
+        getattr(current_app.extensions['statsd_core'], metric)(stat, *args, **kwargs)
 
     def pipeline(self):
         return current_app.extensions['statsd_core'].pipeline()
@@ -150,4 +142,7 @@ def _request_finished(self, app, response):
                     metric_name,
                     int((time.time() - getattr(request, START_TIME_ATTR)) * 1000),
                 )
+                # The timer metric also registers a count, making the counter metric
+                # seemingly redundant, but the counter metric also includes a rate, so
+                # we use both: timer (via `timing`) and counter (via `incr`).
                 self.incr(metric_name)
diff --git a/setup.py b/setup.py
index 0d02a1d6..f375faf6 100644
--- a/setup.py
+++ b/setup.py
@@ -89,7 +89,7 @@ def run(self):
     tests_require=['Flask-SQLAlchemy'],
     cmdclass={'build_py': BaseframeBuildPy},
     dependency_links=[
-        "https://github.com/hasgeek/coaster/archive/master.zip#egg=coaster-dev",
+        "https://github.com/hasgeek/coaster/archive/master.zip#egg=coaster",
         "https://github.com/hasgeek/flask-babelhg/archive/master.zip#egg=Flask-Babelhg-0.12.3",
     ],
 )
diff --git a/tests/test_statsd.py b/tests/test_statsd.py
new file mode 100644
index 00000000..e2a4ad91
--- /dev/null
+++ b/tests/test_statsd.py
@@ -0,0 +1,44 @@
+# -*- coding: utf-8 -*-
+# Tests adapted from https://github.com/bbelyeu/flask-statsdclient
+
+import unittest
+
+from flask import Flask
+
+from baseframe.statsd import Statsd
+
+statsd = Statsd()
+
+
+def create_app(config=None):
+    app = Flask(__name__)
+    if config:
+        app.config.update(config)
+    statsd.init_app(app)
+    return app
+
+
+class TestStatsd(unittest.TestCase):
+    """Test Statsd extension"""
+
+    def setUp(self):
+        self.app = create_app()
+        self.ctx = self.app.app_context()
+        self.ctx.push()
+
+    def tearDown(self):
+        self.ctx.pop()
+
+    def test_default_config(self):
+        assert self.app.extensions['statsd_core']._addr == ('127.0.0.1', 8125)
+
+    def test_custom_config(self):
+        self.app.config['STATSD_HOST'] = '1.2.3.4'
+        self.app.config['STATSD_PORT'] = 12345
+
+        Statsd(self.app)
+        assert self.app.extensions['statsd_core']._addr == ('1.2.3.4', 12345)
+
+
+# TODO: A full set of tests will require unittest.mock, available Py 3.3 onwards (not
+# Py 2.7). Tests are pending for now, or will need a backward compatible method

From 003e12bee75e4ba045a7b1532520f70c8c27ad18 Mon Sep 17 00:00:00 2001
From: Kiran Jonnalagadda <kiran@hasgeek.com>
Date: Sun, 10 May 2020 05:36:29 +0530
Subject: [PATCH 09/11] Add Py3 tests for statsd

---
 tests/test_statsd.py | 96 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 92 insertions(+), 4 deletions(-)

diff --git a/tests/test_statsd.py b/tests/test_statsd.py
index e2a4ad91..a86873b7 100644
--- a/tests/test_statsd.py
+++ b/tests/test_statsd.py
@@ -1,12 +1,20 @@
 # -*- coding: utf-8 -*-
 # Tests adapted from https://github.com/bbelyeu/flask-statsdclient
 
+import six
+
 import unittest
 
 from flask import Flask
 
 from baseframe.statsd import Statsd
 
+try:
+    from unittest.mock import patch
+except ImportError:
+    patch = None
+
+
 statsd = Statsd()
 
 
@@ -29,16 +37,96 @@ def setUp(self):
     def tearDown(self):
         self.ctx.pop()
 
+    @property
+    def statsd_core(self):
+        return self.app.extensions['statsd_core']
+
     def test_default_config(self):
-        assert self.app.extensions['statsd_core']._addr == ('127.0.0.1', 8125)
+        assert self.statsd_core._addr == ('127.0.0.1', 8125)
 
     def test_custom_config(self):
         self.app.config['STATSD_HOST'] = '1.2.3.4'
         self.app.config['STATSD_PORT'] = 12345
 
         Statsd(self.app)
-        assert self.app.extensions['statsd_core']._addr == ('1.2.3.4', 12345)
+        assert self.statsd_core._addr == ('1.2.3.4', 12345)
+
+    if six.PY3:
+
+        # The wrapper in Statsd will:
+        # 1. Prefix ``app.`` and `app.name` to the stat name
+        # 2. Insert STATSD_RATE if no rate is specified
+        # 3. Insert tags if tags are enabled and specified
+        def test_wrapper(self):
+            with patch('statsd.StatsClient.incr') as mock_incr:
+                statsd.incr('test.counter')
+                mock_incr.assert_called_once_with(
+                    'app.tests.test_statsd.test.counter', rate=1
+                )
+            with patch('statsd.StatsClient.incr') as mock_incr:
+                statsd.incr('test.counter', rate=0.5)
+                mock_incr.assert_called_once_with(
+                    'app.tests.test_statsd.test.counter', rate=0.5
+                )
+            with patch('statsd.StatsClient.incr') as mock_incr:
+                statsd.incr('test.counter', 2, rate=0.5)
+                mock_incr.assert_called_once_with(
+                    'app.tests.test_statsd.test.counter', 2, rate=0.5
+                )
+
+        def test_wrapper_custom_rate(self):
+            self.app.config['STATSD_RATE'] = 0.3
+            with patch('statsd.StatsClient.incr') as mock_incr:
+                statsd.incr('test.counter')
+                mock_incr.assert_called_once_with(
+                    'app.tests.test_statsd.test.counter', rate=0.3
+                )
+            with patch('statsd.StatsClient.incr') as mock_incr:
+                statsd.incr('test.counter', rate=0.5)
+                mock_incr.assert_called_once_with(
+                    'app.tests.test_statsd.test.counter', rate=0.5
+                )
+
+        def test_wrapper_tags(self):
+            # Tags are stripped unless supported by config declaration
+            with patch('statsd.StatsClient.incr') as mock_incr:
+                statsd.incr('test.counter', tags={'tag': 'value'})
+                mock_incr.assert_called_once_with(
+                    'app.tests.test_statsd.test.counter', rate=1
+                )
+
+            # Tags are enabled if a separator character is specified in config
+            self.app.config['STATSD_TAGS'] = ';'
+            with patch('statsd.StatsClient.incr') as mock_incr:
+                statsd.incr('test.counter', tags={'tag': 'value'})
+                mock_incr.assert_called_once_with(
+                    'app.tests.test_statsd.test.counter;tag=value', rate=1
+                )
+            with patch('statsd.StatsClient.incr') as mock_incr:
+                statsd.incr('test.counter', tags={'tag': 'value', 't2': 'v2'})
+                mock_incr.assert_called_once_with(
+                    'app.tests.test_statsd.test.counter;tag=value;t2=v2', rate=1
+                )
+            with patch('statsd.StatsClient.incr') as mock_incr:
+                statsd.incr('test.counter', tags={'tag': 'val', 't2': 'v2', 't3': None})
+                mock_incr.assert_called_once_with(
+                    'app.tests.test_statsd.test.counter;tag=val;t2=v2;t3', rate=1
+                )
+            with patch('statsd.StatsClient.incr') as mock_incr:
+                statsd.incr('test.counter', tags={'tag': 'val', 't2': None, 't3': 'v3'})
+                mock_incr.assert_called_once_with(
+                    'app.tests.test_statsd.test.counter;tag=val;t2;t3=v3', rate=1
+                )
+
+            # Other separator characters are supported too
+            self.app.config['STATSD_TAGS'] = ','
+            with patch('statsd.StatsClient.incr') as mock_incr:
+                statsd.incr('test.counter', tags={'tag': 'value', 't2': 'v2'})
+                mock_incr.assert_called_once_with(
+                    'app.tests.test_statsd.test.counter,tag=value,t2=v2', rate=1
+                )
 
 
-# TODO: A full set of tests will require unittest.mock, available Py 3.3 onwards (not
-# Py 2.7). Tests are pending for now, or will need a backward compatible method
+# TODO: Test all wrappers: ('timer', 'timing', 'incr', 'decr', 'gauge', 'set')
+# TODO: Test pipeline()
+# TODO: Test request handler stats

From 07bfe1f7ec70c426028db911b8eba9b06a3b265a Mon Sep 17 00:00:00 2001
From: Kiran Jonnalagadda <kiran@hasgeek.com>
Date: Sun, 10 May 2020 06:02:38 +0530
Subject: [PATCH 10/11] partialmethod is slower

---
 baseframe/statsd.py | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/baseframe/statsd.py b/baseframe/statsd.py
index afd21a40..49733bdf 100644
--- a/baseframe/statsd.py
+++ b/baseframe/statsd.py
@@ -65,21 +65,15 @@ def __init__(self, app=None):
         if app is not None:
             self.init_app(app)
 
-        # TODO: When Baseframe drops Python 2.7 support, replace `partial` here with
-        # `partialmethod` (Py 3.4+) and set these on the class definition
-
+        # Py 3.4+ has `functools.partialmethod`, allowing these to be set directly
+        # on the class, but as per `timeit` it is about 50% slower. Since this class
+        # will be instantiated only once per runtime, we get an overall performance
+        # improvement at the cost of making it slightly harder to find documentation.
         for method in ('timer', 'timing', 'incr', 'decr', 'gauge', 'set'):
             func = partial(self._wrapper, method)
             func.__name__ = method
-            func.__doc__ = getattr(StatsClient, method).__doc__
             setattr(self, method, func)
 
-        self.timer.__doc__ = """
-        Return a Timer object that can be used as a context manager to automatically
-        record timing for a block or function call. Use as a decorator is not supported
-        as an application context is required.
-        """
-
     def init_app(self, app):
         app.config.setdefault('STATSD_RATE', 1)
         app.config.setdefault('SITE_ID', app.name)

From 10a18c00f5dd5d3a034a643b0565d2ec23644cbb Mon Sep 17 00:00:00 2001
From: Kiran Jonnalagadda <kiran@hasgeek.com>
Date: Sun, 10 May 2020 06:12:44 +0530
Subject: [PATCH 11/11] Update documentation

---
 baseframe/statsd.py | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/baseframe/statsd.py b/baseframe/statsd.py
index 49733bdf..57cd62b4 100644
--- a/baseframe/statsd.py
+++ b/baseframe/statsd.py
@@ -38,19 +38,17 @@ class Statsd(object):
         STATSD_TAGS = None
         STATSD_REQUEST_TIMER = True
 
-    If the statsd server supports tags, the ``STATSD_TAGS`` parameter may contain a
-    separator character per the server's syntax::
+    If the statsd server supports tags, the ``STATSD_TAGS`` parameter may be set to a
+    separator character as per the server's syntax.
 
-        # Influxdb:
-        ',' == 'metric_name,tag1=value1,tag2=value2'
-        # Carbon/Graphite:
-        ';' == 'metric_name;tag1=value;tag2=value2'
+    Influxdb uses a comma: ``'metric_name,tag1=value1,tag2=value2'``
+
+    Carbon/Graphite uses a semicolon: ``'metric_name;tag1=value;tag2=value2'``
 
     Tags will be discarded when ``STATSD_TAGS`` is unset. Servers have varying
     limitations on the allowed content in tags and values. Alphanumeric values are
-    generally safe.
-
-    From Carbon/Graphite's documentation:
+    generally safe. This extension does not validate content. From Carbon/Graphite's
+    documentation:
 
     > Tag names must have a length >= 1 and may contain any ascii characters except
     > ``;!^=``. Tag values must also have a length >= 1, they may contain any ascii