Collect Telemetry on how many Fennec users also have Orbot installed

RESOLVED FIXED in Firefox 52

Status

()

Firefox for Android
General
P1
normal
RESOLVED FIXED
10 months ago
3 months ago

People

(Reporter: tjr, Assigned: tjr)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 53
Points:
---

Firefox Tracking Flags

(firefox51 wontfix, firefox52 fixed, firefox53 fixed)

Details

(Whiteboard: [tor-mobile])

MozReview Requests

()

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

Attachments

(1 attachment)

(Assignee)

Description

10 months ago
Orbot is Tor for Android, provided as a service for other apps (like Facebook or Twitter) to take advantage of. Orbot is intended to play nicely with other apps and expose itself as a service for anyone on the device who is interested in using Tor. 


Using NetCipher (https://github.com/guardianproject/NetCipher) we can call 'isOrbotInstalled' to see if Orbot is installed and if so report it via Telemetry.  Alternatively, there's no magic behind that function, we could call context.getPackageManager().getPackageInfo("org.torproject.android", PackageManager.GET_ACTIVITIES); and see if it raises an exception.


We would like to learn what percentage of our users have Orbot installed, as this will guide us in future tor-mobile efforts.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 4

8 months ago
Okay now that I have a patch I'm requesting review (both code and Data Collection). If I flagged the wrong person or the wrong process please let me know.  (I used the try chooser to trigger a try build, which failed - I am unsure why.)
Flags: needinfo?(benjamin)
QA Contact: tom
(Assignee)

Updated

8 months ago
Attachment #8824231 - Flags: review?(s.kaspari)
(Assignee)

Updated

8 months ago
Assignee: nobody → tom
QA Contact: tom

Comment 5

8 months ago
mozreview-review
Comment on attachment 8824231 [details]
Bug 1314784 Add an 'Is Orbot Installed' telemetry probe

https://reviewboard.mozilla.org/r/102754/#review103234

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:1947
(Diff revision 3)
>                  if (Versions.feature16Plus) {
>                      Telemetry.addToHistogram("BROWSER_IS_ASSIST_DEFAULT",
>                              (isDefaultBrowser(Intent.ACTION_ASSIST) ? 1 : 0));
>                  }
> +
> +                boolean orbotInstalled;

If the thing you're logging is an int, shouldn't you just make this an int and set it to 0/1 in the branches?

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:1952
(Diff revision 3)
> +                boolean orbotInstalled;
> +                try {
> +                    PackageManager pm = getContext().getPackageManager();
> +                    pm.getPackageInfo("org.torproject.android", PackageManager.GET_ACTIVITIES);
> +                    orbotInstalled = true;
> +                } catch (PackageManager.NameNotFoundException e) {

Nit: Move the catch up to the previous line, to be consistent with the remainder of the file.

::: toolkit/components/telemetry/Histograms.json:4403
(Diff revision 3)
>      "alert_emails": ["mobile-frontend@mozilla.com"],
>      "bug_numbers": [1244704]
>    },
> +  "FENNEC_ORBOT_INSTALLED": {
> +    "expires_in_version": "60",
> +    "kind": "flag",

Should be "boolean", like BROWSER_IS_ASSIST_DEFAULT ?

::: toolkit/components/telemetry/Histograms.json:4406
(Diff revision 3)
> +  "FENNEC_ORBOT_INSTALLED": {
> +    "expires_in_version": "60",
> +    "kind": "flag",
> +    "cpp_guard": "ANDROID",
> +    "description": "Whether or not users have Orbot installed",
> +    "alert_emails": ["tom@mozilla.com"],

"seceng@mozilla.org", please, to avoid personnel dependencies.

Comment 6

8 months ago
mozreview-review
Comment on attachment 8824231 [details]
Bug 1314784 Add an 'Is Orbot Installed' telemetry probe

https://reviewboard.mozilla.org/r/102754/#review103410

::: toolkit/components/telemetry/Histograms.json:4401
(Diff revision 3)
>      "n_buckets": 20,
>      "description": "Number of bookmarks stored in the browser DB",
>      "alert_emails": ["mobile-frontend@mozilla.com"],
>      "bug_numbers": [1244704]
>    },
> +  "FENNEC_ORBOT_INSTALLED": {

I'm perfectly fine with collecting this temporarily. However, you should be aware that we don't submit this telemetry data from the release population; only from beta or people who have opted-in. So what you're going to get could be a pretty skewed sample and probably not useful for making product decisions.
Attachment #8824231 - Flags: review+

Updated

8 months ago
Flags: needinfo?(benjamin)
(Assignee)

Comment 7

8 months ago
mozreview-review-reply
Comment on attachment 8824231 [details]
Bug 1314784 Add an 'Is Orbot Installed' telemetry probe

https://reviewboard.mozilla.org/r/102754/#review103410

> I'm perfectly fine with collecting this temporarily. However, you should be aware that we don't submit this telemetry data from the release population; only from beta or people who have opted-in. So what you're going to get could be a pretty skewed sample and probably not useful for making product decisions.

I think that's okay. I assume that the beta and opt-in population are _more_ likely to have Orbot installed than the non-opt-in release population. If the percentage we see is abysmally low, we can inference about release being even lower. If the percentage we see is high, we know release will be lower but that there is still some definite probability of reaching people who would use features enabled by Orbot.

The only other option is to make this an opt-out collection, right? I'm not thrilled at the idea of adding this to the default data collection set...
Comment hidden (mozreview-request)
(Assignee)

Comment 9

8 months ago
mozreview-review-reply
Comment on attachment 8824231 [details]
Bug 1314784 Add an 'Is Orbot Installed' telemetry probe

https://reviewboard.mozilla.org/r/102754/#review103234

> If the thing you're logging is an int, shouldn't you just make this an int and set it to 0/1 in the branches?

Fixed

> Nit: Move the catch up to the previous line, to be consistent with the remainder of the file.

This had been fixed in the third patch

> Should be "boolean", like BROWSER_IS_ASSIST_DEFAULT ?

After speaking with the telemetry team - no, flag is correct. Booleans may have multiple values for a user's session (like how many times the 'Yes' or 'No' was chosen on an alert box) while a flag will have only one value in a user's session. I don't know why the other things are booleans.

However - preferable even more to a flag is a scalar. However... there's no API to collect scalars in Java so... a flag-histogram is the answer.

> "seceng@mozilla.org", please, to avoid personnel dependencies.

Fixed.
Joe, could you please find someone to review this?
Flags: needinfo?(jcheng)
ni Sebastian?
Flags: needinfo?(jcheng) → needinfo?(s.kaspari)

Comment 12

8 months ago
mozreview-review
Comment on attachment 8824231 [details]
Bug 1314784 Add an 'Is Orbot Installed' telemetry probe

https://reviewboard.mozilla.org/r/102754/#review104204

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:1947
(Diff revision 4)
> +                int orbotInstalled;
> +                try {
> +                    PackageManager pm = getContext().getPackageManager();
> +                    pm.getPackageInfo("org.torproject.android", PackageManager.GET_ACTIVITIES);
> +                    orbotInstalled = 1;
> +                } catch (PackageManager.NameNotFoundException e) {
> +                    orbotInstalled = 0;
> +                }
> +                Telemetry.addToHistogram("FENNEC_ORBOT_INSTALLED", orbotInstalled);

This code is very similar to other code we have in ContextUtils. The super nice solution would be to move this into ContextUtils and just have one line here:

> Telemetry.addToHistogram("FENNEC_ORBOT_INSTALLED",
>     ContextUtils.isPackageInstalled('org.torproject.android') ? 1 : 0);

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:1950
(Diff revision 4)
>                  }
> +
> +                int orbotInstalled;
> +                try {
> +                    PackageManager pm = getContext().getPackageManager();
> +                    pm.getPackageInfo("org.torproject.android", PackageManager.GET_ACTIVITIES);

You are not really interested in the activities of the package, so the second parameter could be 0.
Attachment #8824231 - Flags: review?(s.kaspari) → review+
Flags: needinfo?(s.kaspari)
Comment hidden (mozreview-request)
(Assignee)

Comment 14

8 months ago
Updated patch.
Flags: needinfo?(s.kaspari)
https://hg.mozilla.org/integration/mozilla-inbound/rev/524d3073789c49cd2136765e64aa2a4c17bbb826
Bug 1314784 - Add an 'Is Orbot Installed' telemetry probe. r=bsmedberg,sebastian
Reviewed the updated patch and landed it.
Flags: needinfo?(s.kaspari)
(Assignee)

Comment 17

8 months ago
Comment on attachment 8824231 [details]
Bug 1314784 Add an 'Is Orbot Installed' telemetry probe

Approval Request Comment
[User impact if declined]: We will be delayed getting Telemetry for this item and delays making product decisions.

[Is this code covered by automated tests?]: Unsure. I see tests that trigger telemetry collection, but I am not sure if these tests run on Mobile.

[Has the fix been verified in Nightly?]: No

[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?]: No, I don't think so.

[Why is the change risky/not risky?]: It adds a Telemetry probe, which seems less risky. The major possible concern would be in the PackageManager code itself caused a crash - but if that was the case we should not crash the browser, and instead just report incorrect Telemetry data.

[String changes made/needed]: None, I think
Attachment #8824231 - Flags: approval-mozilla-beta?
Attachment #8824231 - Flags: approval-mozilla-aurora?
Hi :tjr,
May I know if this is urgent? Since next week is RC week and we only have less than two weeks to release 51, can we let this ride the train on 52?
Flags: needinfo?(tom)

Comment 19

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/524d3073789c
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
(Assignee)

Comment 20

7 months ago
(In reply to Gerry Chang [:gchang] from comment #18)
> Hi :tjr,
> May I know if this is urgent? Since next week is RC week and we only have
> less than two weeks to release 51, can we let this ride the train on 52?

If I understand correctly, you're asking if we can put it into 52 (which will go into beta shortly), but not 51 (which will be released shortly.) If so - yes that's fine.
Flags: needinfo?(tom)
status-firefox51: --- → fix-optional
status-firefox52: --- → affected
Comment on attachment 8824231 [details]
Bug 1314784 Add an 'Is Orbot Installed' telemetry probe

Let's take this for 52 but as gerry mentions, it's very late in the cycle for beta changes. I hope this gets you enough data!
Attachment #8824231 - Flags: approval-mozilla-beta?
Attachment #8824231 - Flags: approval-mozilla-beta-
Attachment #8824231 - Flags: approval-mozilla-aurora?
Attachment #8824231 - Flags: approval-mozilla-aurora+
status-firefox51: fix-optional → wontfix

Comment 22

7 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/98ecf719163d
status-firefox52: affected → fixed

Updated

3 months ago
Blocks: 1357994

Updated

3 months ago
Priority: -- → P1
You need to log in before you can comment on or make changes to this bug.