Bug 107659

Summary: QueueStatusServer needs pages to display historical queue data
Product: WebKit Reporter: Alan Cutter <alancutter>
Component: Tools / TestsAssignee: Alan Cutter <alancutter>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, eric, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Alan Cutter 2013-01-23 03:46:34 PST 
QueueStatusServer is only recording queue data at the moment, a page handler needs to be written to present the information in a user friendly way.
Comment 1 Alan Cutter 2013-01-23 22:39:43 PST 
Created attachment 184402 [details]
Patch
Comment 2 Eric Seidel (no email) 2013-01-23 22:54:04 PST 
Comment on attachment 184402 [details]
Patch

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

> Tools/ChangeLog:8
> +        Created a /queue-charts/<queue-name> handler to present queue and patch data using Google Chart Tools.

I might have just called it charts/ :)

> Tools/QueueStatusServer/config/charts.py:31
> +# How far back to view the history, specified in seconds.
> +default_view_range = 60 * 60 * 24
> +patch_log_limit = 500

Eventually we might want these to be get-parameters to the chart, but OK.
Comment 3 WebKit Review Bot 2013-01-23 23:00:28 PST 
Attachment 184402 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/QueueStatusServer/app.yaml', u'Tools/QueueStatusServer/config/charts.py', u'Tools/QueueStatusServer/filters/webkit_extras.py', u'Tools/QueueStatusServer/handlers/queuecharts.py', u'Tools/QueueStatusServer/handlers/queuestatus.py', u'Tools/QueueStatusServer/index.yaml', u'Tools/QueueStatusServer/main.py', u'Tools/QueueStatusServer/model/queuelog.py', u'Tools/QueueStatusServer/stylesheets/charts.css', u'Tools/QueueStatusServer/templates/queuecharts.html', u'Tools/QueueStatusServer/templates/queuestatus.html']" exit_code: 1
Tools/QueueStatusServer/model/queuelog.py:69:  [QueueLog._get_or_create_txn] Class 'QueueLog' has no 'get_by_key_name' member  [pylint/E1101] [5]
Tools/QueueStatusServer/handlers/queuecharts.py:34:  No name 'appengine' in module 'google'  [pylint/E0611] [5]
Tools/QueueStatusServer/handlers/queuecharts.py:35:  No name 'appengine' in module 'google'  [pylint/E0611] [5]
Tools/QueueStatusServer/handlers/queuecharts.py:50:  [QueueCharts.get] Instance of 'QueueCharts' has no 'error' member  [pylint/E1101] [5]
Tools/QueueStatusServer/handlers/queuecharts.py:64:  [QueueCharts.get] Instance of 'QueueCharts' has no 'response' member  [pylint/E1101] [5]
Tools/QueueStatusServer/handlers/queuecharts.py:130:  [QueueCharts._get_timestamp] Instance of 'QueueCharts' has no 'request' member  [pylint/E1101] [5]
Tools/QueueStatusServer/handlers/queuecharts.py:137:  [QueueCharts._get_view_range] Instance of 'QueueCharts' has no 'request' member  [pylint/E1101] [5]
Total errors found: 7 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Alan Cutter 2013-01-23 23:16:49 PST 
(In reply to comment #2)
> (From update of attachment 184402 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184402&action=review
> 
> > Tools/ChangeLog:8
> > +        Created a /queue-charts/<queue-name> handler to present queue and patch data using Google Chart Tools.
> 
> I might have just called it charts/ :)
> 

I was leaving room to add bot-charts. (:

> > Tools/QueueStatusServer/config/charts.py:31
> > +# How far back to view the history, specified in seconds.
> > +default_view_range = 60 * 60 * 24
> > +patch_log_limit = 500
> 
> Eventually we might want these to be get-parameters to the chart, but OK.

view_range is a get-parameter already. It is currently a future task to make a decent UI for selecting the range and the time to view from on the page.
Comment 5 Alan Cutter 2013-01-24 00:40:24 PST 
Comment on attachment 184402 [details]
Patch

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

> Tools/QueueStatusServer/handlers/queuecharts.py:86
> +                    "process_duration": patch_log.process_duration,
> +                    "retry_count": patch_log.retry_count,
> +                    "status_update_count": patch_log.status_update_count,
> +                    "wait_duration": patch_log.wait_duration,

These two durations should be converted to minutes.
Comment 6 Alan Cutter 2013-01-24 00:41:12 PST 
Created attachment 184426 [details]
Patch
Comment 7 Alan Cutter 2013-01-24 00:53:10 PST 
A "pre-release" of this patch can be viewed at: http://107659.webkit-commit-queue.appspot.com/queue-charts/chromium-ews
Feel free to suggest improvements to the interface.
Comment 8 WebKit Review Bot 2013-01-24 14:17:28 PST 
Attachment 184426 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/QueueStatusServer/app.yaml', u'Tools/QueueStatusServer/config/charts.py', u'Tools/QueueStatusServer/filters/webkit_extras.py', u'Tools/QueueStatusServer/handlers/queuecharts.py', u'Tools/QueueStatusServer/handlers/queuestatus.py', u'Tools/QueueStatusServer/index.yaml', u'Tools/QueueStatusServer/main.py', u'Tools/QueueStatusServer/model/queuelog.py', u'Tools/QueueStatusServer/stylesheets/charts.css', u'Tools/QueueStatusServer/templates/queuecharts.html', u'Tools/QueueStatusServer/templates/queuestatus.html']" exit_code: 1
Tools/QueueStatusServer/model/queuelog.py:69:  [QueueLog._get_or_create_txn] Class 'QueueLog' has no 'get_by_key_name' member  [pylint/E1101] [5]
Tools/QueueStatusServer/handlers/queuecharts.py:34:  No name 'appengine' in module 'google'  [pylint/E0611] [5]
Tools/QueueStatusServer/handlers/queuecharts.py:35:  No name 'appengine' in module 'google'  [pylint/E0611] [5]
Tools/QueueStatusServer/handlers/queuecharts.py:50:  [QueueCharts.get] Instance of 'QueueCharts' has no 'error' member  [pylint/E1101] [5]
Tools/QueueStatusServer/handlers/queuecharts.py:64:  [QueueCharts.get] Instance of 'QueueCharts' has no 'response' member  [pylint/E1101] [5]
Tools/QueueStatusServer/handlers/queuecharts.py:130:  [QueueCharts._get_timestamp] Instance of 'QueueCharts' has no 'request' member  [pylint/E1101] [5]
Tools/QueueStatusServer/handlers/queuecharts.py:137:  [QueueCharts._get_view_range] Instance of 'QueueCharts' has no 'request' member  [pylint/E1101] [5]
Total errors found: 7 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Eric Seidel (no email) 2013-01-24 14:30:36 PST 
(In reply to comment #7)
> A "pre-release" of this patch can be viewed at: http://107659.webkit-commit-queue.appspot.com/queue-charts/chromium-ews
> Feel free to suggest improvements to the interface.

Feedback:
- use "Hours Ago" instead of a big time blob to mark the x-asis  You can add a page timestamp somewhere else if you like instead.
- have the hover/click show "20 hours ago" instead of "-20.35"
- In general, all hover/clicks should be rounded to at most 2 decimals.
- I read left-to-right.  It might make sense for "now" (0) to be on the left. :)  Not sure.
- Patches Waiting and Patch Retries lines don't seem to have any data.
- I'm not sure we're showing the most interesting graph at the top.  But I"m not sure which is most interesting.

It also might make sense to add a table of "interesting stats" at the top, since those may be easier to read than the graphs.
Comment 10 Alan Cutter 2013-01-29 02:51:43 PST 
Created attachment 185210 [details]
Patch
Comment 11 WebKit Review Bot 2013-01-29 02:53:30 PST 
Attachment 185210 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/QueueStatusServer/app.yaml', u'Tools/QueueStatusServer/config/charts.py', u'Tools/QueueStatusServer/filters/webkit_extras.py', u'Tools/QueueStatusServer/handlers/queuecharts.py', u'Tools/QueueStatusServer/handlers/queuestatus.py', u'Tools/QueueStatusServer/index.yaml', u'Tools/QueueStatusServer/main.py', u'Tools/QueueStatusServer/model/queuelog.py', u'Tools/QueueStatusServer/stylesheets/charts.css', u'Tools/QueueStatusServer/templates/queuecharts.html', u'Tools/QueueStatusServer/templates/queuestatus.html']" exit_code: 1
Tools/QueueStatusServer/model/queuelog.py:69:  [QueueLog._get_or_create_txn] Class 'QueueLog' has no 'get_by_key_name' member  [pylint/E1101] [5]
Tools/QueueStatusServer/handlers/queuecharts.py:137:  trailing whitespace  [pep8/W291] [5]
Tools/QueueStatusServer/handlers/queuecharts.py:34:  No name 'appengine' in module 'google'  [pylint/E0611] [5]
Tools/QueueStatusServer/handlers/queuecharts.py:35:  No name 'appengine' in module 'google'  [pylint/E0611] [5]
Tools/QueueStatusServer/handlers/queuecharts.py:47:  [QueueCharts.get] Instance of 'QueueCharts' has no 'error' member  [pylint/E1101] [5]
Tools/QueueStatusServer/handlers/queuecharts.py:69:  [QueueCharts.get] Instance of 'QueueCharts' has no 'response' member  [pylint/E1101] [5]
Tools/QueueStatusServer/handlers/queuecharts.py:140:  [QueueCharts._get_timestamp] Instance of 'QueueCharts' has no 'request' member  [pylint/E1101] [5]
Tools/QueueStatusServer/handlers/queuecharts.py:147:  [QueueCharts._get_view_range] Instance of 'QueueCharts' has no 'request' member  [pylint/E1101] [5]
Total errors found: 8 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 Alan Cutter 2013-01-29 03:03:16 PST 
(In reply to comment #9)
> (In reply to comment #7)
> > A "pre-release" of this patch can be viewed at: http://107659.webkit-commit-queue.appspot.com/queue-charts/chromium-ews
> > Feel free to suggest improvements to the interface.
> 
> Feedback:
> - use "Hours Ago" instead of a big time blob to mark the x-asis  You can add a page timestamp somewhere else if you like instead.

Agreed, changed to use a simpler X axis title.

> - have the hover/click show "20 hours ago" instead of "-20.35"

Tooltip now shows human friendly duration string.

> - In general, all hover/clicks should be rounded to at most 2 decimals.

No more decimals in tooltips.

> - I read left-to-right.  It might make sense for "now" (0) to be on the left. :)  Not sure.

Spoke with others about this, more people seemed to be in favor of a left-to-right timeline.

> - Patches Waiting and Patch Retries lines don't seem to have any data.

The "patches waiting" seems to work for the commit-queue but not others.
There are some issues with the existing data logging that I'd like to change in a separate bug.

> It also might make sense to add a table of "interesting stats" at the top, since those may be easier to read than the graphs.

Thought about this but couldn't decide what additional information would be good to show that's not easily ascertained from the graphs themselves.


Other additions:
 - Ability to choose view range.
 - Automatic time unit selection.
 - More appropriate gridlines.
 - Changed some colours.
 - Added links to other queue charts.

Latest changes can still be seen at: http://107659.webkit-commit-queue.appspot.com/queue-charts/chromium-ews
Comment 13 Eric Seidel (no email) 2013-01-29 10:00:12 PST 
Comment on attachment 185210 [details]
Patch

I think this is a great start and we should ship and iterate.
Comment 14 WebKit Review Bot 2013-01-29 10:09:23 PST 
Comment on attachment 185210 [details]
Patch

Clearing flags on attachment: 185210

Committed r141129: <http://trac.webkit.org/changeset/141129>
Comment 15 WebKit Review Bot 2013-01-29 10:09:28 PST 
All reviewed patches have been landed.  Closing bug.