If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Send startup time measurements along with metadata request

VERIFIED FIXED in mozilla2.0b10

Status

()

Toolkit
Add-ons Manager
VERIFIED FIXED
7 years ago
5 years ago

People

(Reporter: mossop, Assigned: mossop)

Tracking

Trunk
mozilla2.0b10
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

7 years ago
Bug 522375 plans to expose simple information about startup performance to JS. We'd like to include this in the metadata request that we send to AMO in order to be able to gather aggregated data on startup performance with certain add-ons installed. I'm not sure that this constitutes much of an additional fingerprinting risk as the number will almost certainly change for every startup but we can discuss that issue.
This is hugely important for almost all of our plans this year to improve add-on performance, so definitely hope we can do it for Firefox 4.

For the parameter, I'd suggest just tacking on to the end of the ?src=firefox part, e.g. ?src=firefox&ts=1000

Comment 2

7 years ago
Will this involve changes to the privacy policy? The downside to this is that very privacy-sensitive people may turn off addon updates. That's why we have in the past decided not to send extra information with AUS pings, for example.
This is a separate ping from add-on updates; it's purely to update metadata displayed in the Add-ons Manager. We make it very easy to turn this checking off. I think it's fine to mention it in the privacy policy; if we determine this will land I'll work with Julie on adding a couple words to the existing sentence(s) on this ping.

Comment 4

7 years ago
Adding Alex Fowler to help on the PP side.

Comment 5

