analyze: send results to event_mon

analyze: send results to event_mon

BUG=651536
Committed: https://chromium.googlesource.com/chromium/tools/build/+/e10b31caeb97375c3514ae3c9f1a92e28d97b71e

[Paweł Hajdan Jr., 2016-10-05 20:50:31]

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

[commit-bot: I haz the power, 2016-10-05 20:50:37]

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, 2016-10-05 20:55:27]

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

[commit-bot: I haz the power, 2016-10-05 20:55:28]

Dry run: This issue passed the CQ dry run.

[Paweł Hajdan Jr., 2016-10-05 20:57:57]

phajdan.jr@chromium.org changed reviewers:
+ chrishall@chromium.org, philwright@chromium.org, sergeyberezin@chromium.org

[Paweł Hajdan Jr., 2016-10-05 20:57:58]

Chris, Phil: I've added you since it seems SRE now own monitoring. If you have
any questions about recipes, just let me know.

Main files to focus on are scripts/slave/send_analyze_event.py and
scripts/slave/recipe_modules/filter/api.py . JSON files are mechanically
generated expectation changes. DEPS change is there to pull newer version of the
proto, https://chromium-review.googlesource.com/c/390692/ .

I've tested the change locally and it worked.

Note that the new event reporting step is non-fatal so it won't break
production.

Feedback and questions are welcome.

[chrishall, 2016-10-07 06:36:27]

lgtm

Gave it a quick once-over, however I have very little experience with event_mon
so I would like someone else to take a look also.

https://codereview.chromium.org/2397593003/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/filter/api.py (right):

https://codereview.chromium.org/2397593003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/filter/api.py:222:
step_result.presentation.step_text = 'Error: ' + \
not sure if this trailing \ is needed, below we have a plus symbol on
end-of-line that lacks a trailing \.

https://codereview.chromium.org/2397593003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/filter/api.py:228: raise
self.m.step.StepFailure('Error, following targets were not ' + \
same here, not sure if this trailing \ is needed, below we have a plus symbol on
end-of-line that lacks a trailing \.

https://codereview.chromium.org/2397593003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/filter/api.py:232:
step_result.json.output['status'] == 'Found dependency (all)'):
could change this to

