send_monitoring_event: support for default event and gomastats

send_monitoring_event: support for default event and gomastats

Added --event-logrequest-path and --build-event-goma-stats-path
And a lot of tests.

BUG=513882
R=dsansome@chromium.org, sergeyberezin@chromium.org

Committed: https://chromium.googlesource.com/infra/infra/+/6d15e263015edf6a4cfb6a2e4a3081ba8efe4ec7

[pgervais, 2015-10-29 22:46:34]

pgervais@chromium.org changed reviewers:
+ dsansome@chromium.org, sergeyberezin@chromium.org

[pgervais, 2015-10-29 22:46:34]

There are different things in this CL, I realized too late that I could have
done several CLs:

- support for goma proto
- support for specifying default event
- and some cleanup

There are still 5 tests to write, but other than that the CL is ready for
review.

PTAL

[dsansome, 2015-10-29 23:17:33]

lgtm

https://chromiumcodereview.appspot.com/1427903002/diff/1/infra/tools/send_mon...
File infra/tools/send_monitoring_event/send_event.py (left):

https://chromiumcodereview.appspot.com/1427903002/diff/1/infra/tools/send_mon...
infra/tools/send_monitoring_event/send_event.py:52: default='POINT',
Should this still have the default?  If not remove the 'Default: %(default)s'
help text?

https://chromiumcodereview.appspot.com/1427903002/diff/1/infra_libs/event_mon...
File infra_libs/event_mon/test/config_test.py (right):

https://chromiumcodereview.appspot.com/1427903002/diff/1/infra_libs/event_mon...
infra_libs/event_mon/test/config_test.py:123: self.assertEqual(orig_event,
event_mon.get_default_event())
Maybe add an assertIsNot to make sure you get a copy.

[Sergey Berezin, 2015-10-29 23:32:55]

LGTM

My review was a bit rushed, but I guess between me and Dave we covered it :-)

https://chromiumcodereview.appspot.com/1427903002/diff/1/infra/tools/send_mon...
File infra/tools/send_monitoring_event/test/send_event_test.py (right):

https://chromiumcodereview.appspot.com/1427903002/diff/1/infra/tools/send_mon...
infra/tools/send_monitoring_event/test/send_event_test.py:243: # TODO(pgervais):
tests to add:
Is this planned for this CL, or later?

[pgervais, 2015-11-02 19:38:58]

Will file a new patchset later today (hopefully)

https://chromiumcodereview.appspot.com/1427903002/diff/1/infra/tools/send_mon...
File infra/tools/send_monitoring_event/send_event.py (left):

https://chromiumcodereview.appspot.com/1427903002/diff/1/infra/tools/send_mon...
infra/tools/send_monitoring_event/send_event.py:52: default='POINT',
On 2015/10/29 23:17:33, dsansome wrote:
> Should this still have the default?  If not remove the 'Default: %(default)s'
> help text?

Done.

https://chromiumcodereview.appspot.com/1427903002/diff/1/infra/tools/send_mon...
File infra/tools/send_monitoring_event/test/send_event_test.py (right):

https://chromiumcodereview.appspot.com/1427903002/diff/1/infra/tools/send_mon...
infra/tools/send_monitoring_event/test/send_event_test.py:243: # TODO(pgervais):
tests to add:
On 2015/10/29 23:32:54, Sergey Berezin wrote:
> Is this planned for this CL, or later?

This CL. I'll add them today.

https://chromiumcodereview.appspot.com/1427903002/diff/1/infra_libs/event_mon...
File infra_libs/event_mon/test/config_test.py (right):

https://chromiumcodereview.appspot.com/1427903002/diff/1/infra_libs/event_mon...
infra_libs/event_mon/test/config_test.py:123: self.assertEqual(orig_event,
event_mon.get_default_event())
On 2015/10/29 23:17:33, dsansome wrote:
> Maybe add an assertIsNot to make sure you get a copy.

Done.

[pgervais, 2015-11-03 00:17:26]

Finally added the missing tests. PTAL

[Sergey Berezin, 2015-11-03 00:58:42]

LGTM + a couple of small nits.

https://chromiumcodereview.appspot.com/1427903002/diff/20001/infra/tools/send...
File infra/tools/send_monitoring_event/test/send_event_test.py (right):

