Add logdog_helper script.

Add logdog_helper script.

Add helper script to manage uploading data to logdog from the
test runner. Also, add new 'links' entry to results.json file
which will primarily contain logdog URLs.

Review-Url: https://codereview.chromium.org/2664873002
Cr-Commit-Position: refs/heads/master@{#448080}
Committed: https://chromium.googlesource.com/chromium/src/+/c422ef3ada9263e775bd13bef3b0657197d84222

[mikecase (-- gone --), 2017-01-30 23:20:48]

mikecase@chromium.org changed reviewers:
+ hzl@chromium.org, jbudorick@chromium.org

[mikecase (-- gone --), 2017-01-30 23:20:49]

What do you think about something like this.

Would have to update this recipe code....
https://cs.chromium.org/chromium/build/scripts/slave/recipe_modules/chromium_...

...before I land this.


This would allow me to add more things to results_details fairly easily.

https://codereview.chromium.org/2664873002/diff/40001/build/android/pylib/res...
File build/android/pylib/results/json_results.py (right):

https://codereview.chromium.org/2664873002/diff/40001/build/android/pylib/res...
build/android/pylib/results/json_results.py:96: k: v for tr in test_run_result
for k, v in tr.GetLinks().items()})
This is super awkward. Potentially overwrites lots of links if there are similar
entries in multiple test runs.

https://codereview.chromium.org/2664873002/diff/40001/build/android/pylib/uti...
File build/android/pylib/utils/logdog_helper.py (right):

https://codereview.chromium.org/2664873002/diff/40001/build/android/pylib/uti...
build/android/pylib/utils/logdog_helper.py:18:
@python_utils.NoRaiseException(default_return_value='')
Currently, all of the logdog code is surrounded in things like

try:
  logdog code
except Exception:
  logging.exception('Unknown error')

I added a helper to do this on all the logdog helper functions.

https://codereview.chromium.org/2664873002/diff/40001/build/android/pylib/uti...
build/android/pylib/utils/logdog_helper.py:77: @python_utils.Memoize
Pretty sure (99%) you only need to get the stream_client once. So I save it with
the Memoize decorator.

[jbudorick, 2017-01-31 16:11:53]

https://codereview.chromium.org/2664873002/diff/40001/build/android/pylib/and...
File build/android/pylib/android/logdog_logcat_monitor.py (right):

https://codereview.chromium.org/2664873002/diff/40001/build/android/pylib/and...
build/android/pylib/android/logdog_logcat_monitor.py:22: self._logdog_stream =
logdog_helper.open_text(self._stream_name)
While I recognize that you're not changing the behavior in this regard, I'm
wondering about opening the stream here vs below in Start.

https://codereview.chromium.org/2664873002/diff/40001/build/android/pylib/bas...
File build/android/pylib/base/base_test_result.py (right):

https://codereview.chromium.org/2664873002/diff/40001/build/android/pylib/bas...
build/android/pylib/base/base_test_result.py:110: raise Exception('Link with
name "%s" aleady exists.' % name)
This may be better off as SetLink, with new calls overwriting old ones.

https://codereview.chromium.org/2664873002/diff/40001/build/android/pylib/bas...
build/android/pylib/base/base_test_result.py:127: """Add link with test run
results data."""
Same.

https://codereview.chromium.org/2664873002/diff/40001/build/android/pylib/res...
File build/android/pylib/results/json_results.py (right):

https://codereview.chromium.org/2664873002/diff/40001/build/android/pylib/res...
build/android/pylib/results/json_results.py:96: k: v for tr in test_run_result
for k, v in tr.GetLinks().items()})
On 2017/01/30 23:20:49, mikecase wrote:
> This is super awkward. Potentially overwrites lots of links if there are
similar
> entries in multiple test runs.

this is also super awkward because it's a double dict comprehension.

https://codereview.chromium.org/2664873002/diff/40001/build/android/pylib/uti...
File build/android/pylib/utils/logdog_helper.py (right):

https://codereview.chromium.org/2664873002/diff/40001/build/android/pylib/uti...
build/android/pylib/utils/logdog_helper.py:77: @python_utils.Memoize
On 2017/01/30 23:20:49, mikecase wrote:
> Pretty sure (99%) you only need to get the stream_client once. So I save it
with
> the Memoize decorator. 

I'm not sure I like this, though you may be right.

https://codereview.chromium.org/2664873002/diff/40001/build/android/pylib/uti...
File build/android/pylib/utils/python_utils.py (right):

