launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #25676
[Merge] ~cjwatson/launchpad:statsd-implicit-env into launchpad:master
Colin Watson has proposed merging ~cjwatson/launchpad:statsd-implicit-env into launchpad:master.
Commit message:
StatsdClient: Automatically add env label
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/393792
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.
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:statsd-implicit-env into launchpad:master.
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):
