Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
633213002:
Waterfall builder monitoring script. (Closed)
DescriptionWaterfall builder monitoring script.
BUG=None
NOTRY=True
Committed: https://crrev.com/9954e0bd8253062d25861f11e24efd8d8f1d7c28
Cr-Commit-Position: refs/heads/master@{#299356}
Patch Set 1 #
Total comments: 30
Patch Set 2 : Fixed Shuotao's comments. #
Total comments: 12
Patch Set 3 : #Patch Set 4 : #
Total comments: 2
Patch Set 5 : #
Total comments: 6
Patch Set 6 : #
Total comments: 2
Patch Set 7 : #
Messages
Total messages: 16 (2 generated)
pshenoy@chromium.org changed reviewers: + samuong@chromium.org, stgao@chromium.org
Prashant, I posted some comments. You could address them first. Sam may have more comments later. Thanks, Shuotao Gao https://codereview.chromium.org/633213002/diff/1/chrome/test/chromedriver/tes... File chrome/test/chromedriver/test/waterfall_builder_config_sample.json (right): https://codereview.chromium.org/633213002/diff/1/chrome/test/chromedriver/tes... chrome/test/chromedriver/test/waterfall_builder_config_sample.json:31: "days": -2 maybe rename to "old_build_days" or "old_build_hours"? and how about making the value as positive and handle the logic in python code? you may forget to update this to 24 hours :) https://codereview.chromium.org/633213002/diff/1/chrome/test/chromedriver/tes... File chrome/test/chromedriver/test/waterfall_builder_monitor.py (right): https://codereview.chromium.org/633213002/diff/1/chrome/test/chromedriver/tes... chrome/test/chromedriver/test/waterfall_builder_monitor.py:10: 2 days back. (Number of days can be configured in the config file) We could default to 2 days. https://codereview.chromium.org/633213002/diff/1/chrome/test/chromedriver/tes... chrome/test/chromedriver/test/waterfall_builder_monitor.py:13: get email notification for any waterfall specified in the config file. Maybe add a cronjob config sample here? https://codereview.chromium.org/633213002/diff/1/chrome/test/chromedriver/tes... chrome/test/chromedriver/test/waterfall_builder_monitor.py:16: SUCCESS_SUBJECT = '[CHROME TESTING]: Builder status %s: PASSED.' Move these constants below after import. You could run "pylint /path/to/this/python/file" to clean up style issues. https://codereview.chromium.org/633213002/diff/1/chrome/test/chromedriver/tes... chrome/test/chromedriver/test/waterfall_builder_monitor.py:31: class OfficialBuilderParser(object): Top-level functions/classes need two empty lines before them. https://codereview.chromium.org/633213002/diff/1/chrome/test/chromedriver/tes... chrome/test/chromedriver/test/waterfall_builder_monitor.py:37: self.json_url = build_info['json_url'] |json_url| -> |build_json_url|? https://codereview.chromium.org/633213002/diff/1/chrome/test/chromedriver/tes... chrome/test/chromedriver/test/waterfall_builder_monitor.py:41: url = urllib.urlopen(json_url) |url| -> |response| https://codereview.chromium.org/633213002/diff/1/chrome/test/chromedriver/tes... chrome/test/chromedriver/test/waterfall_builder_monitor.py:60: def _GetCompletedLatestBuild(self, keys): _GetCompletedLatestBuild -> _GetLatestCompletedBuild? https://codereview.chromium.org/633213002/diff/1/chrome/test/chromedriver/tes... chrome/test/chromedriver/test/waterfall_builder_monitor.py:69: def _SortBuildListBasedOnVersion(self, build_list): Seems unused, could be deleted. https://codereview.chromium.org/633213002/diff/1/chrome/test/chromedriver/tes... chrome/test/chromedriver/test/waterfall_builder_monitor.py:110: if data is not None: if |data| is None, we'd better report it as error and ask for manual investigation. https://codereview.chromium.org/633213002/diff/1/chrome/test/chromedriver/tes... chrome/test/chromedriver/test/waterfall_builder_monitor.py:123: result['error'] = 'failed' We'd better not mark it as failed. We could add another flag 'build_too_old' or something, make it default as False, and set it as True here. https://codereview.chromium.org/633213002/diff/1/chrome/test/chromedriver/tes... chrome/test/chromedriver/test/waterfall_builder_monitor.py:137: count = CountFailedBuilders(result, count) Let's do the counting directly here instead of a separate function call, because the logic is simple. https://codereview.chromium.org/633213002/diff/1/chrome/test/chromedriver/tes... chrome/test/chromedriver/test/waterfall_builder_monitor.py:165: parser.add_option('--config', type='str', help='Config filename.') How about mentioning absolute path to config file in the |help|? https://codereview.chromium.org/633213002/diff/1/chrome/test/chromedriver/tes... chrome/test/chromedriver/test/waterfall_builder_monitor.py:186: SendStatusEmailViaSendmailCommand(consolidated_results, In this script, email is sent out only when the script executed successfully. Maybe we could add logic like below: try: # normal logic in this script # send email report catch Exception as e: # send email report with some info of the exception. # You may refer to https://docs.python.org/2/library/traceback.html#traceback.format_exc
Hi Shuotao, I have fixed all your comments. Please take a look. -- Prashanth https://codereview.chromium.org/633213002/diff/1/chrome/test/chromedriver/tes... File chrome/test/chromedriver/test/waterfall_builder_config_sample.json (right): https://codereview.chromium.org/633213002/diff/1/chrome/test/chromedriver/tes... chrome/test/chromedriver/test/waterfall_builder_config_sample.json:31: "days": -2 On 2014/10/07 21:01:08, Shuotao wrote: > maybe rename to "old_build_days" or "old_build_hours"? and how about making the > value as positive and handle the logic in python code? > > you may forget to update this to 24 hours :) Done. https://codereview.chromium.org/633213002/diff/1/chrome/test/chromedriver/tes... File chrome/test/chromedriver/test/waterfall_builder_monitor.py (right): https://codereview.chromium.org/633213002/diff/1/chrome/test/chromedriver/tes... chrome/test/chromedriver/test/waterfall_builder_monitor.py:10: 2 days back. (Number of days can be configured in the config file) On 2014/10/07 21:01:08, Shuotao wrote: > We could default to 2 days. Are you suggesting to remove the comment "Number of days can be configured in the config file" ? https://codereview.chromium.org/633213002/diff/1/chrome/test/chromedriver/tes... chrome/test/chromedriver/test/waterfall_builder_monitor.py:13: get email notification for any waterfall specified in the config file. On 2014/10/07 21:01:08, Shuotao wrote: > Maybe add a cronjob config sample here? Done. https://codereview.chromium.org/633213002/diff/1/chrome/test/chromedriver/tes... chrome/test/chromedriver/test/waterfall_builder_monitor.py:16: SUCCESS_SUBJECT = '[CHROME TESTING]: Builder status %s: PASSED.' On 2014/10/07 21:01:08, Shuotao wrote: > Move these constants below after import. > > You could run "pylint /path/to/this/python/file" to clean up style issues. Done. https://codereview.chromium.org/633213002/diff/1/chrome/test/chromedriver/tes... chrome/test/chromedriver/test/waterfall_builder_monitor.py:31: class OfficialBuilderParser(object): On 2014/10/07 21:01:08, Shuotao wrote: > Top-level functions/classes need two empty lines before them. Done. https://codereview.chromium.org/633213002/diff/1/chrome/test/chromedriver/tes... chrome/test/chromedriver/test/waterfall_builder_monitor.py:37: self.json_url = build_info['json_url'] On 2014/10/07 21:01:08, Shuotao wrote: > |json_url| -> |build_json_url|? Done. https://codereview.chromium.org/633213002/diff/1/chrome/test/chromedriver/tes... chrome/test/chromedriver/test/waterfall_builder_monitor.py:41: url = urllib.urlopen(json_url) On 2014/10/07 21:01:08, Shuotao wrote: > |url| -> |response| Done. https://codereview.chromium.org/633213002/diff/1/chrome/test/chromedriver/tes... chrome/test/chromedriver/test/waterfall_builder_monitor.py:60: def _GetCompletedLatestBuild(self, keys): On 2014/10/07 21:01:08, Shuotao wrote: > _GetCompletedLatestBuild -> _GetLatestCompletedBuild? Done. https://codereview.chromium.org/633213002/diff/1/chrome/test/chromedriver/tes... chrome/test/chromedriver/test/waterfall_builder_monitor.py:69: def _SortBuildListBasedOnVersion(self, build_list): On 2014/10/07 21:01:08, Shuotao wrote: > Seems unused, could be deleted. Done. https://codereview.chromium.org/633213002/diff/1/chrome/test/chromedriver/tes... chrome/test/chromedriver/test/waterfall_builder_monitor.py:110: if data is not None: On 2014/10/07 21:01:08, Shuotao wrote: > if |data| is None, we'd better report it as error and ask for manual > investigation. Done. https://codereview.chromium.org/633213002/diff/1/chrome/test/chromedriver/tes... chrome/test/chromedriver/test/waterfall_builder_monitor.py:123: result['error'] = 'failed' On 2014/10/07 21:01:08, Shuotao wrote: > We'd better not mark it as failed. > We could add another flag 'build_too_old' or something, make it default as > False, and set it as True here. Done. https://codereview.chromium.org/633213002/diff/1/chrome/test/chromedriver/tes... chrome/test/chromedriver/test/waterfall_builder_monitor.py:137: count = CountFailedBuilders(result, count) On 2014/10/07 21:01:08, Shuotao wrote: > Let's do the counting directly here instead of a separate function call, because > the logic is simple. Done. https://codereview.chromium.org/633213002/diff/1/chrome/test/chromedriver/tes... chrome/test/chromedriver/test/waterfall_builder_monitor.py:165: parser.add_option('--config', type='str', help='Config filename.') On 2014/10/07 21:01:08, Shuotao wrote: > How about mentioning absolute path to config file in the |help|? Done. https://codereview.chromium.org/633213002/diff/1/chrome/test/chromedriver/tes... chrome/test/chromedriver/test/waterfall_builder_monitor.py:186: SendStatusEmailViaSendmailCommand(consolidated_results, On 2014/10/07 21:01:08, Shuotao wrote: > In this script, email is sent out only when the script executed successfully. > Maybe we could add logic like below: > > try: > # normal logic in this script > # send email report > catch Exception as e: > # send email report with some info of the exception. > # You may refer to > https://docs.python.org/2/library/traceback.html#traceback.format_exc Done.
https://codereview.chromium.org/633213002/diff/1/chrome/test/chromedriver/tes... File chrome/test/chromedriver/test/waterfall_builder_monitor.py (right): https://codereview.chromium.org/633213002/diff/1/chrome/test/chromedriver/tes... chrome/test/chromedriver/test/waterfall_builder_monitor.py:10: 2 days back. (Number of days can be configured in the config file) On 2014/10/07 23:21:52, pshenoy wrote: > On 2014/10/07 21:01:08, Shuotao wrote: > > We could default to 2 days. > > Are you suggesting to remove the comment "Number of days can be configured in > the config file" ? In the comment, we could say "This also reports ... 2 days back by default. ( Number ... config file)". In the code, if the "old_build_days" is not set in the config file, we fallback to 2. The reason of picking 2 days is that over weekend there might be no build because of no CL checkin. https://codereview.chromium.org/633213002/diff/20001/chrome/test/chromedriver... File chrome/test/chromedriver/test/waterfall_builder_monitor.py (right): https://codereview.chromium.org/633213002/diff/20001/chrome/test/chromedriver... chrome/test/chromedriver/test/waterfall_builder_monitor.py:31: Usually we add one more empty line here. https://codereview.chromium.org/633213002/diff/20001/chrome/test/chromedriver... chrome/test/chromedriver/test/waterfall_builder_monitor.py:61: raise ValueError "raise" seems enough. https://codereview.chromium.org/633213002/diff/20001/chrome/test/chromedriver... chrome/test/chromedriver/test/waterfall_builder_monitor.py:67: def SendEmailViaSendmailCommand(sender_email, recipient_emails, One more empty line before this. Did you run pylint already? I think it should catch the style issues like this and more others. https://codereview.chromium.org/633213002/diff/20001/chrome/test/chromedriver... chrome/test/chromedriver/test/waterfall_builder_monitor.py:108: email_body = '' email_body = '\n'.join(exception_message_lines) https://codereview.chromium.org/633213002/diff/20001/chrome/test/chromedriver... chrome/test/chromedriver/test/waterfall_builder_monitor.py:179: raise ValueError raise Exception('There was some problem getting JSON data ' 'from URL: %s' % result['build_link']) https://codereview.chromium.org/633213002/diff/20001/chrome/test/chromedriver... chrome/test/chromedriver/test/waterfall_builder_monitor.py:217: return 0 Move this line to before "except Exception".
https://codereview.chromium.org/633213002/diff/1/chrome/test/chromedriver/tes... File chrome/test/chromedriver/test/waterfall_builder_monitor.py (right): https://codereview.chromium.org/633213002/diff/1/chrome/test/chromedriver/tes... chrome/test/chromedriver/test/waterfall_builder_monitor.py:10: 2 days back. (Number of days can be configured in the config file) On 2014/10/07 23:53:19, Shuotao wrote: > On 2014/10/07 23:21:52, pshenoy wrote: > > On 2014/10/07 21:01:08, Shuotao wrote: > > > We could default to 2 days. > > > > Are you suggesting to remove the comment "Number of days can be configured in > > the config file" ? > > In the comment, we could say "This also reports ... 2 days back by default. ( > Number ... config file)". > > In the code, if the "old_build_days" is not set in the config file, we fallback > to 2. > > The reason of picking 2 days is that over weekend there might be no build > because of no CL checkin. Done. https://codereview.chromium.org/633213002/diff/20001/chrome/test/chromedriver... File chrome/test/chromedriver/test/waterfall_builder_monitor.py (right): https://codereview.chromium.org/633213002/diff/20001/chrome/test/chromedriver... chrome/test/chromedriver/test/waterfall_builder_monitor.py:31: On 2014/10/07 23:53:19, Shuotao wrote: > Usually we add one more empty line here. Done. https://codereview.chromium.org/633213002/diff/20001/chrome/test/chromedriver... chrome/test/chromedriver/test/waterfall_builder_monitor.py:61: raise ValueError On 2014/10/07 23:53:19, Shuotao wrote: > "raise" seems enough. Done. https://codereview.chromium.org/633213002/diff/20001/chrome/test/chromedriver... chrome/test/chromedriver/test/waterfall_builder_monitor.py:67: def SendEmailViaSendmailCommand(sender_email, recipient_emails, On 2014/10/07 23:53:19, Shuotao wrote: > One more empty line before this. > > Did you run pylint already? > I think it should catch the style issues like this and more others. I did run pylint and it's not complaining about this. Fixed now. https://codereview.chromium.org/633213002/diff/20001/chrome/test/chromedriver... chrome/test/chromedriver/test/waterfall_builder_monitor.py:108: email_body = '' On 2014/10/07 23:53:19, Shuotao wrote: > email_body = '\n'.join(exception_message_lines) Done. https://codereview.chromium.org/633213002/diff/20001/chrome/test/chromedriver... chrome/test/chromedriver/test/waterfall_builder_monitor.py:179: raise ValueError On 2014/10/07 23:53:19, Shuotao wrote: > raise Exception('There was some problem getting JSON data ' > 'from URL: %s' % result['build_link']) Done. https://codereview.chromium.org/633213002/diff/20001/chrome/test/chromedriver... chrome/test/chromedriver/test/waterfall_builder_monitor.py:217: return 0 On 2014/10/07 23:53:19, Shuotao wrote: > Move this line to before "except Exception". Done.
https://codereview.chromium.org/633213002/diff/60001/chrome/test/chromedriver... File chrome/test/chromedriver/test/waterfall_builder_monitor.py (right): https://codereview.chromium.org/633213002/diff/60001/chrome/test/chromedriver... chrome/test/chromedriver/test/waterfall_builder_monitor.py:101: result['build_link'] + '\n\n' It might be worth pushing all urls through urllib.quote() (both here and on line 97 above). Alternatively you could make sure they're quoted properly in the json config file. The email from this morning has an Android bot failure, and the URL has spaces in it, so the links don't look right in Gmail.
https://codereview.chromium.org/633213002/diff/60001/chrome/test/chromedriver... File chrome/test/chromedriver/test/waterfall_builder_monitor.py (right): https://codereview.chromium.org/633213002/diff/60001/chrome/test/chromedriver... chrome/test/chromedriver/test/waterfall_builder_monitor.py:101: result['build_link'] + '\n\n' On 2014/10/09 16:56:44, samuong wrote: > It might be worth pushing all urls through urllib.quote() (both here and on line > 97 above). Alternatively you could make sure they're quoted properly in the json > config file. > > The email from this morning has an Android bot failure, and the URL has spaces > in it, so the links don't look right in Gmail. Okay. I have updated the URLs in the config file.
https://codereview.chromium.org/633213002/diff/100001/chrome/test/chromedrive... File chrome/test/chromedriver/test/waterfall_builder_monitor.py (right): https://codereview.chromium.org/633213002/diff/100001/chrome/test/chromedrive... chrome/test/chromedriver/test/waterfall_builder_monitor.py:82: count = 0 Rename |count| to |failure_count|? https://codereview.chromium.org/633213002/diff/100001/chrome/test/chromedrive... chrome/test/chromedriver/test/waterfall_builder_monitor.py:84: if result['error'] != 'passed': if result['error'] != 'passed' and not result['build_too_old']: https://codereview.chromium.org/633213002/diff/100001/chrome/test/chromedrive... chrome/test/chromedriver/test/waterfall_builder_monitor.py:205: old_build_days = int('-' + str(json_data['old_build_days'])) old_build_days = - json_data['old_build_days']
https://codereview.chromium.org/633213002/diff/100001/chrome/test/chromedrive... File chrome/test/chromedriver/test/waterfall_builder_monitor.py (right): https://codereview.chromium.org/633213002/diff/100001/chrome/test/chromedrive... chrome/test/chromedriver/test/waterfall_builder_monitor.py:82: count = 0 On 2014/10/13 19:49:02, Shuotao wrote: > Rename |count| to |failure_count|? Done. https://codereview.chromium.org/633213002/diff/100001/chrome/test/chromedrive... chrome/test/chromedriver/test/waterfall_builder_monitor.py:84: if result['error'] != 'passed': On 2014/10/13 19:49:02, Shuotao wrote: > if result['error'] != 'passed' and not result['build_too_old']: Done. https://codereview.chromium.org/633213002/diff/100001/chrome/test/chromedrive... chrome/test/chromedriver/test/waterfall_builder_monitor.py:205: old_build_days = int('-' + str(json_data['old_build_days'])) On 2014/10/13 19:49:02, Shuotao wrote: > old_build_days = - json_data['old_build_days'] Done.
thanks prashant, lgtm with one nit https://codereview.chromium.org/633213002/diff/170001/chrome/test/chromedrive... File chrome/test/chromedriver/test/waterfall_builder_monitor.py (right): https://codereview.chromium.org/633213002/diff/170001/chrome/test/chromedrive... chrome/test/chromedriver/test/waterfall_builder_monitor.py:44: def GetDateFromEphocFormat(ephoc_time): "epoch"?
https://codereview.chromium.org/633213002/diff/170001/chrome/test/chromedrive... File chrome/test/chromedriver/test/waterfall_builder_monitor.py (right): https://codereview.chromium.org/633213002/diff/170001/chrome/test/chromedrive... chrome/test/chromedriver/test/waterfall_builder_monitor.py:44: def GetDateFromEphocFormat(ephoc_time): On 2014/10/13 20:11:18, samuong wrote: > "epoch"? Done.
The CQ bit was checked by pshenoy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/633213002/220001
Message was sent while issue was closed.
Committed patchset #7 (id:220001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/9954e0bd8253062d25861f11e24efd8d8f1d7c28 Cr-Commit-Position: refs/heads/master@{#299356} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