https://codereview.chromium.org/2664873002/diff/40001/build/android/pylib/uti...
build/android/pylib/utils/python_utils.py:1: # Copyright 2017 The Chromium
Authors. All rights reserved.
python_utils doesn't really say anything as a name.

https://codereview.chromium.org/2664873002/diff/40001/build/android/pylib/uti...
build/android/pylib/utils/python_utils.py:9: def Memoize(f):
I could've sworn that we had this somewhere, but I don't see it any more.

https://codereview.chromium.org/2664873002/diff/40001/build/android/pylib/uti...
build/android/pylib/utils/python_utils.py:15: key = repr((args, kwargs))
4 space indents...? Did you lift this from somewhere else?

https://codereview.chromium.org/2664873002/diff/40001/build/android/pylib/uti...
build/android/pylib/utils/python_utils.py:41: logging.exception('message')
You're just going to log 'message'?

[mikecase (-- gone --), 2017-01-31 23:08:25]

https://codereview.chromium.org/2664873002/diff/40001/build/android/pylib/and...
File build/android/pylib/android/logdog_logcat_monitor.py (right):

https://codereview.chromium.org/2664873002/diff/40001/build/android/pylib/and...
build/android/pylib/android/logdog_logcat_monitor.py:22: self._logdog_stream =
logdog_helper.open_text(self._stream_name)
On 2017/01/31 at 16:11:53, jbudorick wrote:
> While I recognize that you're not changing the behavior in this regard, I'm
wondering about opening the stream here vs below in Start.

Moved opening stream to Start()

https://codereview.chromium.org/2664873002/diff/40001/build/android/pylib/bas...
File build/android/pylib/base/base_test_result.py (right):

https://codereview.chromium.org/2664873002/diff/40001/build/android/pylib/bas...
build/android/pylib/base/base_test_result.py:110: raise Exception('Link with
name "%s" aleady exists.' % name)
On 2017/01/31 at 16:11:53, jbudorick wrote:
> This may be better off as SetLink, with new calls overwriting old ones.

Yeah, I like this. Done

https://codereview.chromium.org/2664873002/diff/40001/build/android/pylib/bas...
build/android/pylib/base/base_test_result.py:127: """Add link with test run
results data."""
On 2017/01/31 at 16:11:53, jbudorick wrote:
> Same.

Done

https://codereview.chromium.org/2664873002/diff/40001/build/android/pylib/res...
File build/android/pylib/results/json_results.py (right):

https://codereview.chromium.org/2664873002/diff/40001/build/android/pylib/res...
build/android/pylib/results/json_results.py:96: k: v for tr in test_run_result
for k, v in tr.GetLinks().items()})
On 2017/01/31 at 16:11:53, jbudorick wrote:
> On 2017/01/30 23:20:49, mikecase wrote:
> > This is super awkward. Potentially overwrites lots of links if there are
similar
> > entries in multiple test runs.
> 
> this is also super awkward because it's a double dict comprehension.

I mean. A few option. Make TestRunResults.SetLinks a @classmethod. Then I don't
have to worry about there being more than 1 test run results.

Second option, keep as is but remove double dict conprehension with some easier
to read code.

Third option, make links a list of dicts or something...

'links': [{test run 1 links}, {test run 2 links}, {...}]

https://codereview.chromium.org/2664873002/diff/40001/build/android/pylib/uti...
File build/android/pylib/utils/python_utils.py (right):

https://codereview.chromium.org/2664873002/diff/40001/build/android/pylib/uti...
build/android/pylib/utils/python_utils.py:9: def Memoize(f):
On 2017/01/31 at 16:11:53, jbudorick wrote:
> I could've sworn that we had this somewhere, but I don't see it any more.

A slightly different implementation of this existed in pylib/constants.py. It
was used to cache something like CHROMIUM_SRC_DIR or soemthing

Its gone now though.

https://codereview.chromium.org/2664873002/diff/40001/build/android/pylib/uti...
build/android/pylib/utils/python_utils.py:15: key = repr((args, kwargs))
On 2017/01/31 at 16:11:53, jbudorick wrote:
> 4 space indents...? Did you lift this from somewhere else?

...

https://codereview.chromium.org/2664873002/diff/40001/build/android/pylib/uti...
build/android/pylib/utils/python_utils.py:41: logging.exception('message')
I mean, logging.exception will log the exception. But it requires at least 1
arg. Maybe there is a cleaner way to just log the exception if I dont care about
having an accompanying message.

[jbudorick, 2017-01-31 23:15:12]

https://codereview.chromium.org/2664873002/diff/40001/build/android/pylib/res...
File build/android/pylib/results/json_results.py (right):

