1296606 - Add telemetry to see how many profiles users have

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, defect, P3)

Comment 1

[Andrea Marchesini [:baku]]

This issue is not so easy to implement because, when a profile is created, often we are in the ProfileManager and at that point we are not running _into_ a profile yet. What we should do is add telemetry when the profile is opened for the first time, and at that point, we count the number of existing profiles.

Comment 2

[Andrea Marchesini [:baku]]

Attachment: telemetry.patch

Comment 3

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8783936 [details] [diff] [review]
telemetry.patch

Review of attachment 8783936 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me for the Telemetry usage, but i'm not sure if this is the right place to collect this data.
Bill, is this something you can review?

Note that you still need data collection review after the code review:
https://wiki.mozilla.org/Firefox/Data_Collection

::: toolkit/components/telemetry/Histograms.json
@@ +10210,5 @@
>      "cpp_guard": "ANDROID"
> +  },
> +  "NUMBER_OF_PROFILES": {
> +    "alert_emails": ["[e-mail]"],
> +    "expires_in_version": "never",

Do we need to collect this forever?
Or is it a enough to collect this data for some releases to answer your questions?

::: toolkit/xre/nsXREDirProvider.cpp
@@ +1188,5 @@
>  
> +    // Telemetry about number of profiles.
> +    nsCOMPtr<nsIToolkitProfileService> profileService =
> +      do_GetService("@mozilla.org/toolkit/profile-service;1");
> +    if (profileService) {

Will this code path be hit for different process types?
If yes, then you should avoid collecting this in more than one process, e.g. using `if (XRE_IsParentProcess()) ...`.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8783936 [details] [diff] [review]
telemetry.patch

Review of attachment 8783936 [details] [diff] [review]:
-----------------------------------------------------------------

I'm afraid I don't know this code at all. All I can say is that nsXREDirProvider::DoStartup is only called in the parent process. Maybe MattN can review? Otherwise bsmedberg.

Comment 5

[Matthew N. [:MattN]]

Please don't file bugs with an empty comment 0. Now I'm left wondering WHY we want to collect this information?

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

So why are we adding this telemetry probe? How will we use the data? We know that some people use profiles, but creating them happens probably very very rarely.
(I have plenty of profiles, but have perhaps created just one of them this year. Others are couple of years old, yet most of them are still being used occasionally).

Hmm, and looks like that patch counts all the profiles, but the bug summary is about creation. So what is this bug about?

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8783936 [details] [diff] [review]
telemetry.patch

(so since I'm not sure what we're supposed to do here and why, I can't really review this.)

Comment 8

[Andrea Marchesini [:baku]]

We want to know how many profiles people use (or have created). This is important to know how to prioritize the new profile manager UI.

> Hmm, and looks like that patch counts all the profiles, but the bug summary
> is about creation. So what is this bug about?

We cannot add telemetry when the profile is created because at that time we don't have a running profile (except if th creation happens via about:profiles, of course). So, the idea is to collect the number of profile at startup time, and send it as telemetry data.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

ok, could you then change the bug summary to reflect what we're trying to do here.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8783936 [details] [diff] [review]
telemetry.patch

It is still a bit unclear how we'll use the data. I assume 99.x% of users have just one profile.
I would assume profiles are used by power users only.

Comment 11

[Matthew N. [:MattN]]

(In reply to Andrea Marchesini [:baku])
> Flags: review?([e-mail])

In case it wasn't clear, I was still waiting on the answer to comment 5 for my review.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8783936 [details] [diff] [review]
telemetry.patch

Though, I think we should not collect this data for ever. Limit to a version or two of FF.

And is kind "count" really right here?
Per MDN "defaults to 0 and it can only be incremented by one with each add/accumulate call. "

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8783936 [details] [diff] [review]
telemetry.patch

So, please make this to be enabled for couple of FF versions only, and please explain why kind: count is right here. MDN documentation seems to hint it is not, but perhaps MDN is wrong?

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Sounds like the documentation is wrong:
chutten> smaugAfk: I believe we add that many. Let me check
chutten> smaugAfk: Yup. The only special handling for count histograms is to deal with the case when no args are provided. Then we assume 1.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8783936 [details] [diff] [review]
telemetry.patch

r+, if expires_in_version is limited to something reasonable soon happening release. (or explain why we need the data forever)

Comment 16

[Matthew N. [:MattN]]

Comment on attachment 8783936 [details] [diff] [review]
telemetry.patch

Review of attachment 8783936 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/Histograms.json
@@ +10213,5 @@
> +    "alert_emails": ["[e-mail]"],
> +    "expires_in_version": "never",
> +    "bug_numbers": [1296606],
> +    "kind": "count",
> +    "description": "Number of profiles."

Nit: you're only counting the number of named profiles (aka. ones that the profile service knows about) not the number of profiles used by the user. I regularly use `--profile /tmp/banking` (for example) and you're not counting that. Please make the description more verbose and accurate.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5062f9aaf645
Add telemetry to see how many profiles users have, r=MattN, r=bugs

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5062f9aaf645

Comment 19

[Georg Fritzsche [:gfritzsche]]

Please don't land new probes without data collection review:
https://wiki.mozilla.org/Firefox/Data_Collection

Comment 20

[Andrea Marchesini [:baku]]

Comment on attachment 8783936 [details] [diff] [review]
telemetry.patch

Francois, can you do a data-review here?

Comment 21

[François Marier [:francois]]

Comment on attachment 8783936 [details] [diff] [review]
telemetry.patch

Review of attachment 8783936 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/Histograms.json
@@ +10210,5 @@
>      "cpp_guard": "ANDROID"
> +  },
> +  "NUMBER_OF_PROFILES": {
> +    "alert_emails": ["[e-mail]"],
> +    "expires_in_version": "never",

If this is not needed forever, please make it expire in 57. It can be renewed later if it's still useful.

If it is needed permanently, what's the plan for permanent monitoring?

@@ +10213,5 @@
> +    "alert_emails": ["[e-mail]"],
> +    "expires_in_version": "never",
> +    "bug_numbers": [1296606],
> +    "kind": "count",
> +    "description": "Number of profiles."

The description needs to be more verbose. Is that count reported at startup? Is it restricted to named profiles as per MattN's comment?

e.g. "Number of named browser profiles for the current user, as reported by the profile service at startup."

Comment 22

[Andrea Marchesini [:baku]]

> If it is needed permanently, what's the plan for permanent monitoring?

Sorry for that: the patch I landed has expires_in_version 58.

Comment 23

[Andrea Marchesini [:baku]]

Attachment: description.patch

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/54ba4ea8bdc1
Fix the description for telemetry id NUMBER_OF_PROFILES, r=francois

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/54ba4ea8bdc1

Comment 26

[Bianca Danforth [:bdanforth]]

:baku, any idea why 11% of users had 0 profiles? chutten showed me this graph from Beta 53 https://mzl.la/2GPTfu2.

Comment 27

[Andrea Marchesini [:baku]]

I spent some time debugging why. Initially I though it was an error in GetProfileCount() but the code is simple enough:

https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/toolkit/profile/nsToolkitProfileService.cpp#857

and it doesn't return error.

So I checked what happens if firefox has never been executed on the current computer, where the NS_APP_USER_PROFILES_ROOT_DIR doesn't exist. Result: GetProfileCount() returns 0.
This means that, probably, 11% of the users have just executed firefox for the first time...
If this is true, it's a big 'wow'.