Closed Bug 1125858 Opened 10 years ago Closed 10 years ago

Add tests for telemetry collection

Categories

(Toolkit :: Password Manager, defect)

defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 1124516

People

(Reporter: Paolo, Assigned: Paolo)

References

Details

Attachments

(1 file)

Attached patch Work in progressSplinter Review
We should add tests for the telemetry we are collecting.

This is a work-in-progress patch where I've added tests and also tweaked the descriptions of the existing probes to make their behavior clearer.
Attachment #8554569 - Flags: feedback?(MattN+bmo)
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Comment on attachment 8554569 [details] [diff] [review]
Work in progress

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

::: toolkit/components/passwordmgr/test/unit/head.js
@@ +27,5 @@
>                                    "resource://gre/modules/Promise.jsm");
>  
> +XPCOMUtils.defineLazyServiceGetter(this, "gUUIDGenerator",
> +                                   "@mozilla.org/uuid-generator;1",
> +                                   "nsIUUIDGenerator");

For the avoidance of doubt, this is a leftover.
(In reply to :Paolo Amadini from comment #0)
> Created attachment 8554569 [details] [diff] [review]
> Work in progress
> 
> We should add tests for the telemetry we are collecting.

Why?

Are there any other areas of the code that test telemetry gathering of specific probes? This seems like a non-goal.
Comment on attachment 8554569 [details] [diff] [review]
Work in progress

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

Dolske and I discussed whether to add tests for telemetry last week and thought that it probably wasn't worth it. If you think bug 1124516 is complicated enough that it deserves a test then that's probably fine but I don't think the existing probes really need them. The time can be better spent fixing password manager IMO.

::: toolkit/components/passwordmgr/nsLoginManager.js
@@ +220,5 @@
> +     *        Current time used to calculate time-based statistics, expressed as
> +     *        the number of milliseconds since January 1, 1970, 00:00:00 UTC.
> +     *        This is set to a fake value during unit testing.
> +     */
> +    _gatherTelemetry : function (referenceTimeMs) {

I think we can add this argument in the patch which actually uses it.

::: toolkit/components/passwordmgr/test/unit/test_telemetry.js
@@ +78,5 @@
> + * Enable local telemetry recording for the duration of the tests, and prepare
> + * the test data that will be used by the following tests.
> + */
> +add_task(function test_initialize()
> +{

Nit: why are you switching to this different indentation for braces here. JS-style is on the same line as the `function` line.

::: toolkit/components/telemetry/Histograms.json
@@ +7173,4 @@
>    },
>    "PWMGR_SAVING_ENABLED": {
>      "expires_in_version": "never",
> +    "kind": "flag",

This will give the wrong result (0) for sessions where gather-telemetry didn't occur. This is why we intentionally chose boolean.

@@ +7178,5 @@
>    },
>    "PWMGR_USERNAME_PRESENT": {
>      "expires_in_version": "never",
>      "kind": "boolean",
> +    "description": "Stores 1 for each login that has a username and 0 for each that doesn't"

Since it's a boolean histogram, I think it's fairly obvious what 1 and 0 represent already…
Attachment #8554569 - Flags: feedback?(MattN+bmo) → feedback-
(In reply to Justin Dolske [:Dolske] from comment #2)
> Are there any other areas of the code that test telemetry gathering of specific probes?

Plenty:

http://mxr.mozilla.org/mozilla-central/search?string=getHistogramById&find=test

> Why?

To ensure in advance that we don't break the probes if an implementation detail changes - it's cheaper than fixing the probe later once we notice a discrepancy in the data. Example, we change the internal variable that tells us whether password saving is enabled.

But I believe the above argument applies to any regression test - a call can be made on a case-by-case basis on whether the time for writing the test is worth the benefit, or we're more likely to catch regressions in code review anyways.

(In reply to Matthew N. [:MattN] from comment #3)
> Dolske and I discussed whether to add tests for telemetry last week and
> thought that it probably wasn't worth it.  If you think bug 1124516 is
> complicated enough that it deserves a test then that's probably fine

I definitely wrote the new test file as a launchpad for bug 1124516. And I used the existing probes as a launchpad to write the test file.

> but I
> don't think the existing probes really need them. The time can be better
> spent fixing password manager IMO.

I agree that writing a test file just for the simple probes added last week was not required. However, as we're looking into more complex telemetry about password distribution, the tests become more useful, and at this point I don't see a reason not to include the one-liners that test and explain the existing probes anyways.

> ::: toolkit/components/passwordmgr/nsLoginManager.js
> I think we can add this argument in the patch which actually uses it.

So, possibly I should close this bug and work on everything together in bug 1124516.

> ::: toolkit/components/passwordmgr/test/unit/test_telemetry.js
> Nit: why are you switching to this different indentation for braces here.
> JS-style is on the same line as the `function` line.

Good catch, copy-and-paste from other tests. Should I sweep the whole directory and fix the style? I think it's better than having to remember to check this every time some code is copied.

> ::: toolkit/components/telemetry/Histograms.json
> @@ +7173,4 @@
> >    },
> >    "PWMGR_SAVING_ENABLED": {
> >      "expires_in_version": "never",
> > +    "kind": "flag",
> 
> This will give the wrong result (0) for sessions where gather-telemetry
> didn't occur. This is why we intentionally chose boolean.

Ah, I missed this, I thought "flag" was just a boolean capped at 1. Maybe I can add a comment to that effect.

> @@ +7178,5 @@
> >    },
> >    "PWMGR_USERNAME_PRESENT": {
> >      "expires_in_version": "never",
> >      "kind": "boolean",
> > +    "description": "Stores 1 for each login that has a username and 0 for each that doesn't"
> 
> Since it's a boolean histogram, I think it's fairly obvious what 1 and 0
> represent already…

Right, I was aligning the style with another similar histogram, but descriptions across Histograms.json are inconsistent anyways so might not be worth the trouble. I included this in the patch to get feedback, so thanks.
Moved to bug 1124516.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: