Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2648683004:
Building the table for speed releasing. (Closed)
DescriptionBuilding the table for speed releasing.
Adds the actual table in. Creates all the JSON needed on the server side to be easily consumed on the client side. Also updates table_config to be more restrictive.
http://i.imgur.com/PrbDCyb.png
Demo: https://dev-jessimb-7e894f97-dot-chromeperf.appspot.com/speed_releasing/MemoryTest5?revA=1479546161&revB=1485025126 (must be logged in)
BUG=catapult:#3141
Review-Url: https://codereview.chromium.org/2648683004
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/ca6e2c31a44021b7aa189333638674d38f6eaec9
Patch Set 1 #Patch Set 2 : Table completed. #
Total comments: 22
Patch Set 3 : response to comments #
Total comments: 7
Patch Set 4 : Response to comments + merging in a different CL #Patch Set 5 : Remove debugging #Patch Set 6 : Removing all traces of avg and Average #
Total comments: 17
Patch Set 7 : comments #
Total comments: 1
Patch Set 8 : Added more logging #
Total comments: 1
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 34 (13 generated)
Description was changed from ========== Building the table for speed releasing. BUG=catapult:#3141 ========== to ========== Building the table for speed releasing. Adds the actual table in. Creates all the JSON needed on the server side to be easily consumed on the client side. http://i.imgur.com/vf9UnsD.png Demo: https://dev-jessimb-be576fcf-dot-chromeperf.appspot.com/speed_releasing/Memor... (must be logged in) BUG=catapult:#3141 ==========
Description was changed from ========== Building the table for speed releasing. Adds the actual table in. Creates all the JSON needed on the server side to be easily consumed on the client side. http://i.imgur.com/vf9UnsD.png Demo: https://dev-jessimb-be576fcf-dot-chromeperf.appspot.com/speed_releasing/Memor... (must be logged in) BUG=catapult:#3141 ========== to ========== Building the table for speed releasing. Adds the actual table in. Creates all the JSON needed on the server side to be easily consumed on the client side. Also updates table_config to be slightly more restrictive. http://i.imgur.com/vf9UnsD.png Demo: https://dev-jessimb-be576fcf-dot-chromeperf.appspot.com/speed_releasing/Memor... (must be logged in) BUG=catapult:#3141 ==========
jessimb@chromium.org changed reviewers: + eakuefner@chromium.org
PTAL. Let me know if you have any questions! https://codereview.chromium.org/2648683004/diff/20001/dashboard/dashboard/cre... File dashboard/dashboard/create_health_report_test.py (right): https://codereview.chromium.org/2648683004/diff/20001/dashboard/dashboard/cre... dashboard/dashboard/create_health_report_test.py:22: 'tableTests': 'my_test_suite/my_test_*\nmy_test_suite/my_other_test_*', _* is a new restriction this must follow (see the reasoning in create-health-report-page.html) https://codereview.chromium.org/2648683004/diff/20001/dashboard/dashboard/cre... dashboard/dashboard/create_health_report_test.py:138: 'tableLayout': '{"Alayout":"isHere"}', Turns out this wasn't valid, it was just failing (as intended) before the check for it. https://codereview.chromium.org/2648683004/diff/20001/dashboard/dashboard/ele... File dashboard/dashboard/elements/create-health-report-page.html (right): https://codereview.chromium.org/2648683004/diff/20001/dashboard/dashboard/ele... dashboard/dashboard/elements/create-health-report-page.html:91: will go. The reason for this is metric names are test_max, test_avg, etc. The table goes column by column to fill this in. Each test in this list constitutes a tr in the table which doesn't make sense if each row is a different _stat. This also makes it easier to specify tests. https://codereview.chromium.org/2648683004/diff/20001/dashboard/dashboard/ele... File dashboard/dashboard/elements/speed-releasing-table.html (right): https://codereview.chromium.org/2648683004/diff/20001/dashboard/dashboard/ele... dashboard/dashboard/elements/speed-releasing-table.html:84: <template is="dom-repeat" items="{{tableConfig.tests}}" We do dom-repeat over tests. Within that (for the center columns) dom-repeat on revisions, stats. After that set, dom-repeat over the changeStats list to create the 'Change' column. https://codereview.chromium.org/2648683004/diff/20001/dashboard/dashboard/ele... dashboard/dashboard/elements/speed-releasing-table.html:161: getValue: function(rev, bot, test, kind) { 'kind' is one of the stats (line 149) https://codereview.chromium.org/2648683004/diff/20001/dashboard/dashboard/ele... dashboard/dashboard/elements/speed-releasing-table.html:212: return this.tableConfig.categories[category] * 2; When I multiplied by 2 it worked. That is the reason. https://codereview.chromium.org/2648683004/diff/20001/dashboard/dashboard/spe... File dashboard/dashboard/speed_releasing.py (right): https://codereview.chromium.org/2648683004/diff/20001/dashboard/dashboard/spe... dashboard/dashboard/speed_releasing.py:62: 'table_tests': table_entity.tests, Need the expanded test to compute values. But when iterating over the tests, we just want the list of _* without the stat filled in. https://codereview.chromium.org/2648683004/diff/20001/dashboard/dashboard/spe... dashboard/dashboard/speed_releasing.py:103: """Grabs the units on each test for only one bot.""" Only needs to do it for one bot as the list is the same for all bots. https://codereview.chromium.org/2648683004/diff/20001/dashboard/dashboard/spe... dashboard/dashboard/speed_releasing.py:123: def _CheckRevisions(rev_a, rev_b): Expect a CL to revamp this to allow for default revs. https://codereview.chromium.org/2648683004/diff/20001/dashboard/dashboard/spe... dashboard/dashboard/speed_releasing.py:157: def _GetCategoryCounts(layout): Useful for rowspan calculation on client side.
sullivan@chromium.org changed reviewers: + sullivan@chromium.org
I took a look and didn't get all the way through this, but added some initial comments. https://codereview.chromium.org/2648683004/diff/20001/dashboard/dashboard/ele... File dashboard/dashboard/elements/speed-releasing-table.html (right): https://codereview.chromium.org/2648683004/diff/20001/dashboard/dashboard/ele... dashboard/dashboard/elements/speed-releasing-table.html:63: <thead>{{bot}}</thead> From https://developer.mozilla.org/en-US/docs/Web/HTML/Element/thead: Permitted content Zero or more <tr> elements. I think you acually just want one <thead> with two rows? https://codereview.chromium.org/2648683004/diff/20001/dashboard/dashboard/ele... dashboard/dashboard/elements/speed-releasing-table.html:88: <tr> It's really confusing that we have two <tr> tags in different templates and only one </tr> tag to close them. And the indentation isn't lined up to look like it's part of the template. Any reason not to pull this <tr> and the one on line 95 up into a single <tr> right after line 85 before these dom-if templates? https://codereview.chromium.org/2648683004/diff/20001/dashboard/dashboard/ele... dashboard/dashboard/elements/speed-releasing-table.html:161: getValue: function(rev, bot, test, kind) { On 2017/01/25 22:47:25, jessimb wrote: > 'kind' is one of the stats (line 149) Having jsdoc to explain the arguments would help. Or at least a code comment explaining that it would be 'avg', 'std', 'max', etc. https://codereview.chromium.org/2648683004/diff/20001/dashboard/dashboard/mod... File dashboard/dashboard/models/table_config.py (right): https://codereview.chromium.org/2648683004/diff/20001/dashboard/dashboard/mod... dashboard/dashboard/models/table_config.py:102: return None, error_message I think you should also actually do a query to make sure these tests exist, and return an error message if not.
I replied to your comments and actually have 3 more CLs that are ready after this one lands. https://codereview.chromium.org/2657043002 - Allows for /TableName (without ?revA= etc) which defaults to the most recent milestone - present. Also renames the version numbers to be more clank friendly. https://codereview.chromium.org/2658643005 - Adds the report URLs to the dashboard (as it is done in https://chrome-health.googleplex.com/health-plan/android-chrome/memory/nexus5...) https://codereview.chromium.org/2662633002 - Removes the _* idea (and corresponding complexity) so we only show averages, per Ethan and I's conversation with @perezju. Many of my comments on this CL note where things will be gone when that one lands. Thanks!! https://codereview.chromium.org/2648683004/diff/20001/dashboard/dashboard/cre... File dashboard/dashboard/create_health_report_test.py (right): https://codereview.chromium.org/2648683004/diff/20001/dashboard/dashboard/cre... dashboard/dashboard/create_health_report_test.py:22: 'tableTests': 'my_test_suite/my_test_*\nmy_test_suite/my_other_test_*', On 2017/01/25 at 22:47:25, jessimb wrote: > _* is a new restriction this must follow (see the reasoning in create-health-report-page.html) (gone with https://codereview.chromium.org/2662633002) https://codereview.chromium.org/2648683004/diff/20001/dashboard/dashboard/ele... File dashboard/dashboard/elements/create-health-report-page.html (right): https://codereview.chromium.org/2648683004/diff/20001/dashboard/dashboard/ele... dashboard/dashboard/elements/create-health-report-page.html:91: will go. On 2017/01/25 at 22:47:25, jessimb wrote: > The reason for this is metric names are test_max, test_avg, etc. The table goes column by column to fill this in. Each test in this list constitutes a tr in the table which doesn't make sense if each row is a different _stat. This also makes it easier to specify tests. (gone with https://codereview.chromium.org/2662633002) https://codereview.chromium.org/2648683004/diff/20001/dashboard/dashboard/ele... File dashboard/dashboard/elements/speed-releasing-table.html (right): https://codereview.chromium.org/2648683004/diff/20001/dashboard/dashboard/ele... dashboard/dashboard/elements/speed-releasing-table.html:63: <thead>{{bot}}</thead> On 2017/01/26 at 03:44:28, sullivan wrote: > From https://developer.mozilla.org/en-US/docs/Web/HTML/Element/thead: > > Permitted content Zero or more <tr> elements. > > I think you acually just want one <thead> with two rows? Hm. I think that I actually just don't want these things in the table. I'm moving them to a <p> above the table. But the actual table header elements are now more properly contained. https://codereview.chromium.org/2648683004/diff/20001/dashboard/dashboard/ele... dashboard/dashboard/elements/speed-releasing-table.html:88: <tr> On 2017/01/26 at 03:44:28, sullivan wrote: > It's really confusing that we have two <tr> tags in different templates and only one </tr> tag to close them. And the indentation isn't lined up to look like it's part of the template. Any reason not to pull this <tr> and the one on line 95 up into a single <tr> right after line 85 before these dom-if templates? Wow, that worked! I was struggling for a while to figure out the how to make the rowspan idea work and was looking at the health dashboard code, which is why I landed there. Since the <tr> needs to be created regardless I can also remove the second dom-if. It also originally included a 'separator' which would create a line across the entire table, but with the way the dom is created with polymer it wasn't able to understand that - the UI CL will be where I figure that out. It also removed the magic *2 from below. Note: This CL (https://codereview.chromium.org/2662633002) ends up removing the stats idea entirely. https://codereview.chromium.org/2648683004/diff/20001/dashboard/dashboard/ele... dashboard/dashboard/elements/speed-releasing-table.html:161: getValue: function(rev, bot, test, kind) { On 2017/01/26 at 03:44:28, sullivan wrote: > On 2017/01/25 22:47:25, jessimb wrote: > > 'kind' is one of the stats (line 149) > > Having jsdoc to explain the arguments would help. Or at least a code comment explaining that it would be 'avg', 'std', 'max', etc. Good call. Note - the kind idea is gone with https://codereview.chromium.org/2662633002. https://codereview.chromium.org/2648683004/diff/20001/dashboard/dashboard/ele... dashboard/dashboard/elements/speed-releasing-table.html:212: return this.tableConfig.categories[category] * 2; On 2017/01/25 at 22:47:25, jessimb wrote: > When I multiplied by 2 it worked. That is the reason. No longer needed. https://codereview.chromium.org/2648683004/diff/20001/dashboard/dashboard/mod... File dashboard/dashboard/models/table_config.py (right): https://codereview.chromium.org/2648683004/diff/20001/dashboard/dashboard/mod... dashboard/dashboard/models/table_config.py:102: return None, error_message On 2017/01/26 at 03:44:28, sullivan wrote: > I think you should also actually do a query to make sure these tests exist, and return an error message if not. The _* idea is no longer a good one - @perezju and co have stated that we only really need average values. This CL (https://codereview.chromium.org/2662633002) removes _* and does the testing you request here. https://codereview.chromium.org/2648683004/diff/20001/dashboard/dashboard/spe... File dashboard/dashboard/speed_releasing.py (right): https://codereview.chromium.org/2648683004/diff/20001/dashboard/dashboard/spe... dashboard/dashboard/speed_releasing.py:123: def _CheckRevisions(rev_a, rev_b): On 2017/01/25 at 22:47:25, jessimb wrote: > Expect a CL to revamp this to allow for default revs. https://codereview.chromium.org/2657043002
https://codereview.chromium.org/2648683004/diff/40001/dashboard/dashboard/cre... File dashboard/dashboard/create_health_report_test.py (right): https://codereview.chromium.org/2648683004/diff/40001/dashboard/dashboard/cre... dashboard/dashboard/create_health_report_test.py:22: 'tableTests': 'my_test_suite/my_test_*\nmy_test_suite/my_other_test_*', I would be in favor of hardcoding _avg here and throughout and modifying the table to just show the averages for now. I wonder if we can break the additional column functionality out into another WIP CL that blocks on histograms being uploaded. https://codereview.chromium.org/2648683004/diff/40001/dashboard/dashboard/ele... File dashboard/dashboard/elements/speed-releasing-table.html (right): https://codereview.chromium.org/2648683004/diff/40001/dashboard/dashboard/ele... dashboard/dashboard/elements/speed-releasing-table.html:185: if (abs == 'true') { nit: can you use === here and throughout? https://codereview.chromium.org/2648683004/diff/40001/dashboard/dashboard/spe... File dashboard/dashboard/speed_releasing.py (right): https://codereview.chromium.org/2648683004/diff/40001/dashboard/dashboard/spe... dashboard/dashboard/speed_releasing.py:90: """Builds the 'values' JSON.""" I think this docstring could be more descriptive: what does this JSON look like? It would be good to document that somewhere in the code.
Description was changed from ========== Building the table for speed releasing. Adds the actual table in. Creates all the JSON needed on the server side to be easily consumed on the client side. Also updates table_config to be slightly more restrictive. http://i.imgur.com/vf9UnsD.png Demo: https://dev-jessimb-be576fcf-dot-chromeperf.appspot.com/speed_releasing/Memor... (must be logged in) BUG=catapult:#3141 ========== to ========== Building the table for speed releasing. Adds the actual table in. Creates all the JSON needed on the server side to be easily consumed on the client side. Also updates table_config to be more restrictive. http://i.imgur.com/nyh3iSA.png Demo: https://dev-jessimb-be576fcf-dot-chromeperf.appspot.com/speed_releasing/Memor... (must be logged in) BUG=catapult:#3141 ==========
Description was changed from ========== Building the table for speed releasing. Adds the actual table in. Creates all the JSON needed on the server side to be easily consumed on the client side. Also updates table_config to be more restrictive. http://i.imgur.com/nyh3iSA.png Demo: https://dev-jessimb-be576fcf-dot-chromeperf.appspot.com/speed_releasing/Memor... (must be logged in) BUG=catapult:#3141 ========== to ========== Building the table for speed releasing. Adds the actual table in. Creates all the JSON needed on the server side to be easily consumed on the client side. Also updates table_config to be more restrictive. http://i.imgur.com/nyh3iSA.png Demo: https://dev-jessimb-34e039e3-dot-chromeperf.appspot.com/speed_releasing/Memor... (must be logged in) BUG=catapult:#3141 ==========
PTAL - I merged https://codereview.chromium.org/2662633002 into this one, so we simply never added the unnecessary complexity in. https://codereview.chromium.org/2648683004/diff/40001/dashboard/dashboard/cre... File dashboard/dashboard/create_health_report_test.py (right): https://codereview.chromium.org/2648683004/diff/40001/dashboard/dashboard/cre... dashboard/dashboard/create_health_report_test.py:22: 'tableTests': 'my_test_suite/my_test_*\nmy_test_suite/my_other_test_*', On 2017/01/27 at 22:12:43, eakuefner wrote: > I would be in favor of hardcoding _avg here and throughout and modifying the table to just show the averages for now. I wonder if we can break the additional column functionality out into another WIP CL that blocks on histograms being uploaded. Done! Bringing the 'Paring down' CL into this one. I think I'll keep out the additional functionality until there is proof of needing it. I can always look back at this patch for it! https://codereview.chromium.org/2648683004/diff/40001/dashboard/dashboard/ele... File dashboard/dashboard/elements/speed-releasing-table.html (right): https://codereview.chromium.org/2648683004/diff/40001/dashboard/dashboard/ele... dashboard/dashboard/elements/speed-releasing-table.html:185: if (abs == 'true') { On 2017/01/27 at 22:12:43, eakuefner wrote: > nit: can you use === here and throughout? Javascript tripping me up again. https://codereview.chromium.org/2648683004/diff/40001/dashboard/dashboard/spe... File dashboard/dashboard/speed_releasing.py (right): https://codereview.chromium.org/2648683004/diff/40001/dashboard/dashboard/spe... dashboard/dashboard/speed_releasing.py:90: """Builds the 'values' JSON.""" On 2017/01/27 at 22:12:43, eakuefner wrote: > I think this docstring could be more descriptive: what does this JSON look like? It would be good to document that somewhere in the code. Yes, the structure of this dict would be good to show here. Done! Also, the speed-releasing-table-test.html file has a good example of what the values dict should end up looking like.
https://codereview.chromium.org/2648683004/diff/40001/dashboard/dashboard/cre... File dashboard/dashboard/create_health_report_test.py (right): https://codereview.chromium.org/2648683004/diff/40001/dashboard/dashboard/cre... dashboard/dashboard/create_health_report_test.py:22: 'tableTests': 'my_test_suite/my_test_*\nmy_test_suite/my_other_test_*', On 2017/01/27 22:12:43, eakuefner wrote: > I would be in favor of hardcoding _avg here and throughout and modifying the > table to just show the averages for now. I wonder if we can break the additional > column functionality out into another WIP CL that blocks on histograms being > uploaded. I'd go even further and just keep this a normal test_path, and not add support for concepts like avg/std/max yet. Did I miss a reason we need those things?
On 2017/01/27 at 22:50:04, sullivan wrote: > https://codereview.chromium.org/2648683004/diff/40001/dashboard/dashboard/cre... > File dashboard/dashboard/create_health_report_test.py (right): > > https://codereview.chromium.org/2648683004/diff/40001/dashboard/dashboard/cre... > dashboard/dashboard/create_health_report_test.py:22: 'tableTests': 'my_test_suite/my_test_*\nmy_test_suite/my_other_test_*', > On 2017/01/27 22:12:43, eakuefner wrote: > > I would be in favor of hardcoding _avg here and throughout and modifying the > > table to just show the averages for now. I wonder if we can break the additional > > column functionality out into another WIP CL that blocks on histograms being > > uploaded. > > I'd go even further and just keep this a normal test_path, and not add support for concepts like avg/std/max yet. Did I miss a reason we need those things? _avg is just a normal test path? If I remove the header that says 'Average' that would be entirely removing the concept. Should I remove all traces of average, or just keep this to average? Juan sounded like he found average useful. The support for max/std/count is already removed. Regardless of which test paths you feed in, the table will work as intended (minus the average heading potentially being incorrect)
On 2017/01/27 23:06:50, jessimb wrote: > On 2017/01/27 at 22:50:04, sullivan wrote: > > > https://codereview.chromium.org/2648683004/diff/40001/dashboard/dashboard/cre... > > File dashboard/dashboard/create_health_report_test.py (right): > > > > > https://codereview.chromium.org/2648683004/diff/40001/dashboard/dashboard/cre... > > dashboard/dashboard/create_health_report_test.py:22: 'tableTests': > 'my_test_suite/my_test_*\nmy_test_suite/my_other_test_*', > > On 2017/01/27 22:12:43, eakuefner wrote: > > > I would be in favor of hardcoding _avg here and throughout and modifying the > > > table to just show the averages for now. I wonder if we can break the > additional > > > column functionality out into another WIP CL that blocks on histograms being > > > uploaded. > > > > I'd go even further and just keep this a normal test_path, and not add support > for concepts like avg/std/max yet. Did I miss a reason we need those things? > > _avg is just a normal test path? If I remove the header that says 'Average' that > would be entirely removing the concept. Should I remove all traces of average, > or just keep this to average? Juan sounded like he found average useful. The > support for max/std/count is already removed. Regardless of which test paths you > feed in, the table will work as intended (minus the average heading potentially > being incorrect) Do you know what Juan meant by average? Is it just the average of the pages? If so, it is just a normal test path, and I think whether or not it has "_avg" in the name depends on whether it is a tbm2 metric.
On 2017/01/27 at 23:08:42, sullivan wrote: > On 2017/01/27 23:06:50, jessimb wrote: > > On 2017/01/27 at 22:50:04, sullivan wrote: > > > > > https://codereview.chromium.org/2648683004/diff/40001/dashboard/dashboard/cre... > > > File dashboard/dashboard/create_health_report_test.py (right): > > > > > > > > https://codereview.chromium.org/2648683004/diff/40001/dashboard/dashboard/cre... > > > dashboard/dashboard/create_health_report_test.py:22: 'tableTests': > > 'my_test_suite/my_test_*\nmy_test_suite/my_other_test_*', > > > On 2017/01/27 22:12:43, eakuefner wrote: > > > > I would be in favor of hardcoding _avg here and throughout and modifying the > > > > table to just show the averages for now. I wonder if we can break the > > additional > > > > column functionality out into another WIP CL that blocks on histograms being > > > > uploaded. > > > > > > I'd go even further and just keep this a normal test_path, and not add support > > for concepts like avg/std/max yet. Did I miss a reason we need those things? > > > > _avg is just a normal test path? If I remove the header that says 'Average' that > > would be entirely removing the concept. Should I remove all traces of average, > > or just keep this to average? Juan sounded like he found average useful. The > > support for max/std/count is already removed. Regardless of which test paths you > > feed in, the table will work as intended (minus the average heading potentially > > being incorrect) > > Do you know what Juan meant by average? Is it just the average of the pages? If so, it is just a normal test path, and I think whether or not it has "_avg" in the name depends on whether it is a tbm2 metric. Hm. In his case average was the summary statistic from chart json. In terms of this table, I think any normal test path will do. I don't require _avg to be in the name; I will remove it from the testing files. Shall I also remove the 'Average' heading? We aren't doing any math here, so there is no guarantee that that is truly the average of anything.
On 2017/01/27 23:13:41, jessimb wrote: > On 2017/01/27 at 23:08:42, sullivan wrote: > > On 2017/01/27 23:06:50, jessimb wrote: > > > On 2017/01/27 at 22:50:04, sullivan wrote: > > > > > > > > https://codereview.chromium.org/2648683004/diff/40001/dashboard/dashboard/cre... > > > > File dashboard/dashboard/create_health_report_test.py (right): > > > > > > > > > > > > https://codereview.chromium.org/2648683004/diff/40001/dashboard/dashboard/cre... > > > > dashboard/dashboard/create_health_report_test.py:22: 'tableTests': > > > 'my_test_suite/my_test_*\nmy_test_suite/my_other_test_*', > > > > On 2017/01/27 22:12:43, eakuefner wrote: > > > > > I would be in favor of hardcoding _avg here and throughout and modifying > the > > > > > table to just show the averages for now. I wonder if we can break the > > > additional > > > > > column functionality out into another WIP CL that blocks on histograms > being > > > > > uploaded. > > > > > > > > I'd go even further and just keep this a normal test_path, and not add > support > > > for concepts like avg/std/max yet. Did I miss a reason we need those things? > > > > > > _avg is just a normal test path? If I remove the header that says 'Average' > that > > > would be entirely removing the concept. Should I remove all traces of > average, > > > or just keep this to average? Juan sounded like he found average useful. The > > > support for max/std/count is already removed. Regardless of which test paths > you > > > feed in, the table will work as intended (minus the average heading > potentially > > > being incorrect) > > > > Do you know what Juan meant by average? Is it just the average of the pages? > If so, it is just a normal test path, and I think whether or not it has "_avg" > in the name depends on whether it is a tbm2 metric. > > Hm. In his case average was the summary statistic from chart json. In terms of > this table, I think any normal test path will do. I don't require _avg to be in > the name; I will remove it from the testing files. Shall I also remove the > 'Average' heading? We aren't doing any math here, so there is no guarantee that > that is truly the average of anything. The summary statistic from chartjson is just stored in a testpath. Yep, that sounds good to me. We can put it back in when we've had a chance to review the math.
Description was changed from ========== Building the table for speed releasing. Adds the actual table in. Creates all the JSON needed on the server side to be easily consumed on the client side. Also updates table_config to be more restrictive. http://i.imgur.com/nyh3iSA.png Demo: https://dev-jessimb-34e039e3-dot-chromeperf.appspot.com/speed_releasing/Memor... (must be logged in) BUG=catapult:#3141 ========== to ========== Building the table for speed releasing. Adds the actual table in. Creates all the JSON needed on the server side to be easily consumed on the client side. Also updates table_config to be more restrictive. http://i.imgur.com/PrbDCyb.png Demo: https://dev-jessimb-7e894f97-dot-chromeperf.appspot.com/speed_releasing/Memor... (must be logged in) BUG=catapult:#3141 ==========
PTAL. All ideas of statistics should be gone now!
Sorry, yeah. This looks correct, because now table authors can just specify test paths (which might end in _avg, for now, for TBMv2 metrics, which is the source of the confusion), and later when those test path/rev pairs happen to point to histograms, we can add more columns. The former version with the columns assumed that they're TBMv2 tests; this one does not and does not need to.
Another round of comments. I think the architecture is probably pretty good at this point. https://codereview.chromium.org/2648683004/diff/100001/dashboard/dashboard/cr... File dashboard/dashboard/create_health_report_test.py (right): https://codereview.chromium.org/2648683004/diff/100001/dashboard/dashboard/cr... dashboard/dashboard/create_health_report_test.py:52: self._AddTests() # This function seems to break the internal_only of the Yikes, this is scary. Can we figure out what's going on here so we don't have to do this? Is it a bug in the mocks? https://codereview.chromium.org/2648683004/diff/100001/dashboard/dashboard/el... File dashboard/dashboard/elements/speed-releasing-table-test.html (right): https://codereview.chromium.org/2648683004/diff/100001/dashboard/dashboard/el... dashboard/dashboard/elements/speed-releasing-table-test.html:28: { why are there so many spaces here? these should be 2-space indented. https://codereview.chromium.org/2648683004/diff/100001/dashboard/dashboard/mo... File dashboard/dashboard/models/table_config.py (right): https://codereview.chromium.org/2648683004/diff/100001/dashboard/dashboard/mo... dashboard/dashboard/models/table_config.py:105: return None, error_message It occurs to me that this pattern is kind of funky. Can you throw exceptions in these cases instead of having this function return a tuple? This is an example of a place where you would throw a BadRequestError, and then let it propagate up to the handler, which can catch it and do self.ReportError. Check out how add_point does this: https://github.com/catapult-project/catapult/blob/master/dashboard/dashboard/... https://codereview.chromium.org/2648683004/diff/100001/dashboard/dashboard/sp... File dashboard/dashboard/speed_releasing.py (right): https://codereview.chromium.org/2648683004/diff/100001/dashboard/dashboard/sp... dashboard/dashboard/speed_releasing.py:83: """Recreates the master/bot path.""" nit: This docstring is unnecessary. https://codereview.chromium.org/2648683004/diff/100001/dashboard/dashboard/sp... dashboard/dashboard/speed_releasing.py:90: """Builds the 'values' JSON. I can't tell what this function does based on the short description. Is it possible to be more descriptive within the line length? Something like: Builds a nested dict organizing values by rev/bot/test. https://codereview.chromium.org/2648683004/diff/100001/dashboard/dashboard/sp... dashboard/dashboard/speed_releasing.py:149: # TODO(jessimb): Check if r_commit_pos (if clank), if so return revision. Can you link a bug here? https://codereview.chromium.org/2648683004/diff/100001/dashboard/dashboard/sp... dashboard/dashboard/speed_releasing.py:153: """Returns a dict of categories: # of times a test is in that category.""" similarly, i think the name of this method stands for itself. https://codereview.chromium.org/2648683004/diff/100001/dashboard/dashboard/sp... dashboard/dashboard/speed_releasing.py:154: categories = {} you should use a defaultdict here categories = collections.defaultdict(lambda: 0) for test in layout: categories[layout[test][0]] += 1 return categories
PTAL https://codereview.chromium.org/2648683004/diff/100001/dashboard/dashboard/cr... File dashboard/dashboard/create_health_report_test.py (right): https://codereview.chromium.org/2648683004/diff/100001/dashboard/dashboard/cr... dashboard/dashboard/create_health_report_test.py:52: self._AddTests() # This function seems to break the internal_only of the On 2017/01/28 at 00:22:17, eakuefner wrote: > Yikes, this is scary. Can we figure out what's going on here so we don't have to do this? Is it a bug in the mocks? Ah! I thought I had looked into this but with fresh eyes, I see that the Master and bots that are passed to testing_common.AddTests() are being recreated. So they are being overwritten and hence losing their internal_only status. I removed the portion where I create the bots, and rely on _AddTests to do that; I am keeping the part to directly change the internal_only. https://codereview.chromium.org/2648683004/diff/100001/dashboard/dashboard/el... File dashboard/dashboard/elements/speed-releasing-table-test.html (right): https://codereview.chromium.org/2648683004/diff/100001/dashboard/dashboard/el... dashboard/dashboard/elements/speed-releasing-table-test.html:28: { On 2017/01/28 at 00:22:17, eakuefner wrote: > why are there so many spaces here? these should be 2-space indented. Fixed. https://codereview.chromium.org/2648683004/diff/100001/dashboard/dashboard/mo... File dashboard/dashboard/models/table_config.py (right): https://codereview.chromium.org/2648683004/diff/100001/dashboard/dashboard/mo... dashboard/dashboard/models/table_config.py:105: return None, error_message On 2017/01/28 at 00:22:17, eakuefner wrote: > It occurs to me that this pattern is kind of funky. Can you throw exceptions in these cases instead of having this function return a tuple? > > This is an example of a place where you would throw a BadRequestError, and then let it propagate up to the handler, which can catch it and do self.ReportError. Check out how add_point does this: https://github.com/catapult-project/catapult/blob/master/dashboard/dashboard/... Feels much cleaner! Note that I didn't use self.ReportError because simple_xhr will display the Http status code if it's not 200 and I want it to display the message regardless of the status. Instead, I still added the logging in and self.response.out.write(the error message). This way the error is recorded on the server and the user knows how to fix their inputs so it can actually validate the next time. https://codereview.chromium.org/2648683004/diff/100001/dashboard/dashboard/sp... File dashboard/dashboard/speed_releasing.py (right): https://codereview.chromium.org/2648683004/diff/100001/dashboard/dashboard/sp... dashboard/dashboard/speed_releasing.py:83: """Recreates the master/bot path.""" On 2017/01/28 at 00:22:17, eakuefner wrote: > nit: This docstring is unnecessary. Removed. https://codereview.chromium.org/2648683004/diff/100001/dashboard/dashboard/sp... dashboard/dashboard/speed_releasing.py:90: """Builds the 'values' JSON. On 2017/01/28 at 00:22:17, eakuefner wrote: > I can't tell what this function does based on the short description. Is it possible to be more descriptive within the line length? Something like: > > Builds a nested dict organizing values by rev/bot/test. Changed. https://codereview.chromium.org/2648683004/diff/100001/dashboard/dashboard/sp... dashboard/dashboard/speed_releasing.py:149: # TODO(jessimb): Check if r_commit_pos (if clank), if so return revision. On 2017/01/28 at 00:22:17, eakuefner wrote: > Can you link a bug here? It's not really a bug so much as the next CL for the table. Should I create a bug? I have the tracking bug for the whole effort; I'm not sure if it would make much sense to create a bug for this when I haven't been for anything else. https://codereview.chromium.org/2648683004/diff/100001/dashboard/dashboard/sp... dashboard/dashboard/speed_releasing.py:153: """Returns a dict of categories: # of times a test is in that category.""" On 2017/01/28 at 00:22:17, eakuefner wrote: > similarly, i think the name of this method stands for itself. Fair enough. https://codereview.chromium.org/2648683004/diff/100001/dashboard/dashboard/sp... dashboard/dashboard/speed_releasing.py:154: categories = {} On 2017/01/28 at 00:22:17, eakuefner wrote: > you should use a defaultdict here > > categories = collections.defaultdict(lambda: 0) > for test in layout: > categories[layout[test][0]] += 1 > > return categories Much cleaner!
lgtm lgtm when Ethan is happy https://codereview.chromium.org/2648683004/diff/120001/dashboard/dashboard/cr... File dashboard/dashboard/create_health_report.py (right): https://codereview.chromium.org/2648683004/diff/120001/dashboard/dashboard/cr... dashboard/dashboard/create_health_report.py:80: 'error': 'Could not create table.', Should probably also log an error here. Imagine that your users are getting this message. You'll want to check go/chromeperf-logs. But which request had the problem? Leaving a breadcrumb here can eliminate wasted time digging into the wrong request.
Thanks Annie! PTAL Ethan.
lgtm https://codereview.chromium.org/2648683004/diff/100001/dashboard/dashboard/sp... File dashboard/dashboard/speed_releasing.py (right): https://codereview.chromium.org/2648683004/diff/100001/dashboard/dashboard/sp... dashboard/dashboard/speed_releasing.py:149: # TODO(jessimb): Check if r_commit_pos (if clank), if so return revision. On 2017/01/30 at 19:23:10, jessimb wrote: > On 2017/01/28 at 00:22:17, eakuefner wrote: > > Can you link a bug here? > > It's not really a bug so much as the next CL for the table. Should I create a bug? I have the tracking bug for the whole effort; I'm not sure if it would make much sense to create a bug for this when I haven't been for anything else. Okay, that's fine.
The CQ bit was checked by jessimb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sullivan@chromium.org Link to the patchset: https://codereview.chromium.org/2648683004/#ps140001 (title: "Added more logging")
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": 140001, "attempt_start_ts": 1486058728537620,
"parent_rev": "dab1be704d83c5b6cf790f52181dd879ff8beed6", "commit_rev":
"ca6e2c31a44021b7aa189333638674d38f6eaec9"}
Message was sent while issue was closed.
Description was changed from ========== Building the table for speed releasing. Adds the actual table in. Creates all the JSON needed on the server side to be easily consumed on the client side. Also updates table_config to be more restrictive. http://i.imgur.com/PrbDCyb.png Demo: https://dev-jessimb-7e894f97-dot-chromeperf.appspot.com/speed_releasing/Memor... (must be logged in) BUG=catapult:#3141 ========== to ========== Building the table for speed releasing. Adds the actual table in. Creates all the JSON needed on the server side to be easily consumed on the client side. Also updates table_config to be more restrictive. http://i.imgur.com/PrbDCyb.png Demo: https://dev-jessimb-7e894f97-dot-chromeperf.appspot.com/speed_releasing/Memor... (must be logged in) BUG=catapult:#3141 Review-Url: https://codereview.chromium.org/2648683004 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
Message was sent while issue was closed.
perezju@chromium.org changed reviewers: + perezju@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2648683004/diff/140001/dashboard/dashboard/sp... File dashboard/dashboard/speed_releasing.py (right): https://codereview.chromium.org/2648683004/diff/140001/dashboard/dashboard/sp... dashboard/dashboard/speed_releasing.py:139: return row_key.get().value I know getting other stats is low priority now; but I believe from here .value gets you the value to report as mean, while .error would give you what we show as stdev.
Message was sent while issue was closed.
saleenarobinson2015@gmail.com changed reviewers: + saleenarobinson2015@gmail.com
Message was sent while issue was closed.
lgtm |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
