Closed Bug 1296606 Opened 3 years ago Closed 3 years ago

Add telemetry to see how many profiles users have

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: baku, Assigned: baku)

Details

Attachments

(2 files)

No description provided.
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.
Attached patch telemetry.patchSplinter Review
Attachment #8783936 - Flags: review?(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": ["amarchesini@mozilla.com"],
> +    "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()) ...`.
Attachment #8783936 - Flags: review?(wmccloskey)
Attachment #8783936 - Flags: review?(gfritzsche)
Attachment #8783936 - Flags: feedback+
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.
Attachment #8783936 - Flags: review?(wmccloskey) → review?(MattN+bmo)
Please don't file bugs with an empty comment 0. Now I'm left wondering WHY we want to collect this information?
Priority: -- → P3
Attachment #8783936 - Flags: review?(bugs)
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 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.)
Attachment #8783936 - Flags: review?(bugs)
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.
Flags: needinfo?(bugs)
ok, could you then change the bug summary to reflect what we're trying to do here.
Flags: needinfo?(bugs)
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.
Attachment #8783936 - Flags: review+
(In reply to Andrea Marchesini [:baku])
> Flags: review?(bugs@pettay.fi)

In case it wasn't clear, I was still waiting on the answer to comment 5 for my review.
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. "
Attachment #8783936 - Flags: review+
Summary: Add telemetry to see how many profiles are created and how often. → Add telemetry to see how many profiles users have
Attachment #8783936 - Flags: review?(bugs)
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?
Attachment #8783936 - Flags: review?(bugs) → review-
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 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)
Attachment #8783936 - Flags: review- → review+
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": ["amarchesini@mozilla.com"],
> +    "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.
Attachment #8783936 - Flags: review?(MattN+bmo) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5062f9aaf645
Add telemetry to see how many profiles users have, r=MattN, r=bugs
https://hg.mozilla.org/mozilla-central/rev/5062f9aaf645
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Please don't land new probes without data collection review:
https://wiki.mozilla.org/Firefox/Data_Collection
Flags: needinfo?(amarchesini)
Comment on attachment 8783936 [details] [diff] [review]
telemetry.patch

Francois, can you do a data-review here?
Flags: needinfo?(amarchesini)
Attachment #8783936 - Flags: review?(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": ["amarchesini@mozilla.com"],
> +    "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": ["amarchesini@mozilla.com"],
> +    "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."
Attachment #8783936 - Flags: review?(francois) → review-
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
> If it is needed permanently, what's the plan for permanent monitoring?

Sorry for that: the patch I landed has expires_in_version 58.
Attachment #8789058 - Flags: review?(francois)
Attachment #8789058 - Flags: review?(francois) → review+
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/54ba4ea8bdc1
Fix the description for telemetry id NUMBER_OF_PROFILES, r=francois
https://hg.mozilla.org/mozilla-central/rev/54ba4ea8bdc1
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
:baku, any idea why 11% of users had 0 profiles? chutten showed me this graph from Beta 53 https://mzl.la/2GPTfu2.
Flags: needinfo?(amarchesini)
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'.
Flags: needinfo?(amarchesini)
You need to log in before you can comment on or make changes to this bug.