1260758 - Add "distribution" field to core ping

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mike Kaply [:mkaply]]

We need the distribution ID (if specified) in the core ping for partner tracking.

Comment 1

[Georg Fritzsche [:gfritzsche]]

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.

Comment 2

[Mike Kaply [:mkaply]]

We need this so we can track partner activations/installs.

Carter is probably a better person to give specific details.

Comment 3

[Mike Connor [:mconnor]]

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).

Comment 4

[Georg Fritzsche [:gfritzsche]]

Thanks for the background.
Talking to Mark earlier, the question came up on whether you'd also require the distribution version for future analysis.

Comment 5

[Mike Connor [:mconnor]]

Possibly, yes. We don't use versioning heavily at present, but that may change.  mkaply, thoughts on how we should manage these going forward?

Comment 6

[Mike Kaply [:mkaply]]

(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.

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

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)

Comment 8

[Mike Kaply [:mkaply]]

Attachment: Patch that doesn't work

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...

Comment 9

[:Margaret Leibovic]

[Tracking Requested - why for this release]: This is required for an impending distribution deal. Last minute, I know, but very important.

Comment 10

[Mike Kaply [:mkaply]]

Attachment: Add distributionID to core ping

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.

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 12

[Mike Kaply [:mkaply]]

Attachment: Start of a new patch

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.

Comment 13

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

Tracking as this is something the Firefox for Android team wants to ship in 46 (from conversation with margaret)

Comment 14

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

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 15

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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.

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 17

[Mike Kaply [:mkaply]]

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.

Comment 18

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1260758 - Add distribution ID field to core ping. r=margaret,mkaply

Shout-out to mkaply for doing the heavy lifting. :)

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

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

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?

Comment 20

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(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 21

[Mike Kaply [:mkaply]]

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.

Comment 22

[Mike Kaply [:mkaply]]

https://hg.mozilla.org/integration/fx-team/rev/50704924c908ae4ec2b5770805df0c8bc9562010
Bug 1260758 - Add distribution field to Fennec core ping; r=mfinkle,mkaply

Comment 23

[Mike Kaply [: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

Comment 24

[:Margaret Leibovic]

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... "

Comment 25

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Handing this off to Mike to be the go-to for this bug.

Comment 26

[Mike Kaply [:mkaply]]

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

Comment 27

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/integration/fx-team/rev/927d52d615857dbc5f8e085e288a10b3fafeab60
Bug 1260758 - Bump core ping version for distribution id. r=trivial

Comment 28

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Please don't uplift the change from comment 27 (in retrospect, perhaps I should have done this in separate bug).

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/50704924c908
https://hg.mozilla.org/mozilla-central/rev/17fd65ac5332
https://hg.mozilla.org/mozilla-central/rev/927d52d61585

Comment 30

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

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.

Comment 31

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(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?

Comment 32

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Mike said he's got this.

Comment 33

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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.

Comment 34

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

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.

Comment 35

[Mike Kaply [:mkaply]]

No, we don't need to resolve these dependencies for for 46, this will stand alone.

Comment 36

[Mike Kaply [:mkaply]]

I've checked in the beta patch. Aurora is going to wait for the dependency uplift.

Comment 37

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/3407730c74e4add5d9fb1b49882474524364afdc

Comment 38

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(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.

Comment 39

[Mike Kaply [:mkaply]]

> 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?

Comment 40

[Mike Kaply [:mkaply]]

I'll put together an Aurora patch ASAP.

Comment 41

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(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.

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0ee49a42aee5