if step_result.json.output['status'] in ['Found dependency', 'Found dependency
(all)']:
  ...

https://codereview.chromium.org/2397593003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/filter/api.py:242: self._compile_targets =
sorted(set(self._compile_targets +
this is the line that ends in + and lacks trailing \

https://codereview.chromium.org/2397593003/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/filter/example.expected/analyzes_error.json
(right):

https://codereview.chromium.org/2397593003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/filter/example.expected/analyzes_error.json:126:
"{\"error\": \"ERROR\"}"
is this file auto-generated?

if not, then it might be worth replacing all the \" with just ', (or changing
the outer quotes from " to ').

https://codereview.chromium.org/2397593003/diff/1/scripts/slave/recipes/chrom...
File scripts/slave/recipes/chromium_trybot.expected/compile_failure_infra.json
(right):

https://codereview.chromium.org/2397593003/diff/1/scripts/slave/recipes/chrom...
scripts/slave/recipes/chromium_trybot.expected/compile_failure_infra.json:325:
"@@@STEP_LOG_END@excluded_files@@@"
was removal of this trailing comma intended?

https://codereview.chromium.org/2397593003/diff/1/scripts/slave/send_analyze_...
File scripts/slave/send_analyze_event.py (right):

https://codereview.chromium.org/2397593003/diff/1/scripts/slave/send_analyze_...
scripts/slave/send_analyze_event.py:8: Needs to be run via
tools/scripts/runit.py"""
is it worth giving an example invocation?

https://codereview.chromium.org/2397593003/diff/1/scripts/slave/send_analyze_...
scripts/slave/send_analyze_event.py:68: print event.proto
should this print be here?

is this print statement the output being tested by all the json files?

[Paweł Hajdan Jr., 2016-10-07 10:50:54]

https://codereview.chromium.org/2397593003/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/filter/api.py (right):

https://codereview.chromium.org/2397593003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/filter/api.py:222:
step_result.presentation.step_text = 'Error: ' + \
On 2016/10/07 at 06:36:26, chrishall wrote:
> not sure if this trailing \ is needed, below we have a plus symbol on
end-of-line that lacks a trailing \.

Done.

https://codereview.chromium.org/2397593003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/filter/api.py:228: raise
self.m.step.StepFailure('Error, following targets were not ' + \
On 2016/10/07 at 06:36:26, chrishall wrote:
> same here, not sure if this trailing \ is needed, below we have a plus symbol
on end-of-line that lacks a trailing \.

Done.

https://codereview.chromium.org/2397593003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/filter/api.py:232:
step_result.json.output['status'] == 'Found dependency (all)'):
On 2016/10/07 at 06:36:26, chrishall wrote:
> could change this to
> 
> if step_result.json.output['status'] in ['Found dependency', 'Found dependency
(all)']:
>   ...

Done.

https://codereview.chromium.org/2397593003/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/filter/example.expected/analyzes_error.json
(right):

https://codereview.chromium.org/2397593003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/filter/example.expected/analyzes_error.json:126:
"{\"error\": \"ERROR\"}"
On 2016/10/07 at 06:36:26, chrishall wrote:
> is this file auto-generated?
> 
> if not, then it might be worth replacing all the \" with just ', (or changing
the outer quotes from " to ').

It's auto-generated.

https://codereview.chromium.org/2397593003/diff/1/scripts/slave/recipes/chrom...
File scripts/slave/recipes/chromium_trybot.expected/compile_failure_infra.json
(right):

https://codereview.chromium.org/2397593003/diff/1/scripts/slave/recipes/chrom...
scripts/slave/recipes/chromium_trybot.expected/compile_failure_infra.json:325:
"@@@STEP_LOG_END@excluded_files@@@"
On 2016/10/07 at 06:36:26, chrishall wrote:
> was removal of this trailing comma intended?

The file is auto-generated, and yes it is for python's json module.

https://codereview.chromium.org/2397593003/diff/1/scripts/slave/send_analyze_...
File scripts/slave/send_analyze_event.py (right):

https://codereview.chromium.org/2397593003/diff/1/scripts/slave/send_analyze_...
scripts/slave/send_analyze_event.py:8: Needs to be run via
tools/scripts/runit.py"""
On 2016/10/07 at 06:36:26, chrishall wrote:
> is it worth giving an example invocation?

Recipe expectations contain some examples.

https://codereview.chromium.org/2397593003/diff/1/scripts/slave/send_analyze_...
scripts/slave/send_analyze_event.py:68: print event.proto
On 2016/10/07 at 06:36:26, chrishall wrote:
> should this print be here?

I find it useful, at least for now.

> is this print statement the output being tested by all the json files?

No, recipe expectations do not actually execute the script.

[emso, 2016-10-07 10:59:16]

emso@chromium.org changed reviewers:
+ emso@chromium.org

[emso, 2016-10-07 10:59:17]

lgtm

Note that I also do not have any experience with event_mon.

[Paweł Hajdan Jr., 2016-10-07 11:06:12]

The CQ bit was checked by phajdan.jr@chromium.org

[Paweł Hajdan Jr., 2016-10-07 11:06:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishall@chromium.org
Link to the patchset: https://codereview.chromium.org/2397593003/#ps20001
(title: "review")

[commit-bot: I haz the power, 2016-10-07 11:06:22]

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, 2016-10-07 11:11:44]

Description was changed from

==========
analyze: send results to event_mon

BUG=651536
==========

to

==========
analyze: send results to event_mon

BUG=651536

Committed:
https://chromium.googlesource.com/chromium/tools/build/+/e10b31caeb97375c3514...
==========

[commit-bot: I haz the power, 2016-10-07 11:11:45]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/tools/build/+/e10b31caeb97375c3514...

[Sergey Berezin, 2016-10-07 18:50:22]

LGTM (post-commit) - sorry it took me longer than expected to look at the CL.

My only meta-comment is that it's kinda sad that we have to use an external
command and an explicit step for sending monitoring data. IMHO it would be much
better if the recipe code just sent the events natively. Monitoring shouldn't
need to be added by the (recipe) user, it should just happen behind the scenes
as part of analyze step. But I guess we're not quite ready for this yet, so this
is fine for now. Thanks!