Closed Bug 1134998 Opened 6 years ago Closed 6 years ago
App Usage Metrics should ignore all events from private windows
46 bytes, text/x-github-pull-request
|Details | Review|
46 bytes, text/x-github-pull-request
|Details | Review|
Similiraly to what places.js is doing here   https://github.com/mozilla-b2g/gaia/blob/f3ccc4c0edb0e0ebc9ea0bac3bb0ef4799554651/apps/system/js/places.js#L121-123
Hrm.. is it possible to load a private browser directly from the launcher?
Francis, what are your thoughts on this?
Metrics is a committed 2.2 feature. Needs verification on how this works on desktop
blocking-b2g: 2.2? → 2.2+
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?
(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.
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?
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.
Assignee: nobody → kgrandon
Status: NEW → ASSIGNED
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.
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.
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?
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.
Assignee: kgrandon → marshall
FC is coming (4/6), we need to be quick to land this.
Hi Marshall, could you provide some status update? Thanks.
Sorry guys, this fell off my radar.. I'll try to get it landed this week
Sorry for nagging, any update?
Any update here?
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 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.)
Attachment #8599533 - Flags: review?(dflanagan) → review+
Thanks for the suggestions, David! I've pushed my changes up and hope to have it landed in master shortly
I'll be pushing up a v2.2 PR and requesting approval once this has landed in master as well
Comment on attachment 8599533 [details] [review] [gaia] marshall:bug1134998_aumPrivateWindows > mozilla-b2g:master thanks!
Attachment #8599533 - Flags: feedback?(etienne) → feedback+
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
Attachment #8601626 - Flags: approval-gaia-v2.2?
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?
Also, the try run does not look happy, can you help check if that's known issue and not tests failures?
Argh, forgot to post this master: https://github.com/mozilla-b2g/gaia/commit/d4a2f8785c69b78471155189fb7f891b6f50c602
it looks like all the test jobs were failing to load somehow, I'm going to rebase and trigger a fresh run
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.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
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.
Attachment #8601626 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
You need to log in before you can comment on or make changes to this bug.