Bug 107612 – Add monitoring of patches and queues to the QueueStatusServer

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Alan Cutter]

Currently it's very difficult to see how long the average patch runs for in the EWS queues or how fast a particular bot is performing compared to others. The QueueStatusServer should be recording data that can easily present this type of information.

[Alan Cutter]

Created attachment 184107 [details]
Patch

[WebKit Review Bot]

Attachment 184107 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/QueueStatusServe..." exit_code: 1
Tools/QueueStatusServer/model/queuelog.py:32:  No name 'appengine' in module 'google'  [pylint/E0611] [5]
Tools/QueueStatusServer/model/queuelog.py:53:  [QueueLog.get_current] Class 'QueueLog' has no 'get_or_insert' member  [pylint/E1101] [5]
Tools/QueueStatusServer/model/patchlog.py:31:  No name 'appengine' in module 'google'  [pylint/E0611] [5]
Tools/QueueStatusServer/model/patchlog.py:48:  [PatchLog.lookup] Class 'PatchLog' has no 'get_or_insert' member  [pylint/E1101] [5]
Tools/QueueStatusServer/handlers/updateworkitems.py:52:  [UpdateWorkItems._update_work_items_from_request] Instance of 'UpdateWorkItems' has no 'request' member  [pylint/E1101] [5]
Tools/QueueStatusServer/handlers/updateworkitems.py:55:  [UpdateWorkItems._update_work_items_from_request] Instance of 'UpdateWorkItems' has no 'response' member  [pylint/E1101] [5]
Tools/QueueStatusServer/handlers/updateworkitems.py:72:  [UpdateWorkItems.post] Instance of 'UpdateWorkItems' has no 'response' member  [pylint/E1101] [5]
Total errors found: 7 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

Comment on attachment 184107 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184107&action=review

> Tools/QueueStatusServer/handlers/updateworkitems.py:58
> +        work_items.date = datetime.now()

Why not utcnow?

[Adam Barth]

First of all, this is a great idea.  It's just going to take me a bit to remember this code well enough to review your patch.  :)

[Alan Cutter]

Created attachment 184126 [details]
Patch

[Alan Cutter]

