Merge ~cjwatson/launchpad:statsd-implicit-env into launchpad:master
- Git
- lp:~cjwatson/launchpad
- statsd-implicit-env
- Merge into master
Proposed by
Colin Watson
Status: | Merged |
---|---|
Approved by: | Colin Watson |
Approved revision: | dcb4e0d4e56b25935379a55894dc5a37dd00ace3 |
Merge reported by: | Otto Co-Pilot |
Merged at revision: | not available |
Proposed branch: | ~cjwatson/launchpad:statsd-implicit-env |
Merge into: | launchpad:master |
Diff against target: |
264 lines (+39/-63) 9 files modified
lib/lp/buildmaster/manager.py (+3/-6) lib/lp/buildmaster/model/buildfarmjobbehaviour.py (+2/-3) lib/lp/services/job/runner.py (+4/-9) lib/lp/services/statsd/model/statsd_client.py (+10/-7) lib/lp/services/statsd/numbercruncher.py (+5/-12) lib/lp/services/statsd/tests/__init__.py (+6/-3) lib/lp/services/statsd/tests/test_statsd_client.py (+0/-10) lib/lp/services/webapp/publication.py (+4/-8) lib/lp/services/webapp/tests/test_publication.py (+5/-5) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Tom Wardill (community) | Approve | ||
Review via email: mp+393792@code.launchpad.net |
Commit message
StatsdClient: Automatically add env label
Description of the change
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.
To post a comment you must log in.
Revision history for this message
Tom Wardill (twom) : | # |
review:
Approve
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | diff --git a/lib/lp/buildmaster/manager.py b/lib/lp/buildmaster/manager.py | |||
2 | index e9732ea..7bbe4aa 100644 | |||
3 | --- a/lib/lp/buildmaster/manager.py | |||
4 | +++ b/lib/lp/buildmaster/manager.py | |||
5 | @@ -532,13 +532,10 @@ class SlaveScanner: | |||
6 | 532 | if builder.current_build is not None: | 532 | if builder.current_build is not None: |
7 | 533 | builder.current_build.gotFailure() | 533 | builder.current_build.gotFailure() |
8 | 534 | self.statsd_client.incr( | 534 | self.statsd_client.incr( |
12 | 535 | 'builders.judged_failed,build=True,arch={},env={}'.format( | 535 | 'builders.judged_failed,build=True,arch={}'.format( |
13 | 536 | builder.current_build.processor.name, | 536 | builder.current_build.processor.name)) |
11 | 537 | self.statsd_client.lp_environment)) | ||
14 | 538 | else: | 537 | else: |
18 | 539 | self.statsd_client.incr( | 538 | self.statsd_client.incr('builders.judged_failed,build=False') |
16 | 540 | 'builders.judged_failed,build=False,env={}'.format( | ||
17 | 541 | self.statsd_client.lp_environment)) | ||
19 | 542 | recover_failure(self.logger, vitals, builder, retry, failure.value) | 539 | recover_failure(self.logger, vitals, builder, retry, failure.value) |
20 | 543 | transaction.commit() | 540 | transaction.commit() |
21 | 544 | except Exception: | 541 | except Exception: |
22 | diff --git a/lib/lp/buildmaster/model/buildfarmjobbehaviour.py b/lib/lp/buildmaster/model/buildfarmjobbehaviour.py | |||
23 | index 88ede6a..13a38d7 100644 | |||
24 | --- a/lib/lp/buildmaster/model/buildfarmjobbehaviour.py | |||
25 | +++ b/lib/lp/buildmaster/model/buildfarmjobbehaviour.py | |||
26 | @@ -158,10 +158,9 @@ class BuildFarmJobBehaviourBase: | |||
27 | 158 | job_type_name = job_type.name if job_type else 'UNKNOWN' | 158 | job_type_name = job_type.name if job_type else 'UNKNOWN' |
28 | 159 | statsd_client = getUtility(IStatsdClient) | 159 | statsd_client = getUtility(IStatsdClient) |
29 | 160 | statsd_client.incr( | 160 | statsd_client.incr( |
31 | 161 | 'build.count,job_type={},builder_name={},env={}'.format( | 161 | 'build.count,job_type={},builder_name={}'.format( |
32 | 162 | job_type_name, | 162 | job_type_name, |
35 | 163 | self._builder.name, | 163 | self._builder.name)) |
34 | 164 | statsd_client.lp_environment)) | ||
36 | 165 | 164 | ||
37 | 166 | logger.info( | 165 | logger.info( |
38 | 167 | "Job %s (%s) started on %s: %s %s" | 166 | "Job %s (%s) started on %s: %s %s" |
39 | diff --git a/lib/lp/services/job/runner.py b/lib/lp/services/job/runner.py | |||
40 | index b0b65a2..15ea4e9 100644 | |||
41 | --- a/lib/lp/services/job/runner.py | |||
42 | +++ b/lib/lp/services/job/runner.py | |||
43 | @@ -326,25 +326,20 @@ class BaseRunnableJob(BaseRunnableJobSource): | |||
44 | 326 | """See `IJob`.""" | 326 | """See `IJob`.""" |
45 | 327 | self.job.start(manage_transaction=manage_transaction) | 327 | self.job.start(manage_transaction=manage_transaction) |
46 | 328 | statsd = getUtility(IStatsdClient) | 328 | statsd = getUtility(IStatsdClient) |
50 | 329 | statsd.incr('job.start_count,type={},env={}'.format( | 329 | statsd.incr('job.start_count,type={}'.format(self.__class__.__name__)) |
48 | 330 | self.__class__.__name__, | ||
49 | 331 | statsd.lp_environment)) | ||
51 | 332 | 330 | ||
52 | 333 | def complete(self, manage_transaction=False): | 331 | def complete(self, manage_transaction=False): |
53 | 334 | """See `IJob`.""" | 332 | """See `IJob`.""" |
54 | 335 | self.job.complete(manage_transaction=manage_transaction) | 333 | self.job.complete(manage_transaction=manage_transaction) |
55 | 336 | statsd = getUtility(IStatsdClient) | 334 | statsd = getUtility(IStatsdClient) |
59 | 337 | statsd.incr('job.complete_count,type={},env={}'.format( | 335 | statsd.incr('job.complete_count,type={}'.format( |
60 | 338 | self.__class__.__name__, | 336 | self.__class__.__name__)) |
58 | 339 | statsd.lp_environment)) | ||
61 | 340 | 337 | ||
62 | 341 | def fail(self, manage_transaction=False): | 338 | def fail(self, manage_transaction=False): |
63 | 342 | """See `IJob`.""" | 339 | """See `IJob`.""" |
64 | 343 | self.job.fail(manage_transaction=manage_transaction) | 340 | self.job.fail(manage_transaction=manage_transaction) |
65 | 344 | statsd = getUtility(IStatsdClient) | 341 | statsd = getUtility(IStatsdClient) |
69 | 345 | statsd.incr('job.fail_count,type={},env={}'.format( | 342 | statsd.incr('job.fail_count,type={}'.format(self.__class__.__name__)) |
67 | 346 | self.__class__.__name__, | ||
68 | 347 | statsd.lp_environment)) | ||
70 | 348 | 343 | ||
71 | 349 | 344 | ||
72 | 350 | class BaseJobRunner(LazrJobRunner): | 345 | class BaseJobRunner(LazrJobRunner): |
73 | diff --git a/lib/lp/services/statsd/model/statsd_client.py b/lib/lp/services/statsd/model/statsd_client.py | |||
74 | index 6ccf027..7d6c3c1 100644 | |||
75 | --- a/lib/lp/services/statsd/model/statsd_client.py | |||
76 | +++ b/lib/lp/services/statsd/model/statsd_client.py | |||
77 | @@ -43,14 +43,17 @@ class StatsdClient: | |||
78 | 43 | self._make_client() | 43 | self._make_client() |
79 | 44 | 44 | ||
80 | 45 | def __getattr__(self, name): | 45 | def __getattr__(self, name): |
81 | 46 | # This is a convenience to keep all statsd client related | ||
82 | 47 | # items on the client. Having this separate from prefix | ||
83 | 48 | # allows us to use it as a label, rather than as part of the | ||
84 | 49 | # gauge name | ||
85 | 50 | if name == 'lp_environment': | ||
86 | 51 | return config.statsd.environment | ||
87 | 52 | if self._client is not None: | 46 | if self._client is not None: |
89 | 53 | return getattr(self._client, name) | 47 | wrapped = getattr(self._client, name) |
90 | 48 | if name in ("timer", "timing", "incr", "decr", "gauge", "set"): | ||
91 | 49 | def wrapper(stat, *args, **kwargs): | ||
92 | 50 | return wrapped( | ||
93 | 51 | "%s,env=%s" % (stat, config.statsd.environment), | ||
94 | 52 | *args, **kwargs) | ||
95 | 53 | |||
96 | 54 | return wrapper | ||
97 | 55 | else: | ||
98 | 56 | return wrapped | ||
99 | 54 | else: | 57 | else: |
100 | 55 | # Prevent unnecessary network traffic if this Launchpad instance | 58 | # Prevent unnecessary network traffic if this Launchpad instance |
101 | 56 | # has no statsd configuration. | 59 | # has no statsd configuration. |
102 | diff --git a/lib/lp/services/statsd/numbercruncher.py b/lib/lp/services/statsd/numbercruncher.py | |||
103 | index 27c61a4..3f1a6e1 100644 | |||
104 | --- a/lib/lp/services/statsd/numbercruncher.py | |||
105 | +++ b/lib/lp/services/statsd/numbercruncher.py | |||
106 | @@ -88,8 +88,7 @@ class NumberCruncher(service.Service): | |||
107 | 88 | virt = queue_type == 'virt' | 88 | virt = queue_type == 'virt' |
108 | 89 | for arch, value in contents.items(): | 89 | for arch, value in contents.items(): |
109 | 90 | gauge_name = ( | 90 | gauge_name = ( |
112 | 91 | "buildqueue,virtualized={},arch={},env={}".format( | 91 | "buildqueue,virtualized={},arch={}".format(virt, arch)) |
111 | 92 | virt, arch, self.statsd_client.lp_environment)) | ||
113 | 93 | self._sendGauge(gauge_name, value[0]) | 92 | self._sendGauge(gauge_name, value[0]) |
114 | 94 | self.logger.debug("Build queue stats update complete.") | 93 | self.logger.debug("Build queue stats update complete.") |
115 | 95 | except Exception: | 94 | except Exception: |
116 | @@ -122,8 +121,8 @@ class NumberCruncher(service.Service): | |||
117 | 122 | counts['idle'] += 1 | 121 | counts['idle'] += 1 |
118 | 123 | for processor, counts in counts_by_processor.items(): | 122 | for processor, counts in counts_by_processor.items(): |
119 | 124 | for count_name, count_value in counts.items(): | 123 | for count_name, count_value in counts.items(): |
122 | 125 | gauge_name = "builders,status={},arch={},env={}".format( | 124 | gauge_name = "builders,status={},arch={}".format( |
123 | 126 | count_name, processor, self.statsd_client.lp_environment) | 125 | count_name, processor) |
124 | 127 | self._sendGauge(gauge_name, count_value) | 126 | self._sendGauge(gauge_name, count_value) |
125 | 128 | self.logger.debug("Builder stats update complete.") | 127 | self.logger.debug("Builder stats update complete.") |
126 | 129 | 128 | ||
127 | @@ -149,14 +148,8 @@ class NumberCruncher(service.Service): | |||
128 | 149 | store = IStore(LibraryFileContent) | 148 | store = IStore(LibraryFileContent) |
129 | 150 | total_files, total_filesize = store.find( | 149 | total_files, total_filesize = store.find( |
130 | 151 | (Count(), Sum(LibraryFileContent.filesize))).one() | 150 | (Count(), Sum(LibraryFileContent.filesize))).one() |
139 | 152 | self._sendGauge( | 151 | self._sendGauge("librarian.total_files", total_files) |
140 | 153 | "librarian.total_files,env={}".format( | 152 | self._sendGauge("librarian.total_filesize", total_filesize) |
133 | 154 | self.statsd_client.lp_environment), | ||
134 | 155 | total_files) | ||
135 | 156 | self._sendGauge( | ||
136 | 157 | "librarian.total_filesize,env={}".format( | ||
137 | 158 | self.statsd_client.lp_environment), | ||
138 | 159 | total_filesize) | ||
141 | 160 | self.logger.debug("Librarian stats update complete.") | 153 | self.logger.debug("Librarian stats update complete.") |
142 | 161 | except Exception: | 154 | except Exception: |
143 | 162 | self.logger.exception("Failure while updating librarian stats:") | 155 | self.logger.exception("Failure while updating librarian stats:") |
144 | diff --git a/lib/lp/services/statsd/tests/__init__.py b/lib/lp/services/statsd/tests/__init__.py | |||
145 | index 23fb067..c9c9ef5 100644 | |||
146 | --- a/lib/lp/services/statsd/tests/__init__.py | |||
147 | +++ b/lib/lp/services/statsd/tests/__init__.py | |||
148 | @@ -8,9 +8,11 @@ from __future__ import absolute_import, print_function, unicode_literals | |||
149 | 8 | __metaclass__ = type | 8 | __metaclass__ = type |
150 | 9 | __all__ = ['StatsMixin'] | 9 | __all__ = ['StatsMixin'] |
151 | 10 | 10 | ||
152 | 11 | from fixtures import MockPatchObject | ||
153 | 12 | from zope.component import getUtility | ||
154 | 13 | |||
155 | 11 | from lp.services.compat import mock | 14 | from lp.services.compat import mock |
156 | 12 | from lp.services.statsd.interfaces.statsd_client import IStatsdClient | 15 | from lp.services.statsd.interfaces.statsd_client import IStatsdClient |
157 | 13 | from lp.testing.fixture import ZopeUtilityFixture | ||
158 | 14 | 16 | ||
159 | 15 | 17 | ||
160 | 16 | class StatsMixin: | 18 | class StatsMixin: |
161 | @@ -18,7 +20,8 @@ class StatsMixin: | |||
162 | 18 | def setUpStats(self): | 20 | def setUpStats(self): |
163 | 19 | # Install a mock statsd client so we can assert against the call | 21 | # Install a mock statsd client so we can assert against the call |
164 | 20 | # counts and args. | 22 | # counts and args. |
165 | 23 | self.pushConfig("statsd", environment="test") | ||
166 | 24 | statsd_client = getUtility(IStatsdClient) | ||
167 | 21 | self.stats_client = mock.Mock() | 25 | self.stats_client = mock.Mock() |
168 | 22 | self.stats_client.lp_environment = "test" | ||
169 | 23 | self.useFixture( | 26 | self.useFixture( |
171 | 24 | ZopeUtilityFixture(self.stats_client, IStatsdClient)) | 27 | MockPatchObject(statsd_client, "_client", self.stats_client)) |
172 | diff --git a/lib/lp/services/statsd/tests/test_statsd_client.py b/lib/lp/services/statsd/tests/test_statsd_client.py | |||
173 | index 2e1a8e1..709b41c 100644 | |||
174 | --- a/lib/lp/services/statsd/tests/test_statsd_client.py | |||
175 | +++ b/lib/lp/services/statsd/tests/test_statsd_client.py | |||
176 | @@ -50,13 +50,3 @@ class TestClientConfiguration(TestCase): | |||
177 | 50 | "port: 9999\nprefix: test\nenvironment: test\n") | 50 | "port: 9999\nprefix: test\nenvironment: test\n") |
178 | 51 | client.reload() | 51 | client.reload() |
179 | 52 | self.assertIsInstance(client._client, StatsClient) | 52 | self.assertIsInstance(client._client, StatsClient) |
180 | 53 | |||
181 | 54 | def test_lp_environment_is_available(self): | ||
182 | 55 | client = getUtility(IStatsdClient) | ||
183 | 56 | self.addCleanup(client.reload) | ||
184 | 57 | config.push( | ||
185 | 58 | 'statsd_test', | ||
186 | 59 | "[statsd]\nhost: 127.0.01\n" | ||
187 | 60 | "port: 9999\nprefix: test\nenvironment: test\n") | ||
188 | 61 | client.reload() | ||
189 | 62 | self.assertEqual(client.lp_environment, "test") | ||
190 | diff --git a/lib/lp/services/webapp/publication.py b/lib/lp/services/webapp/publication.py | |||
191 | index 9a9a0e3..7325d9f 100644 | |||
192 | --- a/lib/lp/services/webapp/publication.py | |||
193 | +++ b/lib/lp/services/webapp/publication.py | |||
194 | @@ -795,16 +795,13 @@ class LaunchpadBrowserPublication( | |||
195 | 795 | status = request.response.getStatus() | 795 | status = request.response.getStatus() |
196 | 796 | if status == 404: # Not Found | 796 | if status == 404: # Not Found |
197 | 797 | OpStats.stats['404s'] += 1 | 797 | OpStats.stats['404s'] += 1 |
200 | 798 | statsd_client.incr('errors.404,env={}'.format( | 798 | statsd_client.incr('errors.404') |
199 | 799 | statsd_client.lp_environment)) | ||
201 | 800 | elif status == 500: # Unhandled exceptions | 799 | elif status == 500: # Unhandled exceptions |
202 | 801 | OpStats.stats['500s'] += 1 | 800 | OpStats.stats['500s'] += 1 |
205 | 802 | statsd_client.incr('errors.500,env={}'.format( | 801 | statsd_client.incr('errors.500') |
204 | 803 | statsd_client.lp_environment)) | ||
206 | 804 | elif status == 503: # Timeouts | 802 | elif status == 503: # Timeouts |
207 | 805 | OpStats.stats['503s'] += 1 | 803 | OpStats.stats['503s'] += 1 |
210 | 806 | statsd_client.incr('errors.503,env={}'.format( | 804 | statsd_client.incr('errors.503') |
209 | 807 | statsd_client.lp_environment)) | ||
211 | 808 | 805 | ||
212 | 809 | # Increment counters for status code groups. | 806 | # Increment counters for status code groups. |
213 | 810 | status_group = str(status)[0] + 'XXs' | 807 | status_group = str(status)[0] + 'XXs' |
214 | @@ -813,8 +810,7 @@ class LaunchpadBrowserPublication( | |||
215 | 813 | # Increment counter for 5XXs_b. | 810 | # Increment counter for 5XXs_b. |
216 | 814 | if is_browser(request) and status_group == '5XXs': | 811 | if is_browser(request) and status_group == '5XXs': |
217 | 815 | OpStats.stats['5XXs_b'] += 1 | 812 | OpStats.stats['5XXs_b'] += 1 |
220 | 816 | statsd_client.incr('errors.5XX,env={}'.format( | 813 | statsd_client.incr('errors.5XX') |
219 | 817 | statsd_client.lp_environment)) | ||
221 | 818 | 814 | ||
222 | 819 | # Make sure our databases are in a sane state for the next request. | 815 | # Make sure our databases are in a sane state for the next request. |
223 | 820 | thread_name = threading.current_thread().name | 816 | thread_name = threading.current_thread().name |
224 | diff --git a/lib/lp/services/webapp/tests/test_publication.py b/lib/lp/services/webapp/tests/test_publication.py | |||
225 | index 245fc28..a2a4953 100644 | |||
226 | --- a/lib/lp/services/webapp/tests/test_publication.py | |||
227 | +++ b/lib/lp/services/webapp/tests/test_publication.py | |||
228 | @@ -326,11 +326,11 @@ class TestPublisherStats(StatsMixin, TestCaseWithFactory): | |||
229 | 326 | MatchesListwise( | 326 | MatchesListwise( |
230 | 327 | [MatchesListwise( | 327 | [MatchesListwise( |
231 | 328 | (Equals('traversal_duration,success=True,' | 328 | (Equals('traversal_duration,success=True,' |
233 | 329 | 'pageid=RootObject-index-html'), | 329 | 'pageid=RootObject-index-html,env=test'), |
234 | 330 | GreaterThan(0))), | 330 | GreaterThan(0))), |
235 | 331 | MatchesListwise( | 331 | MatchesListwise( |
236 | 332 | (Equals('publication_duration,success=True,' | 332 | (Equals('publication_duration,success=True,' |
238 | 333 | 'pageid=RootObject-index-html'), | 333 | 'pageid=RootObject-index-html,env=test'), |
239 | 334 | GreaterThan(0)))])) | 334 | GreaterThan(0)))])) |
240 | 335 | 335 | ||
241 | 336 | def test_traversal_failure_stats(self): | 336 | def test_traversal_failure_stats(self): |
242 | @@ -350,7 +350,7 @@ class TestPublisherStats(StatsMixin, TestCaseWithFactory): | |||
243 | 350 | MatchesListwise( | 350 | MatchesListwise( |
244 | 351 | [MatchesListwise( | 351 | [MatchesListwise( |
245 | 352 | (Equals('traversal_duration,success=False,' | 352 | (Equals('traversal_duration,success=False,' |
247 | 353 | 'pageid=None'), | 353 | 'pageid=None,env=test'), |
248 | 354 | GreaterThan(0)))])) | 354 | GreaterThan(0)))])) |
249 | 355 | 355 | ||
250 | 356 | def test_publication_failure_stats(self): | 356 | def test_publication_failure_stats(self): |
251 | @@ -370,11 +370,11 @@ class TestPublisherStats(StatsMixin, TestCaseWithFactory): | |||
252 | 370 | MatchesListwise( | 370 | MatchesListwise( |
253 | 371 | [MatchesListwise( | 371 | [MatchesListwise( |
254 | 372 | (Equals('traversal_duration,success=True,' | 372 | (Equals('traversal_duration,success=True,' |
256 | 373 | 'pageid=RootObject-index-html'), | 373 | 'pageid=RootObject-index-html,env=test'), |
257 | 374 | GreaterThan(0))), | 374 | GreaterThan(0))), |
258 | 375 | MatchesListwise( | 375 | MatchesListwise( |
259 | 376 | (Equals('publication_duration,success=False,' | 376 | (Equals('publication_duration,success=False,' |
261 | 377 | 'pageid=RootObject-index-html'), | 377 | 'pageid=RootObject-index-html,env=test'), |
262 | 378 | GreaterThan(0)))])) | 378 | GreaterThan(0)))])) |
263 | 379 | 379 | ||
264 | 380 | def test_prepPageIDForMetrics_none(self): | 380 | def test_prepPageIDForMetrics_none(self): |