Merge lp:~jcsackett/charmworld/better-latency-round-2 into lp:charmworld

Proposed by j.c.sackett on 2013-11-13
Status: Merged
Approved by: j.c.sackett on 2013-11-13
Approved revision: 461
Merged at revision: 459
Proposed branch: lp:~jcsackett/charmworld/better-latency-round-2
Merge into: lp:charmworld
Diff against target: 374 lines (+182/-76)
3 files modified
.bzrignore (+1/-0)
charmworld/jobs/review.py (+74/-30)
charmworld/jobs/tests/test_review.py (+107/-46)
To merge this branch: bzr merge lp:~jcsackett/charmworld/better-latency-round-2
Reviewer Review Type Date Requested Status
Juju Gui Bot continuous-integration Approve on 2013-11-13
Aaron Bentley (community) 2013-11-13 Approve on 2013-11-13
Review via email: mp+195091@code.launchpad.net

Commit message

Undoes the rollback for the better latency work, with a fix for the expiring credentials.

Description of the change

This undoes the rollback in r 443 of the better latency work originally done in
r 422.

The majority of the changes were explained and approved in an earlier MP[1].

charmworld/jobs/review.py
-------------------------
The reinstated call to login_with is updated to use consumer_name as a
parameter; this ensures that LP sees the same, already authorized consumer when
the request occurs in dev, staging, or production.

As before, the credentials file will have to be set on the staging and
production instances by setting the lp_credentials parameter on the charm, via
juju set. IS docs on wiki.canonical.com and the deploy script for staging will
both be updated with this information after this branch is QAed.

[1]: https://code.launchpad.net/~jcsackett/charmworld/better-latency-refactor/+merge/191615

To post a comment you must log in.
Aaron Bentley (abentley) wrote :

Looks good to me.

review: Approve
Juju Gui Bot (juju-gui-bot) wrote :

FAILED: Autolanding.
More details in the following jenkins job:
http://162.213.35.27:8080/job/charmworld-autoland-lxc/81/
Executed test runs:

review: Needs Fixing (continuous-integration)
j.c.sackett (jcsackett) wrote :

Looks like it just failed on the npm step, possible network error? Trying again.

