1272176 - Record system resource utilization in Perfherder

Status: RESOLVED FIXED

Description

[Gregory Szorc [:gps]]

+++ This bug was initially created as a clone of Bug #1271077 +++

Bug 1271077 is surfacing system resource usage via TinderboxPrint. This is a follow-up to record the data in Perfherder, if wlach lets us :)

Comment 1

[Gregory Szorc [:gps]]

Attachment: Bug 1272176 - Emit Perfherder data for system resource utilization;

THIS PATCH IS NOT COMPLETE AND IS NOT READY TO LAND.

Submitting to wlach for early feedback so I know if I'm on the
right track.

This commit teaches the resource monitor in mozharness to emit
Perfherder data for system metrics and step times. This will
allow us to see when the timing or resource characteristics
of jobs in automation changes.

The wonkiest part of this patch is likely the mechanism to
define the Perfherder "test" names. We don't appear to have
an identifier in mozharness suitable for distinguishing
between job types. e.g. the "desktop_unittest.py" script is
responsible for running a few dozen jobs. So we invent code
for creating an identifier from the script config options.
I /think/ Treeherder will automatically assign the
project/branch, platform, and build type, which is why these
aren't included in the identifier.

Review commit: https://reviewboard.mozilla.org/r/52303/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52303/

Comment 2

[Gregory Szorc [:gps]]

Comment on attachment 8751959 [details]
Bug 1272176 - Emit Perfherder data for system resource utilization;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52303/diff/1-2/

Comment 3

[Gregory Szorc [:gps]]

Comment on attachment 8751959 [details]
Bug 1272176 - Emit Perfherder data for system resource utilization;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52303/diff/2-3/

Comment 4

[William Lachance (:wlach) (use needinfo!)]

Comment on attachment 8751959 [details]
Bug 1272176 - Emit Perfherder data for system resource utilization;

https://reviewboard.mozilla.org/r/52303/#review49509

This is looking pretty good! We should test this on treeherder stage for a while (at least a few weeks) before enabling on master. Let me think about how to do that in a reasonable way.

::: testing/mozharness/scripts/desktop_unittest.py:198
(Diff revision 3)
> +            perfherder_parts.append(c['this_chunk'])
> +        if c['e10s']:
> +            perfherder_parts.append('e10s')
> +
> +        self.resource_monitor_perfherder_name = '.'.join(perfherder_parts)
> +

You should run this through the perfherder schema, if possible. There's an example of this with talos:

http://mxr.mozilla.org/mozilla-central/source/testing/mozharness/mozharness/mozilla/testing/talos.py#350

Comment 5

[Gregory Szorc [:gps]]

Attachment: Bug 1272176 - Synchronize performance artifact schema with upstream;

Having the latest schema available seems like a good thing. This is a
direct copy of schemas/performance-artifact.json from
https://github.com/mozilla/treeherder.git at commit
7bed1b22ceb01e3e71536fa1c4ecd14ddc87e803.

Review commit: https://reviewboard.mozilla.org/r/53354/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53354/

Comment 6

[Gregory Szorc [:gps]]

Comment on attachment 8751959 [details]
Bug 1272176 - Emit Perfherder data for system resource utilization;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52303/diff/3-4/

Comment 7

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/52303/#review49509

Something else I noticed is that Treeherder failed to parse the logs on the original Try push. See https://treeherder.mozilla.org/#/jobs?repo=try&revision=52754ceac78c0673b3f4499415b858ddcb5d75be. Perhaps that is due to an unknown suite name in the PERFHERDER_DATA?

Comment 8

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/52303/#review50160

::: testing/mozharness/mozharness/base/python.py:619
(Diff revision 4)
> +                schema_path = os.path.join(self.query_abs_dirs()['abs_work_dir'],
> +                                           'tests', 'talos', 'treeherder-schemas',
> +                                           'performance-artifact.json')

This doesn't work because the JSON file appears to be coming from the test archive. We'll probably want to move the schema file into mozharness so all mozharness jobs have access to it.

Comment 9

