Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2918353002:
Swarming UI: Added piechart for timing information. (Closed)
|
Created:
3 years, 6 months ago by ayanaadylova Modified:
3 years, 6 months ago CC:
chromium-reviews, infra-reviews+luci-py_chromium.org Target Ref:
refs/heads/master Project:
luci-py Visibility:
Public. |
DescriptionSwarming UI: Added piechart for timing information.
Uses svg-piechart to display a pie chart of timing information.
BUG=727944
Review-Url: https://codereview.chromium.org/2918353002
Committed: https://github.com/luci/luci-py/commit/4e5ca5c27f98b9b64e129dd8d7fcfee471084180
Patch Set 1 #
Total comments: 15
Patch Set 2 #
Total comments: 6
Patch Set 3 : Removed _convertToInteger() function. #
Total comments: 2
Patch Set 4 : Added corresponding color to pending time and duration. #
Total comments: 4
Patch Set 5 : Changed piechart colors. #Patch Set 6 : Changed piechart colors. #Patch Set 7 : Rebase. #Patch Set 8 : Vulcanized files. #Patch Set 9 : Fixed bug with negative pending time. #
Messages
Total messages: 37 (15 generated)
ayanaadylova@google.com changed reviewers: + hinoka@google.com, sergeyberezin@chromium.org
Description should be prepended like: "Swarming UI: Added piechart for timing information." The description isn't the right place to put in your information (that goes in automatically when you ran "git config --global user.name/email". Instead write something like: === Swarming UI: Added piechart for timing information Uses svg-piechart to display a pie chart of timing information === https://codereview.chromium.org/2918353002/diff/1/appengine/swarming/handlers... File appengine/swarming/handlers_frontend.py (right): https://codereview.chromium.org/2918353002/diff/1/appengine/swarming/handlers... appengine/swarming/handlers_frontend.py:236: #'client_id': 525190223929-uban7ac2410omu2a9e65jvpe8b4rkmue.apps.googleusercontent.com, Remove this entirely. https://codereview.chromium.org/2918353002/diff/1/appengine/swarming/ui/bower... File appengine/swarming/ui/bower.json (right): https://codereview.chromium.org/2918353002/diff/1/appengine/swarming/ui/bower... appengine/swarming/ui/bower.json:21: "svg-piechart": "1.x" Match the other dependencies above, so this should be "svg-piechart": "timeu/svg-piechart#~1.1.0" And then rerun bower install https://codereview.chromium.org/2918353002/diff/1/appengine/swarming/ui/res/i... File appengine/swarming/ui/res/imp/taskpage/task-page.html (right): https://codereview.chromium.org/2918353002/diff/1/appengine/swarming/ui/res/i... appengine/swarming/ui/res/imp/taskpage/task-page.html:644: <script src="/res/imp/bower_components/webcomponentsjs/webcomponents-lite.js"></script> You're using polymer already, you might not actually need this import. I think this polyfill is used for browsers that did not support webcomponents. https://codereview.chromium.org/2918353002/diff/1/appengine/swarming/ui/res/i... appengine/swarming/ui/res/imp/taskpage/task-page.html:828: _convertToInteger: function(time) { I think instead of taking a human duration and converting it to an integer, this function should take two Date objects and return the duration. I favor that approach because we should avoid parsing strings meant for human consumption when machine readable versions are available. And then just call this function _duration(start, end) You can use end.getTime() - start.getTime() to get the duration in seconds. So in _computePiechartData, it would just be function(result) { var created = new Date(result.created_ts); var started = new Date(result.started_ts); var completed = new Date(result.completed_ts); return [ this._duration(created, started), this._duration(started, completed)]; }
sergeyberezin@google.com changed reviewers: + sergeyberezin@google.com
Overall the code looks good! Most comments are for cleanup, and a feature request for some sort of color legend. https://codereview.chromium.org/2918353002/diff/1/appengine/swarming/handlers... File appengine/swarming/handlers_frontend.py (right): https://codereview.chromium.org/2918353002/diff/1/appengine/swarming/handlers... appengine/swarming/handlers_frontend.py:236: #'client_id': 525190223929-uban7ac2410omu2a9e65jvpe8b4rkmue.apps.googleusercontent.com, Remove this from the final code. For local dev_appserver, you can create a config file: .../appengine/swarming/configs/services/swarmingserver/CONFIGS/settings.cfg with the following content: ui_client_id: "525190223929-uban7ac2410omu2a9e65jvpe8b4rkmue.apps.googleusercontent.com" Then the original code will return your client_id. (Of course, don't add the file to the CL, it's only for local debugging). https://codereview.chromium.org/2918353002/diff/1/appengine/swarming/ui/packa... File appengine/swarming/ui/package.json (right): https://codereview.chromium.org/2918353002/diff/1/appengine/swarming/ui/packa... appengine/swarming/ui/package.json:9: "common-substrings": "^1.0.2", Let's revert changes to this file. If you actually need newer versions, let's roll them in a separate CL, so it can be easily reverted in isolation. https://codereview.chromium.org/2918353002/diff/1/appengine/swarming/ui/res/i... File appengine/swarming/ui/res/imp/taskpage/task-page.html (right): https://codereview.chromium.org/2918353002/diff/1/appengine/swarming/ui/res/i... appengine/swarming/ui/res/imp/taskpage/task-page.html:471: <svg-piechart data="[[_computePiechartData(_result, _result.human_duration)]]"></svg-piechart> I'd use _result.duration, which I believe is always in seconds. This way you don't have to translate it back to seconds again. https://codereview.chromium.org/2918353002/diff/1/appengine/swarming/ui/res/i... appengine/swarming/ui/res/imp/taskpage/task-page.html:852: return [this._convertToInteger(this._pending(result)), this._convertToInteger(duration)]; IIUC, this would draw the piechart with no legend. Unfortunately, svg-piechart doesn't support legend, and only allows custom colors. Maybe we can use custom colors, so the color in the piechart matches the color in the corresponding table entry?
Description was changed from ========== Added piechart for timing information. Name: Ayana Adylova Email: ayanaadylova@google.com BUG=727944 ========== to ========== Uses svg-piechart to display a pie chart of timing information BUG=727944 ==========
Description was changed from ========== Uses svg-piechart to display a pie chart of timing information BUG=727944 ========== to ========== Uses svg-piechart to display a pie chart of timing information. BUG=727944 ==========
On 2017/06/05 22:25:48, Ryan Tseng wrote: > Description should be prepended like: > > "Swarming UI: Added piechart for timing information." > > The description isn't the right place to put in your information (that goes in > automatically when you ran "git config --global user.name/email". Instead write > something like: > === > Swarming UI: Added piechart for timing information > > Uses svg-piechart to display a pie chart of timing information > === > > https://codereview.chromium.org/2918353002/diff/1/appengine/swarming/handlers... > File appengine/swarming/handlers_frontend.py (right): > > https://codereview.chromium.org/2918353002/diff/1/appengine/swarming/handlers... > appengine/swarming/handlers_frontend.py:236: #'client_id': > http://525190223929-uban7ac2410omu2a9e65jvpe8b4rkmue.apps.googleusercontent.com, > Remove this entirely. > > https://codereview.chromium.org/2918353002/diff/1/appengine/swarming/ui/bower... > File appengine/swarming/ui/bower.json (right): > > https://codereview.chromium.org/2918353002/diff/1/appengine/swarming/ui/bower... > appengine/swarming/ui/bower.json:21: "svg-piechart": "1.x" > Match the other dependencies above, so this should be > > "svg-piechart": "timeu/svg-piechart#~1.1.0" > > And then rerun bower install > > https://codereview.chromium.org/2918353002/diff/1/appengine/swarming/ui/res/i... > File appengine/swarming/ui/res/imp/taskpage/task-page.html (right): > > https://codereview.chromium.org/2918353002/diff/1/appengine/swarming/ui/res/i... > appengine/swarming/ui/res/imp/taskpage/task-page.html:644: <script > src="/res/imp/bower_components/webcomponentsjs/webcomponents-lite.js"></script> > You're using polymer already, you might not actually need this import. I think > this polyfill is used for browsers that did not support webcomponents. > > https://codereview.chromium.org/2918353002/diff/1/appengine/swarming/ui/res/i... > appengine/swarming/ui/res/imp/taskpage/task-page.html:828: _convertToInteger: > function(time) { > I think instead of taking a human duration and converting it to an integer, this > function should take two Date objects and return the duration. I favor that > approach because we should avoid parsing strings meant for human consumption > when machine readable versions are available. And then just call this function > _duration(start, end) > > You can use end.getTime() - start.getTime() to get the duration in seconds. > > So in _computePiechartData, it would just be > function(result) { > var created = new Date(result.created_ts); > var started = new Date(result.started_ts); > var completed = new Date(result.completed_ts); > return [ > this._duration(created, started), > this._duration(started, completed)]; > } PTAL
On 2017/06/05 22:43:02, Sergey Berezin (google) wrote: > Overall the code looks good! Most comments are for cleanup, and a feature > request for some sort of color legend. > > https://codereview.chromium.org/2918353002/diff/1/appengine/swarming/handlers... > File appengine/swarming/handlers_frontend.py (right): > > https://codereview.chromium.org/2918353002/diff/1/appengine/swarming/handlers... > appengine/swarming/handlers_frontend.py:236: #'client_id': > http://525190223929-uban7ac2410omu2a9e65jvpe8b4rkmue.apps.googleusercontent.com, > Remove this from the final code. > > For local dev_appserver, you can create a config file: > .../appengine/swarming/configs/services/swarmingserver/CONFIGS/settings.cfg with > the following content: > > ui_client_id: > "525190223929-uban7ac2410omu2a9e65jvpe8b4rkmue.apps.googleusercontent.com" > > Then the original code will return your client_id. > > (Of course, don't add the file to the CL, it's only for local debugging). > > https://codereview.chromium.org/2918353002/diff/1/appengine/swarming/ui/packa... > File appengine/swarming/ui/package.json (right): > > https://codereview.chromium.org/2918353002/diff/1/appengine/swarming/ui/packa... > appengine/swarming/ui/package.json:9: "common-substrings": "^1.0.2", > Let's revert changes to this file. If you actually need newer versions, let's > roll them in a separate CL, so it can be easily reverted in isolation. > > https://codereview.chromium.org/2918353002/diff/1/appengine/swarming/ui/res/i... > File appengine/swarming/ui/res/imp/taskpage/task-page.html (right): > > https://codereview.chromium.org/2918353002/diff/1/appengine/swarming/ui/res/i... > appengine/swarming/ui/res/imp/taskpage/task-page.html:471: <svg-piechart > data="[[_computePiechartData(_result, _result.human_duration)]]"></svg-piechart> > I'd use _result.duration, which I believe is always in seconds. This way you > don't have to translate it back to seconds again. > > https://codereview.chromium.org/2918353002/diff/1/appengine/swarming/ui/res/i... > appengine/swarming/ui/res/imp/taskpage/task-page.html:852: return > [this._convertToInteger(this._pending(result)), > this._convertToInteger(duration)]; > IIUC, this would draw the piechart with no legend. Unfortunately, svg-piechart > doesn't support legend, and only allows custom colors. Maybe we can use custom > colors, so the color in the piechart matches the color in the corresponding > table entry? PTAL
A couple of more comments - almost there! https://codereview.chromium.org/2918353002/diff/20001/appengine/swarming/ui/p... File appengine/swarming/ui/package.json (right): https://codereview.chromium.org/2918353002/diff/20001/appengine/swarming/ui/p... appengine/swarming/ui/package.json:13: "sinon": "^1.17.5", There is still one change remaining. This file shouldn't be present in the code review at all. https://codereview.chromium.org/2918353002/diff/20001/appengine/swarming/ui/r... File appengine/swarming/ui/res/imp/taskpage/task-page.html (right): https://codereview.chromium.org/2918353002/diff/20001/appengine/swarming/ui/r... appengine/swarming/ui/res/imp/taskpage/task-page.html:827: _convertToInteger: function(time) { Delete this function, it's no longer used.
Almost there! You need to duplicate the title in the description as the first line, since that's how it gets picked up further down the line. So it should be: === Swarming UI: Added piechart for timing information <empty line> Description here <empty line> BUG=<bug number> === https://codereview.chromium.org/2918353002/diff/20001/appengine/swarming/ui/r... File appengine/swarming/ui/res/imp/taskpage/task-page.html (right): https://codereview.chromium.org/2918353002/diff/20001/appengine/swarming/ui/r... appengine/swarming/ui/res/imp/taskpage/task-page.html:471: <svg-piechart data="[[_computePiechartData(_result)]]" colors="["rgb(238,135,136)", "rgb(30,120,180)"]"></svg-piechart> nit: <svg-piechart data="..." colors="..."> </svg-piechart> So that its more narrow.
https://codereview.chromium.org/2918353002/diff/1/appengine/swarming/handlers... File appengine/swarming/handlers_frontend.py (right): https://codereview.chromium.org/2918353002/diff/1/appengine/swarming/handlers... appengine/swarming/handlers_frontend.py:236: #'client_id': 525190223929-uban7ac2410omu2a9e65jvpe8b4rkmue.apps.googleusercontent.com, On 2017/06/05 22:25:48, Ryan Tseng wrote: > Remove this entirely. Done. https://codereview.chromium.org/2918353002/diff/1/appengine/swarming/handlers... appengine/swarming/handlers_frontend.py:236: #'client_id': 525190223929-uban7ac2410omu2a9e65jvpe8b4rkmue.apps.googleusercontent.com, On 2017/06/05 22:43:02, Sergey Berezin (google) wrote: > Remove this from the final code. > > For local dev_appserver, you can create a config file: > .../appengine/swarming/configs/services/swarmingserver/CONFIGS/settings.cfg with > the following content: > > ui_client_id: > "525190223929-uban7ac2410omu2a9e65jvpe8b4rkmue.apps.googleusercontent.com" > > Then the original code will return your client_id. > > (Of course, don't add the file to the CL, it's only for local debugging). Done. https://codereview.chromium.org/2918353002/diff/1/appengine/swarming/ui/bower... File appengine/swarming/ui/bower.json (right): https://codereview.chromium.org/2918353002/diff/1/appengine/swarming/ui/bower... appengine/swarming/ui/bower.json:21: "svg-piechart": "1.x" On 2017/06/05 22:25:48, Ryan Tseng wrote: > Match the other dependencies above, so this should be > > "svg-piechart": "timeu/svg-piechart#~1.1.0" > > And then rerun bower install Done. https://codereview.chromium.org/2918353002/diff/1/appengine/swarming/ui/packa... File appengine/swarming/ui/package.json (right): https://codereview.chromium.org/2918353002/diff/1/appengine/swarming/ui/packa... appengine/swarming/ui/package.json:9: "common-substrings": "^1.0.2", On 2017/06/05 22:43:02, Sergey Berezin (google) wrote: > Let's revert changes to this file. If you actually need newer versions, let's > roll them in a separate CL, so it can be easily reverted in isolation. Done. https://codereview.chromium.org/2918353002/diff/1/appengine/swarming/ui/res/i... File appengine/swarming/ui/res/imp/taskpage/task-page.html (right): https://codereview.chromium.org/2918353002/diff/1/appengine/swarming/ui/res/i... appengine/swarming/ui/res/imp/taskpage/task-page.html:644: <script src="/res/imp/bower_components/webcomponentsjs/webcomponents-lite.js"></script> On 2017/06/05 22:25:48, Ryan Tseng wrote: > You're using polymer already, you might not actually need this import. I think > this polyfill is used for browsers that did not support webcomponents. Done. https://codereview.chromium.org/2918353002/diff/1/appengine/swarming/ui/res/i... appengine/swarming/ui/res/imp/taskpage/task-page.html:828: _convertToInteger: function(time) { On 2017/06/05 22:25:48, Ryan Tseng wrote: > I think instead of taking a human duration and converting it to an integer, this > function should take two Date objects and return the duration. I favor that > approach because we should avoid parsing strings meant for human consumption > when machine readable versions are available. And then just call this function > _duration(start, end) > > You can use end.getTime() - start.getTime() to get the duration in seconds. > > So in _computePiechartData, it would just be > function(result) { > var created = new Date(result.created_ts); > var started = new Date(result.started_ts); > var completed = new Date(result.completed_ts); > return [ > this._duration(created, started), > this._duration(started, completed)]; > } Done. https://codereview.chromium.org/2918353002/diff/1/appengine/swarming/ui/res/i... appengine/swarming/ui/res/imp/taskpage/task-page.html:852: return [this._convertToInteger(this._pending(result)), this._convertToInteger(duration)]; On 2017/06/05 22:43:02, Sergey Berezin (google) wrote: > IIUC, this would draw the piechart with no legend. Unfortunately, svg-piechart > doesn't support legend, and only allows custom colors. Maybe we can use custom > colors, so the color in the piechart matches the color in the corresponding > table entry? Done. https://codereview.chromium.org/2918353002/diff/20001/appengine/swarming/ui/p... File appengine/swarming/ui/package.json (right): https://codereview.chromium.org/2918353002/diff/20001/appengine/swarming/ui/p... appengine/swarming/ui/package.json:13: "sinon": "^1.17.5", On 2017/06/06 00:17:13, Sergey Berezin wrote: > There is still one change remaining. This file shouldn't be present in the code > review at all. Done. https://codereview.chromium.org/2918353002/diff/20001/appengine/swarming/ui/r... File appengine/swarming/ui/res/imp/taskpage/task-page.html (right): https://codereview.chromium.org/2918353002/diff/20001/appengine/swarming/ui/r... appengine/swarming/ui/res/imp/taskpage/task-page.html:471: <svg-piechart data="[[_computePiechartData(_result)]]" colors="["rgb(238,135,136)", "rgb(30,120,180)"]"></svg-piechart> On 2017/06/06 00:22:07, Ryan Tseng wrote: > nit: > <svg-piechart > data="..." > colors="..."> > </svg-piechart> > > So that its more narrow. Done. https://codereview.chromium.org/2918353002/diff/20001/appengine/swarming/ui/r... appengine/swarming/ui/res/imp/taskpage/task-page.html:827: _convertToInteger: function(time) { On 2017/06/06 00:17:13, Sergey Berezin wrote: > Delete this function, it's no longer used. Done.
On 2017/06/06 00:17:13, Sergey Berezin wrote: > A couple of more comments - almost there! > > https://codereview.chromium.org/2918353002/diff/20001/appengine/swarming/ui/p... > File appengine/swarming/ui/package.json (right): > > https://codereview.chromium.org/2918353002/diff/20001/appengine/swarming/ui/p... > appengine/swarming/ui/package.json:13: "sinon": "^1.17.5", > There is still one change remaining. This file shouldn't be present in the code > review at all. > > https://codereview.chromium.org/2918353002/diff/20001/appengine/swarming/ui/r... > File appengine/swarming/ui/res/imp/taskpage/task-page.html (right): > > https://codereview.chromium.org/2918353002/diff/20001/appengine/swarming/ui/r... > appengine/swarming/ui/res/imp/taskpage/task-page.html:827: _convertToInteger: > function(time) { > Delete this function, it's no longer used. PTAL
On 2017/06/06 00:22:07, Ryan Tseng wrote: > Almost there! > > You need to duplicate the title in the description as the first line, since > that's how it gets picked up further down the line. > > So it should be: > === > Swarming UI: Added piechart for timing information > <empty line> > Description here > <empty line> > BUG=<bug number> > === > > https://codereview.chromium.org/2918353002/diff/20001/appengine/swarming/ui/r... > File appengine/swarming/ui/res/imp/taskpage/task-page.html (right): > > https://codereview.chromium.org/2918353002/diff/20001/appengine/swarming/ui/r... > appengine/swarming/ui/res/imp/taskpage/task-page.html:471: <svg-piechart > data="[[_computePiechartData(_result)]]" colors="["rgb(238,135,136)", > "rgb(30,120,180)"]"></svg-piechart> > nit: > <svg-piechart > data="..." > colors="..."> > </svg-piechart> > > So that its more narrow. PTAL
Description was changed from ========== Uses svg-piechart to display a pie chart of timing information. BUG=727944 ========== to ========== Swarming UI: Added piechart for timing information. Uses svg-piechart to display a pie chart of timing information. BUG=727944 ==========
LGTM with one more comment. Thanks! https://codereview.chromium.org/2918353002/diff/40001/appengine/swarming/ui/r... File appengine/swarming/ui/res/imp/taskpage/task-page.html (right): https://codereview.chromium.org/2918353002/diff/40001/appengine/swarming/ui/r... appengine/swarming/ui/res/imp/taskpage/task-page.html:459: <td>[[_pending(_result)]]</td> Let's also add the corresponding color to pending time and duration, to serve as a legend for the piechart.
https://screenshot.googleplex.com/AtvduQWF8sy https://codereview.chromium.org/2918353002/diff/40001/appengine/swarming/ui/r... File appengine/swarming/ui/res/imp/taskpage/task-page.html (right): https://codereview.chromium.org/2918353002/diff/40001/appengine/swarming/ui/r... appengine/swarming/ui/res/imp/taskpage/task-page.html:459: <td>[[_pending(_result)]]</td> On 2017/06/06 18:29:08, Sergey Berezin wrote: > Let's also add the corresponding color to pending time and duration, to serve as > a legend for the piechart. Done.
The CQ bit was checked by ayanaadylova@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
hinoka@google.com changed reviewers: + kjlubick@chromium.org
+kjlubick thanks! lgtm from me. Wait for Kevin's approval before committing.
kjlubick@google.com changed reviewers: + kjlubick@google.com - kjlubick@chromium.org
How does this look if a browser window is 1200 px wide? (That's the smallest width screen I try to accommodate [because both maruel@ and I have narrower browser windows]) I notice the compiled elements.html isn't in this patch. Make sure you ran `make vulcanize` If you did so and `git cl upload` gave you an error about elements.html being too big, you will need to do `git cl land` when submitting the CL, *not* just hitting the Commit button in the web UI. https://codereview.chromium.org/2918353002/diff/60001/appengine/swarming/ui/r... File appengine/swarming/ui/res/imp/taskpage/task-page.html (right): https://codereview.chromium.org/2918353002/diff/60001/appengine/swarming/ui/r... appengine/swarming/ui/res/imp/taskpage/task-page.html:477: colors="["rgb(238,135,136)", "rgb(30,120,180)"]"> Are these colors okay for people with various types of color blindness? If you aren't sure, see http://mkweb.bcgsc.ca/colorblind/ For example, the orange could be 230,159,0 and the blue could be 0,114, 178 and those colors are sufficiently different under the most common color blind conditions. https://codereview.chromium.org/2918353002/diff/60001/appengine/swarming/ui/r... appengine/swarming/ui/res/imp/taskpage/task-page.html:835: var created = new Date(result.created_ts); I don't think you need to create Date objects, *_ts should already be Dates because of https://github.com/luci/luci-py/blob/84f310bf2dc0b37dfbefc3ad207fb3175413e41e... which is called in task-page-data: https://github.com/luci/luci-py/blob/84f310bf2dc0b37dfbefc3ad207fb3175413e41e...
PTAL. If a browser window is 1200 px wide or less the piechart is either displayed under Task Timing Information Section or on the right side from. It depends on whether "Milo Output" and "Raw Output" sections are displayed on the right or at the end of the page. https://codereview.chromium.org/2918353002/diff/60001/appengine/swarming/ui/r... File appengine/swarming/ui/res/imp/taskpage/task-page.html (right): https://codereview.chromium.org/2918353002/diff/60001/appengine/swarming/ui/r... appengine/swarming/ui/res/imp/taskpage/task-page.html:477: colors="["rgb(238,135,136)", "rgb(30,120,180)"]"> On 2017/06/07 12:12:58, kjlubick wrote: > Are these colors okay for people with various types of color blindness? > > If you aren't sure, see http://mkweb.bcgsc.ca/colorblind/ > For example, the orange could be 230,159,0 and the blue could be 0,114, 178 and > those colors are sufficiently different under the most common color blind > conditions. Done. https://codereview.chromium.org/2918353002/diff/60001/appengine/swarming/ui/r... appengine/swarming/ui/res/imp/taskpage/task-page.html:835: var created = new Date(result.created_ts); On 2017/06/07 12:12:58, kjlubick wrote: > I don't think you need to create Date objects, *_ts should already be Dates > because of > https://github.com/luci/luci-py/blob/84f310bf2dc0b37dfbefc3ad207fb3175413e41e... > which is called in task-page-data: > https://github.com/luci/luci-py/blob/84f310bf2dc0b37dfbefc3ad207fb3175413e41e... Done.
lgtm
The CQ bit was checked by ayanaadylova@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyberezin@chromium.org, hinoka@google.com Link to the patchset: https://codereview.chromium.org/2918353002/#ps120001 (title: "Vulcanized files.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1496855988289380,
"parent_rev": "faafbdc7f9d333e1dcf46a980b2ff5a1337534d8", "commit_rev":
"4e5ca5c27f98b9b64e129dd8d7fcfee471084180"}
Message was sent while issue was closed.
Description was changed from ========== Swarming UI: Added piechart for timing information. Uses svg-piechart to display a pie chart of timing information. BUG=727944 ========== to ========== Swarming UI: Added piechart for timing information. Uses svg-piechart to display a pie chart of timing information. BUG=727944 Review-Url: https://codereview.chromium.org/2918353002 Committed: https://github.com/luci/luci-py/commit/4e5ca5c27f98b9b64e129dd8d7fcfee471084180 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:120001) as https://github.com/luci/luci-py/commit/4e5ca5c27f98b9b64e129dd8d7fcfee471084180
Message was sent while issue was closed.
PTAL
Message was sent while issue was closed.
Can you create a new CL for the newest fix?
Message was sent while issue was closed.
On 2017/06/07 21:42:43, Ryan Tseng wrote: > Can you create a new CL for the newest fix? Without a legend, I don't think most user will be able to understand what the graph represent. Please tweak the UX to make it more self-describing.
Message was sent while issue was closed.
On 2017/06/08 13:52:29, M-A Ruel wrote: > On 2017/06/07 21:42:43, Ryan Tseng wrote: > > Can you create a new CL for the newest fix? > > Without a legend, I don't think most user will be able to understand what the > graph represent. Please tweak the UX to make it more self-describing. By looking more carefully, I realized the numbers are colored so there is indeed a legend, I somehow missed it. I feel the graph is not representing what is currently important for the user looking at it. - I pie chart represents subdivisions of a whole summing to 100%. Technically speaking the pending time and duration times have no relationship to sum to "100%" - Having a graph would be more useful with regards to overheads (downloading inputs, uploading outputs, rest of overhead) vs task duration - I feel like a bar chart would probably make more sense here, with a Y axis in seconds. I'd see two bars, one for pending, another for the task. - For the task duration, it would be the sum of the durations one on top of the other. - I'm fine with iterations on UX ideas, feel free to discuss with Kevin. Thanks! |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
