1227428 - Partner information missing from Telemetry Environment

Status: VERIFIED FIXED

(Toolkit :: Telemetry, defect, P3)

Description

[Hector Zhao [:hectorz]]

Attachment: Patch for feedback

When I tried to run a customized telemetry job on Fx China repack, I cannot get any records with "environment/partner/distributor" being "mozillaonline". Then I found out the partner entries in about:telemetry are all null.

Looks like it's because these information are only applied to Fx after final-ui-startup => reload-default-prefs => prefservice:after-app-defaults, but EnvironmentCache is initialized earlier than that.

The attached patch fixes about:telemetry for me, but I have no idea whether and how should I write a test for it.

Comment 1

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8691264 [details] [diff] [review]
Patch for feedback

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

Thanks, good find!

We should add documentation for this to the in-tree docs:
* a short mention around here:
https://dxr.mozilla.org/mozilla-central/rev/d3d286102ba7f8801e9dfe12d534f49554ba50c0/toolkit/components/telemetry/docs/environment.rst#66
* a proper explanation of when and why partner data is available (and what data we have when not) here:
https://dxr.mozilla.org/mozilla-central/rev/d3d286102ba7f8801e9dfe12d534f49554ba50c0/toolkit/components/telemetry/docs/environment.rst#245

The test coverage for this would go into test_TelemetryEnvironment.js:
* don't spoof partner data initially:
https://dxr.mozilla.org/mozilla-central/rev/d3d286102ba7f8801e9dfe12d534f49554ba50c0/toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js#648
* add a test function for partner data after this one:
https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js#665
* in it, check that initially partner data is empty/null
* then spoofPartnerInfo()
* then fire "distribution-customization-complete" to TelemetryEnvironment (Services.obs.notifyObservers())
* then check the environment data again and that partner info is present as expected

You run the test with:
mach xpcshell-test toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js