7 years ago
(In reply to comment #2)
> Will this involve changes to the privacy policy? The downside to this is that
> very privacy-sensitive people may turn off addon updates. That's why we have in
> the past decided not to send extra information with AUS pings, for example.

Seems like technically this is a simple change. Perhaps we should implement this and keep it pref()ed off while the privacy policy issues are worked out?

Updated

7 years ago
Blocks: 585196
(In reply to comment #5)
> Seems like technically this is a simple change. Perhaps we should implement
> this and keep it pref()ed off while the privacy policy issues are worked out?

I've filed legal bug 624591 for the minor change to the privacy policy and cc'd relevant parties from this bug (unfortunately legal bugs can't be removed from their security group). This PP update shouldn't block any forward progress on this bug though.
Depends on: 624591
As I'm not CC'ed on that; could you please ensure that a slightly more-general-than-just-startup concept, such as "gathering performance counters" concept, is addressed in the proposed PP change? If we have to have legal revise the PP for each and every counter, few counters will ever be added.
(In reply to comment #7)
> As I'm not CC'ed on that; could you please ensure that a slightly
> more-general-than-just-startup concept, such as "gathering performance
> counters" concept, is addressed in the proposed PP change? If we have to have
> legal revise the PP for each and every counter, few counters will ever be
> added.

The change is specific to the daily add-on ping, which I don't imagine will be the vehicle for all of the performance information we'd like to collect in the future. I think a more vague version is something that will need a larger fight and I'm focused on getting this in for Firefox 4. I think it's a worthy request but would rather it be separate from this small, focused effort.
One other request for this bug: this ping doesn't currently include the platform, and while we could get it from the user agent, it'd be better if it was included as another parameter. Thanks.

Comment 10

7 years ago
I am fine with that change.
(Assignee)

Comment 11

7 years ago
Created attachment 503617 [details] [diff] [review]
patch rev 1

WIP, needs tests
Assignee: nobody → dtownsend
Status: NEW → ASSIGNED

Comment 12

7 years ago
Comment on attachment 503617 [details] [diff] [review]
patch rev 1

>+      if (startupInfo.main)
>+        params.MAIN = startupInfo.main.getTime() - process;
I usually subtract date objects directly. Not sure what the benefit of the extra getTime call is.
(In reply to comment #12)
> Comment on attachment 503617 [details] [diff] [review]
> patch rev 1
> 
> >+      if (startupInfo.main)
> >+        params.MAIN = startupInfo.main.getTime() - process;
> I usually subtract date objects directly. Not sure what the benefit of the
> extra getTime call is.

Shouldn't be necessary if you know startupInfo.main is a Date instance. Don't know about perf diffs with JM, but with TM and the interpreter, I believe the implicit conversion is faster than .getTime().

/be
(Assignee)

Updated

7 years ago
Blocks: 626714
(Assignee)

Comment 14

7 years ago
Created attachment 504902 [details] [diff] [review]
patch rev 2

Haven't figured out a straightforward way to test this yet but at least we can start the review process
Attachment #503617 - Attachment is obsolete: true
Attachment #504902 - Flags: review?(bmcbride)
Comment on attachment 504902 [details] [diff] [review]
patch rev 2

>+pref("extensions.getAddons.get.url", "https://services.addons.mozilla.org/%LOCALE%/%APP%/api/%API_VERSION%/search/guid:%IDS%?src=firefox&appOS=%OS%&appVersion=%VERSION%&tMain=%MAIN%&tFirstPaint=%FIRST_PAINT%&tSessionRestored=%SESSION_RESTORED%");

%MAIN% seems a bit too generic - perhaps prefix these with "TIME_" or something? That'd also make it obvious that they're all related.

>+    let appStartup = Cc["@mozilla.org/toolkit/app-startup;1"].
>+                     getService(Ci.nsIAppStartup_MOZILLA_2_0);
>+    let startupInfo = appStartup.getStartupInfo();

Nit: No need to keep appStartup in a variable.

>-// By default, don't cache add-ons in AddonRepository.jsm
>-Services.prefs.setBoolPref("extensions.getAddons.cache.enabled", false);

Was this intentional? And if so, what's up with that?
Attachment #504902 - Flags: review?(bmcbride) → review-
(Assignee)

Comment 16

7 years ago
Created attachment 505128 [details] [diff] [review]
patch rev 3

Made those changes. The removal from head_addons.js is because that pref setting appears twice for some reason, I just happened across it when thinking about testing.
Attachment #504902 - Attachment is obsolete: true
Attachment #505128 - Flags: review?(bmcbride)
Comment on attachment 505128 [details] [diff] [review]
patch rev 3

WTB test.
Attachment #505128 - Flags: review?(bmcbride) → review+
(Assignee)

Comment 18

7 years ago
Created attachment 505473 [details] [diff] [review]
patch rev 4

That patch was actually broken, this one works and has a test
Attachment #505128 - Attachment is obsolete: true
Attachment #505473 - Flags: review?(bmcbride)
(Assignee)

Comment 19

7 years ago
Created attachment 505523 [details] [diff] [review]
patch rev 4

Forgot to add the test file
Attachment #505473 - Attachment is obsolete: true
Attachment #505523 - Flags: review?(bmcbride)
Attachment #505473 - Flags: review?(bmcbride)
Attachment #505523 - Flags: review?(bmcbride) → review+
(Assignee)

Comment 20

7 years ago
Comment on attachment 505523 [details] [diff] [review]
patch rev 4

Low risk patch that will give us very useful metrics to help guide us to finding potential add-on issues as well as just getting a good idea of our users startup metrics
Attachment #505523 - Flags: approval2.0?

Updated

7 years ago
Attachment #505523 - Flags: approval2.0? → approval2.0+
(Assignee)

Comment 21

7 years ago
Landed on trunk: http://hg.mozilla.org/mozilla-central/rev/4f296157ad3e
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b10
Blocks: 627907

Comment 22

7 years ago
I look forward to seeing some data about startup time across platforms.

I've noticed that the numbers are affected if you use the profile manager, and presumably will also be affected by things like the compatibilty update that occurs on an application version change. Something to bear in mind perhaps.
Daniel - do u want the metrics team to run queries to analyze startup times across platforms? If yes, can u file a separate bug with the details and adding it to me?
Thanks
We've got the data but I'm waiting on a couple things to happen before we make it public in aggregate graphs.
Verified with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b10) Gecko/20100101 Firefox/4.0b10

?src=firefox&appOS=Darwin&appVersion=4.0b10&tMain=661&tFirstPaint=3492&tSessionRestored=3555
Status: RESOLVED → VERIFIED
Blocks: 654053
You need to log in before you can comment on or make changes to this bug.