[William Lachance (:wlach) (use needinfo!)]

Comment on attachment 8753566 [details]
Bug 1272176 - Synchronize performance artifact schema with upstream;

https://reviewboard.mozilla.org/r/53354/#review50366

Yup, this is a no-brainer

Comment 10

[William Lachance (:wlach) (use needinfo!)]

Comment on attachment 8751959 [details]
Bug 1272176 - Emit Perfherder data for system resource utilization;

https://reviewboard.mozilla.org/r/52303/#review50370

This all looks fine to me. I'd like to see some samples of what PERFHERDER_DATA looks like in the try runs before giving an r+.

I'd also like to test this on treeherder stage (treeherder.allizom.org) for a while before enabling this on production. I filed a bug https://bugzilla.mozilla.org/show_bug.cgi?id=1273983 to make this sort of easier, but I can hack something in so we don't have to gate landing this patch on that.

Comment 11

[Gregory Szorc [:gps]]

Attachment: Bug 1272176 - Move performance artifact schema into mozharness directory;

Currently, only Talos accesses this file. An uncoming commit will add
a non-Talos consumer. Enable all mozharness consumers to access the
file by including it in the mozharness directory (previously it was
part of the Talos test archive).

Review commit: https://reviewboard.mozilla.org/r/53638/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53638/

Comment 12

[Gregory Szorc [:gps]]

Comment on attachment 8753566 [details]
Bug 1272176 - Synchronize performance artifact schema with upstream;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53354/diff/1-2/

Comment 13

[Gregory Szorc [:gps]]

Comment on attachment 8751959 [details]
Bug 1272176 - Emit Perfherder data for system resource utilization;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52303/diff/4-5/

Comment 14

[William Lachance (:wlach) (use needinfo!)]

https://reviewboard.mozilla.org/r/52303/#review49509

That's odd -- it clearly did parse the logs because I see all sorts of job metadata in there. I think this is just a treeherder error. Presently perfherder just ignores unknown suite names, so that shouldn't be a problem.

Comment 15

[William Lachance (:wlach) (use needinfo!)]

Comment on attachment 8751959 [details]
Bug 1272176 - Emit Perfherder data for system resource utilization;

https://reviewboard.mozilla.org/r/52303/#review50424

I don't see anything wrong with this, but we should figure out why it's failing on try before landing.

Comment 16

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/52303/#review50424

Schema failure on the last push. New patch coming shortly.

Comment 17

[Gregory Szorc [:gps]]

Comment on attachment 8751959 [details]
Bug 1272176 - Emit Perfherder data for system resource utilization;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52303/diff/5-6/

Comment 18

[William Lachance (:wlach) (use needinfo!)]

Comment on attachment 8751959 [details]
Bug 1272176 - Emit Perfherder data for system resource utilization;

https://reviewboard.mozilla.org/r/52303/#review50744

I'm seeing some errors validating the schema on try (even though it claims to be green??) e.g. https://public-artifacts.taskcluster.net/XxeXH77bToOtSEYipNa1AQ/0/public/logs/live_backing.log (search for perfherder)

I also should have mentioned this before, but we can specify "e10s" as an extra property of the suite (via extraOptions), which will probably look a little bit nicer in the UI. You can see an example of this in the e10s talos jobs (again, search for perfherder):

http://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-inbound-linux64/1463679999/mozilla-inbound_ubuntu64_hw_test-dromaeojs-e10s-bm105-tests1-linux-build4330.txt.gz

Comment 19

[Gregory Szorc [:gps]]

Comment on attachment 8753993 [details]
Bug 1272176 - Move performance artifact schema into mozharness directory;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53638/diff/1-2/

Comment 20

[Gregory Szorc [:gps]]

Comment on attachment 8753566 [details]
Bug 1272176 - Synchronize performance artifact schema with upstream;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53354/diff/2-3/

Comment 21

[Gregory Szorc [:gps]]

Comment on attachment 8751959 [details]
Bug 1272176 - Emit Perfherder data for system resource utilization;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52303/diff/6-7/

Comment 22