You can check for side effects on the other Telemetry components by running:
mach xpcshell-test toolkit/components/telemetry/tests/unit/

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ +844,5 @@
>      }
>    },
>  
> +  _addObserverForPartner: function() {
> +    Services.obs.addObserver(this, DISTRIBUTION_CUSTOMIZATION_COMPLETE_TOPIC, false);

Lets move this into _addObservers() below, there is no reason for a stand-alone function.

@@ +888,5 @@
> +      case DISTRIBUTION_CUSTOMIZATION_COMPLETE_TOPIC:
> +        // Distribution customizations are applied after final-ui-startup. query
> +        // partner prefs again when they are ready.
> +        this._updatePartner();
> +        Services.obs.removeObserver(this, aTopic);

If the EnvironmentCache() is created after DISTRIBUTION_CUSTOMIZATION_COMPLETE_TOPIC already fired, we would never remove that topic.
I think we should do this in _removeObservers().
If you don't want to keep the observer around after it fired you can
* remove it here as it is now and
* remove it from _removeObservers() protected in a try-catch

Comment 2

[Hector Zhao [:hectorz]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #1)
Thanks for the feedback, I'll try to get the patch ready for review tomorrow. One thing to explain:

> 
> ::: toolkit/components/telemetry/TelemetryEnvironment.jsm
> @@ +844,5 @@
> >      }
> >    },
> >  
> > +  _addObserverForPartner: function() {
> > +    Services.obs.addObserver(this, DISTRIBUTION_CUSTOMIZATION_COMPLETE_TOPIC, false);
> 
> Lets move this into _addObservers() below, there is no reason for a
> stand-alone function.

After the initial value of _currentEnvironment is set, there's a delay before setup(_addObservers) for both of this._addonBuilder.init() and this._updateProfile() to finish. It's possible that DISTRIBUTION_CUSTOMIZATION_COMPLETE_TOPIC be fired during this delay. I'm not quite sure whether this delay of _addObservers is intentional or not.

Comment 3

[Benjamin Smedberg]

[Tracking Requested - why for this release]:

This is really bad. Funnelcakes are implemented as partner builds so if we don't have this data correctly we'll be unable to implement the retention reports for those builds. I'd like to avoid shipping Fx43 without a fix here.

Comment 4

[Georg Fritzsche [:gfritzsche]]

(In reply to Hector Zhao [:hectorz] from comment #2)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #1)
> Thanks for the feedback, I'll try to get the patch ready for review
> tomorrow. One thing to explain:
> 
> > 
> > ::: toolkit/components/telemetry/TelemetryEnvironment.jsm
> > @@ +844,5 @@
> > >      }
> > >    },
> > >  
> > > +  _addObserverForPartner: function() {
> > > +    Services.obs.addObserver(this, DISTRIBUTION_CUSTOMIZATION_COMPLETE_TOPIC, false);
> > 
> > Lets move this into _addObservers() below, there is no reason for a
> > stand-alone function.
> 
> After the initial value of _currentEnvironment is set, there's a delay
> before setup(_addObservers) for both of this._addonBuilder.init() and
> this._updateProfile() to finish. It's possible that
> DISTRIBUTION_CUSTOMIZATION_COMPLETE_TOPIC be fired during this delay. I'm
> not quite sure whether this delay of _addObservers is intentional or not.

That is a good point.
I think we should actually:
* move the _addObservers() call up to where you triggered your call
* except the COMPOSITOR_CREATED_TOPIC observer, which should be added after that first _updateGraphicsFeatures()

Comment 5

[Hector Zhao [:hectorz]]

Attachment: MozReview Request: Bug 1227428 - Update partner section with correct information once available. r?gfritzsche

Bug 1227428 - Update partner section with correct information once available. r?gfritzsche

Comment 6

[Hector Zhao [:hectorz]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #4)
> 
> That is a good point.
> I think we should actually:
> * move the _addObservers() call up to where you triggered your call
> * except the COMPOSITOR_CREATED_TOPIC observer, which should be added after
> that first _updateGraphicsFeatures()

The first call to nsIGfxInfo.getFeatures() is in _getSystem() => _getGFXData(), so I assumed it's okay to add observer for COMPOSITOR_CREATED_TOPIC at the same time with others?

Comment 7

[Georg Fritzsche [:gfritzsche]]

(In reply to Hector Zhao [:hectorz] from comment #6)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #4)
> > 
> > That is a good point.
> > I think we should actually:
> > * move the _addObservers() call up to where you triggered your call
> > * except the COMPOSITOR_CREATED_TOPIC observer, which should be added after
> > that first _updateGraphicsFeatures()
> 
> The first call to nsIGfxInfo.getFeatures() is in _getSystem() =>
> _getGFXData(), so I assumed it's okay to add observer for
> COMPOSITOR_CREATED_TOPIC at the same time with others?

Ah, yes, thanks. This looks redundant with other calls, i'll file a separate bug about this.

Comment 8

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8691833 [details]
MozReview Request: Bug 1227428 - Update partner section with correct information once available. r?gfritzsche

https://reviewboard.mozilla.org/r/26193/#review23565

Thanks, this looks good, just a few smaller comments.

::: toolkit/components/telemetry/TelemetryEnvironment.jsm:683
(Diff revision 1)
> +  this._addObservers();

Nit: Logically this should be below `_updateSearchEngine()` - my bad.

::: toolkit/components/telemetry/docs/environment.rst:306
(Diff revision 1)
> +If the user is using a partner repack, this contains information identifying the repack being used, otherwise the entries will be empty/null. The information may be missing when the profile just becomes available. In Firefox for desktop, the information along with other customizations defined in distribution.ini are processed later in the startup phase, and will be fully applied when "distribution-customization-complete" notification is sent.

"empty/null" is a bit vague - they are all `null` presumably, except the `partnerNames` array, which is empty.

::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js:308
(Diff revision 1)
> -  const EXPECTED_FIELDS = {
> +  const EXPECTED_FIELDS = isInitial ? {
> +    distributionId: null,

I think this is hard to read - can we instead just do this in the `Assert.strictEqual` check below?
I.e.:
```
let expected = isInitial ? null : EXPECTED_FIELDS[f];
Asssert.strictEqual(..., expected, ...
```

::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js:625
(Diff revision 1)
> -function checkEnvironmentData(data) {
> +function checkEnvironmentData(data, isInitial=false) {

Nit: spacing: `isInitial = false`

::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js:682
(Diff revision 1)
> +  let data = TelemetryEnvironment.currentEnvironment;

Nit: why two separate variables and not just `environmentData = TelemetryEnvironment.currentEnvironment`?

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Let's try to verify that we're getting the correct data once a patch lands on m-c, and then uplift to beta. 
The beta 7 build will be tomorrow (thursday), beta 8 build will be next Tuesday.

Comment 10

[Hector Zhao [:hectorz]]

Comment on attachment 8691833 [details]
MozReview Request: Bug 1227428 - Update partner section with correct information once available. r?gfritzsche

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

Comment 11

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8691833 [details]
MozReview Request: Bug 1227428 - Update partner section with correct information once available. r?gfritzsche

https://reviewboard.mozilla.org/r/26193/#review23717

Thanks!
Lets get this landed soon so we can get this verified and consider uplifts :)

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/59129a59a1a2

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/59129a59a1a2

Comment 14

[Georg Fritzsche [:gfritzsche]]

Alexandra, can you verify this?

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Georg, does this need to be uplifted to Aurora44?

Comment 16

[Georg Fritzsche [:gfritzsche]]

This should be uplifted up to beta 43, but didn't have verification yet.
Do we have other QA that are available for this now?

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Florin, re-directing comment 16 to you. We need to verify the fix and then uplift to Beta43 (soon).

Comment 18

[Liz Henry (:lizzard) (relman/hg->git project)]

We already started the build for 43 beta 8, but this could make it to the beta 9 build Thursday morning.

Comment 19

[Ada [:adalucinet]]

Reproduced on Nightly from 2015-11-22 with this https://goo.gl/L2BkKv distribution.ini file.
Verified fixed with latest Nightly 45.0a1 (2015-12-01), across platforms [1] - partner values are displayed via about:telemetry page.

[1] Windows 8 32-bit, Ubuntu 14.04 32-bit and Mac OS X 10.11

Comment 20

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8691833 [details]
MozReview Request: Bug 1227428 - Update partner section with correct information once available. r?gfritzsche

Approval Request Comment
[Feature/regressing bug #]: Unified Telemetry
[User impact if declined]: No partner information in Telemetry and hence no funnelcake info, impacts growth team.
[Describe test coverage new/current, TreeHerder]: Automated tests, QA verification.
[Risks and why]: Low-risk, any effects should be limited to the partner info.
[String/UUID change made/needed]: None.

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8691833 [details]
MozReview Request: Bug 1227428 - Update partner section with correct information once available. r?gfritzsche

This fix has been verified by QE on Nighly and the patch has been in m-c for almost a week. Seems safe to uplift to Aurora44.

Comment 22

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8691833 [details]
MozReview Request: Bug 1227428 - Update partner section with correct information once available. r?gfritzsche

Please uplift to beta as well, this was verified on nightly and we need funnelcake to work.

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0fec17c6d310

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/2ae27780d1b5

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/0fec17c6d310

Comment 26

[Andrei Vaida [:avaida]]

Setting [qe-verify+] for a quick check on 43b9 as well.

Comment 27

[Ada [:adalucinet]]

Confirming this fix with 43.0b9 build 2 too (Build ID: 20151203163240), across platforms [1].

[1] Windows 7 64-bit, Mac OS X 10.10.4 and Ubuntu 12.04 32-bit