Merge ~cjwatson/launchpad:statsd-implicit-env into launchpad:master

[Colin Watson]

StatsdClient: Automatically add env label

Having to add the appropriate env label manually at each call site is a bit too cumbersome and error-prone, and indeed we'd forgotten to include it for traversal and publication duration timings. Add it automatically in the StatsdClient wrapper instead.

[Tom Wardill (twom)]

review: Approve

diff --git a/lib/lp/buildmaster/manager.py b/lib/lp/buildmaster/manager.py
index e9732ea..7bbe4aa 100644
--- a/lib/lp/buildmaster/manager.py
+++ b/lib/lp/buildmaster/manager.py
@@ -532,13 +532,10 @@ class SlaveScanner:
             if builder.current_build is not None:
                 builder.current_build.gotFailure()
                 self.statsd_client.incr(
-                    'builders.judged_failed,build=True,arch={},env={}'.format(
-                        builder.current_build.processor.name,
-                        self.statsd_client.lp_environment))
+                    'builders.judged_failed,build=True,arch={}'.format(
+                        builder.current_build.processor.name))
             else:
-                self.statsd_client.incr(
-                    'builders.judged_failed,build=False,env={}'.format(
-                        self.statsd_client.lp_environment))
+                self.statsd_client.incr('builders.judged_failed,build=False')
             recover_failure(self.logger, vitals, builder, retry, failure.value)
             transaction.commit()
         except Exception:
diff --git a/lib/lp/buildmaster/model/buildfarmjobbehaviour.py b/lib/lp/buildmaster/model/buildfarmjobbehaviour.py
index 88ede6a..13a38d7 100644
--- a/lib/lp/buildmaster/model/buildfarmjobbehaviour.py
+++ b/lib/lp/buildmaster/model/buildfarmjobbehaviour.py
@@ -158,10 +158,9 @@ class BuildFarmJobBehaviourBase:
         job_type_name = job_type.name if job_type else 'UNKNOWN'
         statsd_client = getUtility(IStatsdClient)
         statsd_client.incr(
-            'build.count,job_type={},builder_name={},env={}'.format(
+            'build.count,job_type={},builder_name={}'.format(
                 job_type_name,
-                self._builder.name,
-                statsd_client.lp_environment))
+                self._builder.name))
 
         logger.info(
             "Job %s (%s) started on %s: %s %s"
diff --git a/lib/lp/services/job/runner.py b/lib/lp/services/job/runner.py
index b0b65a2..15ea4e9 100644
--- a/lib/lp/services/job/runner.py
+++ b/lib/lp/services/job/runner.py
@@ -326,25 +326,20 @@ class BaseRunnableJob(BaseRunnableJobSource):
         """See `IJob`."""
         self.job.start(manage_transaction=manage_transaction)
         statsd = getUtility(IStatsdClient)
-        statsd.incr('job.start_count,type={},env={}'.format(
-            self.__class__.__name__,
-            statsd.lp_environment))
+        statsd.incr('job.start_count,type={}'.format(self.__class__.__name__))
 
     def complete(self, manage_transaction=False):
         """See `IJob`."""
         self.job.complete(manage_transaction=manage_transaction)
         statsd = getUtility(IStatsdClient)
-        statsd.incr('job.complete_count,type={},env={}'.format(
-            self.__class__.__name__,
-            statsd.lp_environment))
+        statsd.incr('job.complete_count,type={}'.format(
+            self.__class__.__name__))
 
     def fail(self, manage_transaction=False):
         """See `IJob`."""
         self.job.fail(manage_transaction=manage_transaction)
         statsd = getUtility(IStatsdClient)
-        statsd.incr('job.fail_count,type={},env={}'.format(
-            self.__class__.__name__,
-            statsd.lp_environment))
+        statsd.incr('job.fail_count,type={}'.format(self.__class__.__name__))
 
 
 class BaseJobRunner(LazrJobRunner):
diff --git a/lib/lp/services/statsd/model/statsd_client.py b/lib/lp/services/statsd/model/statsd_client.py
index 6ccf027..7d6c3c1 100644
--- a/lib/lp/services/statsd/model/statsd_client.py
+++ b/lib/lp/services/statsd/model/statsd_client.py
@@ -43,14 +43,17 @@ class StatsdClient:
         self._make_client()
 
     def __getattr__(self, name):
-        # This is a convenience to keep all statsd client related
-        # items on the client. Having this separate from prefix
-        # allows us to use it as a label, rather than as part of the
-        # gauge name
-        if name == 'lp_environment':
-            return config.statsd.environment
         if self._client is not None:
-            return getattr(self._client, name)
+            wrapped = getattr(self._client, name)
+            if name in ("timer", "timing", "incr", "decr", "gauge", "set"):
+                def wrapper(stat, *args, **kwargs):
+                    return wrapped(
+                        "%s,env=%s" % (stat, config.statsd.environment),
+                        *args, **kwargs)
+
+                return wrapper
+            else:
+                return wrapped
         else:
             # Prevent unnecessary network traffic if this Launchpad instance
             # has no statsd configuration.
diff --git a/lib/lp/services/statsd/numbercruncher.py b/lib/lp/services/statsd/numbercruncher.py
index 27c61a4..3f1a6e1 100644
--- a/lib/lp/services/statsd/numbercruncher.py
+++ b/lib/lp/services/statsd/numbercruncher.py
@@ -88,8 +88,7 @@ class NumberCruncher(service.Service):
                 virt = queue_type == 'virt'
                 for arch, value in contents.items():
                     gauge_name = (
-                        "buildqueue,virtualized={},arch={},env={}".format(
-                            virt, arch, self.statsd_client.lp_environment))
+                        "buildqueue,virtualized={},arch={}".format(virt, arch))
                     self._sendGauge(gauge_name, value[0])
             self.logger.debug("Build queue stats update complete.")
         except Exception:
