Convert Addon Manager telemetry timings to use Components.utils.now()

RESOLVED FIXED in mozilla35

Status

()

Toolkit
Add-ons Manager
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: Irving, Assigned: Manu Jain, Mentored)

Tracking

unspecified
mozilla35
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=js][good first bug])

Attachments

(1 attachment, 1 obsolete attachment)

Bug 969490 gave us a better time base to use for interval measurements, we should switch to it for our telemetry time interval measurements in the Addon Manager, XPI Provider, etc. Anywhere we record two "new Date()" or "Date.now()" values and subtract them, we should switch to Components.utils.now().

Note that Components.utils.now() returns floating point milliseconds with microsecond precision, and our existing Telemetry expects an integer milliseconds, so the time interval should be rounded with Math.round()

Comment 1

4 years ago
I am new to bugzilla and I will like to work on this bug. I have setup mozilla central. Can you give me further instructions ?
Flags: needinfo?(irving)
Thanks for taking a look at this. I may have set this bug for mentoring a bit too soon, because I'm in the middle of making changes to some of the Addon Manager files, in ways that overlap with what needs to be fixed for this bug. You're welcome to go ahead and work on this, but it might be easier to work on bug 1063559, which is the same sort of change but in a different module.

Once you can build and run your own copy of Firefox (see the instructions at https://developer.mozilla.org/en/docs/Simple_Firefox_build), the files that need to be changed are in the toolkit/mozapps/extensions directory of mozilla-central.

In toolkit/mozapps/extensions/AddonManager.jsm we use Date.now() to calculate a time interval in the 'simpleTimer' method (around line 2433). The Date.now() calls need to be replaced with Components.utils.now() and the calculated time interval needs to be rounded as described in comment 1.

toolkit/mozapps/extensions/internal/XPIProvider.jsm is the other file in this module where we calculate time intervals to record as Telemetry measurements, but I'm in the middle of changing all of the places we do this, as part of bug 1049142, so you should wait until those changes have been made before editing XPIProvider.jsm.
Flags: needinfo?(irving)

Comment 3

4 years ago
Ok,cool
No problem

Thanks for a good suggestion
The bug that was blocking this has now landed, so if someone wants to start working on this it's ready to go.
Depends on: 1049142
(Assignee)

Comment 5

4 years ago
Created attachment 8491462 [details] [diff] [review]
bug1063561.patch

I've attached the patch. Please check it. Thanks!!
Attachment #8491462 - Flags: review?(irving)
Assignee: nobody → manu.jain13
Status: NEW → ASSIGNED
Comment on attachment 8491462 [details] [diff] [review]
bug1063561.patch

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

::: toolkit/mozapps/extensions/AddonManager.jsm
@@ +2430,5 @@
>  
>    // Start a timer, record a simple measure of the time interval when
>    // timer.done() is called
>    simpleTimer: function(aName) {
> +    let startTime = Components.utils.now();

I didn't say this in the bug description, but in most Firefox JS code we define a shortcut for Components.utils; see http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/AddonManager.jsm?from=AddonManager.jsm#10

so throughout this patch you can use Cu.now() instead of Components.utils.now()

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm
@@ +2215,5 @@
>     */
>    getAddonStates: function XPI_getAddonStates(aLocation) {
>      let addonStates = {};
>      for (let file of aLocation.addonLocations) {
> +      let scanStarted = Components.utils.now();

Oops, you must have worked from a copy of mozilla-central that wasn't updated with the changes from bug 1049142 yet. You'll need to undo the changes in XPIProvider.jsm, merge in the latest mozilla-central changes, and then modify the new places where Date.now() is used. If you need help with hg to get through that, it's best to ask on IRC in irc.mozilla.org:#introduction
Attachment #8491462 - Flags: review?(irving) → review-
(Assignee)

Comment 7

4 years ago
Created attachment 8491697 [details] [diff] [review]
bug_1063561_v2.patch

All changes made.
Attachment #8491462 - Attachment is obsolete: true
Attachment #8491697 - Flags: review?(irving)
Comment on attachment 8491697 [details] [diff] [review]
bug_1063561_v2.patch

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

Thanks for the patch!
Attachment #8491697 - Flags: review?(irving) → review+
https://hg.mozilla.org/integration/fx-team/rev/92d6f9e6cbc3
Whiteboard: [lang=js][good first bug] → [lang=js][good first bug][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/92d6f9e6cbc3
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [lang=js][good first bug][fixed-in-fx-team] → [lang=js][good first bug]
Target Milestone: --- → mozilla35
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.