1226322 - Add probe to measure whether Firefox is launched in restricted profile

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: Profile Handling, defect, P5)

Description

[:Margaret Leibovic]

We need to know whether or not people are using Firefox in restricted profiles. John Jensen suggested adding an extra flag to the FHR environment to capture this data, and mentioned that his team could help with the analysis on the server side.

Richard, can you advise on how we implement this on the client?

Comment 1

[:Margaret Leibovic]

Actually, it sounds like FHR is being killed sooner than I thought... maybe we'll just need to wait until we have opt-out telemetry pings to send this.

Alternately, in the interim, I wonder if we could somehow leverage Adjust to track this?

Comment 2

[Barbara Bermes [:barbara] - NI please!]

"sooner than I thought"

Is it being killed before we would land this in 45?

What do you mean by opt-out telemetry pings?

Comment 3

[:Margaret Leibovic]

(In reply to Barbara Bermes [:barbara] from comment #2)
> "sooner than I thought"
> 
> Is it being killed before we would land this in 45?

The pipeline is being shut down Dec 31, so yes, killed before we would land this.

> What do you mean by opt-out telemetry pings?

I mean unified telemetry, which is the system that will replace FHR (bug 1220177).

Comment 4

[Richard Newman [:rnewman]]

BrowserHealthRecorder shows environment gathering, which will probably still be relevant for UT. I can advise more closer to the time.

Relevant, though: if a profile starts restricted and never changes, we can simplify. If it can change outside our knowledge, then the lifecycle of BrowserLocaleManager will be a good inspiration.

Comment 5

[Barbara Bermes [:barbara] - NI please!]

(In reply to :Margaret Leibovic from comment #3)
> (In reply to Barbara Bermes [:barbara] from comment #2)
> > "sooner than I thought"
> > 
> > Is it being killed before we would land this in 45?
> 
> The pipeline is being shut down Dec 31, so yes, killed before we would land
> this.
> 
> > What do you mean by opt-out telemetry pings?
> 
> I mean unified telemetry, which is the system that will replace FHR (bug
> 1220177).

Ok, so that means, we should just wait and not put any metrics in there for now. 

Moving this to unified telemetry, do we see any changes in the concept that we have to run by legal or is it the same approach?

Comment 6

[:Margaret Leibovic]

(In reply to Barbara Bermes [:barbara] from comment #5)
> (In reply to :Margaret Leibovic from comment #3)
> > (In reply to Barbara Bermes [:barbara] from comment #2)
> > > "sooner than I thought"
> > > 
> > > Is it being killed before we would land this in 45?
> > 
> > The pipeline is being shut down Dec 31, so yes, killed before we would land
> > this.
> > 
> > > What do you mean by opt-out telemetry pings?
> > 
> > I mean unified telemetry, which is the system that will replace FHR (bug
> > 1220177).
> 
> Ok, so that means, we should just wait and not put any metrics in there for
> now. 
> 
> Moving this to unified telemetry, do we see any changes in the concept that
> we have to run by legal or is it the same approach?

Same approach, different implementation.

It may be possible to get something landed for 45 here, but it would need to be a simplified version of what we hope to achieve in the future.

gfritzsche, given the current UT system on Android, do you think it would be possible to send an opt-out ping that includes a boolean field to indicate whether or not Firefox is running in a restricted profile?

I know that eventually (hopefully soon) we want to be uploading data from Java, but maybe in the very immediate future, we could find a way to leverage the Gecko uploader to send this data in an opt-out telemetry ping.

Comment 7

[Georg Fritzsche [:gfritzsche]]

It depends on what exactly you want to do and how quickly we need this.
Two possible approaches:

* sending the proper opt-out pings like on desktop will probably need a bit of investigation and work
  (probably: data choices policy, checking the code paths, turning on some UT features)

* a faster possibility is sending a small separate ping type now, lets say including client-id, no environment data, minimal payload.
  We'd still have to check how this works with the data choices policy though.
  The API is TelemetryController.submitExternalPing() - we would have to give it a test-run to see whether it works as expected on Android.

For testing we can point Fennec to submit to a local server (pref toolkit.telemetry.server):
https://github.com/vdjeric/gzipServer

Then test submitting pings:
TelemetryController.submitExternalPing("your-custom-ping-type",
{
  restrictedProfile: <true/false>
},
{
  addClientId: true,
  addEnvironment: false,
});


... have we made a call yet on how data collection review works for Fennec?
Is it the same as Fx Desktop?
https://wiki.mozilla.org/Firefox/Data_Collection

Comment 8

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #7)
> ... have we made a call yet on how data collection review works for Fennec?
> Is it the same as Fx Desktop?
> https://wiki.mozilla.org/Firefox/Data_Collection

Comment 9

[Benjamin Smedberg]

Yes it is all-Firefox.

Comment 10

[:Margaret Leibovic]

(In reply to Georg Fritzsche [:gfritzsche] [away Dec 19 - Jan 3] from comment #7)

> * a faster possibility is sending a small separate ping type now, lets say
> including client-id, no environment data, minimal payload.
>   We'd still have to check how this works with the data choices policy
> though.
>   The API is TelemetryController.submitExternalPing() - we would have to
> give it a test-run to see whether it works as expected on Android.
> 
> For testing we can point Fennec to submit to a local server (pref
> toolkit.telemetry.server):
> https://github.com/vdjeric/gzipServer
> 
> Then test submitting pings:
> TelemetryController.submitExternalPing("your-custom-ping-type",
> {
>   restrictedProfile: <true/false>
> },
> {
>   addClientId: true,
>   addEnvironment: false,
> });

I think we should go ahead and implement this approach.

We had a meeting with legal a few weeks ago, and they approved us sending an opt-out boolean to indicate whether the browser was launched in a restricted profile.

But we will also flag a data steward for review before landing the patch, following the Firefox data collection guidelines.

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

After some tinkering I was able to receive the following ping on a locally running server:

> {
>   "clientId": "6b1e8c4a-1698-47b2-bd66-1727ffae5c99",
>   "payload": {
>     "restrictedProfile": false
>   },
>   "application": {
>     "vendor": "Mozilla",
>     "name": "Fennec",
>     "buildId": "20151216034107",
>     "version": "46.0a1",
>     "architecture": "arm",
>     "platformVersion": "46.0a1",
>     "xpcomAbi": "arm-eabi-gcc3",
>     "channel": "default"
>   },
>   "version": 4,
>   "creationDate": "2015-12-16T15:19:58.809Z",
>   "type": "fennec-startup-ping",
>   "id": "3b1c0a0b-fe15-47d1-a925-29aafa90d922"
> }

For testing I've send this ping from startup() in browser.js. This telemetry topic is still new to me: Would we want to send something like this on every startup?

> > TelemetryController.submitExternalPing("your-custom-ping-type",

How do we define custom ping types? Can it just be anything? Is there any guideline I should follow? In the example above I just used "fennec-startup-ping" for testing.

Using TelemetryController.jsm caused a bunch of JavaScript errors - all related to logging (in configureLogging() and get _log()). I patched the methods locally to use the Android logger or just do nothing. We might need some actual patches to make this work.

Comment 12

[:Margaret Leibovic]

(In reply to Sebastian Kaspari (:sebastian) from comment #11)
> After some tinkering I was able to receive the following ping on a locally
> running server:
> 
> > {
> >   "clientId": "6b1e8c4a-1698-47b2-bd66-1727ffae5c99",
> >   "payload": {
> >     "restrictedProfile": false
> >   },
> >   "application": {
> >     "vendor": "Mozilla",
> >     "name": "Fennec",
> >     "buildId": "20151216034107",
> >     "version": "46.0a1",
> >     "architecture": "arm",
> >     "platformVersion": "46.0a1",
> >     "xpcomAbi": "arm-eabi-gcc3",
> >     "channel": "default"
> >   },
> >   "version": 4,
> >   "creationDate": "2015-12-16T15:19:58.809Z",
> >   "type": "fennec-startup-ping",
> >   "id": "3b1c0a0b-fe15-47d1-a925-29aafa90d922"
> > }
> 
> For testing I've send this ping from startup() in browser.js. This telemetry
> topic is still new to me: Would we want to send something like this on every
> startup?

A ping sent on every startup starts to sound like our plan for a mobile-friendly "core" data ping:
https://docs.google.com/document/d/1Ap5Z48Rh4t1r5lKmDAVCHNWpLS4nJIG4abZmw0nQiq4/edit#heading=h.j26q8bbe65p0

If we modify this custom ping slightly, maybe we can just start using that as our "core" data ping.

> > > TelemetryController.submitExternalPing("your-custom-ping-type",
> 
> How do we define custom ping types? Can it just be anything? Is there any
> guideline I should follow? In the example above I just used
> "fennec-startup-ping" for testing.

NI to gfritzsche to answer this.

> Using TelemetryController.jsm caused a bunch of JavaScript errors - all
> related to logging (in configureLogging() and get _log()). I patched the
> methods locally to use the Android logger or just do nothing. We might need
> some actual patches to make this work.

Hm... we've been planning to implement our own uploader in Java. If actual patches turn out to be a lot of work, we may want to go straight for a Java implementation.

Comment 13

[Georg Fritzsche [:gfritzsche]]

(In reply to :Margaret Leibovic from comment #12)
> A ping sent on every startup starts to sound like our plan for a
> mobile-friendly "core" data ping:
> https://docs.google.com/document/d/
> 1Ap5Z48Rh4t1r5lKmDAVCHNWpLS4nJIG4abZmw0nQiq4/edit#heading=h.j26q8bbe65p0
> 
> If we modify this custom ping slightly, maybe we can just start using that
> as our "core" data ping.

Modifying the Telemetry JS modules to do that would involve a bit of work (everything except the "payload" content is common ping data added to all pings currently).
If we want to do core-data pings we should start a new clean code path somewhere (Java or modifying the JS modules for now, whatever works best for the mobile team).

> > > > TelemetryController.submitExternalPing("your-custom-ping-type",
> > 
> > How do we define custom ping types? Can it just be anything? Is there any
> > guideline I should follow? In the example above I just used
> > "fennec-startup-ping" for testing.
> 
> NI to gfritzsche to answer this.

This can be pretty much anything, it should be descriptive though and only contain simple characters.
The only other thing needed is to file a bug so that the ping will not be sorted into the "unknown pings"/"other" bucket on the server-side.
The bug would go into "Cloud Services :: Metrics: Pipeline", for further questions you can check with mreid.

> 
> > Using TelemetryController.jsm caused a bunch of JavaScript errors - all
> > related to logging (in configureLogging() and get _log()). I patched the
> > methods locally to use the Android logger or just do nothing. We might need
> > some actual patches to make this work.
> 
> Hm... we've been planning to implement our own uploader in Java. If actual
> patches turn out to be a lot of work, we may want to go straight for a Java
> implementation.

What is the problem there? Does Log.jsm not work properly on Android?

Comment 14

[:Margaret Leibovic]

Revisiting this bug now that bug 1205835 landed... is there a way for us to use the new core ping to track whether or not Firefox was launched in a restricted profile? Or maybe we should hack the experiments field and include this as an experiment?

Comment 15

[Georg Fritzsche [:gfritzsche]]

If it is considered important enough to be in the "core" ping, it would be much cleaner to put this in an optional separate field.

Comment 16

[:Margaret Leibovic]

(In reply to Georg Fritzsche [:gfritzsche] from comment #15)
> If it is considered important enough to be in the "core" ping, it would be
> much cleaner to put this in an optional separate field.

Okay, that sounds like a better idea.

My main concern is that the majority of browser use doesn't happen in a restricted profile, so I don't want to be sending additional data in those cases. But if we can add this as an optional field, that sounds like it satisfies our use case.

Similarly, if we ever drop support for restricted profiles (based on this valuable data we're getting here), I don't want us to need to legacy support some unnecessary field.

Maybe I'm scope creeping, but this would also be useful for tracking guest profile usage, since we are currently skeptical of our telemetry data around that feature as well.

Comment 17

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

we could consider a "launch" field which has states like: normal, url, guest, restricted, shortcut

Comment 18

[Georg Fritzsche [:gfritzsche]]

(In reply to Mark Finkle (:mfinkle) from comment #17)
> we could consider a "launch" field which has states like: normal, url,
> guest, restricted, shortcut

That sounds useful. Is this an important core metric that should be part of the "core" ping?
Are all cases except "normal" relatively rare? If so, we can optimize by keeping it optional (only set for non-"normal" launches).

Comment 19

[:Margaret Leibovic]

(In reply to Georg Fritzsche [:gfritzsche] from comment #18)
> (In reply to Mark Finkle (:mfinkle) from comment #17)
> > we could consider a "launch" field which has states like: normal, url,
> > guest, restricted, shortcut
> 
> That sounds useful. Is this an important core metric that should be part of
> the "core" ping?
> Are all cases except "normal" relatively rare? If so, we can optimize by
> keeping it optional (only set for non-"normal" launches).

"url" is very frequent (that's would be what happens when you open a URL from another app).

Given that engagement is one of our top priorities, and that we're trying to increase the number of ways people enter the app, I could see this as an important field to include. We have some goals around increasing homescreen shortcut usage, so this could be a good data point for our KPIs.

If it doesn't hurt us to exclude the field for the default case (launching from the Firefox icon), I think it makes sense to keep it optional. I'd just want to make sure we can handle the analysis on the pipeline side.

Comment 20

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Mark Finkle (:mfinkle) from comment #17)
> we could consider a "launch" field which has states like: normal, url,
> guest, restricted, shortcut

Don't they overlap? "url" could be a restricted, guest or normal profile. Will we still be able to accurately measure restricted profiles usage if some of the usage is included in "url"?

Comment 21

[:Margaret Leibovic]

(In reply to Sebastian Kaspari (:sebastian) from comment #20)
> (In reply to Mark Finkle (:mfinkle) from comment #17)
> > we could consider a "launch" field which has states like: normal, url,
> > guest, restricted, shortcut
> 
> Don't they overlap? "url" could be a restricted, guest or normal profile.
> Will we still be able to accurately measure restricted profiles usage if
> some of the usage is included in "url"?

That's a really excellent point, and evidence that this has certainly scope creeped. Restricted profiles could also create homescreen shortcuts.

So I think for the sake of this bug we should just create an optional "restricted" field, which will be sent with a very small % of our core pings.

Then we can discuss whether it makes sense to include launch data as well. We already have opt-in telemetry probes in place for launching the browser, so we do have some information about that to help guide us.

Comment 22

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

One issue I forsee is that afaict, we don't currently have a way to uniquely identify installs.

We're currently uploading the client ID from Gecko and afaik the client ID is specific to a given profile (as opposed to an install). Also afaik, Restricted & Guest profiles use a different profile on disk and thus a separate client ID.

Assuming I understand both the Gecko client ID & the restricted/guest profiles, we'd both have to add a client ID specific to the install and a new field to identify alternative profiles, or change client ID to be used per install, rather than per profile.

Comment 23

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

Margaret, is my understand for restricted/guest profiles correct in comment 22?

Comment 24

[:Margaret Leibovic]

(In reply to Michael Comella (:mcomella) from comment #22)
> One issue I forsee is that afaict, we don't currently have a way to uniquely
> identify installs.
> 
> We're currently uploading the client ID from Gecko and afaik the client ID
> is specific to a given profile (as opposed to an install). Also afaik,
> Restricted & Guest profiles use a different profile on disk and thus a
> separate client ID.

Correct, restricted/guest profiles both use a separate gecko profile. Restricted profiles go one step further and act as a complete separate install on the Android system (we don't have a shared /data/data directory we can read from with the admin profile app install).

> Assuming I understand both the Gecko client ID & the restricted/guest
> profiles, we'd both have to add a client ID specific to the install and a
> new field to identify alternative profiles, or change client ID to be used
> per install, rather than per profile.

I don't think we should try to match these guest/restricted profiles with their main profile. It's fine to treat these as separate clients/installs in telemetry.

The main question we're trying to answer is "how many people use this feature?".

You do raise a point that if that's a lot of people, that might skew our overall MAU/DAU numbers, but let's cross that bridge when we get to it. We could always filter these out in that analysis. Or alternately, maybe we *should* count these as separate users, since technically it is actually another person using the browser.

Comment 25

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

Margaret asked on IRC if we were blocked on this and afaict we're not. To add this value, I assume we'd need to grab it from the Activity context so it'd need to be passed in with the Intent (see BrowserApp.uploadTelemetry). Then the value would need to be passed around TelemetryUploadService until TelemetryUploadService.uploadCorePing, where it'd need to get passed into TelemetryPingGenerator.createCorePing. Then it'd get passed into TelemetryPingGenerator.createCorePingPayload, where it can be added to the ping itself. A corresponding JSON key would need to be added to the CorePing class.

fwiw, to avoid having too many arguments and better handling optional arguments, I wanted to move TelemetryPingGenerator.createCorePing over to a Builder (either for creating the argument to createCorePing or for just creating the core ping itself), but we don't have to do this here.

Comment 26

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

My proposed strategy to change the format is we add the "restricted" field if the profile is restricted, otherwise we omit the field and it's assumed to be false. This doesn't deal with guest mode in Firefox – do we want to send information about that too, Margaret?

Comment 27

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

Attachment: MozReview Request: Bug 1226322 - (WIP) Add 'restricted' field to core telemetry ping. r=sebastian

This patch seems simple enough to work but I can't generate a client ID on this
new profile (see bug 1244295) which prevents us from uploading the telemetry.

Data choices are disabled in the restricted profile so I presume we don't
upload telemetry via gecko and thus I assume this client ID will never be
created without bug 1244295.

Still TODO:
* Update core ping docs
* If we're sending core pings from this profile, we should probably give it an
opt-out data choice.
* Probably fix bug 1244295 first

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

Comment 28

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

(In reply to Michael Comella (:mcomella) from comment #27)
> * If we're sending core pings from this profile, we should probably give it
> an
> opt-out data choice.

Any thoughts, Margaret? Should we be sending data from a restricted profile at all?

Comment 29

[:Margaret Leibovic]

(In reply to Michael Comella (:mcomella) from comment #26)
> My proposed strategy to change the format is we add the "restricted" field
> if the profile is restricted, otherwise we omit the field and it's assumed
> to be false. This doesn't deal with guest mode in Firefox – do we want to
> send information about that too, Margaret?

I don't care as much about guest mode, but this is another case where users are very unlikely to opt in to the opt-in telemetry. Can we send this data in the same field? As much as I want to add a quick fix to get restricted profile data, I don't want us to go crazy adding all sort of new custom fields.

(In reply to Michael Comella (:mcomella) from comment #28)
> (In reply to Michael Comella (:mcomella) from comment #27)
> > * If we're sending core pings from this profile, we should probably give it
> > an
> > opt-out data choice.
> 
> Any thoughts, Margaret? Should we be sending data from a restricted profile
> at all?

The admin UI controls the opt-out preference, so this should already be taken care of.

Comment 30

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

(In reply to :Margaret Leibovic from comment #24)

> You do raise a point that if that's a lot of people, that might skew our
> overall MAU/DAU numbers, but let's cross that bridge when we get to it. We
> could always filter these out in that analysis. Or alternately, maybe we
> *should* count these as separate users, since technically it is actually
> another person using the browser.

We should definitely treat these people as unique active users.

Comment 31

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

(In reply to Michael Comella (:mcomella) from comment #27)
> Created attachment 8718160 [details]
> MozReview Request: Bug 1226322 - (WIP) Add 'restricted' field to core
> telemetry ping. r=sebastian

This type of patch could be setting a bad precedent. We spent time working on the fields to include in the "core" ping and now we are willy-nilly adding a new one. It's not added in a scalable manner. Adding a boolean field like this for a one-off situation is not a good plan. What about the next simple, one-off? 

Lack of the field is not | false | either. It's more like "undefined" because we don't know if the ping format in this version made an explicit choice to _not_ include the field or just didn't know about the data. In order to better handle that, we'd bump the version (v) to 2. But that has potential impact on the server side and analysis scripts.

Comment 32

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Mark Finkle (:mfinkle) from comment #31)
> This type of patch could be setting a bad precedent. We spent time working
> on the fields to include in the "core" ping and now we are willy-nilly
> adding a new one. It's not added in a scalable manner. Adding a boolean
> field like this for a one-off situation is not a good plan. What about the
> next simple, one-off? 
> 
> Lack of the field is not | false | either. It's more like "undefined"
> because we don't know if the ping format in this version made an explicit
> choice to _not_ include the field or just didn't know about the data. In
> order to better handle that, we'd bump the version (v) to 2. But that has
> potential impact on the server side and analysis scripts.

Maybe we could change this to a field saying whether the current profile is "normal", "guest" or "restricted"? This is okay too and not hard. We just should not mix them with launch modes like we discussed above.

Comment 33

[Georg Fritzsche [:gfritzsche]]

(In reply to Mark Finkle (:mfinkle) from comment #31)
> Lack of the field is not | false | either. It's more like "undefined"
> because we don't know if the ping format in this version made an explicit
> choice to _not_ include the field or just didn't know about the data. In
> order to better handle that, we'd bump the version (v) to 2. But that has
> potential impact on the server side and analysis scripts.

I think once we have the basic fields in the "core" ping that we need for launch (we should?), we should definitely bump versions with each new addition.
We will need this for validation and future integrated schema checks.

Comment 34

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

Not actively working on this.

I imagine we'll reconsider this when we create a Java-based main ping or move the existing Gecko pings to the Java uploader (bug 1251639).

Comment 35

[Firefox Bug Husbandry Bot]

Bulk Move: https://bugzilla.mozilla.org/show_bug.cgi?id=1399539

Comment 36

[Firefox Bug Husbandry Bot]

Re-triaging per https://bugzilla.mozilla.org/show_bug.cgi?id=1473195

Needinfo :susheel if you think this bug should be re-triaged.

Comment 37

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.