review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file '.bzrignore'
2--- .bzrignore 2013-10-17 15:58:13 +0000
3+++ .bzrignore 2013-11-13 16:15:48 +0000
4@@ -29,3 +29,4 @@
5 supervisord.log
6 supervisord.pid
7 _run
8+charmbot_credentials.txt
9
10=== modified file 'charmworld/jobs/review.py'
11--- charmworld/jobs/review.py 2013-11-05 18:40:38 +0000
12+++ charmworld/jobs/review.py 2013-11-13 16:15:48 +0000
13@@ -38,10 +38,21 @@
14 # older than the window that is still open. Right now this is probably the
15 # more performant way to go about it, but that's going to change as the
16 # data outside of our window gets larger.
17+ try:
18+ lp_credentials = get_ini()['lp_credentials_file']
19+ except KeyError:
20+ lp_credentials = None
21 with Timer() as timer:
22- lp = Launchpad.login_anonymously('charm-tools',
23- 'production',
24- version='devel')
25+ if lp_credentials:
26+ lp = Launchpad.login_with(application_name='charm-tools',
27+ service_root='production',
28+ version='devel',
29+ consumer_name='charmworld',
30+ credentials_file=lp_credentials)
31+ else:
32+ lp = Launchpad.login_anonymously('charm-tools',
33+ 'production',
34+ version='devel')
35
36 # Entity roots for items of interests
37 charm = lp.distributions['charms']
38@@ -50,13 +61,13 @@
39
40 log.debug("lp login time %0.2f", timer.duration())
41
42+ branch_filter = "Show only Bugs with linked Branches"
43 with Timer() as timer:
44 bugs = charm.searchTasks(
45 tags=['new-formula', 'new-charm'],
46- tags_combinator="Any")
47- # Best practice dictates that charmers be subscribed to all bugs
48- # and merge proposals for official source package branches.
49- charmers_bugs = charmers.searchTasks()
50+ tags_combinator="Any",
51+ linked_branches=branch_filter)
52+ charmers_bugs = charmers.searchTasks(linked_branches=branch_filter)
53 proposals = charmers.getRequestedReviews()
54 contributor_proposals = charm_contributors.getRequestedReviews()
55
56@@ -69,27 +80,67 @@
57 itertools.chain(proposals, contributor_proposals, doc_proposals))
58
59
60+def calculate_proposal_latency(proposal, now):
61+ if proposal.date_reviewed:
62+ latency = (
63+ proposal.date_reviewed - proposal.date_review_requested)
64+ else:
65+ # `now` has been calculated in UTC, and LP returns UTC TZ
66+ # datetimes. We can safely remove the TZ, letting us get the
67+ latency = (
68+ now - proposal.date_review_requested.replace(tzinfo=None))
69+ return latency.total_seconds()
70+
71+
72+def calculate_task_latency(task, now):
73+ link_entries = [e for e in task.bug.activity.entries if
74+ e['whatchanged'] == 'branch linked']
75+ if len(link_entries) == 1:
76+ start_date = link_entries[0]['datechanged']
77+ start_date = datetime.datetime.strptime(
78+ start_date.split('.')[0], '%Y-%m-%dT%H:%M:%S')
79+ elif len(link_entries) > 1:
80+ #Filter down to links associated with the currently linked branches
81+ linked_branches = task.bug.linked_branches
82+ branch_names = [link.branch.display_name for link in linked_branches]
83+ current_entries = [e for e in link_entries if
84+ e['newvalue'] in branch_names]
85+ if current_entries != []:
86+ start_date = current_entries[0]['datechanged']
87+ start_date = datetime.datetime.strptime(
88+ start_date.split('.')[0], '%Y-%m-%dT%H:%M:%S')
89+ else:
90+ # At some point, the name of the branch changed, so we can't find
91+ # the event when it was added. This happens rarely, so we just
92+ # abandon stats for this task.
93+ return
94+ elif link_entries == []:
95+ # We have no entries, probably b/c the lp step failed to login with
96+ # authentication.
97+ return
98+
99+ if task.date_fix_committed:
100+ end_date = task.date_fix_committed.replace(tzinfo=None)
101+ else:
102+ end_date = now
103+ latency = end_date - start_date
104+ return latency.total_seconds()
105+
106+
107 def update_review_queue(db, tasks, proposals, log):
108 '''Updates the review queue information.'''
109+ now = datetime.datetime.utcnow()
110+ window = now - datetime.timedelta(days=180) # 6 month window for latency
111 bug_statuses = [
112 'New', 'Confirmed', 'Triaged', 'In Progress', 'Fix Committed']
113- proposal_statuses = ['Needs review', 'Needs fixing', 'Needs information']
114- now = datetime.datetime.utcnow()
115- window = now - datetime.timedelta(days=180) # 6 month window for latency
116-
117 r_queue = []
118 latencies = []
119 for task in tasks:
120 if (task.date_created.replace(tzinfo=None) >= window or
121 task.status in bug_statuses):
122- if task.date_confirmed:
123- latency = task.date_confirmed - task.date_created
124- else:
125- # Now has been calculated in UTC, and LP returns UTC TZ
126- # datetimes. We can safely remove the TZ, letting us get the
127- # timedelta.
128- latency = now - task.date_created.replace(tzinfo=None)
129- latencies.append(latency.total_seconds())
130+ latency = calculate_task_latency(task, now)
131+ if latency:
132+ latencies.append(latency)
133
134 if task.status in bug_statuses:
135 entry = {
136@@ -102,19 +153,13 @@
137 }
138 r_queue.append(entry)
139
140+ proposal_statuses = [
141+ 'Needs review', 'Needs fixing', 'Needs information']
142 for proposal in proposals:
143 if (proposal.date_created.replace(tzinfo=None) >= window or
144 proposal.queue_status in proposal_statuses):
145- if proposal.date_reviewed:
146- latency = (
147- proposal.date_reviewed - proposal.date_review_requested)
148- else:
149- # Now has been calculated in UTC, and LP returns UTC TZ
150- # datetimes. We can safely remove the TZ, letting us get the
151- latency = (
152- now - proposal.date_review_requested.replace(tzinfo=None))
153- latencies.append(latency.total_seconds())
154-
155+ latency = calculate_proposal_latency(proposal, now)
156+ latencies.append(latency)
157 if proposal.queue_status in proposal_statuses:
158 parts = proposal.target_branch_link.rsplit('/', 3)
159 if parts[-1] == "trunk":
160@@ -131,7 +176,6 @@
161
162 entry = {
163 'type': 'proposal',
164- 'latency': latency.total_seconds(),
165 'date_modified': last_modified,
166 'date_created': proposal.date_created,
167 'summary': "Merge for %s from %s" % (target, origin),
168
169=== modified file 'charmworld/jobs/tests/test_review.py'
170--- charmworld/jobs/tests/test_review.py 2013-11-05 18:40:38 +0000
171+++ charmworld/jobs/tests/test_review.py 2013-11-13 16:15:48 +0000
172@@ -3,7 +3,13 @@
173 import datetime
174 import logging
175
176-from charmworld.jobs.review import update_review_queue
177+import mock
178+
179+from charmworld.jobs.review import (
180+ calculate_proposal_latency,
181+ calculate_task_latency,
182+ update_review_queue,
183+)
184 from charmworld.testing import MongoTestBase
185
186
187@@ -12,45 +18,45 @@
188 TWO_DAYS = NOW - datetime.timedelta(2)
189
190
191-class MockBug:
192- date_last_updated = NOW
193-
194-
195-class MockTask:
196-
197- def __init__(self, confirmed=False):
198- self.date_created = TWO_DAYS
199- self.title = u'Bug #0 in a collection: "A bug task"'
200- self.web_link = "http://example.com"
201- self.bug = MockBug()
202- if confirmed:
203- self.status = "Confirmed"
204- self.date_confirmed = YESTERDAY
205- else:
206- self.date_confirmed = None
207- self.status = "New"
208-
209-
210-class MockCommentCollection:
211- total_size = 0
212-
213-
214-class MockProposal:
215-
216- def __init__(self, reviewed=False):
217- base = 'https://api.launchpad.net/devel/'
218- self.target_branch_link = base + '~charmers/charms/precise/foo/trunk'
219- self.source_branch_link = base + '~bar/charms/precise/foo/fnord'
220- self.all_comments = MockCommentCollection()
221- self.date_review_requested = TWO_DAYS
222- self.date_created = TWO_DAYS
223- self.web_link = 'http://example.com'
224- if reviewed:
225- self.date_reviewed = YESTERDAY
226- self.queue_status = "Approved"
227- else:
228- self.date_reviewed = None
229- self.queue_status = "Needs review"
230+def get_mock_task(confirmed=False, entries=[], linked_branches=[]):
231+ if confirmed:
232+ status = "Confirmed"
233+ date_fix_committed = YESTERDAY
234+ else:
235+ date_fix_committed = None
236+ status = "New"
237+ return mock.Mock(
238+ status=status,
239+ date_fix_committed=date_fix_committed,
240+ date_created=TWO_DAYS,
241+ title=u'Bug #0 in a collection: "A bug task"',
242+ web_link="http://example.com",
243+ bug=mock.Mock(
244+ date_last_updated=NOW,
245+ linked_branches=linked_branches,
246+ activity=mock.Mock(entries=entries)
247+ ),
248+ )
249+
250+
251+def get_mock_proposal(reviewed=False):
252+ base = 'https://api.launchpad.net/devel/'
253+ if reviewed:
254+ date_reviewed = YESTERDAY
255+ queue_status = "Approved"
256+ else:
257+ date_reviewed = None
258+ queue_status = "Needs review"
259+ return mock.Mock(
260+ target_branch_link=base + '~charmers/charms/precise/foo/trunk',
261+ source_branch_link=base + '~bar/charms/precise/foo/fnord',
262+ all_comments=mock.Mock(total_size=0),
263+ date_review_requested=TWO_DAYS,
264+ date_created=TWO_DAYS,
265+ web_link='http://example.com',
266+ date_reviewed=date_reviewed,
267+ queue_status=queue_status,
268+ )
269
270
271 class ReviewTest(MongoTestBase):
272@@ -63,7 +69,7 @@
273
274 def test_review_handles_bugs(self):
275 log = logging.getLogger('foo')
276- update_review_queue(self.db, [MockTask()], [], log)
277+ update_review_queue(self.db, [get_mock_task()], [], log)
278 self.assertEqual(1, self.db.review_queue.count())
279 item = self.db.review_queue.find_one()
280 self.assertEqual(NOW.date(), item['date_modified'].date())
281@@ -74,7 +80,7 @@
282
283 def test_review_handles_proposals(self):
284 log = logging.getLogger('foo')
285- update_review_queue(self.db, [], [MockProposal()], log)
286+ update_review_queue(self.db, [], [get_mock_proposal()], log)
287 self.assertEqual(1, self.db.review_queue.count())
288 item = self.db.review_queue.find_one()
289 self.assertEqual(TWO_DAYS.date(), item['date_modified'].date())
290@@ -84,10 +90,65 @@
291 self.assertEqual('http://example.com', item['item'])
292 self.assertEqual('Needs review', item['status'])
293
294- def test_review_calculates_latency(self):
295+ def test_calculate_task_latency_with_two_valid_links(self):
296+ linked_branches = [
297+ mock.Mock(branch=mock.Mock(display_name='lp:~foo/bar/bjorn')),
298+ mock.Mock(branch=mock.Mock(display_name='lp:~foo/bar/baz'))
299+ ]
300+ two_days = datetime.datetime.strftime(
301+ TWO_DAYS, '%Y-%m-%dT%H:%M:%S')
302+ yesterday = datetime.datetime.strftime(
303+ YESTERDAY, '%Y-%m-%dT%H:%M:%S')
304+ entries = [
305+ {'whatchanged': 'branch linked',
306+ 'newvalue': 'lp:~foo/bar/baz',
307+ 'datechanged': two_days},
308+ {'whatchanged': 'branch linked',
309+ 'newvalue': 'lp:~foo/bar/bjorn',
310+ 'datechanged': yesterday}]
311+ latency = calculate_task_latency(
312+ get_mock_task(
313+ True, entries=entries, linked_branches=linked_branches), NOW)
314+ self.assertEqual(60 * 60 * 24, int(latency))
315+
316+ def test_calculate_task_latency_bug_with_one_valid_link(self):
317+ linked_branches = [
318+ mock.Mock(branch=mock.Mock(display_name='lp:~foo/bar/bjorn'))
319+ ]
320+ two_days = datetime.datetime.strftime(
321+ TWO_DAYS, '%Y-%m-%dT%H:%M:%S')
322+ yesterday = datetime.datetime.strftime(
323+ YESTERDAY, '%Y-%m-%dT%H:%M:%S')
324+ entries = [
325+ {'whatchanged': 'branch linked',
326+ 'newvalue': 'lp:~foo/bar/baz',
327+ 'datechanged': yesterday},
328+ {'whatchanged': 'branch linked',
329+ 'newvalue': 'lp:~foo/bar/bjorn',
330+ 'datechanged': two_days}]
331+ latency = calculate_task_latency(
332+ get_mock_task(
333+ True, entries=entries, linked_branches=linked_branches), NOW)
334+ self.assertEqual(60 * 60 * 24, int(latency))
335+
336+ def test_calculate_task_latency_bug_with_one_link(self):
337+ date_changed = datetime.datetime.strftime(
338+ TWO_DAYS, '%Y-%m-%dT%H:%M:%S')
339+ entries = [{'whatchanged': 'branch linked',
340+ 'newvalue': 'lp:~foo/bar/baz',
341+ 'datechanged': date_changed}]
342+ latency = calculate_task_latency(
343+ get_mock_task(True, entries=entries), NOW)
344+ self.assertEqual(60 * 60 * 24, int(latency))
345+
346+ def test_calculate_proposal_latency(self):
347+ latency = calculate_proposal_latency(get_mock_proposal(True), NOW)
348+ self.assertEqual(60 * 60 * 24, int(latency))
349+
350+ def test_review_job_saves_latency(self):
351 log = logging.getLogger('foo')
352 self.assertEqual(0, self.db.review_queue_latency.count())
353- update_review_queue(self.db, [], [MockProposal(True)], log)
354+ update_review_queue(self.db, [], [get_mock_proposal(True)], log)
355 self.assertEqual(1, self.db.review_queue_latency.count())
356 item = self.db.review_queue_latency.find_one()
357 self.assertEqual(NOW.date(), item['date'].date())
358@@ -96,14 +157,14 @@
359 self.assertEqual(1, datetime.timedelta(seconds=item['min']).days)
360
361 def test_review_discards_closed_bugs(self):
362- tasks = [MockTask(), MockTask()]
363+ tasks = [get_mock_task(), get_mock_task()]
364 tasks[0].status = 'Fix released'
365 log = logging.getLogger('foo')
366 update_review_queue(self.db, tasks, [], log)
367 self.assertEqual(1, self.db.review_queue.count())
368
369 def test_review_discards_approved_proposals(self):
370- proposals = [MockProposal(), MockProposal()]
371+ proposals = [get_mock_proposal(), get_mock_proposal()]
372 proposals[0].queue_status = 'Approved'
373 log = logging.getLogger('foo')
374 update_review_queue(self.db, [], proposals, log)

Subscribers

People subscribed via source and target branches

to all changes: