Closed Bug 1143888 Opened 9 years ago Closed 9 years ago

Integrate the Adjust campaign tracking SDK into Firefox

Categories

(Firefox Build System :: Android Studio and Gradle Integration, defect)

All
Android
defect
Not set
normal

Tracking

(firefox38 wontfix, firefox38.0.5 fixed, firefox39 fixed, firefox40 fixed, relnote-firefox 38+, fennec38+)

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- wontfix
firefox38.0.5 --- fixed
firefox39 --- fixed
firefox40 --- fixed
relnote-firefox --- 38+
fennec 38+ ---

People

(Reporter: mfinkle, Assigned: mfinkle)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 3 obsolete files)

We need to start work to integrate the Adjust SDK into Firefox.

1. Needs a specific build flag so we can disable at any time.
2. Will likely be RELEASE only, but we'll need to test in custom builds.
3. We only send a single "ping", on the first use via the INSTALL_REFERRER
4. We pull the code into our repo in third-parties. We should always have strict control over the version of the SDK we are using.


Code can be found here:
https://github.com/adjust/android_sdk
OS: Mac OS X → Android
Hardware: x86 → All
(In reply to Mark Finkle (:mfinkle) from comment #0)
> We need to start work to integrate the Adjust SDK into Firefox.
> 
> 1. Needs a specific build flag so we can disable at any time.
> 2. Will likely be RELEASE only, but we'll need to test in custom builds.
> 3. We only send a single "ping", on the first use via the INSTALL_REFERRER
> 4. We pull the code into our repo in third-parties. We should always have
> strict control over the version of the SDK we are using.

I anticipate handling 1,2,4 shortly, although I'm happy for someone else to do the dirty and me to review.  I would prefer not to be involved with 3, since I know nothing about distributions and am fully engaged with Firefox for iOS.  I suggest filing 3 as a separate ticket.
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
I'm gonna put this out there: distributions should be able to turn this off completely via a trivial file existence check (so we don't have to parse prefs).

Might as well add this now, 'cos it's easy to do.

Scream if you disagree.
I'm happy to guide the integration with distribution and referrer handling code, because I have accepted that I own that now. Such is life.
tracking-fennec: --- → 38+
Since we have our own INSTALL_REFERRER handler, we'll need to follow these instructions: https://github.com/adjust/android_sdk/blob/master/doc/referrer.md

Also note that the SDK requires android.permission.INTERNET and android.permission.ACCESS_WIFI_STATE permissions. I think we already ask for those.

Lastly, the SDK uses the Google Advertising ID to uniquely identify devices and requires Google Play Services "Ads" library.
(In reply to Mark Finkle (:mfinkle) from comment #4)

> and requires Google Play Services "Ads" library.

Would anyone object if we didn't ship this in our resource-constrained build?
Attached patch install_tracking WIP (obsolete) — Splinter Review
Basics. This will get a build with Install Tracking and the SDK seems to do something.

I need to get the Google Play build parts cleaned up. I also need to use the MOZ_INSTALL_TRACKING var in the code.
Assignee: nalexander → mark.finkle
Comment on attachment 8583130 [details] [diff] [review]
install_tracking WIP

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

This looks like a great start!

::: build/autoconf/android.m4
@@ +288,5 @@
>  
> +if test -n "$MOZ_INSTALL_TRACKING" ; then
> +    AC_SUBST(MOZ_INSTALL_TRACKING)
> +
> +    dnl We may have already setup the Play Services library

It seems like MOZ_INSTALL_TRACKING and MOZ_NATIVE_DEVICES overlap here, and it's time to grow a MOZ_ANDROID_GOOGLE_PLAY_SERVICES or similar.

::: mobile/android/base/GeckoApplication.java
@@ +157,5 @@
>  
>          super.onCreate();
> +
> +        // Initialize the Adjust SDK
> +        String appToken = "ABCDEFGHIJKL";

nit: final all the things!

Extract an AppConstant for this?

::: mobile/android/confvars.sh
@@ +70,5 @@
>    MOZ_NATIVE_DEVICES=1
>  fi
>  
> +# Enable install tracking SDK
> +MOZ_INSTALL_TRACKING=1

This is an inflamatory name :)  But it does reveal the intent, but not the tech.

And this, defaulted on, means that local developer builds will require Play Services and will build Adjust, possibly tracking.  That seems wrong.
Attachment #8583130 - Flags: feedback+
Blocks: 1150550
(In reply to Nick Alexander :nalexander from comment #8)

> Extract an AppConstant for this?

Presumably this should actually be a build define…


> And this, defaulted on, means that local developer builds will require Play
> Services and will build Adjust, possibly tracking.  That seems wrong.

I think this is mfinkle's working patch (hence the ABCDEFGHIJKL!). When landed this should be

MOZ_INSTALL_TRACKING=RELEASE_BUILD
Attached file adjust-data-post.txt
Attaching an example of the data posted via the SDK by just starting Firefox. No INSTALL_REFERRER was used. Anyone starting Firefox would have this data posted.
Comment on attachment 8588260 [details]
adjust-data-post.txt

Presumably this is happening immediately after launch?
Attached patch install_tracking v0.1 (obsolete) — Splinter Review
This patch:
1. Adds the Adjust SDK source code, if MOZ_INSTALL_TRACKING is set
2. Inits the Adjust SDK in GeckoApplication and handles the INSTALL_REFERRER, if MOZ_INSTALL_TRACKING is set
3. Sets MOZ_INSTALL_TRACKING if RELEASE_BUILD and MOZ_NATIVE_DEVICES is set

We depend on MOZ_NATIVE_DEVICES as a simple, lazy way to make sure the Google Play Services libs are included, without needing to create a separate configure path for multiple features depending on Google Play Services.

We can add such a path in the future. This simple patch will be easier to uplift to Beta (ugh). We also need to decide how to handle the | appToken | which I have just left hardcoded in GeckoApplication.
Attachment #8583130 - Attachment is obsolete: true
Attachment #8588319 - Flags: review?(nalexander)
Comment on attachment 8588319 [details] [diff] [review]
install_tracking v0.1

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

::: mobile/android/base/GeckoApplication.java
@@ +16,5 @@
>  import org.mozilla.gecko.util.ThreadUtils;
>  
> +import com.adjust.sdk.Adjust;
> +import com.adjust.sdk.AdjustConfig;
> +import com.adjust.sdk.LogLevel;

Did you test a build with MOZ_INSTALL_TRACKING not set? You don't seem to unconditionally include any of these files in moz.build, so I would expect this to break.

@@ +159,5 @@
>          super.onCreate();
> +
> +        if (AppConstants.MOZ_INSTALL_TRACKING) {
> +            // Initialize the Adjust SDK
> +            final String appToken = "ABCDEFGHIJKL";

This should be a build define. (Probably the environment, too.)

::: mobile/android/base/distribution/ReferrerReceiver.java
@@ +55,5 @@
>          if (!TextUtils.equals(referrer.source, MOZILLA_UTM_SOURCE)) {
> +            if (AppConstants.MOZ_INSTALL_TRACKING) {
> +                // Allow the Adjust handler to process
> +                new AdjustReferrerReceiver().onReceive(context, intent);
> +            }

Let's do this *after* our own code runs -- we want to block distro stuff as little as possible.

Did you determine whether there's any chance of their onReceive method throwing?

Also, why did you opt to init the Adjust SDK in GeckoApplication rather than right here? This seems like it'd be the best place.
(In reply to Richard Newman [:rnewman] from comment #13)
> Comment on attachment 8588319 [details] [diff] [review]
> install_tracking v0.1
> 
> Review of attachment 8588319 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/GeckoApplication.java
> @@ +16,5 @@
> >  import org.mozilla.gecko.util.ThreadUtils;
> >  
> > +import com.adjust.sdk.Adjust;
> > +import com.adjust.sdk.AdjustConfig;
> > +import com.adjust.sdk.LogLevel;
> 
> Did you test a build with MOZ_INSTALL_TRACKING not set? You don't seem to
> unconditionally include any of these files in moz.build, so I would expect
> this to break.

I did test with and without MOZ_INSTALL_TRACKING set. Changing the RELEASE_BUILD to NIGHTLY_BUILD in confvars will do it. I was also expecting failure, but the imports did not break the build _and_ Adjust data was not sent. Because no data was sent, I concluded MOZ_RELEASE_TRACKING was working correctly.

Maybe I needed a full clobber when switching?

If we get the process failing, as we expect it should, I'll probably just always add the source files to the 3rd party JAR and just use the MOZ_INSTALL_TRACKING in the code to control using/not_using the SDK.

> > +        if (AppConstants.MOZ_INSTALL_TRACKING) {
> > +            // Initialize the Adjust SDK
> > +            final String appToken = "ABCDEFGHIJKL";
> 
> This should be a build define. (Probably the environment, too.)

Sure. I don't see the pattern for making that happen, and I don't care too much about it for now.

> ::: mobile/android/base/distribution/ReferrerReceiver.java
> >          if (!TextUtils.equals(referrer.source, MOZILLA_UTM_SOURCE)) {
> > +            if (AppConstants.MOZ_INSTALL_TRACKING) {
> > +                // Allow the Adjust handler to process
> > +                new AdjustReferrerReceiver().onReceive(context, intent);
> > +            }
> 
> Let's do this *after* our own code runs -- we want to block distro stuff as
> little as possible.

This spot should be the flow where we know it's not a Distribution or Campaign event. We are early bailing.

> Did you determine whether there's any chance of their onReceive method
> throwing?

I do not know.

> Also, why did you opt to init the Adjust SDK in GeckoApplication rather than
> right here? This seems like it'd be the best place.

I just followed the suggested practices in the Adjust docs. We can tweak that later. If some advertisers don't use INSTALL_REFERRER, we'll need to use GeckoApplication anyway or the ad/install will not be tracked.
(In reply to Mark Finkle (:mfinkle) from comment #14)

> Maybe I needed a full clobber when switching?

You definitely need to re-configure. Just changing the mozconfig or confvars and doing an incremental build won't be enough.


> If we get the process failing, as we expect it should, I'll probably just
> always add the source files to the 3rd party JAR and just use the
> MOZ_INSTALL_TRACKING in the code to control using/not_using the SDK.

Sounds fine to me. I'm happy to spend time (when we're all less in a crunch) on a follow-up to make this more compartmentalized, given that we'll be turning this off altogether when there's no campaign running.


> > Did you determine whether there's any chance of their onReceive method
> > throwing?
> 
> I do not know.

Best to stick a try..catch around it just in case. I really really don't want to find that we're crashing on first run in some odd configuration.


> > Also, why did you opt to init the Adjust SDK in GeckoApplication rather than
> > right here? This seems like it'd be the best place.
> 
> I just followed the suggested practices in the Adjust docs. We can tweak
> that later. If some advertisers don't use INSTALL_REFERRER, we'll need to
> use GeckoApplication anyway or the ad/install will not be tracked.

I think their recommendation is based on tracking actions throughout the app; the Application onCreate is guaranteed to run before any other code. In our case we want it to be as late as possible, because we know we'll only be sending one ping.

If we don't think INSTALL_REFERRER will always apply, then let's come up with a place to put this that won't negatively affect every cold startup -- a post-distribution task (like our tasks for adding default bookmarks) might be a good option.

Again, a follow-up will do for now if you can't spend the time.
I found I had a piece of this patch stuck in a different patch. Yes, "qnew before qref" disease.

This patch has all the pieces in it. That part in question was from confvars.sh which now looks like:

+# Enable install tracking SDK
+# if we have Google Play support, provided by MOZ_NATIVE_DEVICES
+if test "$RELEASE_BUILD"; then
+if test "$MOZ_NATIVE_DEVICES"; then
+  MOZ_INSTALL_TRACKING=1
+fi
+fi

Because using | MOZ_INSTALL_TRACKING=$(RELEASE_BUILD) | never worked right. Also tried | MOZ_INSTALL_TRACKING=RELEASE_BUILD | but didn't work. I think it has to do with "being defined vs being defined as a value".

Anyway, the above code block worked right for nightly and release.
Attachment #8588319 - Attachment is obsolete: true
Attachment #8588319 - Flags: review?(nalexander)
Attachment #8588695 - Flags: review?(nalexander)
Comment on attachment 8588695 [details] [diff] [review]
install_tracking v0.2

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

Unfortunately this won't work when the flag is off.  There are other cases like this: the crash reporter and the native devices stuff.  Preprocessing an always-included helper is quick and dirty and regresses our nice preprocessed story.  Including either a real or a stub helper is slightly nicer but makes IDE integration slightly harder.  I prefer the latter but leave it to your schedule.

::: configure.in
@@ +3955,5 @@
>  MOZ_ANDROID_SEARCH_ACTIVITY=
>  MOZ_ANDROID_DOWNLOADS_INTEGRATION=
>  MOZ_ANDROID_MLS_STUMBLER=
>  MOZ_ANDROID_SHARE_OVERLAY=
> +MOZ_INSTALL_TRACKING=

I have a mild preference for marking this as Android only, and a mild preference to have the flag describe the tech rather than the behaviour: include Adjust SDK.  But I can't be bothered.

::: mobile/android/base/distribution/ReferrerReceiver.java
@@ +9,5 @@
>  import org.mozilla.gecko.GeckoAppShell;
>  import org.mozilla.gecko.GeckoEvent;
>  import org.mozilla.gecko.mozglue.RobocopTarget;
>  
> +import com.adjust.sdk.AdjustReferrerReceiver;

This integration style will never work if Adjust is not compiled in.  I guess you can add an optionally compiled file, or maybe we have to pre-process for this?  Or we do it dynamically with introspection?

I think I'd lean to a preprocessed helper file that has empty methods where appropriate :(  Better than preprocessing big files.

::: mobile/android/config/proguard/proguard.cfg
@@ +218,5 @@
>  # Don't print spurious warnings from the support library.
>  # See: http://stackoverflow.com/questions/22441366/note-android-support-v4-text-icucompatics-cant-find-dynamically-referenced-cl
>  -dontnote android.support.**
> +
> +# Needed to keep some constants in the install tracking library

Move this stuff into adjust-keeps.cfg, like play-services-keeps.cfg.
Attachment #8588695 - Flags: review?(nalexander) → review-
/r/6771 - Bug 1143888 - "Integrate the Adjust campaign tracking SDK into Firefox" [r=mark.finkle]
/r/6773 - Bug 1143888 - Build system updates.

Pull down these commits:

hg pull -r b29ecf386eff09830e6b291af901dda9d45baf0a https://reviewboard-hg.mozilla.org/gecko/
Comment on attachment 8590078 [details]
MozReview Request: bz://1143888/nalexander

/r/6771 - Bug 1143888 - "Integrate the Adjust campaign tracking SDK into Firefox" [r=mark.finkle]
/r/6773 - Bug 1143888 - Build system updates.

Pull down these commits:

hg pull -r b29ecf386eff09830e6b291af901dda9d45baf0a https://reviewboard-hg.mozilla.org/gecko/
Attachment #8590078 - Flags: review?(mark.finkle)
mfinkle: for testing, you don't need a full clobber.  You get enough with just:

(touch confvars.sh)
rm -rf objdir-droid/mobile/android/base && ./mach configure
./mach build mobile/android

You can verify sensible things happen at build time by trying, for example

find objdir-droid/mobile/android/base -iname '*Adjust*.class'

You can inspect the variables from the build system by interpreting

objdir-droid/config.status
Comment on attachment 8590078 [details]
MozReview Request: bz://1143888/nalexander

Does review-board no longer set r+? Very frustrated review-board as a "reviewer".
Attachment #8590078 - Flags: review?(mark.finkle) → review+
See comments about duplicate google-api code that I added in 3 places now. Very frustrated with review-board.
https://hg.mozilla.org/mozilla-central/rev/f76f02793f7a
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Comment on attachment 8590078 [details]
MozReview Request: bz://1143888/nalexander

Approval Request Comment
[Feature/regressing bug #]: 38 campaign tracking.
[User impact if declined]: very little, but marketing will be disappointed that they can't track their investment.
[Describe test coverage new/current, TreeHerder]: essentially none.  We need to get this into a build, with secrets (see https://bugzilla.mozilla.org/show_bug.cgi?id=1152871) as soon as possible for QA and mfinkle to test. 
[Risks and why]: quite low.  Builds in both configurations (on and off).
[String/UUID change made/needed]: none.

We'll want the single patch uplifted to aurora and beta; since we can't flag a landed HG commit, I've flagged the RB request.
Attachment #8590078 - Flags: approval-mozilla-aurora?
Comment on attachment 8590078 [details]
MozReview Request: bz://1143888/nalexander

Approval Request Comment
[Feature/regressing bug #]: 38 campaign tracking.
[User impact if declined]: very little, but marketing will be disappointed that they can't track their investment.
[Describe test coverage new/current, TreeHerder]: essentially none.  We need to get this into a build, with secrets (see https://bugzilla.mozilla.org/show_bug.cgi?id=1152871) as soon as possible for QA and mfinkle to test. 
[Risks and why]: quite low.  Builds in both configurations (on and off).
[String/UUID change made/needed]: none.

We'll want the single patch uplifted to aurora and beta; since we can't flag a landed HG commit, I've flagged the RB request.
Attachment #8590078 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Attachment #8590078 - Flags: approval-mozilla-aurora?
(In reply to Nick Alexander :nalexander from comment #27)
> Comment on attachment 8590078 [details]
> MozReview Request: bz://1143888/nalexander
> 
> Approval Request Comment
> [Feature/regressing bug #]: 38 campaign tracking.
I guess you are talking about the "spring" release (38.0.5), not 38.0.0.
If it is the later, this patch seems very big for that late in the cycle (even if it ships thirdparty code)...
(In reply to Sylvestre Ledru [:sylvestre] from comment #28)
> (In reply to Nick Alexander :nalexander from comment #27)
> > Comment on attachment 8590078 [details]
> > MozReview Request: bz://1143888/nalexander
> > 
> > Approval Request Comment
> > [Feature/regressing bug #]: 38 campaign tracking.
> I guess you are talking about the "spring" release (38.0.5), not 38.0.0.
> If it is the later, this patch seems very big for that late in the cycle
> (even if it ships thirdparty code)...

This code is required for the "spring" release. We had to do privacy and data audits before moving ahead. Yes, it's late, but it's a required addition.
For data collection, I still need the final review in the form of a patch to in-tree docs that enumerate the data that this collects.
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #30)
> For data collection, I still need the final review in the form of a patch to
> in-tree docs that enumerate the data that this collects.

Got an example of what you mean?
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/docs/uitelemetry.rst (which becomes https://ci.mozilla.org/job/mozilla-central-docs/Tree_Documentation/mobile/android/base/fennec/uitelemetry.html)

And also http://mxr.mozilla.org/mozilla-central/source/services/healthreport/docs/identifiers.rst

This needs to document the endpoints that data is sent to, the circumstances under which it is sent, and the datapoints. Normally this would also include the wire format, but since the SDK controls that I don't think we need that in this case.
Comment on attachment 8590078 [details]
MozReview Request: bz://1143888/nalexander

OK, we have time for the 38.0.5 cycle then.
Attachment #8590078 - Flags: approval-mozilla-beta?
Attachment #8590078 - Flags: approval-mozilla-beta+
Attachment #8590078 - Flags: approval-mozilla-aurora?
Attachment #8590078 - Flags: approval-mozilla-aurora+
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #32)
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/docs/
> uitelemetry.rst (which becomes
> https://ci.mozilla.org/job/mozilla-central-docs/Tree_Documentation/mobile/
> android/base/fennec/uitelemetry.html)
> 
> And also
> http://mxr.mozilla.org/mozilla-central/source/services/healthreport/docs/
> identifiers.rst
> 
> This needs to document the endpoints that data is sent to, the circumstances
> under which it is sent, and the datapoints. Normally this would also include
> the wire format, but since the SDK controls that I don't think we need that
> in this case.

rnewman, mfinkle: in the course of your investigations did you already determine the data sent from device to the Adjust endpoint?  If so, can you provide it to me?  If not, I'll collect this myself and start a documentation patch.
rnewman, mfinkle: https://bugzilla.mozilla.org/show_bug.cgi?id=1143888#c34
Flags: needinfo?(rnewman)
Flags: needinfo?(mark.finkle)
(In reply to Nick Alexander :nalexander from comment #35)
> rnewman, mfinkle: https://bugzilla.mozilla.org/show_bug.cgi?id=1143888#c34

Sent via email.
Flags: needinfo?(rnewman)
Hi lmandel: this is the main landing for integrating the Adjust SDK, which tracks Firefox for Android installs.  As you can see from https://bugzilla.mozilla.org/show_bug.cgi?id=1143888#c37, it has tracked 38.0.5 but not 38.0 (and I believe that all sub-tickets have done the same).

To get an idea of the feature, I recommend reading the documentation at

https://gecko.readthedocs.org/en/latest/mobile/android/base/fennec/adjust.html
Flags: needinfo?(mark.finkle) → needinfo?(lmandel)
Thanks for flagging Nick. Sylvestre is already on top of this for the 38.0.5 release.

Release Note Request (optional, but appreciated)
[Why is this notable]: Addition of campaign tracking to Fennec. I don't think this has a direct user benefit but is the type of change that we should be transparent about making.
[Suggested wording]: Support for Android campaign tracking with the Adjust SDK 
[Links (documentation, blog post, etc)]:
Would be good to include a link to documentation (perhaps on Sumo) that describes why we're making this change, the impact on user privacy, and how a user can opt-out (if this is possible).

Nick, Mark - Can one of you please help with the wording for the note and the documentation?
relnote-firefox: --- → ?
Flags: needinfo?(nalexander)
Flags: needinfo?(mark.finkle)
Flags: needinfo?(lmandel)
Winston Bowden has a mailing list / blog post ready to go.
Flags: needinfo?(nalexander)
Flags: needinfo?(mark.finkle)
(In reply to Mark Finkle (:mfinkle) from comment #41)
> Winston Bowden has a mailing list / blog post ready to go.

Sweet!  Will Winston also handle the relnote?

For everybody's situational awareness, Bug 1159277 landed some technically focused documentation, which is at

https://gecko.readthedocs.org/en/latest/mobile/android/base/fennec/adjust.html
Release Note Request (optional, but appreciated)
[Why is this notable]:
[Suggested wording]:
[Links (documentation, blog post, etc)]:

Sylvestre, I didn't notice this is going to be released in 38.0.5.  I removed the relnote from 39 on Lawrence's suggestion that it should be in 38.0.5 instead.
Flags: needinfo?(sledru)
Yes. I asked for a wording for this as I am not sure what to write.
Flags: needinfo?(sledru)
Depends on: 1169393
Added to the release notes with "Integrated Adjust SDK to measure aggregate installs" as wording.
We will also probably add a blog post to explain how we respect our user privacy.
Target Milestone: Firefox 40 → Firefox 38
Attachment #8590078 - Attachment is obsolete: true
Attachment #8619770 - Flags: review+
Attachment #8619771 - Flags: review+
Depends on: 1208240
Product: Firefox for Android → Firefox Build System
Target Milestone: Firefox 38 → mozilla38
You need to log in before you can comment on or make changes to this bug.