Add class for recording system resource usage

RESOLVED FIXED

Status

RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: gps, Assigned: gps)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
Created attachment 672089 [details] [diff] [review]
Add mozsysteminfo package with resource monitory class, v1

I have implemented a class for monitoring entire system resource usage. I think it's pretty useful and would make a good addition to mozbase.

This could arguably be added to mozinfo instead of creating a whole new package. I'll leave that to the mozbase people to decide.

Future additions to this package could include modules for easily looking at running processes, memory usage, etc. Basically, I'm thinking this will be high-level API for psutil.

The specific use case of this class that I have in mind is to record and analyze resource usage as part of performing builds and running test suites. Currently, we don't record any of this info. I'd like to have data such as average CPU usage during builds easily available. This class will facilitate that.

I'm not sure if I tagged the proper reviewer. Also, I don't care that this requires Python 2.7+. I understand some test runners aren't currently running 2.7. Long term they will run 2.7, so that's what I am targeting.

This is pretty low priority. But, as soon as it lands I will update mach to display data porn ;)
Attachment #672089 - Flags: review?(jhammel)

Comment 1

6 years ago
So, I'm not really sure what to do with this.  My personal position is that it is already extremely confusing to figure out what version of python we are required to support in mozbase and I don't really want to make those waters even muddier.  Our official policy is "2.5+ except 2.4 for talos (sorta, since we only care about 2.4 for windows talos, so...)" :wlach, what's your opinion here?  As much as I want to get on 2.7, I don't really want to start enabling others writing 2.7 code and then me having to backport it, as has often been the situation in the past 6 months or so.

Also, and more minor, we use markdown syntax vs rst syntax for our text files.  FWIW,I prefer rst.  I do want consistency though.
(Assignee)

Comment 2

6 years ago
If mozbase isn't the correct place for this code, I can certainly check it into m-c under python/ if that seems like a better fit. I'm "offering" it to mozbase because I think it will make a good addition.

Comment 3

6 years ago
Comment on attachment 672089 [details] [diff] [review]
Add mozsysteminfo package with resource monitory class, v1

I'm going to take myself down as a reviewer.  I really would like to take this patch, and I do think that mozbase is the appropriate place for it, but the README remains in restructured text vs markdown.  Talking with :wlach, he is fine taking packages that have inconsistent versions.  This has been a nightmare for me in the past and there is no particular place that tells what versions of python we require.  The thought of trying to untangle this right now, TBH, I find quite daunting.  This all makes me very sad.

At the risk of saying too much unrelated, I really wish others could step up and try to coordinate this a bit better.  If we want to be on python 2.7 we should throw some effort at that and not try to reap the benefits ahead of time.  If we want to have different mozbase packages require different versions we should note that in some way that doesn't require reading code or failing e.g. on try were we to try to use this in a test harness that is run on an older version.  I've really tried hard to coordinate the metadata about the project to support others.  I guess I have failed.
Attachment #672089 - Flags: review?(jhammel)
(Assignee)

Comment 4

6 years ago
Releng has stated that they will move all the machines to Python 2.7. I have heard Chris Cooper say recently that they are working on it.
Is there an ETA on it or a bug we can follow?
(Assignee)

Comment 6

6 years ago
See the bug tree for bug 724191.
See Also: → bug 724191

Comment 8

6 years ago
see also bug 803714
(Assignee)

Comment 9

5 years ago
Created attachment 736599 [details] [diff] [review]
Add mozsystemmonitor package, v2

I cleaned up the patch a bit. It now reports individual CPU times in addition to high-level CPU percent. I also made things behave better if psutil is not available.

You can also view at https://github.com/indygreg/mozbase/tree/bug-802420 if that is your thing.

This is my first contribution to mozbase. I tried to follow the directions at https://wiki.mozilla.org/Auto-tools/Projects/MozBase. No clue how well I did.
Assignee: nobody → gps
Attachment #672089 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #736599 - Flags: review?(jhammel)
(Assignee)

Comment 10

5 years ago
Review ping.

Comment 11

5 years ago
Comment on attachment 736599 [details] [diff] [review]
Add mozsystemmonitor package, v2

Review of attachment 736599 [details] [diff] [review]:
-----------------------------------------------------------------

I'm sorry I let this lag so long.  My recommendation is to take this as-is with the setup.py change.  I would like a few follow-ups, or you can roll them into the patch:

* the test should probably have a manifest: https://wiki.mozilla.org/Auto-tools/Projects/Mozbase#Tests
* we now do documentation differently; https://wiki.mozilla.org/Auto-tools/Projects/MozBase/WritingDocs ; so the README should go in the top-level docs directory, https://github.com/mozilla/mozbase/tree/master/docs , and setup.py should be changed to not read this file and use a long description a la https://github.com/mozilla/mozbase/blob/master/mozfile/setup.py#L12

Other than that, great work, and apologies again for the huge lag.  If no one opposes, go for landing w/ at least setup.py fixed.

::: mozsystemmonitor/setup.py
@@ +5,5 @@
> +import os
> +
> +from setuptools import setup
> +
> +PACKAGE_VERSION = '0.1'

Please change this to 0.0 as per https://wiki.mozilla.org/Auto-tools/Projects/Mozbase#Adding_a_New_Mozbase_Package

@@ +9,5 @@
> +PACKAGE_VERSION = '0.1'
> +
> +try:
> +    pwd = os.path.dirname(os.path.abspath(__file__))
> +    description = open(os.path.join(here, 'README.rst')).read()

We don't do this anymore. Sorry, my fault that the patch has rotted.
Attachment #736599 - Flags: review?(jhammel) → review+
(Assignee)

Comment 12

5 years ago
https://github.com/mozilla/mozbase/commit/0a1214782a81dc0cde21d001bcfd737c8a860ebb

Will file follow-up for review feedback.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Blocks: 870570
(Assignee)

Updated

5 years ago
Blocks: 883205
(Assignee)

Updated

5 years ago
Blocks: 907331
You need to log in before you can comment on or make changes to this bug.