Merge ~twom/launchpad:stats-5xx-errors into launchpad:master

Proposed by Tom Wardill
Status: Superseded
Proposed branch: ~twom/launchpad:stats-5xx-errors
Merge into: launchpad:master
Diff against target: 414 lines (+149/-54)
11 files modified
lib/lp/buildmaster/manager.py (+5/-2)
lib/lp/buildmaster/tests/test_manager.py (+34/-7)
lib/lp/services/configure.zcml (+1/-0)
lib/lp/services/statsd/__init__.py (+0/-35)
lib/lp/services/statsd/configure.zcml (+17/-0)
lib/lp/services/statsd/interfaces/__init__.py (+0/-0)
lib/lp/services/statsd/interfaces/statsd_client.py (+19/-0)
lib/lp/services/statsd/model/__init__.py (+0/-0)
lib/lp/services/statsd/model/statsd_client.py (+48/-0)
lib/lp/services/statsd/tests/test_statsd_client.py (+19/-10)
lib/lp/services/webapp/publication.py (+6/-0)
Reviewer Review Type Date Requested Status
Launchpad code reviewers Pending
Review via email: mp+390142@code.launchpad.net

This proposal has been superseded by a proposal from 2020-09-02.

Commit message

Add statsd output for 400 and 5XX errors

Description of the change

We already gather these stats into the OpStats and then into LPStats.
Add statsd output for the same things in the same places.

To post a comment you must log in.

Unmerged commits

ea82f25... by Tom Wardill

Add statsd output for 400 and 5XX errors

d10b4d5... by Tom Wardill

Change to stats_client naming

18e5535... by Tom Wardill

Correct import, better test check

66013ef... by Tom Wardill

Increment counter on failed builder

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/lib/lp/buildmaster/manager.py b/lib/lp/buildmaster/manager.py
2index fb9baaa..8f3ff00 100644
3--- a/lib/lp/buildmaster/manager.py
4+++ b/lib/lp/buildmaster/manager.py
5@@ -61,7 +61,7 @@ from lp.services.database.stormexpr import (
6 Values,
7 )
8 from lp.services.propertycache import get_property_cache
9-from lp.services.statsd import get_statsd_client
10+from lp.services.statsd.interfaces.statsd_client import IStatsdClient
11
12
13 BUILDD_MANAGER_LOG_NAME = "slave-scanner"
14@@ -460,6 +460,8 @@ class SlaveScanner:
15 self._cached_build_cookie = None
16 self._cached_build_queue = None
17
18+ self.statsd_client = getUtility(IStatsdClient).getClient()
19+
20 def startCycle(self):
21 """Scan the builder and dispatch to it or deal with failures."""
22 self.loop = LoopingCall(self.singleCycle)
23@@ -529,6 +531,7 @@ class SlaveScanner:
24 if builder.current_build is not None:
25 builder.current_build.gotFailure()
26 recover_failure(self.logger, vitals, builder, retry, failure.value)
27+ self.statsd_client.incr('builders.judged_failed')
28 transaction.commit()
29 except Exception:
30 # Catastrophic code failure! Not much we can do.
31@@ -712,7 +715,7 @@ class BuilddManager(service.Service):
32 self.logger = self._setupLogger()
33 self.current_builders = []
34 self.pending_logtails = {}
35- self.statsd_client = get_statsd_client()
36+ self.statsd_client = getUtility(IStatsdClient).getClient()
37
38 def _setupLogger(self):
39 """Set up a 'slave-scanner' logger that redirects to twisted.
40diff --git a/lib/lp/buildmaster/tests/test_manager.py b/lib/lp/buildmaster/tests/test_manager.py
41index 06b4e2d..e2eb080 100644
42--- a/lib/lp/buildmaster/tests/test_manager.py
43+++ b/lib/lp/buildmaster/tests/test_manager.py
44@@ -13,7 +13,6 @@ import os
45 import signal
46 import time
47
48-from fixtures import MockPatch
49 from six.moves import xmlrpc_client
50 from testtools.matchers import (
51 Equals,
52@@ -76,8 +75,10 @@ from lp.buildmaster.tests.test_interactor import (
53 MockBuilderFactory,
54 )
55 from lp.registry.interfaces.distribution import IDistributionSet
56+from lp.services.compat import mock
57 from lp.services.config import config
58 from lp.services.log.logger import BufferLogger
59+from lp.services.statsd.interfaces.statsd_client import IStatsdClient
60 from lp.soyuz.interfaces.binarypackagebuild import IBinaryPackageBuildSet
61 from lp.soyuz.model.binarypackagebuildbehaviour import (
62 BinaryPackageBuildBehaviour,
63@@ -92,6 +93,7 @@ from lp.testing import (
64 from lp.testing.dbuser import switch_dbuser
65 from lp.testing.factory import LaunchpadObjectFactory
66 from lp.testing.fakemethod import FakeMethod
67+from lp.testing.fixture import ZopeUtilityFixture
68 from lp.testing.layers import (
69 LaunchpadScriptLayer,
70 LaunchpadZopelessLayer,
71@@ -101,7 +103,19 @@ from lp.testing.matchers import HasQueryCount
72 from lp.testing.sampledata import BOB_THE_BUILDER_NAME
73
74
75-class TestSlaveScannerScan(TestCaseWithFactory):
76+class StatsMixin:
77+
78+ def setUpStats(self):
79+ # Mock the utility class, then return a known value
80+ # from getClient(), so we can assert against the call counts and args.
81+ utility_class = mock.Mock()
82+ self.stats_client = mock.Mock()
83+ utility_class.getClient.return_value = self.stats_client
84+ self.useFixture(
85+ ZopeUtilityFixture(utility_class, IStatsdClient))
86+
87+
88+class TestSlaveScannerScan(StatsMixin, TestCaseWithFactory):
89 """Tests `SlaveScanner.scan` method.
90
91 This method uses the old framework for scanning and dispatching builds.
92@@ -123,6 +137,7 @@ class TestSlaveScannerScan(TestCaseWithFactory):
93 self.test_publisher.setUpDefaultDistroSeries(hoary)
94 self.test_publisher.addFakeChroots(db_only=True)
95 self.addCleanup(shut_down_default_process_pool)
96+ self.setUpStats()
97
98 def _resetBuilder(self, builder):
99 """Reset the given builder and its job."""
100@@ -528,6 +543,20 @@ class TestSlaveScannerScan(TestCaseWithFactory):
101 self.assertFalse(builder.builderok)
102
103 @defer.inlineCallbacks
104+ def test_scanFailed_increments_counter(self):
105+ def failing_scan():
106+ return defer.fail(Exception("fake exception"))
107+ scanner = self._getScanner()
108+ scanner.scan = failing_scan
109+ builder = getUtility(IBuilderSet)[scanner.builder_name]
110+ builder.failure_count = BUILDER_FAILURE_THRESHOLD
111+ builder.currentjob.reset()
112+ transaction.commit()
113+
114+ yield scanner.singleCycle()
115+ self.assertEqual(1, self.stats_client.incr.call_count)
116+
117+ @defer.inlineCallbacks
118 def test_fail_to_resume_leaves_it_dirty(self):
119 # If an attempt to resume a slave fails, its failure count is
120 # incremented and it is left DIRTY.
121@@ -893,6 +922,7 @@ class FakeBuilddManager:
122
123 class TestSlaveScannerWithoutDB(TestCase):
124
125+ layer = ZopelessDatabaseLayer
126 run_tests_with = AsynchronousDeferredRunTest
127
128 def setUp(self):
129@@ -1619,17 +1649,14 @@ class TestBuilddManagerScript(TestCaseWithFactory):
130 "Twistd's log file was rotated by twistd.")
131
132
133-class TestStats(TestCaseWithFactory):
134+class TestStats(StatsMixin, TestCaseWithFactory):
135
136 layer = ZopelessDatabaseLayer
137 run_tests_with = AsynchronousDeferredRunTest.make_factory(timeout=20)
138
139 def setUp(self):
140 super(TestStats, self).setUp()
141- self.stats_client = self.useFixture(
142- MockPatch(
143- 'lp.buildmaster.manager.get_statsd_client'
144- )).mock()
145+ self.setUpStats()
146
147 def test_single_processor(self):
148 builder = self.factory.makeBuilder()
149diff --git a/lib/lp/services/configure.zcml b/lib/lp/services/configure.zcml
150index f23faea..bdde7df 100644
151--- a/lib/lp/services/configure.zcml
152+++ b/lib/lp/services/configure.zcml
153@@ -29,6 +29,7 @@
154 <include package=".signing" />
155 <include package=".sitesearch" />
156 <include package=".statistics" />
157+ <include package=".statsd" />
158 <include package=".temporaryblobstorage" />
159 <include package=".verification" />
160 <include package=".webhooks" />
161diff --git a/lib/lp/services/statsd/__init__.py b/lib/lp/services/statsd/__init__.py
162index 22c839f..e69de29 100644
163--- a/lib/lp/services/statsd/__init__.py
164+++ b/lib/lp/services/statsd/__init__.py
165@@ -1,35 +0,0 @@
166-# Copyright 2020 Canonical Ltd. This software is licensed under the
167-# GNU Affero General Public License version 3 (see the file LICENSE).
168-
169-"""Statsd client wrapper with Launchpad configuration"""
170-
171-from __future__ import absolute_import, print_function, unicode_literals
172-
173-__metaclass__ = type
174-__all__ = ['get_statsd_client']
175-
176-
177-from statsd import StatsClient
178-
179-from lp.services.config import config
180-
181-
182-class UnconfiguredStatsdClient:
183- """Dummy client for if statsd is not configured in the environment.
184-
185- This client will be used if the statsd settings are not available to
186- Launchpad. Prevents unnecessary network traffic.
187- """
188-
189- def __getattr__(self, name):
190- return lambda *args, **kwargs: None
191-
192-
193-def get_statsd_client():
194- if config.statsd.host:
195- return StatsClient(
196- host=config.statsd.host,
197- port=config.statsd.port,
198- prefix=config.statsd.prefix)
199- else:
200- return UnconfiguredStatsdClient()
201diff --git a/lib/lp/services/statsd/configure.zcml b/lib/lp/services/statsd/configure.zcml
202new file mode 100644
203index 0000000..6e4b192
204--- /dev/null
205+++ b/lib/lp/services/statsd/configure.zcml
206@@ -0,0 +1,17 @@
207+<!-- Copyright 2015-2020 Canonical Ltd. This software is licensed under the
208+ GNU Affero General Public License version 3 (see the file LICENSE).
209+-->
210+<configure
211+ xmlns="http://namespaces.zope.org/zope"
212+ xmlns:i18n="http://namespaces.zope.org/i18n"
213+ xmlns:lp="http://namespaces.canonical.com/lp"
214+ xmlns:webservice="http://namespaces.canonical.com/webservice"
215+ i18n_domain="launchpad">
216+
217+ <!-- utility, rather than securedutility as we don't want the child
218+ objects (the statsd clients) to be wrapped in the proxy -->
219+ <utility
220+ factory="lp.services.statsd.model.statsd_client.StatsdClient"
221+ provides="lp.services.statsd.interfaces.statsd_client.IStatsdClient">
222+ </utility>
223+</configure>
224diff --git a/lib/lp/services/statsd/interfaces/__init__.py b/lib/lp/services/statsd/interfaces/__init__.py
225new file mode 100644
226index 0000000..e69de29
227--- /dev/null
228+++ b/lib/lp/services/statsd/interfaces/__init__.py
229diff --git a/lib/lp/services/statsd/interfaces/statsd_client.py b/lib/lp/services/statsd/interfaces/statsd_client.py
230new file mode 100644
231index 0000000..992ef34
232--- /dev/null
233+++ b/lib/lp/services/statsd/interfaces/statsd_client.py
234@@ -0,0 +1,19 @@
235+# Copyright 2020 Canonical Ltd. This software is licensed under the
236+# GNU Affero General Public License version 3 (see the file LICENSE).
237+
238+"""Interfaces for configuring and retrieving a statsd client."""
239+
240+from __future__ import absolute_import, print_function, unicode_literals
241+
242+__metaclass__ = type
243+__all__ = ['IStatsdClient']
244+
245+
246+from zope.interface import Interface
247+
248+
249+class IStatsdClient(Interface):
250+ """Methods for retrieving a statsd client using Launchpad config."""
251+
252+ def getClient():
253+ """Return an appropriately configured statsd client."""
254diff --git a/lib/lp/services/statsd/model/__init__.py b/lib/lp/services/statsd/model/__init__.py
255new file mode 100644
256index 0000000..e69de29
257--- /dev/null
258+++ b/lib/lp/services/statsd/model/__init__.py
259diff --git a/lib/lp/services/statsd/model/statsd_client.py b/lib/lp/services/statsd/model/statsd_client.py
260new file mode 100644
261index 0000000..f91def1
262--- /dev/null
263+++ b/lib/lp/services/statsd/model/statsd_client.py
264@@ -0,0 +1,48 @@
265+# Copyright 2020 Canonical Ltd. This software is licensed under the
266+# GNU Affero General Public License version 3 (see the file LICENSE).
267+
268+"""Statsd client wrapper with Launchpad configuration"""
269+
270+from __future__ import absolute_import, print_function, unicode_literals
271+
272+__metaclass__ = type
273+__all__ = ['StatsdClient']
274+
275+
276+from statsd import StatsClient
277+from zope.interface import implementer
278+
279+from lp.services.config import config
280+from lp.services.statsd.interfaces.statsd_client import IStatsdClient
281+
282+
283+client = None
284+
285+
286+class UnconfiguredStatsdClient:
287+ """Dummy client for if statsd is not configured in the environment.
288+
289+ This client will be used if the statsd settings are not available to
290+ Launchpad. Prevents unnecessary network traffic.
291+ """
292+
293+ def __getattr__(self, name):
294+ return lambda *args, **kwargs: None
295+
296+
297+@implementer(IStatsdClient)
298+class StatsdClient:
299+ """See `IStatsdClient`."""
300+
301+ client = None
302+
303+ def getClient(self):
304+ if not self.client:
305+ if config.statsd.host:
306+ self.client = StatsClient(
307+ host=config.statsd.host,
308+ port=config.statsd.port,
309+ prefix=config.statsd.prefix)
310+ else:
311+ self.client = UnconfiguredStatsdClient()
312+ return self.client
313diff --git a/lib/lp/services/statsd/tests/test_lp_statsd.py b/lib/lp/services/statsd/tests/test_statsd_client.py
314similarity index 58%
315rename from lib/lp/services/statsd/tests/test_lp_statsd.py
316rename to lib/lp/services/statsd/tests/test_statsd_client.py
317index 691e93d..d9fd777 100644
318--- a/lib/lp/services/statsd/tests/test_lp_statsd.py
319+++ b/lib/lp/services/statsd/tests/test_statsd_client.py
320@@ -8,33 +8,42 @@ from __future__ import absolute_import, print_function, unicode_literals
321 __metaclass__ = type
322
323 from statsd import StatsClient
324+from zope.component import getUtility
325+from zope.security.proxy import removeSecurityProxy
326
327 from lp.services.config import config
328-from lp.services.statsd import (
329- get_statsd_client,
330+from lp.services.statsd.interfaces.statsd_client import IStatsdClient
331+from lp.services.statsd.model.statsd_client import (
332+ StatsdClient,
333 UnconfiguredStatsdClient,
334 )
335 from lp.testing import TestCase
336-from lp.testing.layers import BaseLayer
337+from lp.testing.layers import ZopelessLayer
338
339
340 class TestClientConfiguration(TestCase):
341
342- layer = BaseLayer
343+ layer = ZopelessLayer
344+
345+ def test_accessible_via_utility(self):
346+ """Test that we can access the class via a zope utility."""
347+ config.push(
348+ 'statsd_test',
349+ "[statsd]\nhost: 127.0.01\nport: 9999\nprefix: test\n")
350+ client = getUtility(IStatsdClient).getClient()
351+ self.assertIsInstance(client, StatsClient)
352
353 def test_get_correct_instance_unconfigured(self):
354 """Test that we get the correct client, depending on config values."""
355 config.push(
356 'statsd_test',
357 "[statsd]\nhost:")
358- client = get_statsd_client()
359- self.assertEqual(
360- type(client), UnconfiguredStatsdClient)
361+ client = StatsdClient().getClient()
362+ self.assertIsInstance(client, UnconfiguredStatsdClient)
363
364 def test_get_correct_instance_configured(self):
365 config.push(
366 'statsd_test',
367 "[statsd]\nhost: 127.0.01\nport: 9999\nprefix: test\n")
368- client = get_statsd_client()
369- self.assertEqual(
370- type(client), StatsClient)
371+ client = StatsdClient().getClient()
372+ self.assertIsInstance(client, StatsClient)
373diff --git a/lib/lp/services/webapp/publication.py b/lib/lp/services/webapp/publication.py
374index d0ad83e..ec6983b 100644
375--- a/lib/lp/services/webapp/publication.py
376+++ b/lib/lp/services/webapp/publication.py
377@@ -73,6 +73,7 @@ from lp.services.database.policy import LaunchpadDatabasePolicy
378 from lp.services.features.flags import NullFeatureController
379 from lp.services.oauth.interfaces import IOAuthSignedRequest
380 from lp.services.osutils import open_for_writing
381+from lp.services.statsd.interfaces.statsd_client import IStatsdClient
382 import lp.services.webapp.adapter as da
383 from lp.services.webapp.interfaces import (
384 FinishReadOnlyRequestEvent,
385@@ -727,6 +728,7 @@ class LaunchpadBrowserPublication(
386 da.clear_request_started()
387
388 getUtility(IOpenLaunchBag).clear()
389+ statsd_client = getUtility(IStatsdClient).getClient()
390
391 # Maintain operational statistics.
392 if getattr(request, '_wants_retry', False):
393@@ -744,10 +746,13 @@ class LaunchpadBrowserPublication(
394 status = request.response.getStatus()
395 if status == 404: # Not Found
396 OpStats.stats['404s'] += 1
397+ statsd_client.incr('errors.404')
398 elif status == 500: # Unhandled exceptions
399 OpStats.stats['500s'] += 1
400+ statsd_client.incr('errors.500')
401 elif status == 503: # Timeouts
402 OpStats.stats['503s'] += 1
403+ statsd_client.incr('errors.503')
404
405 # Increment counters for status code groups.
406 status_group = str(status)[0] + 'XXs'
407@@ -756,6 +761,7 @@ class LaunchpadBrowserPublication(
408 # Increment counter for 5XXs_b.
409 if is_browser(request) and status_group == '5XXs':
410 OpStats.stats['5XXs_b'] += 1
411+ statsd_client.incr('errors.5XX')
412
413 # Make sure our databases are in a sane state for the next request.
414 thread_name = threading.current_thread().name

Subscribers

People subscribed via source and target branches

to status/vote changes: