Closed Bug 1260758 Opened 8 years ago Closed 8 years ago

Add "distribution" field to core ping

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set
normal

Tracking

(firefox46+ fixed, firefox47+ fixed, firefox48+ fixed, fennec46+)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox46 + fixed
firefox47 + fixed
firefox48 + fixed
fennec 46+ ---

People

(Reporter: mkaply, Assigned: mkaply)

References

Details

Attachments

(1 file, 3 obsolete files)

We need the distribution ID (if specified) in the core ping for partner tracking.
It would be good if we start from the needs behind this data request before we decide about implementation specifics:
What kind of questions are you trying to answer with this?
What other data do you need to correlate this to?
What dimensions does this need to be broken down over? (OS version, device, ...)

In general we are trying to keep a high bar for future "core" ping additions to limit ping size growth.
Flags: needinfo?(mozilla)
We need this so we can track partner activations/installs.

Carter is probably a better person to give specific details.
Flags: needinfo?(mozilla) → needinfo?(ctrout)
Distribution ID is pretty core to everything we do tracking marketing campaigns as well as with partners distributing Firefox (preload or otherwise). Google Play installs tied to a campaign ID use this value for longitudinal tracking, and we also use this for Funnelcake on desktop, though we don't have the same options on Google Play.

Distribution partners are and will be critical to future growth of the Fennec user base, and we need to be able to clearly identify which partners are driving effective distribution.  I'm not sure we can define or limit the scope of how we'll want to compare these channels, as our understanding of effective distribution will continue to evolve as we focus more on usage hours and other measures of product engagement.  I'd argue anything worth collecting is something we'll want to be able to split along distribution lines (e.g. to determine if the users we're gaining from a channel are engaged/viable users or not).
Flags: needinfo?(ctrout)
Thanks for the background.
Talking to Mark earlier, the question came up on whether you'd also require the distribution version for future analysis.
Possibly, yes. We don't use versioning heavily at present, but that may change.  mkaply, thoughts on how we should manage these going forward?
(In reply to Mike Connor [:mconnor] from comment #5)
> Possibly, yes. We don't use versioning heavily at present, but that may
> change.  mkaply, thoughts on how we should manage these going forward?

I can't picture a scenario where the version has a ton of meaning, especially since distribution changes are tied to a particular Firefox version, so the distribution version doesn't really standalone.
FYI: We recently removed getDescriptor() from the Distribution class (bug 1256236):
https://hg.mozilla.org/mozilla-central/rev/76967a6cba22
(We might want to re-add it to extract the distribution id)
Attached patch Patch that doesn't work (obsolete) — Splinter Review
I think I understand the telemetry part.

At this point, trying to figure out how to get the data we need from Distribution.java.

DistributionDescriptor is a static class, so I don't know how to call getDesc on mDistribution and get something useful.

I don't understand why a public function is implemented that can't technically be used outside of the function...
[Tracking Requested - why for this release]: This is required for an impending distribution deal. Last minute, I know, but very important.
Assignee: nobody → mozilla
tracking-fennec: --- → 46+
Attached patch Add distributionID to core ping (obsolete) — Splinter Review
OK, here's my first pass at this (and it's working in my testing).

I went with using the browser intent to pass the distributionID along.

I do have the distribution ready stuff as well so that works.

I did find in my testing that the distribution ready stuff happens BEFORE the onStart.

That doesn't mean we don't need it though.
Attachment #8740604 - Flags: review?(michael.l.comella)
Comment on attachment 8740604 [details] [diff] [review]
Add distributionID to core ping

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

This doesn't apply for me locally. From some of your comments, you mentioned

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
@@ +717,5 @@
> +
> +            @Override
> +            public void distributionFound(Distribution distribution) {
> +                final GeckoProfile profile = getProfile();
> +                uploadTelemetry(profile);

Richard suggested setting this in SharedPrefs as a key-value store – I like that idea.

To elaborate, we add the distribution callbacks here, in onCreate (though I'd prefer to move the declaration outside of onCreate because it's already so bloated, i.e. don't use an anonymous class). The callbacks will get the descriptor and save it to SharedPrefs. When we call into uploadTelemetry, we can access the value from SharedPrefs.

In theory, the distribution callbacks are not guaranteed to be called by the time we upload so we may not have the ID on first run – is that okay? The telemetry documentation says this field is optional.

In practice, uploadTelemetry is called from getEngine, which is actually the result of distribution callbacks. If we add the SharedPrefs callback first, the callbacks run off a queue so we should have the distribution ID for the first run.

If it's not okay to miss first run, I'd rather put the UploadTelemetryCallback inside a distribution ready callback (or vice versa). Ideally, we wouldn't run them synchronously but that sounds like a messy bit of code.

Unrelated: we shouldn't be running telemetry upload twice: once from onCreate (and then from onStart).

@@ +727,5 @@
> +                final GeckoProfile profile = getProfile();
> +                uploadTelemetry(profile);
> +            }
> +        });
> +        

