Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
545803002:
Update buildbots to parse new telemetry JSON format. (Closed)
|
Created:
6 years, 3 months ago by sullivan Modified:
6 years, 3 months ago CC:
chromium-reviews, pgervais+watch_chromium.org, kjellander-cc_chromium.org, cmp-cc_chromium.org, stip+watch_chromium.org, tonyg, smut, luqui Base URL:
https://chromium.googlesource.com/chromium/tools/build.git@master Project:
tools Visibility:
Public. |
DescriptionAdd a new parser for telemetry JSON format, and tell telemetry to use the new format.
BUG=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=291952
Patch Set 1 #
Total comments: 14
Patch Set 2 : Implement sending results to dashboard. This still needs tests. #Patch Set 3 : Switched to having telemetry output json to files. #
Total comments: 19
Patch Set 4 : Addressed review comments; still need to write/update test and remove temp code #Patch Set 5 : updated existing tests #Patch Set 6 : Added unit tests and removed debugging code #
Total comments: 4
Patch Set 7 : Addressed review comments #
Messages
Total messages: 29 (5 generated)
sullivan@chromium.org changed reviewers: + stip@chromium.org
Mike, this isn't ready for review at all, but I had a bunch of questions when implementing and putting them inline in a code review seemed like the best way to ask. https://codereview.chromium.org/545803002/diff/1/scripts/slave/runtest.py File scripts/slave/runtest.py (right): https://codereview.chromium.org/545803002/diff/1/scripts/slave/runtest.py#new... scripts/slave/runtest.py:571: return telemetry_utils.TelemetryLogParser Is this the best way to tell if it's a telemetry test? https://codereview.chromium.org/545803002/diff/1/scripts/slave/runtest.py#new... scripts/slave/runtest.py:699: results_dashboard.SendChartJsonResults( Still working on implementing this. https://codereview.chromium.org/545803002/diff/1/scripts/slave/runtest.py#new... scripts/slave/runtest.py:976: tracker_class = _SelectResultsTracker(options, None) This should never get called for Telemetry tests, right? https://codereview.chromium.org/545803002/diff/1/scripts/slave/runtest.py#new... scripts/slave/runtest.py:1105: def _MainIOS(options, args, extra_env): Is telemetry available on iOS? Not sure how perf test result parsing works on iOS now. https://codereview.chromium.org/545803002/diff/1/scripts/slave/runtest.py#new... scripts/slave/runtest.py:1503: def _MainAndroid(options, args, extra_env): The command line looks different for Android, I think because recipes has been enabled on Android: http://chromegw/i/chromium.perf/builders/Android%20Nexus5%20Perf/builds/1580/... Do you know any way to check if we should use the TelemetryLogParser here? Ideally we'd do something future-proof for when the other platforms switch over to recipes. https://codereview.chromium.org/545803002/diff/1/scripts/slave/telemetry_util... File scripts/slave/telemetry_utils.py (right): https://codereview.chromium.org/545803002/diff/1/scripts/slave/telemetry_util... scripts/slave/telemetry_utils.py:28: def ProcessLine(self, line): Here is sample output from running the test: https://paste.googleplex.com/6080686586855424 It looks to me like the first test results are the build under test, and the second (if any) are the ref build. Is that a reasonable assumption? Is there a better way to get the two results?
I finished the implementation. I'm still working on tests, but would really like some feedback on this! https://codereview.chromium.org/545803002/diff/1/scripts/slave/runtest.py File scripts/slave/runtest.py (right): https://codereview.chromium.org/545803002/diff/1/scripts/slave/runtest.py#new... scripts/slave/runtest.py:699: results_dashboard.SendChartJsonResults( On 2014/09/05 04:30:00, sullivan wrote: > Still working on implementing this. This is implemented now.
https://chromiumcodereview.appspot.com/545803002/diff/1/scripts/slave/runtest.py File scripts/slave/runtest.py (right): https://chromiumcodereview.appspot.com/545803002/diff/1/scripts/slave/runtest... scripts/slave/runtest.py:571: return telemetry_utils.TelemetryLogParser On 2014/09/05 04:30:00, sullivan wrote: > Is this the best way to tell if it's a telemetry test? Unfortunately yeah, if you want skip a master restart this is the best way. https://chromiumcodereview.appspot.com/545803002/diff/1/scripts/slave/runtest... scripts/slave/runtest.py:976: tracker_class = _SelectResultsTracker(options, None) On 2014/09/05 04:30:00, sullivan wrote: > This should never get called for Telemetry tests, right? Right, there is no binary here https://chromiumcodereview.appspot.com/545803002/diff/1/scripts/slave/runtest... scripts/slave/runtest.py:1105: def _MainIOS(options, args, extra_env): On 2014/09/05 04:30:00, sullivan wrote: > Is telemetry available on iOS? Not sure how perf test result parsing works on > iOS now. I'm not sure. smut@? https://chromiumcodereview.appspot.com/545803002/diff/1/scripts/slave/runtest... scripts/slave/runtest.py:1503: def _MainAndroid(options, args, extra_env): On 2014/09/05 04:30:00, sullivan wrote: > The command line looks different for Android, I think because recipes has been > enabled on Android: > > http://chromegw/i/chromium.perf/builders/Android%20Nexus5%20Perf/builds/1580/... > > Do you know any way to check if we should use the TelemetryLogParser here? > Ideally we'd do something future-proof for when the other platforms switch over > to recipes. Android uses a far different way of invoking tests, because they have their own custom runner script. I'm looping in luqui@ who might be able to help. https://chromiumcodereview.appspot.com/545803002/diff/1/scripts/slave/telemet... File scripts/slave/telemetry_utils.py (right): https://chromiumcodereview.appspot.com/545803002/diff/1/scripts/slave/telemet... scripts/slave/telemetry_utils.py:28: def ProcessLine(self, line): On 2014/09/05 04:30:00, sullivan wrote: > Here is sample output from running the test: > https://paste.googleplex.com/6080686586855424 > > It looks to me like the first test results are the build under test, and the > second (if any) are the ref build. Is that a reasonable assumption? Is there a > better way to get the two results? I was under the impression that telemetry would drop its json to a file, instead of spitting out to stdout. We're *strongly* discouraging log parsing, and emitting the result json to a file is the easiest way to do this.
+smut for question about if/how telemetry runs on iOS +luqui for question about how to check if an android build is running telemetry https://chromiumcodereview.appspot.com/545803002/diff/1/scripts/slave/telemet... File scripts/slave/telemetry_utils.py (right): https://chromiumcodereview.appspot.com/545803002/diff/1/scripts/slave/telemet... scripts/slave/telemetry_utils.py:28: def ProcessLine(self, line): On 2014/09/05 21:39:06, stip wrote: > On 2014/09/05 04:30:00, sullivan wrote: > > Here is sample output from running the test: > > https://paste.googleplex.com/6080686586855424 > > > > It looks to me like the first test results are the build under test, and the > > second (if any) are the ref build. Is that a reasonable assumption? Is there a > > better way to get the two results? > > I was under the impression that telemetry would drop its json to a file, instead > of spitting out to stdout. We're *strongly* discouraging log parsing, and > emitting the result json to a file is the easiest way to do this. Oops, I think I gave eakuefner some out-of-date advice here. Ethan, would you be okay re-implementing the telemetry side to spit out files? I assume we'll need to pass some argument for the filename to telemetry, right? And a different one for the ref vs non-ref builds. What's the best way to do that?
smut@google.com changed reviewers: + smut@google.com
https://chromiumcodereview.appspot.com/545803002/diff/1/scripts/slave/runtest.py File scripts/slave/runtest.py (right): https://chromiumcodereview.appspot.com/545803002/diff/1/scripts/slave/runtest... scripts/slave/runtest.py:1105: def _MainIOS(options, args, extra_env): On 2014/09/05 04:30:00, sullivan wrote: > Is telemetry available on iOS? Not sure how perf test result parsing works on > iOS now. Perf tests only run downstream on iOS, and downstream doesn't invoke runtest.py at all. Telemetry tests don't currently run on iOS up- or downstream, but baxley@ is working on it. If we get telemetry tests running upstream, we'll probably need support here, and for downstream I can point you to where the TelemetryLogParser can be integrated.
stip@chromium.org changed reviewers: + luqui@chromium.org
(actually add luqui)
On 2014/09/05 21:47:03, sullivan wrote: > +smut for question about if/how telemetry runs on iOS > > +luqui for question about how to check if an android build is running telemetry > > https://chromiumcodereview.appspot.com/545803002/diff/1/scripts/slave/telemet... > File scripts/slave/telemetry_utils.py (right): > > https://chromiumcodereview.appspot.com/545803002/diff/1/scripts/slave/telemet... > scripts/slave/telemetry_utils.py:28: def ProcessLine(self, line): > On 2014/09/05 21:39:06, stip wrote: > > On 2014/09/05 04:30:00, sullivan wrote: > > > Here is sample output from running the test: > > > https://paste.googleplex.com/6080686586855424 > > > > > > It looks to me like the first test results are the build under test, and the > > > second (if any) are the ref build. Is that a reasonable assumption? Is there > a > > > better way to get the two results? > > > > I was under the impression that telemetry would drop its json to a file, > instead > > of spitting out to stdout. We're *strongly* discouraging log parsing, and > > emitting the result json to a file is the easiest way to do this. > > Oops, I think I gave eakuefner some out-of-date advice here. Ethan, would you be > okay re-implementing the telemetry side to spit out files? > > I assume we'll need to pass some argument for the filename to telemetry, right? > And a different one for the ref vs non-ref builds. What's the best way to do > that? Yes, the gtest tests currently accept a target json file (usually a name created in /tmp). Could you specify that in the json itself? {'ref': ..., 'non-ref': ...}
On 2014/09/05 21:47:03, sullivan wrote: > Oops, I think I gave eakuefner some out-of-date advice here. Ethan, would you be > okay re-implementing the telemetry side to spit out files? Whoops, we spoke about this out-of-band but just to confirm, yes, that's not a problem on my end. I have a series of patches I'm trying to put in flight and I'll be able to include this change in those.
On 2014/09/06 01:14:27, stip wrote: > On 2014/09/05 21:47:03, sullivan wrote: > > +smut for question about if/how telemetry runs on iOS > > > > +luqui for question about how to check if an android build is running > telemetry > > > > > https://chromiumcodereview.appspot.com/545803002/diff/1/scripts/slave/telemet... > > File scripts/slave/telemetry_utils.py (right): > > > > > https://chromiumcodereview.appspot.com/545803002/diff/1/scripts/slave/telemet... > > scripts/slave/telemetry_utils.py:28: def ProcessLine(self, line): > > On 2014/09/05 21:39:06, stip wrote: > > > On 2014/09/05 04:30:00, sullivan wrote: > > > > Here is sample output from running the test: > > > > https://paste.googleplex.com/6080686586855424 > > > > > > > > It looks to me like the first test results are the build under test, and > the > > > > second (if any) are the ref build. Is that a reasonable assumption? Is > there > > a > > > > better way to get the two results? > > > > > > I was under the impression that telemetry would drop its json to a file, > > instead > > > of spitting out to stdout. We're *strongly* discouraging log parsing, and > > > emitting the result json to a file is the easiest way to do this. > > > > Oops, I think I gave eakuefner some out-of-date advice here. Ethan, would you > be > > okay re-implementing the telemetry side to spit out files? > > > > I assume we'll need to pass some argument for the filename to telemetry, > right? > > And a different one for the ref vs non-ref builds. What's the best way to do > > that? > > Yes, the gtest tests currently accept a target json file (usually a name created > in /tmp). Could you specify that in the json itself? {'ref': ..., 'non-ref': > ...} I'm confused with all the different places this code is using JSON. Which JSON would I specify that in? I'm assuming once it's specified, I'd read it in telemetry.py to send an argument to telemetry for the build under test and the ref build in _GenerateTelemetryCommandSequence() Once I'd figured out how to share the filenames between runtest.py and telemetry.py, I'd create a temp directory using tempfile.mkdtemp() in runtest.py, pass that to telemetry.py, and in telemetry.py, pass the actual telemetry command constant filenames in that directory like results.json and ref.json. Is that consistent with how other buildbot code does stuff like this?
On 2014/09/06 03:26:58, sullivan wrote: > On 2014/09/06 01:14:27, stip wrote: > > On 2014/09/05 21:47:03, sullivan wrote: > > > +smut for question about if/how telemetry runs on iOS > > > > > > +luqui for question about how to check if an android build is running > > telemetry > > > > > > > > > https://chromiumcodereview.appspot.com/545803002/diff/1/scripts/slave/telemet... > > > File scripts/slave/telemetry_utils.py (right): > > > > > > > > > https://chromiumcodereview.appspot.com/545803002/diff/1/scripts/slave/telemet... > > > scripts/slave/telemetry_utils.py:28: def ProcessLine(self, line): > > > On 2014/09/05 21:39:06, stip wrote: > > > > On 2014/09/05 04:30:00, sullivan wrote: > > > > > Here is sample output from running the test: > > > > > https://paste.googleplex.com/6080686586855424 > > > > > > > > > > It looks to me like the first test results are the build under test, and > > the > > > > > second (if any) are the ref build. Is that a reasonable assumption? Is > > there > > > a > > > > > better way to get the two results? > > > > > > > > I was under the impression that telemetry would drop its json to a file, > > > instead > > > > of spitting out to stdout. We're *strongly* discouraging log parsing, and > > > > emitting the result json to a file is the easiest way to do this. > > > > > > Oops, I think I gave eakuefner some out-of-date advice here. Ethan, would > you > > be > > > okay re-implementing the telemetry side to spit out files? > > > > > > I assume we'll need to pass some argument for the filename to telemetry, > > right? > > > And a different one for the ref vs non-ref builds. What's the best way to do > > > that? > > > > Yes, the gtest tests currently accept a target json file (usually a name > created > > in /tmp). Could you specify that in the json itself? {'ref': ..., 'non-ref': > > ...} > > I'm confused with all the different places this code is using JSON. Which JSON > would I specify that in? > > I'm assuming once it's specified, I'd read it in telemetry.py to send an > argument to telemetry for the build under test and the ref build in > _GenerateTelemetryCommandSequence() > > Once I'd figured out how to share the filenames between runtest.py and > telemetry.py, I'd create a temp directory using tempfile.mkdtemp() in > runtest.py, pass that to telemetry.py, and in telemetry.py, pass the actual > telemetry command constant filenames in that directory like results.json and > ref.json. Is that consistent with how other buildbot code does stuff like this? I updated this with a draft that uses the TelemetryResultsTracker to manage the json files. @stip, if this basic approach looks okay to you, I will clean up the code and add tests. WDYT?
I think this approach looks pretty good. I've written a few comments, I mainly wanted to get some clarification on how the reference files worked. https://chromiumcodereview.appspot.com/545803002/diff/40001/scripts/common/ch... File scripts/common/chromium_utils.py (right): https://chromiumcodereview.appspot.com/545803002/diff/40001/scripts/common/ch... scripts/common/chromium_utils.py:1232: # TESTING. DO NOT SUBMIT. lol https://chromiumcodereview.appspot.com/545803002/diff/40001/scripts/slave/res... File scripts/slave/results_dashboard.py (right): https://chromiumcodereview.appspot.com/545803002/diff/40001/scripts/slave/res... scripts/slave/results_dashboard.py:12: import logging left in from debugging? https://chromiumcodereview.appspot.com/545803002/diff/40001/scripts/slave/res... scripts/slave/results_dashboard.py:56: if ref_json: so the file will always exist, but the telemeterized binary may just write an empty {}? https://chromiumcodereview.appspot.com/545803002/diff/40001/scripts/slave/run... File scripts/slave/runtest.py (right): https://chromiumcodereview.appspot.com/545803002/diff/40001/scripts/slave/run... scripts/slave/runtest.py:691: def _SendResultsToDashboard(results_tracker, options): iannucci@ strongly dislikes slinging the options object around. what I think makes sense is preprocessing all this in the beginning, and have a 'results_dashboard dict' that gets passed to sendresultstodashboard when needed. { 'system': _GetPerfID(options), 'test': options.test_type, 'url': options.results_url, } etc https://chromiumcodereview.appspot.com/545803002/diff/40001/scripts/slave/tel... File scripts/slave/telemetry_utils.py (right): https://chromiumcodereview.appspot.com/545803002/diff/40001/scripts/slave/tel... scripts/slave/telemetry_utils.py:22: (_, self._chart_filename) = tempfile.mkstemp() you may need to close the file handle here, I'm not sure. https://chromiumcodereview.appspot.com/545803002/diff/40001/scripts/slave/tel... scripts/slave/telemetry_utils.py:26: '--ref-output-filename', self._ref_chart_filename]) I'm a bit confused about the ref output filename -- will the telemeterized binary always write to it? https://chromiumcodereview.appspot.com/545803002/diff/40001/scripts/slave/tel... scripts/slave/telemetry_utils.py:44: os.remove(self._ref_chart_filename) if the first one fails, we'll never get to the second one -- probably a good idea to have a _try_to_delete() function with the try block that gets called twice here
some extra comments https://chromiumcodereview.appspot.com/545803002/diff/40001/scripts/slave/res... File scripts/slave/results_dashboard.py (right): https://chromiumcodereview.appspot.com/545803002/diff/40001/scripts/slave/res... scripts/slave/results_dashboard.py:216: chart_data, master) this needs a prefix, right? 'r_' ? https://chromiumcodereview.appspot.com/545803002/diff/40001/scripts/slave/tel... File scripts/slave/telemetry_utils.py (right): https://chromiumcodereview.appspot.com/545803002/diff/40001/scripts/slave/tel... scripts/slave/telemetry_utils.py:41: def Cleanup(self): I don't think cleanup is called by runtest.py in this patchset
take a look at https://chromiumcodereview.appspot.com/548773004/ which may conflict a tiny bit with this CL
qyearsley@chromium.org changed reviewers: + eakuefner@chromium.org, qyearsley@chromium.org
Excellent, I didn't know that this change was already under-way :-)
https://chromiumcodereview.appspot.com/545803002/diff/40001/scripts/slave/res... File scripts/slave/results_dashboard.py (right): https://chromiumcodereview.appspot.com/545803002/diff/40001/scripts/slave/res... scripts/slave/results_dashboard.py:31: build_dir): What about having a function which just constructs the "Dashboard JSON v1" (which doesn't need to take a url or build_dir argument), and then use SendResults to send the output of that?
https://chromiumcodereview.appspot.com/545803002/diff/40001/scripts/slave/res... File scripts/slave/results_dashboard.py (right): https://chromiumcodereview.appspot.com/545803002/diff/40001/scripts/slave/res... scripts/slave/results_dashboard.py:216: chart_data, master) On 2014/09/09 02:24:16, stip wrote: > this needs a prefix, right? 'r_' ? Actually, to clarify, the r_ and a_ prefixes are added by the dashboard to make it match the current schema. Versions (r_ fields) should go in a dict associated with the 'versions' key, and annotations (a_ fields) should go in a dict associated with the 'supplemental' key.
On 2014/09/09 21:00:54, eakuefner wrote: > https://chromiumcodereview.appspot.com/545803002/diff/40001/scripts/slave/res... > File scripts/slave/results_dashboard.py (right): > > https://chromiumcodereview.appspot.com/545803002/diff/40001/scripts/slave/res... > scripts/slave/results_dashboard.py:216: chart_data, master) > On 2014/09/09 02:24:16, stip wrote: > > this needs a prefix, right? 'r_' ? > > Actually, to clarify, the r_ and a_ prefixes are added by the dashboard to make > it match the current schema. > > Versions (r_ fields) should go in a dict associated with the 'versions' key, and > annotations (a_ fields) should go in a dict associated with the 'supplemental' > key. Sorry, to conclude: it's not necessary to add the prefixes because the dashboard now knows to add r_ to 'versions' items and a_ to 'supplemental' items.
This isn't ready for review yet (still need to update tests and remove testing code), but I addressed the comments so I'm publishing the drafts on those. Will ping back when this is ready for full review. https://chromiumcodereview.appspot.com/545803002/diff/40001/scripts/slave/res... File scripts/slave/results_dashboard.py (right): https://chromiumcodereview.appspot.com/545803002/diff/40001/scripts/slave/res... scripts/slave/results_dashboard.py:31: build_dir): On 2014/09/09 20:22:54, qyearsley wrote: > What about having a function which just constructs the "Dashboard JSON v1" > (which doesn't need to take a url or build_dir argument), and then use > SendResults to send the output of that? Done. https://chromiumcodereview.appspot.com/545803002/diff/40001/scripts/slave/res... scripts/slave/results_dashboard.py:56: if ref_json: On 2014/09/09 02:20:06, stip wrote: > so the file will always exist, but the telemeterized binary may just write an > empty {}? In telemetry_utils.TelemetryResultsTracker._GetFileJson, if the file doesn't exist or isn't valid JSON, it will return None. In telemetry._GenerateTelemetryCommandSequence, if there is no reference build, the command to run telemetry and dump into this file will not be run. The file will be created and will be empty. https://chromiumcodereview.appspot.com/545803002/diff/40001/scripts/slave/res... scripts/slave/results_dashboard.py:216: chart_data, master) On 2014/09/09 21:00:54, eakuefner wrote: > On 2014/09/09 02:24:16, stip wrote: > > this needs a prefix, right? 'r_' ? > > Actually, to clarify, the r_ and a_ prefixes are added by the dashboard to make > it match the current schema. > > Versions (r_ fields) should go in a dict associated with the 'versions' key, and > annotations (a_ fields) should go in a dict associated with the 'supplemental' > key. Yep. https://chromiumcodereview.appspot.com/545803002/diff/40001/scripts/slave/run... File scripts/slave/runtest.py (right): https://chromiumcodereview.appspot.com/545803002/diff/40001/scripts/slave/run... scripts/slave/runtest.py:691: def _SendResultsToDashboard(results_tracker, options): On 2014/09/09 02:20:06, stip wrote: > iannucci@ strongly dislikes slinging the options object around. what I think > makes sense is preprocessing all this in the beginning, and have a > 'results_dashboard dict' that gets passed to sendresultstodashboard when needed. > > { > 'system': _GetPerfID(options), > 'test': options.test_type, > 'url': options.results_url, > } > > etc Done. https://chromiumcodereview.appspot.com/545803002/diff/40001/scripts/slave/tel... File scripts/slave/telemetry_utils.py (right): https://chromiumcodereview.appspot.com/545803002/diff/40001/scripts/slave/tel... scripts/slave/telemetry_utils.py:22: (_, self._chart_filename) = tempfile.mkstemp() On 2014/09/09 02:20:06, stip wrote: > you may need to close the file handle here, I'm not sure. Done. https://chromiumcodereview.appspot.com/545803002/diff/40001/scripts/slave/tel... scripts/slave/telemetry_utils.py:26: '--ref-output-filename', self._ref_chart_filename]) On 2014/09/09 02:20:06, stip wrote: > I'm a bit confused about the ref output filename -- will the telemeterized > binary always write to it? No, only if there is a ref build in telemery._GenerateTelemetryCommandSequence. Part of the reason I sent this for feedback before full review is that it's pretty roundabout how the files get created and passed into telemetry. Not sure if there's a better way to do it? https://chromiumcodereview.appspot.com/545803002/diff/40001/scripts/slave/tel... scripts/slave/telemetry_utils.py:41: def Cleanup(self): On 2014/09/09 02:24:16, stip wrote: > I don't think cleanup is called by runtest.py in this patchset Done. https://chromiumcodereview.appspot.com/545803002/diff/40001/scripts/slave/tel... scripts/slave/telemetry_utils.py:44: os.remove(self._ref_chart_filename) On 2014/09/09 02:20:06, stip wrote: > if the first one fails, we'll never get to the second one -- probably a good > idea to have a _try_to_delete() function with the try block that gets called > twice here Done.
This is ready for review now. Still not sure how to handle android. luqui, any suggestions?
lgtm w/ nits https://chromiumcodereview.appspot.com/545803002/diff/100001/scripts/slave/re... File scripts/slave/results_dashboard.py (right): https://chromiumcodereview.appspot.com/545803002/diff/100001/scripts/slave/re... scripts/slave/results_dashboard.py:423: if type(data) is list: usually better to do if isinstance(data, list): https://chromiumcodereview.appspot.com/545803002/diff/100001/scripts/slave/te... File scripts/slave/telemetry_utils.py (right): https://chromiumcodereview.appspot.com/545803002/diff/100001/scripts/slave/te... scripts/slave/telemetry_utils.py:1: # pylint: disable=R0201 nit: move pylint declaration below copyright
Before I submit, should we have a plan for rolling this out to the buildbots? There are hundreds of different tests, and it's possible one of them has a JSON output we didn't plan for either in this CL or in the dashboard side. https://chromiumcodereview.appspot.com/545803002/diff/100001/scripts/slave/re... File scripts/slave/results_dashboard.py (right): https://chromiumcodereview.appspot.com/545803002/diff/100001/scripts/slave/re... scripts/slave/results_dashboard.py:423: if type(data) is list: On 2014/09/12 18:17:25, stip wrote: > usually better to do > > if isinstance(data, list): Done. https://chromiumcodereview.appspot.com/545803002/diff/100001/scripts/slave/te... File scripts/slave/telemetry_utils.py (right): https://chromiumcodereview.appspot.com/545803002/diff/100001/scripts/slave/te... scripts/slave/telemetry_utils.py:1: # pylint: disable=R0201 On 2014/09/12 18:17:25, stip wrote: > nit: move pylint declaration below copyright Done.
The CQ bit was checked by sullivan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/545803002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as 291952
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/565973005/ by sullivan@chromium.org. The reason for reverting is: Broke the build: http://build.chromium.org/p/chromium.perf/builders/Mac%2010.8%20Perf%20%282%2.... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
