907331 - Resource monitor API for dumping data as JSON

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Gregory Szorc [:gps]]

In bug 883209, we hooked resource monitoring up to the build system and added an HTML viewer. Looking towards the future, we'd like to converge all the users of resource monitoring (build, mozharness, etc) so they can share the same interface for rendering results. This likely means sharing a unified data format. I think it makes sense to add an API to the resource monitor class for dumping the resource data as JSON (or at least as a Python object that can be serialized to JSON).

Comment 1

[Gregory Szorc [:gps]]

Attachment: 0001-Bug-907331-Dump-resource-usage-data-to-a-serializabl.patch

Here's my first stab at it. I didn't add a JSON API, but merely a "convert to dict" which can then be serialized into JSON easily enough. This also facilitates downstream consumers adding additional metadata to the data structure.

Comment 2

[Andrew Halberstadt [:ahal]]

Comment on attachment 794296 [details] [diff] [review]
0001-Bug-907331-Dump-resource-usage-data-to-a-serializabl.patch

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

Most of the terminology here is over my head, but I don't see anything wrong with code itself.

::: mozsystemmonitor/mozsystemmonitor/resourcemonitor.py
@@ +495,5 @@
> +            has 2 elements, the float wall time of the event and the string
> +            event name.
> +          phases - A list of dicts describing phases. Each phase looks a lot
> +            like an entry from samples (see below). Some phases may not have
> +            data recorded against them, so some keys may be None.

nit: some values may be None?

::: mozsystemmonitor/mozsystemmonitor/test/test_resource_monitor.py
@@ +172,5 @@
> +        d = monitor.as_dict()
> +
> +        self.assertEqual(d['version'], 1)
> +        self.assertEqual(len(d['events']), 2)
> +        self.assertEqual(len(d['phases']), 2)

Wouldn't hurt to add a check for the other keys as well.

Comment 3

[Gregory Szorc [:gps]]

And here we have an r+ that's been lingering for 9 months. What a huge fail on my part. Why doesn't Bugzilla nag about this? If I can land this easily, I'll do it.

Comment 4

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f50bbf4738d0

Trivial patch applied cleanly.

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f50bbf4738d0