(In reply to comment #3)
> (From update of attachment 184107 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184107&action=review
> 
> > Tools/QueueStatusServer/handlers/updateworkitems.py:58
> > +        work_items.date = datetime.now()
> 
> Why not utcnow?

Ah! That one slipped me. Fixed in second attachment.

[WebKit Review Bot]

Attachment 184126 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/QueueStatusServe..." exit_code: 1
Tools/QueueStatusServer/model/queuelog.py:32:  No name 'appengine' in module 'google'  [pylint/E0611] [5]
Tools/QueueStatusServer/model/queuelog.py:53:  [QueueLog.get_current] Class 'QueueLog' has no 'get_or_insert' member  [pylint/E1101] [5]
Tools/QueueStatusServer/model/patchlog.py:31:  No name 'appengine' in module 'google'  [pylint/E0611] [5]
Tools/QueueStatusServer/model/patchlog.py:48:  [PatchLog.lookup] Class 'PatchLog' has no 'get_or_insert' member  [pylint/E1101] [5]
Tools/QueueStatusServer/handlers/updateworkitems.py:52:  [UpdateWorkItems._update_work_items_from_request] Instance of 'UpdateWorkItems' has no 'request' member  [pylint/E1101] [5]
Tools/QueueStatusServer/handlers/updateworkitems.py:55:  [UpdateWorkItems._update_work_items_from_request] Instance of 'UpdateWorkItems' has no 'response' member  [pylint/E1101] [5]
Tools/QueueStatusServer/handlers/updateworkitems.py:72:  [UpdateWorkItems.post] Instance of 'UpdateWorkItems' has no 'response' member  [pylint/E1101] [5]
Total errors found: 7 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

Comment on attachment 184126 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184126&action=review

This looks good.  I don't remember this code very well, so this review isn't based on any deep knowledge of the stuff you're changing.  ;)

> Tools/QueueStatusServer/loggers/recordpatchevent.py:42
> +        print patches_waiting

Do we want to call print in production?

[Eric Seidel (no email)]

Comment on attachment 184126 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184126&action=review

> Tools/QueueStatusServer/handlers/updateworkitems.py:49
> +            return

return None is clearer IMO.

>> Tools/QueueStatusServer/handlers/updateworkitems.py:55
>> +            self.response.out.write("Failed to parse work items: %s" % items_string)
> 
> [UpdateWorkItems._update_work_items_from_request] Instance of 'UpdateWorkItems' has no 'response' member  [pylint/E1101] [5]

We need to figure out why pylint is so stupid and fix it to not be.

> Tools/QueueStatusServer/loggers/recordbotevent.py:35
> +    def activity(cls, queue_name, bot_id):

I might have given this a more "active" name. :)  like record_activity.

>> Tools/QueueStatusServer/loggers/recordpatchevent.py:42
>> +        print patches_waiting
> 
> Do we want to call print in production?

That's how you print to the page in simple GAE, but that may not be what alan meant to do here.

>> Tools/QueueStatusServer/model/patchlog.py:31
>> +from google.appengine.ext import db
> 
> No name 'appengine' in module 'google'  [pylint/E0611] [5]

Yeah, clearly we need to fix sys.path on the bot or something.  We probably just need to install the gae sdk on the bot, or make it auto-installed.

> Tools/QueueStatusServer/model/queuelog.py:41
> +    max_patches_waiting = db.IntegerProperty(default=0)

I'm surprised 0 isn't the default.  I guess None is?

[Eric Seidel (no email)]

I didn't mean to block this commit, btw.

[Adam Barth]

We probably should check with Alan about the print statement before landing the patch.

[Alan Cutter]

Created attachment 184154 [details]
Patch

[WebKit Review Bot]

Attachment 184154 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/QueueStatusServe..." exit_code: 1
Tools/QueueStatusServer/model/queuelog.py:32:  No name 'appengine' in module 'google'  [pylint/E0611] [5]
Tools/QueueStatusServer/model/queuelog.py:53:  [QueueLog.get_current] Class 'QueueLog' has no 'get_or_insert' member  [pylint/E1101] [5]
Tools/QueueStatusServer/model/patchlog.py:31:  No name 'appengine' in module 'google'  [pylint/E0611] [5]
Tools/QueueStatusServer/model/patchlog.py:48:  [PatchLog.lookup] Class 'PatchLog' has no 'get_or_insert' member  [pylint/E1101] [5]
Tools/QueueStatusServer/handlers/updateworkitems.py:52:  [UpdateWorkItems._update_work_items_from_request] Instance of 'UpdateWorkItems' has no 'request' member  [pylint/E1101] [5]
Tools/QueueStatusServer/handlers/updateworkitems.py:55:  [UpdateWorkItems._update_work_items_from_request] Instance of 'UpdateWorkItems' has no 'response' member  [pylint/E1101] [5]
Tools/QueueStatusServer/handlers/updateworkitems.py:72:  [UpdateWorkItems.post] Instance of 'UpdateWorkItems' has no 'response' member  [pylint/E1101] [5]
Total errors found: 7 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alan Cutter]