nit: excess whitespace

::: mobile/android/base/java/org/mozilla/gecko/telemetry/TelemetryConstants.java
@@ +41,5 @@
>          public static final String OS_VERSION = "osversion";
>          public static final String PROFILE_CREATION_DATE = "profileDate";
>          public static final String SEQ = "seq";
>          public static final String VERSION_ATTR = "v";
> +        public static final String DISTRIBUTION_ID = "distributionId";

For future reference, these are alphabetized. But I'll fix it when I land the builder.

::: mobile/android/base/java/org/mozilla/gecko/telemetry/TelemetryPingGenerator.java
@@ +96,5 @@
>          if (profileCreationDate >= 0) {
>              ping.put(CorePing.PROFILE_CREATION_DATE, profileCreationDate);
>          }
> +        if (distributionId != null) {
> +          ping.put(CorePing.DISTRIBUTION_ID, distributionId);

We should update the telemetry docs (another bug to avoid silly uplift errors? it only needs to be on central).

Can you file? fwiw, the docs are here: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/docs/core-ping.rst
Attachment #8740604 - Flags: review?(michael.l.comella) → feedback+
Attached patch Start of a new patch (obsolete) — Splinter Review
Before I finish this patch up, please take a look and let me know what you think.

A few things:

1. Haven't resolved the anonymous function thing yet.
2. I ended up still having to pass the distributionId in a couple places because we can't access shared preferences in TelemetryPingGenerator.
3. There's some debug stuff in here.
Attachment #8740458 - Attachment is obsolete: true
Attachment #8740604 - Attachment is obsolete: true
Attachment #8740968 - Flags: feedback?(michael.l.comella)
Tracking as this is something the Firefox for Android team wants to ship in 46 (from conversation with margaret)
We discussed doing a beta 11 build and release for this.  But let's stick with aiming this at the beta 12 build on Monday.   Once this lands in m-c and then in beta, we could test the builds from treeherder. Michael and Mike, can you test from there and is there any particular help you might need from QE?
Comment on attachment 8740968 [details] [diff] [review]
Start of a new patch

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

Looking on the right track. Notes:
 * I don't know the distribution file logic. I reviewed it to show it would not crash but I can't guarantee it will pick up the distribution. Is there something I should look at to confirm this is the correct logic? I don't see a preferences.json file locally.
 * Emphasis on the anonymous class thing

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
@@ +690,5 @@
>              "Updater:Launch");
>  
>          // We want to upload the telemetry core ping as soon after startup as possible. It relies on the
>          // Distribution being initialized. If you move this initialization, ensure it plays well with telemetry.
> +        mDistribution = Distribution.init(this);

This no longer needs to be a member variable.

@@ +702,5 @@
> +                final GeckoProfile profile = getProfile();
> +                final SharedPreferences sharedPrefs = GeckoSharedPrefs.forProfileName(appContext, profile.getName());
> +                DistributionDescriptor desc = distribution.getDescriptor();
> +                if (desc != null) {
> +                  sharedPrefs.edit().putString("distributionId", desc.id).commit();

Can you make the shared preference name a constant? e.g. PREF_DISTRIBUTION_ID. Maybe in the Distribution class.

Also, namespace the value, maybe "distribution.id".

For example: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/db/SuggestedSites.java#74

@@ +708,5 @@
> +            }
> +
> +            @Override
> +            public void distributionArrivedLate(Distribution distribution) {
> +                final GeckoProfile profile = getProfile();

Can we move this to a helper function so we don't have to duplicate it?

@@ +709,5 @@
> +
> +            @Override
> +            public void distributionArrivedLate(Distribution distribution) {
> +                final GeckoProfile profile = getProfile();
> +                final SharedPreferences sharedPrefs = GeckoSharedPrefs.forProfileName(appContext, profile.getName());

I don't think this has to be per profile but considering:
 1) we don't support alternative profiles, in the practical sense
 2) it keeps the upload service code simpler

I'm cool with it.

@@ +712,5 @@
> +                final GeckoProfile profile = getProfile();
> +                final SharedPreferences sharedPrefs = GeckoSharedPrefs.forProfileName(appContext, profile.getName());
> +                DistributionDescriptor desc = distribution.getDescriptor();
> +                if (desc != null) {
> +                  sharedPrefs.edit().putString("distributionId", desc.id).commit();

nit: Add a comment explaining why this is synchronous and not async (via `apply()`).

@@ +716,5 @@
> +                  sharedPrefs.edit().putString("distributionId", desc.id).commit();
> +                }
> +            }
> +        });
> +        

nit: this whitespace

::: mobile/android/base/java/org/mozilla/gecko/distribution/Distribution.java
@@ +352,5 @@
>          return descFile;
>      }
>  
> +    public DistributionDescriptor getDescriptor() {
> +        File descFile = getDistributionFile("preferences.json");

Make "preferences.json" a constant.

nit: `final` here & below

::: mobile/android/base/java/org/mozilla/gecko/telemetry/TelemetryPingGenerator.java
@@ +72,5 @@
>          return new TelemetryPing(serverURL, payload);
>      }
>  
>      private static ExtendedJSONObject createCorePingPayload(final Context context, final String clientId,
> +            final int seq, final long profileCreationDate, @Nullable final String distributionId, 

nit: added ws

You should be able to run `./mach gradle checkstyle` to find excess whitespace.
Attachment #8740968 - Flags: feedback?(michael.l.comella) → feedback+
Comment on attachment 8740968 [details] [diff] [review]
Start of a new patch

>diff --git a/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java b/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java

>+        mDistribution = Distribution.init(this);
>+        mDistribution.addOnDistributionReadyCallback(new Distribution.ReadyCallback() {

>+            public void distributionFound(Distribution distribution) {
>+                final GeckoProfile profile = getProfile();
>+                final SharedPreferences sharedPrefs = GeckoSharedPrefs.forProfileName(appContext, profile.getName());
>+                DistributionDescriptor desc = distribution.getDescriptor();
>+                if (desc != null) {
>+                  sharedPrefs.edit().putString("distributionId", desc.id).commit();
>+                }
>+            }
>+
>+            @Override
>+            public void distributionArrivedLate(Distribution distribution) {
>+                final GeckoProfile profile = getProfile();
>+                final SharedPreferences sharedPrefs = GeckoSharedPrefs.forProfileName(appContext, profile.getName());
>+                DistributionDescriptor desc = distribution.getDescriptor();
>+                if (desc != null) {
>+                  sharedPrefs.edit().putString("distributionId", desc.id).commit();
>+                }
>+            }
>+        });


I think the Distribution is the same for all profiles, so we should probably use the App-level (GeckoSharedPrefs.forApp), like Distribution.java does:
http://mxr.mozilla.org/mozilla-release/source/mobile/android/base/java/org/mozilla/gecko/distribution/Distribution.java#941

prefsBranch is used for tests only.

Distribution.java saves something in shared prefs too and uses the following as a way to generate a keyname:
http://mxr.mozilla.org/mozilla-release/source/mobile/android/base/java/org/mozilla/gecko/distribution/Distribution.java#938

Which could mena we'd use: context.getPackageName() + ".distribution_id"

In fact, could we just save this in Distribution.java ? Maybe that creates too much work for now. Don't block on it.


>diff --git a/mobile/android/base/java/org/mozilla/gecko/telemetry/TelemetryConstants.java b/mobile/android/base/java/org/mozilla/gecko/telemetry/TelemetryConstants.java

>     // Change these two values to enable upload in developer builds.
>-    public static final boolean UPLOAD_ENABLED = AppConstants.MOZILLA_OFFICIAL; // Disabled for developer builds.
>-    public static final String DEFAULT_SERVER_URL = "https://incoming.telemetry.mozilla.org";
>+    public static final boolean UPLOAD_ENABLED = true;
>+    public static final String DEFAULT_SERVER_URL = "http://10.0.0.15:8080";

Remember to undo these

>diff --git a/mobile/android/base/java/org/mozilla/gecko/telemetry/TelemetryPingGenerator.java b/mobile/android/base/java/org/mozilla/gecko/telemetry/TelemetryPingGenerator.java

>         // Each profile can have different telemetry data so we intentionally grab the shared prefs for the profile.
>         final SharedPreferences sharedPrefs = GeckoSharedPrefs.forProfileName(this, profileName);
>         // TODO (bug 1241685): Sync this preference with the gecko preference.
>         final String serverURLSchemeHostPort =
>                 sharedPrefs.getString(TelemetryConstants.PREF_SERVER_URL, TelemetryConstants.DEFAULT_SERVER_URL);
>-
>+        final String distributionId = sharedPrefs.getString("distributionId", null);

Storing in GeckoSharedPrefs.forApp would obviously require a change here. I suppose I could live with GeckoSharedPrefs.forProfileName to keep things simple.
When I was looking at this, I wondered if distribution should simply store its own stuff in prefs during init. I honestly don't know what the right thing here is.
Assignee: mozilla → michael.l.comella
https://reviewboard.mozilla.org/r/46193/#review42687

::: mobile/android/base/java/org/mozilla/gecko/distribution/DistributionIDStoreCallback.java:19
(Diff revision 1)
> +import java.lang.ref.WeakReference;
> +
> +/**
> + * A distribution ready callback that will store the distribution ID to profile-specific shared preferences.
> + */
> +public class DistributionIDStoreCallback implements Distribution.ReadyCallback {

Possible rename: DistributionStoreCallback

We save ID for now, but Version could happen too. Better to be a little general?
(In reply to Mike Kaply [:mkaply] from comment #17)
> When I was looking at this, I wondered if distribution should simply store
> its own stuff in prefs during init. I honestly don't know what the right
> thing here is.

I think this is possible but it'd couple the preference to the init process. It seems all the other listeners react to the distribution ready callbacks.

I think it really depends on if we consider this information directly part of the distribution or not.

But let's not block on this. :)
Comment on attachment 8741095 [details]
MozReview Request: Bug 1260758 - Add distribution ID field to core ping. r=margaret,mkaply

https://reviewboard.mozilla.org/r/46193/#review42691

Agree with mfinkle on the naming. Otherwsise looks good to me. Ship It.
Attachment #8741095 - Flags: review?(mozilla) → review+
https://hg.mozilla.org/integration/fx-team/rev/50704924c908ae4ec2b5770805df0c8bc9562010
Bug 1260758 - Add distribution field to Fennec core ping; r=mfinkle,mkaply
https://hg.mozilla.org/integration/fx-team/rev/17fd65ac53326996071a16fa2f72b364df2f3fdb
Bug 1260758 - Add distribution field to Fennec core ping - forgot to add file; r=mfinkle,mkaply
Attachment #8741095 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 8741095 [details]
MozReview Request: Bug 1260758 - Add distribution ID field to core ping. r=margaret,mkaply

https://reviewboard.mozilla.org/r/46193/#review42711

Looks pretty straightforward to me. Thanks for the quick turnaround, team!

::: mobile/android/base/java/org/mozilla/gecko/distribution/DistributionIDStoreCallback.java:47
(Diff revision 1)
> +    }
> +
> +    private void storeDistribution(final Distribution distribution) {
> +        final Context context = contextReference.get();
> +        if (context == null) {
> +            Log.w(LOGTAG, "Context is no longer alive, could retrieve shared prefs to store distribution");

Nit: I think you meant "...couldn't retreieve... "
Handing this off to Mike to be the go-to for this bug.
Assignee: michael.l.comella → mozilla
Comment on attachment 8741095 [details]
MozReview Request: Bug 1260758 - Add distribution ID field to core ping. r=margaret,mkaply

Approval Request Comment
[Feature/regressing bug #]: Add distribution ID to allow for partner tracking
[User impact if declined]: Needed for partner work scheduled for FF46
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: Low. Adds something to the existing core ping. No change for non distribution builds.
[String/UUID change made/needed]:

Side note: New patches will be needed for Aurora/Beta
Attachment #8741095 - Flags: approval-mozilla-beta?
Attachment #8741095 - Flags: approval-mozilla-aurora?
Please don't uplift the change from comment 27 (in retrospect, perhaps I should have done this in separate bug).
Comment on attachment 8741095 [details]
MozReview Request: Bug 1260758 - Add distribution ID field to core ping. r=margaret,mkaply

Please uplift to aurora and beta, needed for partner work.
Attachment #8741095 - Flags: approval-mozilla-beta?
Attachment #8741095 - Flags: approval-mozilla-beta+
Attachment #8741095 - Flags: approval-mozilla-aurora?
Attachment #8741095 - Flags: approval-mozilla-aurora+
(In reply to Mike Kaply [:mkaply] from comment #26)
> Side note: New patches will be needed for Aurora/Beta

These don't apply cleanly to aurora. Are there new patches coming?
Flags: needinfo?(mozilla)
Flags: needinfo?(michael.l.comella)
Mike said he's got this.
Flags: needinfo?(michael.l.comella)
FYI, this might apply to Aurora properly if we uplift its deps first (starting at bug 1249288).

Mike is working on a branch patch for Beta atm.
Do we need work for all these dependencies? From https://bugzilla.mozilla.org/showdependencytree.cgi?id=1249288&hide_resolved=1 ? That doesn't sound realistic for 46.
Flags: needinfo?(michael.l.comella)
Flags: needinfo?(mconnor)
No, we don't need to resolve these dependencies for for 46, this will stand alone.
Flags: needinfo?(mozilla)
Flags: needinfo?(michael.l.comella)
Flags: needinfo?(mconnor)
I've checked in the beta patch. Aurora is going to wait for the dependency uplift.
(In reply to Mike Kaply [:mkaply] from comment #36)
> I've checked in the beta patch. Aurora is going to wait for the dependency
> uplift.

Mike, what is the dependency uplift? I'm wondering if we could get moving on the 47 patch.
Flags: needinfo?(mozilla)
> I've checked in the beta patch. Aurora is going to wait for the dependency
> uplift.

I think we were waiting for the searchEngineDefault change to go in to Aurora, right? Then I could merge the distibutionID?
Flags: needinfo?(mozilla)
I'll put together an Aurora patch ASAP.
(In reply to Mike Kaply [:mkaply] from comment #39)
> I think we were waiting for the searchEngineDefault change to go in to
> Aurora, right? Then I could merge the distibutionID?

Yep – bug 1249288 comment 92! Sorry, I should have notified you that those had landed.
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.