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.

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

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

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

ni Sebastian?

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.

Updated patch.

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.

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

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?

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

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!