launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #25242
[Merge] ~twom/launchpad:stats-5xx-errors into launchpad:master
Tom Wardill has proposed merging ~twom/launchpad:stats-5xx-errors into launchpad:master.
Commit message:
Add statsd output for 400 and 5XX errors
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~twom/launchpad/+git/launchpad/+merge/390142
We already gather these stats into the OpStats and then into LPStats.
Add statsd output for the same things in the same places.
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~twom/launchpad:stats-5xx-errors into launchpad:master.
diff --git a/lib/lp/buildmaster/manager.py b/lib/lp/buildmaster/manager.py
index fb9baaa..8f3ff00 100644
--- a/lib/lp/buildmaster/manager.py
+++ b/lib/lp/buildmaster/manager.py
@@ -61,7 +61,7 @@ from lp.services.database.stormexpr import (
Values,
)
from lp.services.propertycache import get_property_cache
-from lp.services.statsd import get_statsd_client
+from lp.services.statsd.interfaces.statsd_client import IStatsdClient
BUILDD_MANAGER_LOG_NAME = "slave-scanner"
@@ -460,6 +460,8 @@ class SlaveScanner:
self._cached_build_cookie = None
self._cached_build_queue = None
+ self.statsd_client = getUtility(IStatsdClient).getClient()
+
def startCycle(self):
"""Scan the builder and dispatch to it or deal with failures."""
self.loop = LoopingCall(self.singleCycle)
@@ -529,6 +531,7 @@ class SlaveScanner:
if builder.current_build is not None:
builder.current_build.gotFailure()
recover_failure(self.logger, vitals, builder, retry, failure.value)
+ self.statsd_client.incr('builders.judged_failed')
transaction.commit()
except Exception:
# Catastrophic code failure! Not much we can do.
@@ -712,7 +715,7 @@ class BuilddManager(service.Service):
self.logger = self._setupLogger()
self.current_builders = []
self.pending_logtails = {}
- self.statsd_client = get_statsd_client()
+ self.statsd_client = getUtility(IStatsdClient).getClient()
def _setupLogger(self):
"""Set up a 'slave-scanner' logger that redirects to twisted.
diff --git a/lib/lp/buildmaster/tests/test_manager.py b/lib/lp/buildmaster/tests/test_manager.py
index 06b4e2d..e2eb080 100644
--- a/lib/lp/buildmaster/tests/test_manager.py
+++ b/lib/lp/buildmaster/tests/test_manager.py
@@ -13,7 +13,6 @@ import os
import signal
import time
-from fixtures import MockPatch
from six.moves import xmlrpc_client
from testtools.matchers import (
Equals,
@@ -76,8 +75,10 @@ from lp.buildmaster.tests.test_interactor import (
MockBuilderFactory,
)
from lp.registry.interfaces.distribution import IDistributionSet
+from lp.services.compat import mock
from lp.services.config import config
from lp.services.log.logger import BufferLogger
+from lp.services.statsd.interfaces.statsd_client import IStatsdClient
from lp.soyuz.interfaces.binarypackagebuild import IBinaryPackageBuildSet
from lp.soyuz.model.binarypackagebuildbehaviour import (
BinaryPackageBuildBehaviour,
@@ -92,6 +93,7 @@ from lp.testing import (
from lp.testing.dbuser import switch_dbuser
from lp.testing.factory import LaunchpadObjectFactory
from lp.testing.fakemethod import FakeMethod
+from lp.testing.fixture import ZopeUtilityFixture
from lp.testing.layers import (
LaunchpadScriptLayer,
LaunchpadZopelessLayer,
@@ -101,7 +103,19 @@ from lp.testing.matchers import HasQueryCount
from lp.testing.sampledata import BOB_THE_BUILDER_NAME
-class TestSlaveScannerScan(TestCaseWithFactory):
+class StatsMixin:
+
+ def setUpStats(self):
+ # Mock the utility class, then return a known value
+ # from getClient(), so we can assert against the call counts and args.
+ utility_class = mock.Mock()
+ self.stats_client = mock.Mock()
+ utility_class.getClient.return_value = self.stats_client
+ self.useFixture(
+ ZopeUtilityFixture(utility_class, IStatsdClient))
+
+
+class TestSlaveScannerScan(StatsMixin, TestCaseWithFactory):
"""Tests `SlaveScanner.scan` method.
This method uses the old framework for scanning and dispatching builds.
@@ -123,6 +137,7 @@ class TestSlaveScannerScan(TestCaseWithFactory):
self.test_publisher.setUpDefaultDistroSeries(hoary)
self.test_publisher.addFakeChroots(db_only=True)
self.addCleanup(shut_down_default_process_pool)
+ self.setUpStats()
def _resetBuilder(self, builder):
"""Reset the given builder and its job."""
@@ -528,6 +543,20 @@ class TestSlaveScannerScan(TestCaseWithFactory):
self.assertFalse(builder.builderok)
@defer.inlineCallbacks
+ def test_scanFailed_increments_counter(self):
+ def failing_scan():
+ return defer.fail(Exception("fake exception"))
+ scanner = self._getScanner()
+ scanner.scan = failing_scan
+ builder = getUtility(IBuilderSet)[scanner.builder_name]
+ builder.failure_count = BUILDER_FAILURE_THRESHOLD
+ builder.currentjob.reset()
+ transaction.commit()
+
+ yield scanner.singleCycle()
+ self.assertEqual(1, self.stats_client.incr.call_count)
+
+ @defer.inlineCallbacks
def test_fail_to_resume_leaves_it_dirty(self):
# If an attempt to resume a slave fails, its failure count is
# incremented and it is left DIRTY.
@@ -893,6 +922,7 @@ class FakeBuilddManager:
class TestSlaveScannerWithoutDB(TestCase):
+ layer = ZopelessDatabaseLayer
run_tests_with = AsynchronousDeferredRunTest
def setUp(self):
@@ -1619,17 +1649,14 @@ class TestBuilddManagerScript(TestCaseWithFactory):
"Twistd's log file was rotated by twistd.")
-class TestStats(TestCaseWithFactory):
+class TestStats(StatsMixin, TestCaseWithFactory):
layer = ZopelessDatabaseLayer
run_tests_with = AsynchronousDeferredRunTest.make_factory(timeout=20)
def setUp(self):
super(TestStats, self).setUp()
- self.stats_client = self.useFixture(
- MockPatch(
- 'lp.buildmaster.manager.get_statsd_client'
- )).mock()
+ self.setUpStats()
def test_single_processor(self):
builder = self.factory.makeBuilder()
diff --git a/lib/lp/services/configure.zcml b/lib/lp/services/configure.zcml
index f23faea..bdde7df 100644
--- a/lib/lp/services/configure.zcml
+++ b/lib/lp/services/configure.zcml
@@ -29,6 +29,7 @@
<include package=".signing" />
<include package=".sitesearch" />
<include package=".statistics" />
+ <include package=".statsd" />
<include package=".temporaryblobstorage" />
<include package=".verification" />
<include package=".webhooks" />
diff --git a/lib/lp/services/statsd/__init__.py b/lib/lp/services/statsd/__init__.py
index 22c839f..e69de29 100644
--- a/lib/lp/services/statsd/__init__.py
+++ b/lib/lp/services/statsd/__init__.py
@@ -1,35 +0,0 @@
-# Copyright 2020 Canonical Ltd. This software is licensed under the
-# GNU Affero General Public License version 3 (see the file LICENSE).
-
-"""Statsd client wrapper with Launchpad configuration"""
-
-from __future__ import absolute_import, print_function, unicode_literals
-
-__metaclass__ = type
-__all__ = ['get_statsd_client']
-
-
-from statsd import StatsClient
-
-from lp.services.config import config
-
-
-class UnconfiguredStatsdClient:
- """Dummy client for if statsd is not configured in the environment.
-
- This client will be used if the statsd settings are not available to
- Launchpad. Prevents unnecessary network traffic.
- """
-
- def __getattr__(self, name):
- return lambda *args, **kwargs: None
-
-
-def get_statsd_client():
- if config.statsd.host:
- return StatsClient(
- host=config.statsd.host,
- port=config.statsd.port,
- prefix=config.statsd.prefix)
- else:
- return UnconfiguredStatsdClient()
diff --git a/lib/lp/services/statsd/configure.zcml b/lib/lp/services/statsd/configure.zcml
new file mode 100644
index 0000000..6e4b192
--- /dev/null
+++ b/lib/lp/services/statsd/configure.zcml
@@ -0,0 +1,17 @@
+<!-- Copyright 2015-2020 Canonical Ltd. This software is licensed under the
+ GNU Affero General Public License version 3 (see the file LICENSE).
+-->
+<configure
+ xmlns="http://namespaces.zope.org/zope";
+ xmlns:i18n="http://namespaces.zope.org/i18n";
+ xmlns:lp="http://namespaces.canonical.com/lp";
+ xmlns:webservice="http://namespaces.canonical.com/webservice";
+ i18n_domain="launchpad">
+
+ <!-- utility, rather than securedutility as we don't want the child
+ objects (the statsd clients) to be wrapped in the proxy -->
+ <utility
+ factory="lp.services.statsd.model.statsd_client.StatsdClient"
+ provides="lp.services.statsd.interfaces.statsd_client.IStatsdClient">
+ </utility>
+</configure>
diff --git a/lib/lp/services/statsd/interfaces/__init__.py b/lib/lp/services/statsd/interfaces/__init__.py
new file mode 100644
index 0000000..e69de29
--- /dev/null
+++ b/lib/lp/services/statsd/interfaces/__init__.py
diff --git a/lib/lp/services/statsd/interfaces/statsd_client.py b/lib/lp/services/statsd/interfaces/statsd_client.py
new file mode 100644
index 0000000..992ef34
--- /dev/null
+++ b/lib/lp/services/statsd/interfaces/statsd_client.py
@@ -0,0 +1,19 @@
+# Copyright 2020 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Interfaces for configuring and retrieving a statsd client."""
+
+from __future__ import absolute_import, print_function, unicode_literals
+
+__metaclass__ = type
+__all__ = ['IStatsdClient']
+
+
+from zope.interface import Interface
+
+
+class IStatsdClient(Interface):
+ """Methods for retrieving a statsd client using Launchpad config."""
+
+ def getClient():
+ """Return an appropriately configured statsd client."""
diff --git a/lib/lp/services/statsd/model/__init__.py b/lib/lp/services/statsd/model/__init__.py
new file mode 100644
index 0000000..e69de29
--- /dev/null
+++ b/lib/lp/services/statsd/model/__init__.py
diff --git a/lib/lp/services/statsd/model/statsd_client.py b/lib/lp/services/statsd/model/statsd_client.py
new file mode 100644
index 0000000..f91def1
--- /dev/null
+++ b/lib/lp/services/statsd/model/statsd_client.py
@@ -0,0 +1,48 @@
+# Copyright 2020 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Statsd client wrapper with Launchpad configuration"""
+
+from __future__ import absolute_import, print_function, unicode_literals
+
+__metaclass__ = type
+__all__ = ['StatsdClient']
+
+
+from statsd import StatsClient
+from zope.interface import implementer
+
+from lp.services.config import config
+from lp.services.statsd.interfaces.statsd_client import IStatsdClient
+
+
+client = None
+
+
+class UnconfiguredStatsdClient:
+ """Dummy client for if statsd is not configured in the environment.
+
+ This client will be used if the statsd settings are not available to
+ Launchpad. Prevents unnecessary network traffic.
+ """
+
+ def __getattr__(self, name):
+ return lambda *args, **kwargs: None
+
+
+@implementer(IStatsdClient)
+class StatsdClient:
+ """See `IStatsdClient`."""
+
+ client = None
+
+ def getClient(self):
+ if not self.client:
+ if config.statsd.host:
+ self.client = StatsClient(
+ host=config.statsd.host,
+ port=config.statsd.port,
+ prefix=config.statsd.prefix)
+ else:
+ self.client = UnconfiguredStatsdClient()
+ return self.client
diff --git a/lib/lp/services/statsd/tests/test_lp_statsd.py b/lib/lp/services/statsd/tests/test_statsd_client.py
similarity index 58%
rename from lib/lp/services/statsd/tests/test_lp_statsd.py
rename to lib/lp/services/statsd/tests/test_statsd_client.py
index 691e93d..d9fd777 100644
--- a/lib/lp/services/statsd/tests/test_lp_statsd.py
+++ b/lib/lp/services/statsd/tests/test_statsd_client.py
@@ -8,33 +8,42 @@ from __future__ import absolute_import, print_function, unicode_literals
__metaclass__ = type
from statsd import StatsClient
+from zope.component import getUtility
+from zope.security.proxy import removeSecurityProxy
from lp.services.config import config
-from lp.services.statsd import (
- get_statsd_client,
+from lp.services.statsd.interfaces.statsd_client import IStatsdClient
+from lp.services.statsd.model.statsd_client import (
+ StatsdClient,
UnconfiguredStatsdClient,
)
from lp.testing import TestCase
-from lp.testing.layers import BaseLayer
+from lp.testing.layers import ZopelessLayer
class TestClientConfiguration(TestCase):
- layer = BaseLayer
+ layer = ZopelessLayer
+
+ def test_accessible_via_utility(self):
+ """Test that we can access the class via a zope utility."""
+ config.push(
+ 'statsd_test',
+ "[statsd]\nhost: 127.0.01\nport: 9999\nprefix: test\n")
+ client = getUtility(IStatsdClient).getClient()
+ self.assertIsInstance(client, StatsClient)
def test_get_correct_instance_unconfigured(self):
"""Test that we get the correct client, depending on config values."""
config.push(
'statsd_test',
"[statsd]\nhost:")
- client = get_statsd_client()
- self.assertEqual(
- type(client), UnconfiguredStatsdClient)
+ client = StatsdClient().getClient()
+ self.assertIsInstance(client, UnconfiguredStatsdClient)
def test_get_correct_instance_configured(self):
config.push(
'statsd_test',
"[statsd]\nhost: 127.0.01\nport: 9999\nprefix: test\n")
- client = get_statsd_client()
- self.assertEqual(
- type(client), StatsClient)
+ client = StatsdClient().getClient()
+ self.assertIsInstance(client, StatsClient)
diff --git a/lib/lp/services/webapp/publication.py b/lib/lp/services/webapp/publication.py
index d0ad83e..ec6983b 100644
--- a/lib/lp/services/webapp/publication.py
+++ b/lib/lp/services/webapp/publication.py
@@ -73,6 +73,7 @@ from lp.services.database.policy import LaunchpadDatabasePolicy
from lp.services.features.flags import NullFeatureController
from lp.services.oauth.interfaces import IOAuthSignedRequest
from lp.services.osutils import open_for_writing
+from lp.services.statsd.interfaces.statsd_client import IStatsdClient
import lp.services.webapp.adapter as da
from lp.services.webapp.interfaces import (
FinishReadOnlyRequestEvent,
@@ -727,6 +728,7 @@ class LaunchpadBrowserPublication(
da.clear_request_started()
getUtility(IOpenLaunchBag).clear()
+ statsd_client = getUtility(IStatsdClient).getClient()
# Maintain operational statistics.
if getattr(request, '_wants_retry', False):
@@ -744,10 +746,13 @@ class LaunchpadBrowserPublication(
status = request.response.getStatus()
if status == 404: # Not Found
OpStats.stats['404s'] += 1
+ statsd_client.incr('errors.404')
elif status == 500: # Unhandled exceptions
OpStats.stats['500s'] += 1
+ statsd_client.incr('errors.500')
elif status == 503: # Timeouts
OpStats.stats['503s'] += 1
+ statsd_client.incr('errors.503')
# Increment counters for status code groups.
status_group = str(status)[0] + 'XXs'
@@ -756,6 +761,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')
# Make sure our databases are in a sane state for the next request.
thread_name = threading.current_thread().name
Follow ups
