(telemetry v2) Add A/B experiment data to java telemetry uploads

RESOLVED FIXED in Firefox 45

Status

()

Firefox for Android
General
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mcomella, Assigned: mcomella)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 47
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed, firefox46 fixed, firefox47 fixed, fennec45+)

Details

MozReview Requests

()

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

Attachments

(2 attachments)

Comment hidden (empty)

Comment 1

2 years ago
I'm not sure how you plan to do this, but I want us to make sure we're aware of the fact that onboarding uses "local" experiments, not ones controlled by the switchboard server. This was a point of confusion in bug 1241591.
(In reply to :Margaret Leibovic from comment #1)
> I'm not sure how you plan to do this, but I want us to make sure we're aware
> of the fact that onboarding uses "local" experiments, not ones controlled by
> the switchboard server. This was a point of confusion in bug 1241591.

The SwitchBoard server should have the same experiment setup as the local config. Since the local and server code both segment users that same way, we don't need to worry about local vs server.
(In reply to Mark Finkle (:mfinkle) from bug 1205835 comment #22)
> https://reviewboard.mozilla.org/r/30215/#review27415
> 
> ::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:966
> (Diff revision 2)
> > +        // TODO: Remove me.
> 
> Just an FYI, maybe for the futrue or maybe for now. SwitchBoard will fire a
> local broadcast, letting any code know when the configuration has been
> fetched from the network. At the point, we know we have the list of active
> experiments:
> 
> fired:
http://mxr.mozilla.org/mozilla-aurora/source/mobile/android/thirdparty/com/keepsafe/switchboard/SwitchBoard.java#239
> 
> example catching:
http://mxr.mozilla.org/mozilla-aurora/source/mobile/android/base/java/org/mozilla/gecko/health/BrowserHealthRecorder.java#538
http://mxr.mozilla.org/mozilla-aurora/source/mobile/android/base/java/org/mozilla/gecko/health/BrowserHealthRecorder.java#734
Created attachment 8712919 [details]
MozReview Request: Bug 1241697 - Add docs for 'experiments' field in core ping. r=mfinkle

Review commit: https://reviewboard.mozilla.org/r/32699/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32699/
Attachment #8712919 - Flags: review?(mark.finkle)
Created attachment 8712920 [details]
MozReview Request: Bug 1241697 - Upload experiments with core telemetry ping. r=mfinkle

We generate the experiments in the TelemetryPingGenerator, rather than passing
them into the Intent, meaning that if the UploadSerivce gets killed and the
Intent gets redelivered, the experiments will be re-generated and there is a
chance the uploaded document won't be identical to the one that would have been
uploaded (or was uploaded, if the service is killed after the upload occurs).
The way to fix this is to pass the experiments into the Intent so it gets
redelivered identically.

However, it's not worth making the documents identical for the moment. It will
be fixed when we implement the functionality to reupload already-generated
documents which we believe to have failed to upload (which is necessary for us
to specify "counts since last ping").

Review commit: https://reviewboard.mozilla.org/r/32701/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32701/
Attachment #8712920 - Flags: review?(mark.finkle)
Oh, I forgot to add an upload when the experiment data changes, working on it...
Assignee: nobody → michael.l.comella
Attachment #8712919 - Flags: review?(mark.finkle) → review+
Comment on attachment 8712919 [details]
MozReview Request: Bug 1241697 - Add docs for 'experiments' field in core ping. r=mfinkle

https://reviewboard.mozilla.org/r/32699/#review29485

LGTM
Attachment #8712920 - Flags: review?(mark.finkle) → review+
Comment on attachment 8712920 [details]
MozReview Request: Bug 1241697 - Upload experiments with core telemetry ping. r=mfinkle

https://reviewboard.mozilla.org/r/32701/#review29487

Oh Yeah
Comment on attachment 8712920 [details]
MozReview Request: Bug 1241697 - Upload experiments with core telemetry ping. r=mfinkle

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32701/diff/1-2/
Added some MOZ_SWITCHBOARD guard flags that probably don't need reviewing. Still working on comment 6.
This patch will probably not get the experiment data on the ping sent from the first run but hey, the first ping didn't have experiment data without this patch anyway so this is an improvement and I decided to land it anyway.

It might not be worth fixing this issue at all because we probably want to get a ping out ASAP when the application is first run and don't want to wait on a response from the Switchboard servers before we try to upload. Then again, do we care about a user who manages to install Firefox and closes it (and never reopens it) before they get a Switchboard response? But then again, what if we never get a Switchboard response?

In any case, I don't think it's worth investing time to fix this right now because the upload flow will change with bug 1243585.
Blocks bug 1241599 too – we added some documentation.
Depends on: 1241599
Summary: Add A/B experiment data to java telemetry uploads → (telemetry v2) Add A/B experiment data to java telemetry uploads

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d1845bc6fa7d
https://hg.mozilla.org/mozilla-central/rev/77c38dd031d4
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47

Updated

2 years ago
Blocks: 1246130
We should try to land this with the initial telemetry ping (bug 1205835).
tracking-fennec: --- → 45+
Comment on attachment 8712919 [details]
MozReview Request: Bug 1241697 - Add docs for 'experiments' field in core ping. r=mfinkle

Suggested uplift with bug 1205835. It'd be easier to uplift bug 1241599 first because this patch updates documentation that that bug had added.

Approval Request Comment
[Feature/regressing bug #]: bug 1205835
[User impact if declined]: We don't get A/B testing data about users and will have to wait another cycle before we can do effective A/B testing.

[Describe test coverage new/current, TreeHerder]: Tested locally.
[Risks and why]: Low – we used some pre-existing methods to add information to the telemetry pings which was implemented in bug 1205835.

[String/UUID change made/needed]: None
Attachment #8712919 - Flags: approval-mozilla-beta?
Attachment #8712919 - Flags: approval-mozilla-aurora?
Attachment #8712920 - Flags: approval-mozilla-beta?
Attachment #8712920 - Flags: approval-mozilla-aurora?
status-firefox45: --- → affected
status-firefox46: --- → affected
This (and probably a bunch of the related uplift requests) needs to be rebased to work around the lack of bug 1107811 on the release branches.
Flags: needinfo?(michael.l.comella)

Updated

2 years ago
Flags: needinfo?(michael.l.comella)
Comment on attachment 8712919 [details]
MozReview Request: Bug 1241697 - Add docs for 'experiments' field in core ping. r=mfinkle

michael had my approval. Updating the uplift flags for posterity.
Should be in 45 beta 4
Attachment #8712919 - Flags: approval-mozilla-beta?
Attachment #8712919 - Flags: approval-mozilla-beta+
Attachment #8712919 - Flags: approval-mozilla-aurora?
Attachment #8712919 - Flags: approval-mozilla-aurora+
Attachment #8712920 - Flags: approval-mozilla-beta?
Attachment #8712920 - Flags: approval-mozilla-beta+
Attachment #8712920 - Flags: approval-mozilla-aurora?
Attachment #8712920 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.