1400870 - Investigate the missing attribution code in main pings

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Alessio Placitelli [:Dexter]]

(In reply to Matt Howell [:mhowell] from bug 1292360 comment #49)
> Oh, I wasn't aware of that archived pings feature, that's really neat. Using
> that I do see the issue; as soon as I can get about:telemetry open in a new
> profile (before there are any archived pings), the current ping view shows
> the attribution data, but once the first batch of archived pings appears
> (which I'm forcing by restarting the browser), they are missing the
> attribution object.
> 
> I am at a complete loss and do not understand how this is possible even in
> theory. I don't see anything being done differently from, e.g., the profile
> creation date, and that's always there. I don't think I'm the right person
> to be investigating this (but then I also didn't think I was the right
> person to write this code in the first place...).

Comment 1

[Alessio Placitelli [:Dexter]]

Attachment: Bug 1400870 - Keep the attribution code on environment changes.

This patch makes sure |_updateSettings| carries over the attribution code,
if present. Without this patch the whole "environment.settings" section
is replaced and the attribution code disppears after the first environment
change/subsession split.

Review commit: https://reviewboard.mozilla.org/r/182074/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/182074/

Comment 2

[Alessio Placitelli [:Dexter]]

Here's what was happening: as soon as the FF was starting, the attribution code was correctly getting loaded and added to the environment [1]; however, whenever a watched preference was changed, the whole "environment.settings" section was getting replaced with a new one [2].

This is the reason why we were seeing the attribution code in the initial ping but not in the following pings.

I've slightly changed the test to make sure that the attribution code is required in every environment, after some change happen, if present.

@matt, is there any reason why the spoofing was windows only in the test at [3]?

[1] - http://searchfox.org/mozilla-central/rev/2ef8bd8a46a02c68ddbb1d5f25fa254dd7be1fbd/toolkit/components/telemetry/TelemetryEnvironment.jsm#870
[2] - http://searchfox.org/mozilla-central/rev/2ef8bd8a46a02c68ddbb1d5f25fa254dd7be1fbd/toolkit/components/telemetry/TelemetryEnvironment.jsm#1366
[3] - http://searchfox.org/mozilla-central/rev/d08b24e613cac8c9c5a4131452459241010701e0/toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js#321

Comment 3

[Alessio Placitelli [:Dexter]]

Hey Chris, does this fix need to ride 57?

Comment 4

[Alessio Placitelli [:Dexter]]

(In reply to Alessio Placitelli [:Dexter] from comment #2)
> @matt, is there any reason why the spoofing was windows only in the test at
> [3]?

Ok, now I see that the attribution file test is disabled on Windows as well: http://searchfox.org/mozilla-central/rev/2ef8bd8a46a02c68ddbb1d5f25fa254dd7be1fbd/browser/modules/test/unit/xpcshell.ini#7

Still I'm curious about the reason :)

Comment 5

[Alessio Placitelli [:Dexter]]

Comment on attachment 8910636 [details]
Bug 1400870 - Keep the attribution code on environment changes.

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

Comment 6

[Chris H-C :chutten]

Comment on attachment 8910636 [details]
Bug 1400870 - Keep the attribution code on environment changes.

https://reviewboard.mozilla.org/r/182074/#review187522

Comment 7

[Alessio Placitelli [:Dexter]]

Attempting to land this on 57 (or eventually requesting the uplift), since this is a regression/bug in the data.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/76d2a93a4549
Keep the attribution code on environment changes. r=chutten

Comment 9

[Molly Howell (she/her) (mostly inactive)]

(In reply to Alessio Placitelli [:Dexter] from comment #2)
> Here's what was happening: as soon as the FF was starting, the attribution
> code was correctly getting loaded and added to the environment [1]; however,
> whenever a watched preference was changed, the whole "environment.settings"
> section was getting replaced with a new one [2].
> 
> This is the reason why we were seeing the attribution code in the initial
> ping but not in the following pings.
> 
> I've slightly changed the test to make sure that the attribution code is
> required in every environment, after some change happen, if present.

Oh, wow, okay. Thanks for the great work figuring that out and getting it patched!

> @matt, is there any reason why the spoofing was windows only in the test at
> [3]?

I probably wanted to avoid trying to do that testing on not-Windows, since the attribution data file won't exist on any other OS in a real installation. Also looking at it now, I'm not sure that getAttributionFile() would return something usable instead of failing spectacularly on not-Windows, since it needs to use LocalAppData and that directory doesn't exist on any other OS.

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/76d2a93a4549

Comment 11

[Alessio Placitelli [:Dexter]]

Hey Matt,

would you kindly verify this fixes the problem on your machine once it hits Nightly?

Comment 12

[Molly Howell (she/her) (mostly inactive)]

Will do. Leaving ni? set to remind me.

Comment 13

[Molly Howell (she/her) (mostly inactive)]

The Windows nightly with this patch just became available, and I have verified that I can no longer reproduce the problem with it. \o/

Comment 14

[Alessio Placitelli [:Dexter]]

Comment on attachment 8910636 [details]
Bug 1400870 - Keep the attribution code on environment changes.

Approval Request Comment
[Feature/Bug causing the regression]: This fixes a bug that was around since FF52: when a new ping was generated, the attribution code information was erased.
[User impact if declined]: No attribution data will be sent in pings other than the first one. Having this fix in will increase the quality of the data for 57.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]:  No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Low risk
[Why is the change risky/not risky?]: This change is only affecting the telemetry environment ability to report the attribution code.
[String changes made/needed]: None

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8910636 [details]
Bug 1400870 - Keep the attribution code on environment changes.

If we can improve the quality of the data of 57, taking it.
Is there a reason why we don't have a non regression test on this?
Thanks
Should be in 57b3

Comment 16

[Alessio Placitelli [:Dexter]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #15)
> Comment on attachment 8910636 [details]
> Bug 1400870 - Keep the attribution code on environment changes.
> 
> If we can improve the quality of the data of 57, taking it.
> Is there a reason why we don't have a non regression test on this?

Thanks for taking this!

I think the changes to test_TelemetryEnvironment.js are what you're looking for (non-regression test).

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7e35ed4a8c47

Comment 18

[Chris More [:cmore]]

(In reply to Alessio Placitelli [:Dexter] from comment #3)
> Hey Chris, does this fix need to ride 57?

yeah, we really need this in place for 57. If there was a dot release before fx57, that would be ideal too since we are still flying blind with our acquisition funnel. Thanks

Comment 19

[Chris More [:cmore]]

anyone here:

Just wanted to confirm something. If we are trying to do an experiment with the attribution codes during the fx56 release channel train, we would be blocked, right? We are trying to use attribution codes + shield side loaded add-ons target at specific codes as an ability to perform onboarding cohort tests without having to build unique funnelcakes. But, this would require attribution codes being consistently being passed through to Firefox where shield could read those values. 

Thanks!

Comment 20

[Andrei Vaida [:avaida]]

(In reply to Alessio Placitelli [:Dexter] from comment #14)
> [Is this code covered by automated tests?]: Yes
> [Has the fix been verified in Nightly?]: Yes
> [Needs manual test from QE? If yes, steps to reproduce]:  No

Setting qe-verify- based on Alessio's assessment on manual testing needs and the fact that this fix has automated coverage.

Comment 21

[Alessio Placitelli [:Dexter]]

(In reply to Chris More [:cmore] from comment #18)
> (In reply to Alessio Placitelli [:Dexter] from comment #3)
> > Hey Chris, does this fix need to ride 57?
> 
> yeah, we really need this in place for 57. If there was a dot release before
> fx57, that would be ideal too since we are still flying blind with our
> acquisition funnel. Thanks

Liz, I'm not sure if you're the right person to ask this to, but is there any plan for a dot release before 57 (this fix is already on 57)?

Comment 22

[Alessio Placitelli [:Dexter]]

(In reply to Chris More [:cmore] from comment #19)
> anyone here:
> 
> Just wanted to confirm something. If we are trying to do an experiment with
> the attribution codes during the fx56 release channel train, we would be
> blocked, right? We are trying to use attribution codes + shield side loaded
> add-ons target at specific codes as an ability to perform onboarding cohort
> tests without having to build unique funnelcakes. But, this would require
> attribution codes being consistently being passed through to Firefox where
> shield could read those values. 
> 
> Thanks!

Not necessarily, but I would need to know a bit more about this process to better tell.
We still have the attribution information before the first main ping is sent. This also depends on where SHIELD is reading the attribution information: if it's in the environment and the check happens after the first ping is sent, then this may be a problem. Otherwise, it's not.

Feel free to schedule a meeting with me (and some SHIELD people) to dive into the details if that helps!

Comment 23

[Chris More [:cmore]]

(In reply to Alessio Placitelli [:Dexter] from comment #22)
> (In reply to Chris More [:cmore] from comment #19)
> > anyone here:
> > 
> > Just wanted to confirm something. If we are trying to do an experiment with
> > the attribution codes during the fx56 release channel train, we would be
> > blocked, right? We are trying to use attribution codes + shield side loaded
> > add-ons target at specific codes as an ability to perform onboarding cohort
> > tests without having to build unique funnelcakes. But, this would require
> > attribution codes being consistently being passed through to Firefox where
> > shield could read those values. 
> > 
> > Thanks!
> 
> Not necessarily, but I would need to know a bit more about this process to
> better tell.
> We still have the attribution information before the first main ping is
> sent. This also depends on where SHIELD is reading the attribution
> information: if it's in the environment and the check happens after the
> first ping is sent, then this may be a problem. Otherwise, it's not.
> 
> Feel free to schedule a meeting with me (and some SHIELD people) to dive
> into the details if that helps!

:rhelmer: given our Tracking Protection experiment we are working on now and how we are using a side loaded Shield add-on to deploy the experiment upon the creation of a new profile, do you think this issue described above with attribution will impact Shield being able to deploy to only users with specific attribution fields upon first launch? I figured you may be able to determine this best since you wrote the Shield patch to be able to do this.

Comment 24

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

I'll track this but I'd like for it not to be a release blocker for 56. We can consider it as a ride along for a dot release in 56 though.

Comment 25

[Georg Fritzsche [:gfritzsche]]

Alessio, Matt, one question about the fix here:
Can we, in a follow-up bug, improve this for the TelemetryEnvironment to not manually remember the attribution code?
It seems that with the caching in AttributionCode.jsm, TelemetryEnvironment should be able to just always use AttributionCode.getCachedAttributionCode() or so?

Comment 26

[Alessio Placitelli [:Dexter]]

(In reply to Georg Fritzsche [:gfritzsche] [away Sep 19 - Oct 3] from comment #25)
> Alessio, Matt, one question about the fix here:
> Can we, in a follow-up bug, improve this for the TelemetryEnvironment to not
> manually remember the attribution code?
> It seems that with the caching in AttributionCode.jsm, TelemetryEnvironment
> should be able to just always use AttributionCode.getCachedAttributionCode()
> or so?

Filed and taken bug 1406097.