Closed
Bug 1400870
Opened 7 years ago
Closed 7 years ago
Investigate the missing attribution code in main pings
Categories
(Toolkit :: Telemetry, defect, P1)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla58
People
(Reporter: Dexter, Assigned: Dexter)
References
Details
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
chutten
:
review+
Sylvestre
:
approval-mozilla-beta+
|
Details |
(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...).
Assignee | ||
Updated•7 years ago
|
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•7 years ago
|
||
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
Flags: needinfo?(mhowell)
Assignee | ||
Comment 3•7 years ago
|
||
Hey Chris, does this fix need to ride 57?
Flags: needinfo?(chrismore.bugzilla)
Assignee | ||
Comment 4•7 years ago
|
||
(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 hidden (mozreview-request) |
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8910636 [details]
Bug 1400870 - Keep the attribution code on environment changes.
https://reviewboard.mozilla.org/r/182074/#review187522
Attachment #8910636 -
Flags: review?(chutten) → review+
Assignee | ||
Comment 7•7 years ago
|
||
Attempting to land this on 57 (or eventually requesting the uplift), since this is a regression/bug in the data.
Flags: needinfo?(chrismore.bugzilla)
Pushed by alessio.placitelli@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/76d2a93a4549
Keep the attribution code on environment changes. r=chutten
Comment 9•7 years ago
|
||
(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.
Flags: needinfo?(mhowell)
Comment 10•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Assignee | ||
Comment 11•7 years ago
|
||
Hey Matt,
would you kindly verify this fixes the problem on your machine once it hits Nightly?
Flags: needinfo?(mhowell)
Comment 12•7 years ago
|
||
Will do. Leaving ni? set to remind me.
Comment 13•7 years ago
|
||
The Windows nightly with this patch just became available, and I have verified that I can no longer reproduce the problem with it. \o/
Flags: needinfo?(mhowell)
Assignee | ||
Comment 14•7 years ago
|
||
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
Attachment #8910636 -
Flags: approval-mozilla-beta?
Comment 15•7 years ago
|
||
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
Attachment #8910636 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Assignee | ||
Comment 16•7 years ago
|
||
(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•7 years ago
|
||
bugherder uplift |
Flags: in-testsuite+
Comment 18•7 years ago
|
||
(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•7 years ago
|
||
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•7 years ago
|
||
(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.
Flags: qe-verify-
Assignee | ||
Comment 21•7 years ago
|
||
(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)?
Flags: needinfo?(lhenry)
Assignee | ||
Comment 22•7 years ago
|
||
(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!
Flags: needinfo?(chrismore.bugzilla)
Comment 23•7 years ago
|
||
(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.
Flags: needinfo?(chrismore.bugzilla) → needinfo?(rhelmer)
Comment 24•7 years ago
|
||
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.
Updated•7 years ago
|
Updated•7 years ago
|
Flags: needinfo?(rhelmer)
Comment 25•7 years ago
|
||
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?
Flags: needinfo?(mhowell)
Flags: needinfo?(alessio.placitelli)
Assignee | ||
Comment 26•7 years ago
|
||
(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.
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(mhowell)
Flags: needinfo?(alessio.placitelli)
You need to log in
before you can comment on or make changes to this bug.
Description
•