(In reply to comment #9)
> (From update of attachment 184126 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184126&action=review
> 
> > Tools/QueueStatusServer/handlers/updateworkitems.py:49
> > +            return
> 
> return None is clearer IMO.
> 

Added None to return.

> > Tools/QueueStatusServer/loggers/recordbotevent.py:35
> > +    def activity(cls, queue_name, bot_id):
> 
> I might have given this a more "active" name. :)  like record_activity.
> 

Changed name to record_activity.

> >> Tools/QueueStatusServer/loggers/recordpatchevent.py:42
> >> +        print patches_waiting
> > 
> > Do we want to call print in production?
> 
> That's how you print to the page in simple GAE, but that may not be what alan meant to do here.
> 

Indeed not, removed print.

> >> Tools/QueueStatusServer/model/patchlog.py:31
> >> +from google.appengine.ext import db
> > 
> > No name 'appengine' in module 'google'  [pylint/E0611] [5]
> 
> Yeah, clearly we need to fix sys.path on the bot or something.  We probably just need to install the gae sdk on the bot, or make it auto-installed.
> 

That is a good idea! We might be able to get some automated tests going for QSS. (:

> > Tools/QueueStatusServer/model/queuelog.py:41
> > +    max_patches_waiting = db.IntegerProperty(default=0)
> 
> I'm surprised 0 isn't the default.  I guess None is?

None is the default for primitive values. The GAE docs aren't very clear on that detail.

[WebKit Review Bot]

Comment on attachment 184154 [details]
Patch

Clearing flags on attachment: 184154

Committed r140513: <http://trac.webkit.org/changeset/140513>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #15)
> (From update of attachment 184154 [details])
> Clearing flags on attachment: 184154
> 
> Committed r140513: <http://trac.webkit.org/changeset/140513>

It seems it broke all EWS bots somehow. For example Qt EWS bots 
are stucked in the following exception when they try to fetch http://queues.webkit.org/next-patch/qt-ews:

Traceback (most recent call last):
  File "/storage/WebKit-qt-ews/Tools/Scripts/webkitpy/tool/bot/queueengine.py", line 96, in run
    work_item = self._delegate.next_work_item()
  File "/storage/WebKit-qt-ews/Tools/Scripts/webkitpy/tool/commands/queues.py", line 419, in next_work_item
    return self._next_patch()
  File "/storage/WebKit-qt-ews/Tools/Scripts/webkitpy/tool/commands/queues.py", line 216, in _next_patch
    patch_id = self._tool.status_server.next_work_item(self.name)
  File "/storage/WebKit-qt-ews/Tools/Scripts/webkitpy/common/net/statusserver.py", line 128, in next_work_item
    return self._fetch_url(next_patch_url)
  File "/storage/WebKit-qt-ews/Tools/Scripts/webkitpy/common/net/statusserver.py", line 162, in _fetch_url
    raise e
HTTPError: HTTP Error 500: Internal Server Error


I tried to fetch this URL from the browser and I got this exception:
Traceback (most recent call last):
  File "/base/python_runtime/python_lib/versions/1/google/appengine/ext/webapp/_webapp25.py", line 714, in __call__
    handler.get(*groups)
  File "/base/data/home/apps/webkit-commit-queue/107612.364787856447592722/handlers/nextpatch.py", line 51, in get
    RecordPatchEvent.started(patch_id, queue_name)
  File "/base/data/home/apps/webkit-commit-queue/107612.364787856447592722/loggers/recordpatchevent.py", line 63, in started
    patch_log.calculate_wait_duration()
  File "/base/data/home/apps/webkit-commit-queue/107612.364787856447592722/model/patchlog.py", line 52, in calculate_wait_duration
    self.wait_duration = int(time_delta.total_seconds())
AttributeError: 'datetime.timedelta' object has no attribute 'total_seconds'

Could you guys check it, please?

[Csaba Osztrogonác]

ping?

[Adam Barth]

> ping?

Alan is probably asleep, but I think I've rolled back the server to the version before this change.

[Csaba Osztrogonác]

(In reply to comment #19)
> > ping?
> 
> Alan is probably asleep, but I think I've rolled back the server to the version before this change.

Thanks, EWS bots started to work again.

[Csaba Osztrogonác]

Reopen to fix the bug.

[Alan Cutter]

(In reply to comment #21)
> Reopen to fix the bug.

Created bug 107775 to resolve problem.