@@ -122,8 +121,8 @@ class NumberCruncher(service.Service):
                     counts['idle'] += 1
         for processor, counts in counts_by_processor.items():
             for count_name, count_value in counts.items():
-                gauge_name = "builders,status={},arch={},env={}".format(
-                    count_name, processor, self.statsd_client.lp_environment)
+                gauge_name = "builders,status={},arch={}".format(
+                    count_name, processor)
                 self._sendGauge(gauge_name, count_value)
         self.logger.debug("Builder stats update complete.")
 
@@ -149,14 +148,8 @@ class NumberCruncher(service.Service):
             store = IStore(LibraryFileContent)
             total_files, total_filesize = store.find(
                 (Count(), Sum(LibraryFileContent.filesize))).one()
-            self._sendGauge(
-                "librarian.total_files,env={}".format(
-                    self.statsd_client.lp_environment),
-                total_files)
-            self._sendGauge(
-                "librarian.total_filesize,env={}".format(
-                    self.statsd_client.lp_environment),
-                total_filesize)
+            self._sendGauge("librarian.total_files", total_files)
+            self._sendGauge("librarian.total_filesize", total_filesize)
             self.logger.debug("Librarian stats update complete.")
         except Exception:
             self.logger.exception("Failure while updating librarian stats:")
diff --git a/lib/lp/services/statsd/tests/__init__.py b/lib/lp/services/statsd/tests/__init__.py
index 23fb067..c9c9ef5 100644
--- a/lib/lp/services/statsd/tests/__init__.py
+++ b/lib/lp/services/statsd/tests/__init__.py
@@ -8,9 +8,11 @@ from __future__ import absolute_import, print_function, unicode_literals
 __metaclass__ = type
 __all__ = ['StatsMixin']
 
+from fixtures import MockPatchObject
+from zope.component import getUtility
+
 from lp.services.compat import mock
 from lp.services.statsd.interfaces.statsd_client import IStatsdClient
-from lp.testing.fixture import ZopeUtilityFixture
 
 
 class StatsMixin:
@@ -18,7 +20,8 @@ class StatsMixin:
     def setUpStats(self):
         # Install a mock statsd client so we can assert against the call
         # counts and args.
+        self.pushConfig("statsd", environment="test")
+        statsd_client = getUtility(IStatsdClient)
         self.stats_client = mock.Mock()
-        self.stats_client.lp_environment = "test"
         self.useFixture(
-            ZopeUtilityFixture(self.stats_client, IStatsdClient))
+            MockPatchObject(statsd_client, "_client", self.stats_client))
diff --git a/lib/lp/services/statsd/tests/test_statsd_client.py b/lib/lp/services/statsd/tests/test_statsd_client.py
index 2e1a8e1..709b41c 100644
--- a/lib/lp/services/statsd/tests/test_statsd_client.py
+++ b/lib/lp/services/statsd/tests/test_statsd_client.py
@@ -50,13 +50,3 @@ class TestClientConfiguration(TestCase):
             "port: 9999\nprefix: test\nenvironment: test\n")
         client.reload()
         self.assertIsInstance(client._client, StatsClient)
-
-    def test_lp_environment_is_available(self):
-        client = getUtility(IStatsdClient)
-        self.addCleanup(client.reload)
-        config.push(
-            'statsd_test',
-            "[statsd]\nhost: 127.0.01\n"
-            "port: 9999\nprefix: test\nenvironment: test\n")
-        client.reload()
-        self.assertEqual(client.lp_environment, "test")
diff --git a/lib/lp/services/webapp/publication.py b/lib/lp/services/webapp/publication.py
index 9a9a0e3..7325d9f 100644
--- a/lib/lp/services/webapp/publication.py
+++ b/lib/lp/services/webapp/publication.py
@@ -795,16 +795,13 @@ class LaunchpadBrowserPublication(
                 status = request.response.getStatus()
                 if status == 404:  # Not Found
                     OpStats.stats['404s'] += 1
-                    statsd_client.incr('errors.404,env={}'.format(
-                        statsd_client.lp_environment))
+                    statsd_client.incr('errors.404')
                 elif status == 500:  # Unhandled exceptions
                     OpStats.stats['500s'] += 1
-                    statsd_client.incr('errors.500,env={}'.format(
-                        statsd_client.lp_environment))
+                    statsd_client.incr('errors.500')
                 elif status == 503:  # Timeouts
                     OpStats.stats['503s'] += 1
-                    statsd_client.incr('errors.503,env={}'.format(
-                        statsd_client.lp_environment))
+                    statsd_client.incr('errors.503')
 
                 # Increment counters for status code groups.
                 status_group = str(status)[0] + 'XXs'
@@ -813,8 +810,7 @@ class LaunchpadBrowserPublication(
                 # Increment counter for 5XXs_b.
                 if is_browser(request) and status_group == '5XXs':
                     OpStats.stats['5XXs_b'] += 1
-                    statsd_client.incr('errors.5XX,env={}'.format(
-                        statsd_client.lp_environment))
+                    statsd_client.incr('errors.5XX')
 
         # Make sure our databases are in a sane state for the next request.
         thread_name = threading.current_thread().name
diff --git a/lib/lp/services/webapp/tests/test_publication.py b/lib/lp/services/webapp/tests/test_publication.py
index 245fc28..a2a4953 100644
--- a/lib/lp/services/webapp/tests/test_publication.py
+++ b/lib/lp/services/webapp/tests/test_publication.py
@@ -326,11 +326,11 @@ class TestPublisherStats(StatsMixin, TestCaseWithFactory):
             MatchesListwise(
                 [MatchesListwise(
                     (Equals('traversal_duration,success=True,'
-                     'pageid=RootObject-index-html'),
+                     'pageid=RootObject-index-html,env=test'),
                      GreaterThan(0))),
                  MatchesListwise(
                      (Equals('publication_duration,success=True,'
-                      'pageid=RootObject-index-html'),
+                      'pageid=RootObject-index-html,env=test'),
                       GreaterThan(0)))]))
 
     def test_traversal_failure_stats(self):
@@ -350,7 +350,7 @@ class TestPublisherStats(StatsMixin, TestCaseWithFactory):
             MatchesListwise(
                 [MatchesListwise(
                     (Equals('traversal_duration,success=False,'
-                     'pageid=None'),
+                     'pageid=None,env=test'),
                      GreaterThan(0)))]))
 
     def test_publication_failure_stats(self):
@@ -370,11 +370,11 @@ class TestPublisherStats(StatsMixin, TestCaseWithFactory):
             MatchesListwise(
                 [MatchesListwise(
                     (Equals('traversal_duration,success=True,'
-                     'pageid=RootObject-index-html'),
+                     'pageid=RootObject-index-html,env=test'),
                      GreaterThan(0))),
                  MatchesListwise(
                      (Equals('publication_duration,success=False,'
-                      'pageid=RootObject-index-html'),
+                      'pageid=RootObject-index-html,env=test'),
                       GreaterThan(0)))]))
 
     def test_prepPageIDForMetrics_none(self):