847662 - HealthReporter and providers refactoring

Status: RESOLVED FIXED

(Firefox Health Report Graveyard :: Client: Desktop, defect)

Description

[Gregory Szorc [:gps]]

A lot of the code in healthreporter.jsm related to provider management. The goal is to more loosely couple this from the high-level service code to better facilitate Android, readable code, etc.

Goals of this bug:

* Rename Metrics.Collector -> Metrics.ProviderManager
* Move all provider management code from HealthReporter into Metrics.ProviderManager
* Have ProviderManager be more intelligent about automatically instantiating pull-only providers, etc.

Comment 1

[Gregory Szorc [:gps]]

Attachment: Part 1: Rename Collector -> ProviderManager, v1

This is largely mechanical. I'm pretty sure I got all the references.

Comment 2

[Gregory Szorc [:gps]]

Attachment: Part 2: Add finalizers to tests, v1

While I was here, I noticed that tests could hang if there was an error in the test. The reason is the health reporter not shutting down will result in the process waiting for a shutdown signal which it never receives (at least in tests).

I added finally blocks to each test susceptible to this. This may also help us track down some intermittent test hangs in these tests!

There is a larger problem: why doesn't FHR shut down gracefully in xpcshell tests? My guess is the xpcshell runner doesn't broadcast the expected app shutdown notifications if a test fails. Hmmm.

Comment 3

[Richard Newman [:rnewman]]

Comment on attachment 721372 [details] [diff] [review]
Part 2: Add finalizers to tests, v1

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

Did you reproduce a test hang, even informally, and verify that this fixed the hang?

Comment 4

[Gregory Szorc [:gps]]

(In reply to Richard Newman [:rnewman] from comment #3)
> Did you reproduce a test hang, even informally, and verify that this fixed
> the hang?

Yes.

Comment 5

[Gregory Szorc [:gps]]

Attachment: Part 3: Move provider code into providermanager, v1

This is a largely mechanical move of provider management function from healthreporter.jsm into providermanager.jsm. I tried to not change semantics except where necessary. A future patch will rework ProviderManager's internals to be more robust so things like pull-only provider management is transparent to the outside world. It will also lock down the public API so providers can only be registered with their constructor function, etc.

Comment 6

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/872cb5946d2f
https://hg.mozilla.org/integration/mozilla-inbound/rev/20e7a7fa3bdc

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/872cb5946d2f
https://hg.mozilla.org/mozilla-central/rev/20e7a7fa3bdc

Comment 8

[Richard Newman [:rnewman]]

Comment on attachment 721478 [details] [diff] [review]
Part 3: Move provider code into providermanager, v1

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

Largely a mechanical review, too :D

Looking forward to seeing this keep moving.

Comment 9

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/97d6871f02e7

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/97d6871f02e7

Comment 11

[Richard Newman [:rnewman]]

This'll do for now.

Comment 12

[Gregory Szorc [:gps]]

Comment on attachment 721369 [details] [diff] [review]
Part 1: Rename Collector -> ProviderManager, v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): None
User impact if declined: None
Testing completed (on m-c, etc.): Code has been baking for a while. FHR would be significantly broken if this patch were broken.
Risk to taking this patch (and alternatives if risky): Little
String or UUID changes made by this patch: None

This falls into the same bucket as bug 841074. The uplift isn't required but it will make applying future tracked uplifts much easier and will result in a more unified code configuration. If it's not accepted, we can work around it.

The patches in this bug are mostly a movement of code between files and a renaming of symbols. The risk should be very low.

Comment 13

[Gregory Szorc [:gps]]

Comment on attachment 721372 [details] [diff] [review]
Part 2: Add finalizers to tests, v1

[Approval Request Comment]
See initial uplift request.

Comment 14

[Gregory Szorc [:gps]]

Comment on attachment 721478 [details] [diff] [review]
Part 3: Move provider code into providermanager, v1

[Approval Request Comment]
See initial uplift request.

Comment 15

[Gregory Szorc [:gps]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9723bf992f50
https://hg.mozilla.org/releases/mozilla-aurora/rev/7fa0ed380ba9
https://hg.mozilla.org/releases/mozilla-aurora/rev/51893bbcd397