[Gregory Szorc [:gps]]

Comment on attachment 8751959 [details]
Bug 1272176 - Emit Perfherder data for system resource utilization;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52303/diff/7-8/

Comment 23

[William Lachance (:wlach) (use needinfo!)]

Comment on attachment 8753993 [details]
Bug 1272176 - Move performance artifact schema into mozharness directory;

https://reviewboard.mozilla.org/r/53638/#review51532

Comment 24

[William Lachance (:wlach) (use needinfo!)]

Comment on attachment 8751959 [details]
Bug 1272176 - Emit Perfherder data for system resource utilization;

https://reviewboard.mozilla.org/r/52303/#review51534

The code looks fine (didn't check the try runs) except for one thing which I mentioned but forgot to open an issue for.

::: testing/mozharness/scripts/desktop_unittest.py:200
(Diff revision 8)
> +            perfherder_parts.append(c['this_chunk'])
> +
> +        if c['e10s']:
> +            perfherder_options.append('e10s')
> +        else:
> +            perfherder_options.append('no-e10s')

I mentioned this a few days ago but forgot to "open an issue".

You can specify 'e10s' as extraOptions for a suite, which I think would be better than putting it in the suite name. You can see an example of this in the e10s talos jobs:

http://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-inbound-linux64/1463679999/mozilla-inbound_ubuntu64_hw_test-dromaeojs-e10s-bm105-tests1-linux-build4330.txt.gz

Just omit "no-e10s" entirely, I think.

Comment 25

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/52303/#review51534

> I mentioned this a few days ago but forgot to "open an issue".
> 
> You can specify 'e10s' as extraOptions for a suite, which I think would be better than putting it in the suite name. You can see an example of this in the e10s talos jobs:
> 
> http://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-inbound-linux64/1463679999/mozilla-inbound_ubuntu64_hw_test-dromaeojs-e10s-bm105-tests1-linux-build4330.txt.gz
> 
> Just omit "no-e10s" entirely, I think.

Uh, isn't this what this patch is doing now? That was changed in revision 8 IIRC.

Comment 26

[Gregory Szorc [:gps]]

Comment on attachment 8753993 [details]
Bug 1272176 - Move performance artifact schema into mozharness directory;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53638/diff/2-3/

Comment 27

[Gregory Szorc [:gps]]

Comment on attachment 8753566 [details]
Bug 1272176 - Synchronize performance artifact schema with upstream;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53354/diff/3-4/

Comment 28

[Gregory Szorc [:gps]]

Comment on attachment 8751959 [details]
Bug 1272176 - Emit Perfherder data for system resource utilization;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52303/diff/8-9/

Comment 29

[William Lachance (:wlach) (use needinfo!)]

https://reviewboard.mozilla.org/r/52303/#review51534

> Uh, isn't this what this patch is doing now? That was changed in revision 8 IIRC.

Sorry, I guess I misread your changes. This does look right now (haven't checked the output). But you should still remove the "no-e10s" option, that's redundant.

Comment 30

[William Lachance (:wlach) (use needinfo!)]

Comment on attachment 8751959 [details]
Bug 1272176 - Emit Perfherder data for system resource utilization;

https://reviewboard.mozilla.org/r/52303/#review54120

Let's land this! Note that ingestion of this won't happen until we add a performance framework for this type of data in perfherder. Let's figure out how to do that seperately, on stage only at first.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/901aee13bd76
Move performance artifact schema into mozharness directory; r=wlach
https://hg.mozilla.org/integration/mozilla-inbound/rev/82fb632252de
Synchronize performance artifact schema with upstream; r=wlach
https://hg.mozilla.org/integration/mozilla-inbound/rev/7bff68b26943
Emit Perfherder data for system resource utilization; r=wlach

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b1961339522
Import mozharness to avoid missing symbol; r=me

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/901aee13bd76
https://hg.mozilla.org/mozilla-central/rev/82fb632252de
https://hg.mozilla.org/mozilla-central/rev/7bff68b26943
https://hg.mozilla.org/mozilla-central/rev/4b1961339522