https://codereview.chromium.org/2664873002/diff/40001/build/android/pylib/res...
build/android/pylib/results/json_results.py:96: k: v for tr in test_run_result
for k, v in tr.GetLinks().items()})
On 2017/01/31 23:08:25, mikecase wrote:
> On 2017/01/31 at 16:11:53, jbudorick wrote:
> > On 2017/01/30 23:20:49, mikecase wrote:
> > > This is super awkward. Potentially overwrites lots of links if there are
> similar
> > > entries in multiple test runs.
> > 
> > this is also super awkward because it's a double dict comprehension.
> 
> I mean. A few option. Make TestRunResults.SetLinks a @classmethod. Then I
don't
> have to worry about there being more than 1 test run results.
> 
> Second option, keep as is but remove double dict conprehension with some
easier
> to read code.
> 
> Third option, make links a list of dicts or something...
> 
> 'links': [{test run 1 links}, {test run 2 links}, {...}]
> 

option 2 > option 3 > option 1

https://codereview.chromium.org/2664873002/diff/40001/build/android/pylib/uti...
File build/android/pylib/utils/python_utils.py (right):

https://codereview.chromium.org/2664873002/diff/40001/build/android/pylib/uti...
build/android/pylib/utils/python_utils.py:41: logging.exception('message')
On 2017/01/31 23:08:25, mikecase wrote:
> I mean, logging.exception will log the exception. But it requires at least 1
> arg. Maybe there is a cleaner way to just log the exception if I dont care
about
> having an accompanying message.

Can it be an additional argument to NoRaiseException?

https://codereview.chromium.org/2664873002/diff/60001/build/android/pylib/loc...
File build/android/pylib/local/device/local_device_instrumentation_test_run.py
(left):