https://chromiumcodereview.appspot.com/1427903002/diff/20001/infra/tools/send...
infra/tools/send_monitoring_event/test/send_event_test.py:262: ])
nit: the indent looks odd to me - I'd expect 2 spaces to match the opening '['
(here and in the other tests)

https://chromiumcodereview.appspot.com/1427903002/diff/20001/infra/tools/send...
infra/tools/send_monitoring_event/test/send_event_test.py:276:
self.assertEqual(event.timestamp_kind, ChromeInfraEvent.BEGIN)
Another option is to use expectations:
  return str(event).splitlines()

https://chromiumcodereview.appspot.com/1427903002/diff/20001/infra/tools/send...
infra/tools/send_monitoring_event/test/send_event_test.py:321: # the logrequest
provided contains both a service and a build type,
nit: capitalize the first letter in a sentence.

[pgervais, 2015-11-03 01:03:49]

Nits fixed

https://chromiumcodereview.appspot.com/1427903002/diff/20001/infra/tools/send...
File infra/tools/send_monitoring_event/test/send_event_test.py (right):

https://chromiumcodereview.appspot.com/1427903002/diff/20001/infra/tools/send...
infra/tools/send_monitoring_event/test/send_event_test.py:262: ])
On 2015/11/03 00:58:42, Sergey Berezin wrote:
> nit: the indent looks odd to me - I'd expect 2 spaces to match the opening '['
> (here and in the other tests)

Done.

https://chromiumcodereview.appspot.com/1427903002/diff/20001/infra/tools/send...
infra/tools/send_monitoring_event/test/send_event_test.py:276:
self.assertEqual(event.timestamp_kind, ChromeInfraEvent.BEGIN)
On 2015/11/03 00:58:42, Sergey Berezin wrote:
> Another option is to use expectations:
>   return str(event).splitlines()

This test is already confusing because we don't know what's inside
service-bar-service.bin, using expectations would make the situation worse.

https://chromiumcodereview.appspot.com/1427903002/diff/20001/infra/tools/send...
infra/tools/send_monitoring_event/test/send_event_test.py:321: # the logrequest
provided contains both a service and a build type,
On 2015/11/03 00:58:42, Sergey Berezin wrote:
> nit: capitalize the first letter in a sentence.

Done.

[Sergey Berezin, 2015-11-03 01:19:09]

https://chromiumcodereview.appspot.com/1427903002/diff/20001/infra/tools/send...
File infra/tools/send_monitoring_event/test/send_event_test.py (right):

https://chromiumcodereview.appspot.com/1427903002/diff/20001/infra/tools/send...
infra/tools/send_monitoring_event/test/send_event_test.py:276:
self.assertEqual(event.timestamp_kind, ChromeInfraEvent.BEGIN)
On 2015/11/03 01:03:49, pgervais wrote:
> On 2015/11/03 00:58:42, Sergey Berezin wrote:
> > Another option is to use expectations:
> >   return str(event).splitlines()
> 
> This test is already confusing because we don't know what's inside
> service-bar-service.bin, using expectations would make the situation worse. 

Hmm... Would it make more sense then to generate the binary file as part of the
test, say in setUp()?

[pgervais, 2015-11-03 01:46:44]

The CQ bit was checked by pgervais@chromium.org

[pgervais, 2015-11-03 01:46:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
dsansome@chromium.org, sergeyberezin@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/1427903002/#ps40001
(title: "Fixed nits")

[commit-bot: I haz the power, 2015-11-03 01:46:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1427903002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1427903002/40001

[commit-bot: I haz the power, 2015-11-03 01:48:52]

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

[commit-bot: I haz the power, 2015-11-03 01:48:53]

Try jobs failed on following builders:
  Infra Mac Tester on tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Infra%20Mac%20Tester/bu...)

[pgervais, 2015-11-03 01:52:57]

The CQ bit was checked by pgervais@chromium.org

[pgervais, 2015-11-03 01:52:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
sergeyberezin@chromium.org, dsansome@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/1427903002/#ps60001
(title: "Added missing files")

[commit-bot: I haz the power, 2015-11-03 01:53:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1427903002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1427903002/60001

[commit-bot: I haz the power, 2015-11-03 01:55:02]

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

[commit-bot: I haz the power, 2015-11-03 01:55:03]

Try jobs failed on following builders:
  Infra Mac Tester on tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Infra%20Mac%20Tester/bu...)

[pgervais, 2015-11-03 19:45:06]

Committed patchset #5 (id:80001) manually as
6d15e263015edf6a4cfb6a2e4a3081ba8efe4ec7 (presubmit successful).