1134998 - App Usage Metrics should ignore all events from private windows

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System, defect)

Description

[Etienne Segonzac (:etienne)]

Similiraly to what places.js is doing here [1]

[1] https://github.com/mozilla-b2g/gaia/blob/f3ccc4c0edb0e0ebc9ea0bac3bb0ef4799554651/apps/system/js/places.js#L121-123

Comment 1

[Marshall Culpepper [:marshall_law]]

Hrm.. is it possible to load a private browser directly from the launcher?

Comment 2

[Gregor Wagner [:gwagner]]

Francis, what are your thoughts on this?

Comment 3

[Candice Serran (:cserran)]

Metrics is a committed 2.2 feature. Needs verification on how this works on desktop

Comment 4

[Kevin Grandon :kgrandon]

Etienne, Marshall - Do we know what desktop/fennec does here? Are we sending data that users don't want to be sent, or should be anonymized?

Comment 5

[Etienne Segonzac (:etienne)]

(In reply to Kevin Grandon :kgrandon from comment #4)
> Etienne, Marshall - Do we know what desktop/fennec does here? Are we sending
> data that users don't want to be sent, or should be anonymized?

TBH I opened this bug because I would feel safer with a clear check in app_usage_metrics.js.
Haven't actually checked that anything is leaking.

I just don't want to have to audit every window manager change to make sure we're not dispatching an event that shouldn't be part of metrics.

Comment 6

[Marshall Culpepper [:marshall_law]]

I don't think Desktop does any kind of application usage tracking.

I've taken a look at the code that Etienne linked again, and AFAICT places.js is only handling events that AppUsageMetrics doesn't, specifically:
* applocationchange
* apptitlechange
* appiconchange
* apploaded

AppUsageMetrics only pays attention to these events in the application lifecycle (outside of lock screen / homescreen events):
* appopened
* activitycreated
* attentionopened
* attentionclosed

I guess I'm mainly unclear whether/when an app is actually flagged as a private browser. We aren't tracking URL changes in apps, just the entry points when apps are launched, and any activity or attention windows that are opened from there. Is it plausible that an activity or attention window could be opened from a private browser?

Comment 7

[Kevin Grandon :kgrandon]

Ok, so it sounds like we are all clear from the app launch metrics. I think we can probably close this then as long as bug 1126524 did not regress this, and start sending navigation info when in a private browser. Just need to verify that first, I'll take a look.

Comment 8

[Kevin Grandon :kgrandon]

It looks like we might indeed record search info when in private browsers: https://github.com/mozilla-b2g/gaia/commit/0cd14e8b64d5e750588adacf4ad027aa90889648#diff-cc8fa85f73ada24b3dea2bce82e86e92R563

I guess the question is if this data is anonymized enough (I think it might be), that we are ok collecting it with private browsing searches? It also doesn't persist on the device in any viewable form, so I think we should be ok. We should hash this out with UX/policy people I suppose.

Comment 9

[Marshall Culpepper [:marshall_law]]

For the search metrics we are simply recording the number of searches done on a specific provider, we don't actually collect search terms themselves.

Comment 10

[Marshall Culpepper [:marshall_law]]

OK a quick update, I have a sample batch of app usage metrics that was collected when I tried opening a private browser window and searching within the private window here:
https://pastebin.mozilla.org/8825165

tl;dr We are counting searches performed in private windows (but again, not the search terms) and app usage looks to just be filed under search.gaiamobile.org.

Ideally I think under a private window, we shouldn't be counting searches. App usage in a private window looks the same as a regular window, seemingly pretty innocuous. Thoughts?

Comment 11

[Marshall Culpepper [:marshall_law]]

We just had a quick discussion about this, and concluded that we should not record app usage info or searches for private browsing windows. Looks like this already nomed for 2.2, so I'll try to get this fixed ASAP.

Comment 12

[Shing Lyu [:shinglyu]]

FC is coming (4/6), we need to be quick to land this.

Comment 13

[Shing Lyu [:shinglyu]]

Hi Marshall, could you provide some status update? Thanks.

Comment 14

[Marshall Culpepper [:marshall_law]]

Sorry guys, this fell off my radar.. I'll try to get it landed this week

Comment 15

[Shing Lyu [:shinglyu]]

Sorry for nagging, any update?

Comment 16

[Gregor Wagner [:gwagner]]

Any update here?

Comment 17

[GitHub Autolander Bot]

Attachment: [gaia] marshall:bug1134998_aumPrivateWindows > mozilla-b2g:master

Comment 18

[Marshall Culpepper [:marshall_law]]

Comment on attachment 8599533 [details] [review]
[gaia] marshall:bug1134998_aumPrivateWindows > mozilla-b2g:master

This should ensure that no events are collected for private browser windows..

Comment 19

[David Flanagan [:djf]]

Comment on attachment 8599533 [details] [review]
[gaia] marshall:bug1134998_aumPrivateWindows > mozilla-b2g:master

I think I found one error in your tests. And I had a couple of other suggestions on github. But overall this code is simple and pretty clearly correctly (Assuming that isPrivateApp() does what it should... I don't know anything at all about that API.)

Comment 20

[Marshall Culpepper [:marshall_law]]

Thanks for the suggestions, David! I've pushed my changes up and hope to have it landed in master shortly

Comment 21

[Marshall Culpepper [:marshall_law]]

I'll be pushing up a v2.2 PR and requesting approval once this has landed in master as well

Comment 22

[Etienne Segonzac (:etienne)]

Comment on attachment 8599533 [details] [review]
[gaia] marshall:bug1134998_aumPrivateWindows > mozilla-b2g:master

thanks!

Comment 23

[GitHub Autolander Bot]

Attachment: [gaia] marshall:bug1134998_aumPrivateWindows_v2.2 > mozilla-b2g:v2.2

Comment 24

[Marshall Culpepper [:marshall_law]]

Comment on attachment 8601626 [details] [review]
[gaia] marshall:bug1134998_aumPrivateWindows_v2.2 > mozilla-b2g:v2.2

This disables any sort of app usage data collection for private browsers. Before this patch, we were counting the minutes of use for the private browser under the same manifest URL as the browser app (search.gaiamobile..), which made the metrics hard to discern. With this patch, no usage app usage should be counted in the private browser, whether or not it's under the heading of 'search.gaiamobile..'

[Bug caused by] Bug 982663
[User impact] if declined: Usage time of private browser will be recorded, but will be indistinguishable from the standard browser app
[Testing completed]: unit tests, flame
[Risk to taking this patch] (and alternatives if risky): 
[String changes made]: none

Comment 25

[bhavana bajaj [:bajaj]]

Still waiting for this to land on master.

Also observed the risk isn't specified, but looking at the patch it looks low risk enough, can you confirm?

Comment 26

[bhavana bajaj [:bajaj]]

Also, the try run does not look happy, can you help check if that's known issue and not tests failures?

Comment 27

[Marshall Culpepper [:marshall_law]]

Argh, forgot to post this
master: https://github.com/mozilla-b2g/gaia/commit/d4a2f8785c69b78471155189fb7f891b6f50c602

Comment 28

[Marshall Culpepper [:marshall_law]]

it looks like all the test jobs were failing to load somehow, I'm going to rebase and trigger a fresh run

Comment 29

[Marshall Culpepper [:marshall_law]]

The failures here seem to be something going on with the runner, each GU test is retried 5 times before it ultimately fails, but the failure log doesn't mention anything about the actual unit tests.

Comment 30

[Josh Cheng [:josh]]

Comment on attachment 8601626 [details] [review]
[gaia] marshall:bug1134998_aumPrivateWindows_v2.2 > mozilla-b2g:v2.2

Approving this patch for landing on 2.2. PLease NI if the try run infra issue do not sort out and we have to back this out.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/e048df68f6f4853b5826a8816e143d95258149de