Partner information missing from Telemetry Environment

VERIFIED FIXED in Firefox 43, Firefox OS v2.5

Status

()

Toolkit
Telemetry
P3
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: hectorz, Assigned: hectorz)

Tracking

Trunk
mozilla45
Points:
---

Firefox Tracking Flags

(firefox43+ verified, firefox44+ fixed, firefox45+ verified, b2g-v2.5 fixed)

Details

(Whiteboard: [measurement:client] )

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
Created attachment 8691264 [details] [diff] [review]
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.
Attachment #8691264 - Flags: feedback?(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
Attachment #8691264 - Flags: feedback?(gfritzsche) → feedback+
Status: NEW → ASSIGNED
Priority: -- → P3
Whiteboard: [measurement:client]
Component: Client: Desktop → Telemetry
Product: Firefox Health Report → Toolkit
(Assignee)

Comment 2

2 years ago
(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

2 years ago
[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.
status-firefox43: --- → affected
status-firefox44: --- → affected
tracking-firefox43: --- → ?
tracking-firefox44: --- → ?
tracking-firefox45: --- → ?
Priority: P3 → P1
(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()
Priority: P1 → P3
Whiteboard: [measurement:client] → [measurement:client] [measurement:client:tracking]
(Assignee)

Comment 5

2 years ago
Created attachment 8691833 [details]
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
Attachment #8691833 - Flags: review?(gfritzsche)
(Assignee)

Comment 6

2 years ago
(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?
(Assignee)

Updated

2 years ago
Attachment #8691264 - Attachment is obsolete: true
(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.
Attachment #8691833 - Flags: review?(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`?
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.
tracking-firefox43: ? → +
tracking-firefox44: ? → +
tracking-firefox45: ? → +
(Assignee)

Comment 10

2 years ago
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/
Attachment #8691833 - Flags: review?(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 :)
Attachment #8691833 - Flags: review?(gfritzsche) → review+

Comment 13

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/59129a59a1a2
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox45: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Alexandra, can you verify this?
Flags: needinfo?(alexandra.lucinet)
Georg, does this need to be uplifted to Aurora44?
Flags: needinfo?(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?
Flags: needinfo?(gfritzsche)
Florin, re-directing comment 16 to you. We need to verify the fix and then uplift to Beta43 (soon).
Flags: needinfo?(florin.mezei)
We already started the build for 43 beta 8, but this could make it to the beta 9 build Thursday morning.

Comment 19

2 years ago
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
Status: RESOLVED → VERIFIED
status-firefox45: fixed → verified
Flags: needinfo?(florin.mezei)
Flags: needinfo?(alexandra.lucinet)
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.
Attachment #8691833 - Flags: approval-mozilla-beta?
Attachment #8691833 - Flags: approval-mozilla-aurora?
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.
Attachment #8691833 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Attachment #8691833 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 23

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/0fec17c6d310
status-firefox44: affected → fixed

Comment 24

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/2ae27780d1b5
status-firefox43: affected → fixed
Setting [qe-verify+] for a quick check on 43b9 as well.
Flags: qe-verify+
QA Contact: alexandra.lucinet

Comment 27

2 years ago
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
status-firefox43: fixed → verified
Flags: qe-verify+
Whiteboard: [measurement:client] [measurement:client:tracking] → [measurement:client]
You need to log in before you can comment on or make changes to this bug.