https://codereview.chromium.org/2664873002/diff/60001/build/android/pylib/loc...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:360:
tombstones_url = tombstones.LogdogTombstones(resolved_tombstones,
Can you remove tombstones.LogdogTombstones with this change?

https://codereview.chromium.org/2664873002/diff/60001/build/android/pylib/uti...
File build/android/pylib/utils/python_utils.py (right):

https://codereview.chromium.org/2664873002/diff/60001/build/android/pylib/uti...
build/android/pylib/utils/python_utils.py:1: # Copyright 2017 The Chromium
Authors. All rights reserved.
This name is still bad.

[mikecase (-- gone --), 2017-02-01 22:13:45]

https://codereview.chromium.org/2664873002/diff/60001/build/android/pylib/loc...
File build/android/pylib/local/device/local_device_instrumentation_test_run.py
(left):

https://codereview.chromium.org/2664873002/diff/60001/build/android/pylib/loc...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:360:
tombstones_url = tombstones.LogdogTombstones(resolved_tombstones,
On 2017/01/31 at 23:15:12, jbudorick wrote:
> Can you remove tombstones.LogdogTombstones with this change?

Yes. And I thought I had done this. But I must have not saved changes. Fixed.

https://codereview.chromium.org/2664873002/diff/60001/build/android/pylib/uti...
File build/android/pylib/utils/python_utils.py (right):

https://codereview.chromium.org/2664873002/diff/60001/build/android/pylib/uti...
build/android/pylib/utils/python_utils.py:1: # Copyright 2017 The Chromium
Authors. All rights reserved.
On 2017/01/31 at 23:15:12, jbudorick wrote:
> This name is still bad.

Renamed to make the name fantastic....

presenting...

function_utils.py

https://codereview.chromium.org/2664873002/diff/100001/build/android/pylib/re...
File build/android/pylib/results/json_results.py (right):

https://codereview.chromium.org/2664873002/diff/100001/build/android/pylib/re...
build/android/pylib/results/json_results.py:97:
test_run_links.update(tr.GetLinks())
Removed confusing double dict comprehension from here.

https://codereview.chromium.org/2664873002/diff/100001/build/android/pylib/ut...
File build/android/pylib/utils/function_utils.py (right):

https://codereview.chromium.org/2664873002/diff/100001/build/android/pylib/ut...
build/android/pylib/utils/function_utils.py:29: def
NoRaiseException(default_return_value=None, exception_message=''):
Added exception_message arg instead of always confusingly logging
logging.exception('message')

[mikecase (-- gone --), 2017-02-01 22:42:05]

renamed function_utils.py to decorators.py

[mikecase (-- gone --), 2017-02-03 01:25:39]

ping, please take a look. Will let me land some other things I want to get in.

[jbudorick, 2017-02-03 01:30:30]

jbudorick@chromium.org changed reviewers:
+ dnj@chromium.org

[jbudorick, 2017-02-03 01:30:30]

https://codereview.chromium.org/2664873002/diff/120001/build/android/pylib/ut...
File build/android/pylib/utils/decorators.py (right):

https://codereview.chromium.org/2664873002/diff/120001/build/android/pylib/ut...
build/android/pylib/utils/decorators.py:20: memoize_dict[key] = e
This is... interesting. I'm not sure we want to do this.

https://codereview.chromium.org/2664873002/diff/120001/build/android/pylib/ut...
File build/android/pylib/utils/logdog_helper.py (right):

https://codereview.chromium.org/2664873002/diff/120001/build/android/pylib/ut...
build/android/pylib/utils/logdog_helper.py:78: def get_logdog_client():
+dnj is it ok to memoize a stream client?

[dnj, 2017-02-03 01:33:55]

https://codereview.chromium.org/2664873002/diff/120001/build/android/pylib/ut...
File build/android/pylib/utils/logdog_helper.py (right):

https://codereview.chromium.org/2664873002/diff/120001/build/android/pylib/ut...
build/android/pylib/utils/logdog_helper.py:78: def get_logdog_client():
On 2017/02/03 01:30:30, jbudorick wrote:
> +dnj is it ok to memoize a stream client?

It's intended to be a singleton, so this seems fine to me.

[jbudorick, 2017-02-03 01:35:42]

https://codereview.chromium.org/2664873002/diff/120001/build/android/pylib/ut...
File build/android/pylib/utils/decorators.py (right):

https://codereview.chromium.org/2664873002/diff/120001/build/android/pylib/ut...
build/android/pylib/utils/decorators.py:1: # Copyright 2017 The Chromium
Authors. All rights reserved.
This should have tests.

https://codereview.chromium.org/2664873002/diff/120001/build/android/pylib/ut...
File build/android/pylib/utils/logdog_helper.py (right):

https://codereview.chromium.org/2664873002/diff/120001/build/android/pylib/ut...
build/android/pylib/utils/logdog_helper.py:78: def get_logdog_client():
On 2017/02/03 01:33:55, dnj wrote:
> On 2017/02/03 01:30:30, jbudorick wrote:
> > +dnj is it ok to memoize a stream client?
> 
> It's intended to be a singleton, so this seems fine to me.

Great. Thanks for the quick response!

[mikecase (-- gone --), 2017-02-03 18:28:39]

- Added decorators_test.py
- Now @Memoize does not cache Exception results

https://codereview.chromium.org/2664873002/diff/120001/build/android/pylib/ut...
File build/android/pylib/utils/decorators.py (right):

https://codereview.chromium.org/2664873002/diff/120001/build/android/pylib/ut...
build/android/pylib/utils/decorators.py:20: memoize_dict[key] = e
On 2017/02/03 at 01:30:30, jbudorick wrote:
> This is... interesting. I'm not sure we want to do this.

Changed to not cache Exceptions.

[jbudorick, 2017-02-03 18:33:50]

lgtm

[mikecase (-- gone --), 2017-02-03 18:44:10]

The CQ bit was checked by mikecase@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-03 18:44:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-03 19:43:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-03 19:44:00]

Dry run: This issue passed the CQ dry run.

[mikecase (-- gone --), 2017-02-03 19:50:40]

The CQ bit was checked by mikecase@chromium.org

[mikecase (-- gone --), 2017-02-03 19:50:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
jbudorick@chromium.org
Link to the patchset: https://codereview.chromium.org/2664873002/#ps160001
(title: "Update one use of AddLink to SetLink I missed")

[commit-bot: I haz the power, 2017-02-03 19:51:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-03 22:00:15]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1486151440360280,
"parent_rev": "2b30e8534ceb11cd138fa1360e7ae6efc6b4c046", "commit_rev":
"c422ef3ada9263e775bd13bef3b0657197d84222"}

[commit-bot: I haz the power, 2017-02-03 22:01:25]

Description was changed from

==========
Add logdog_helper script.

Add helper script to manage uploading data to logdog from the
test runner. Also, add new 'links' entry to results.json file
which will primarily contain logdog URLs.
==========

to

==========
Add logdog_helper script.

Add helper script to manage uploading data to logdog from the
test runner. Also, add new 'links' entry to results.json file
which will primarily contain logdog URLs.

Review-Url: https://codereview.chromium.org/2664873002
Cr-Commit-Position: refs/heads/master@{#448080}
Committed:
https://chromium.googlesource.com/chromium/src/+/c422ef3ada9263e775bd13bef3b0...
==========

[commit-bot: I haz the power, 2017-02-03 22:01:26]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/c422ef3ada9263e775bd13bef3b0...