623950 - Send startup time measurements along with metadata request

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Dave Townsend [:mossop]]

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.

Comment 1

[Justin Scott [:fligtar]]

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

[Benjamin Smedberg]

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.

Comment 3

[Justin Scott [:fligtar]]

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

[Julie Martin]

Adding Alex Fowler to help on the PP side.

Comment 5

[(dormant account)]

(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?

Comment 6

[Justin Scott [:fligtar]]

(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.

Comment 7

[Graydon Hoare :graydon]

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.

Comment 8

[Justin Scott [:fligtar]]

(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.

Comment 9

[Justin Scott [:fligtar]]

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

[Julie Martin]

I am fine with that change.

Comment 11

[Dave Townsend [:mossop]]

Attachment: patch rev 1

WIP, needs tests

Comment 12

[(dormant account)]

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.

Comment 13

[Brendan Eich [:brendan]]

(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

Comment 14

[Dave Townsend [:mossop]]

Attachment: patch rev 2

Haven't figured out a straightforward way to test this yet but at least we can start the review process

Comment 15

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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?

Comment 16

[Dave Townsend [:mossop]]

Attachment: 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.

Comment 17

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 505128 [details] [diff] [review]
patch rev 3

WTB test.

Comment 18

[Dave Townsend [:mossop]]

Attachment: patch rev 4

That patch was actually broken, this one works and has a test

Comment 19

[Dave Townsend [:mossop]]

Attachment: patch rev 4

Forgot to add the test file

Comment 20

[Dave Townsend [:mossop]]

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

Comment 21

[Dave Townsend [:mossop]]

Landed on trunk: http://hg.mozilla.org/mozilla-central/rev/4f296157ad3e

Comment 22

[Daniel C]

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.

Comment 23

[Anurag Phadke[:[e-mail]]]

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

Comment 24

[Justin Scott [:fligtar]]

We've got the data but I'm waiting on a couple things to happen before we make it public in aggregate graphs.

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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