Startup numbers don't account for interactive startup interruptions

RESOLVED FIXED in mozilla9

Status

()

Toolkit
General
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: (dormant account), Assigned: Margaret)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs, {dev-doc-complete})

Trunk
mozilla9
x86
Linux
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
Since startup numbers do not make sense when interrupted by interactive processes, we either need to add a .interactive component or a flag so we can track these.

Given how startups take minutes due to interruptions, it might even make sense to change when those prompts appear(ie make them happen after sessionrestore).

Interruptions:
* profile manager
* extensions update manager
* windows updater UAC
* update process is currently counted as part of startup

Fixing this should allow us to get a useful startup metric
(Reporter)

Comment 1

6 years ago
Margaret, do you have an ETA on this?
(Assignee)

Comment 2

6 years ago
I am starting to look into this today. Do we want to start by just excluding these interrupted startups from our startup metrics, or jump right into trying to change when we present these prompts? It seems like we wouldn't be able to move some of them, like the profile manager, so I guess we would still need a way to exclude this from startup metrics.

Taras, do you have any pointers to where/how we measure startup time? I'm unfamiliar with this, so any random resources would be helpful.
(Reporter)

Comment 3

6 years ago
Actually this is easier than that. We just need to set a flag some where *startup was interrupted*. No need to try to compensate for it.
(Assignee)

Comment 4

6 years ago
Okay, that seems straight forward enough. Do we want to set this flag on the telemetry service? Or is there another place you were thinking of storing it?
(Reporter)

Comment 5

6 years ago
(In reply to Margaret Leibovic [:margaret] from comment #4)
> Okay, that seems straight forward enough. Do we want to set this flag on the
> telemetry service? Or is there another place you were thinking of storing it?

If you cant think of a better place nsITelemetry sounds good.
(Reporter)

Comment 6

6 years ago
(In reply to Taras Glek (:taras) from comment #5)
> (In reply to Margaret Leibovic [:margaret] from comment #4)
> > Okay, that seems straight forward enough. Do we want to set this flag on the
> > telemetry service? Or is there another place you were thinking of storing it?
> 
> If you cant think of a better place nsITelemetry sounds good.

Sorry, I forgot the obvious interface for this: nsIAppStartup.interrupted = true would be a good way to go here
(Assignee)

Comment 7

6 years ago
Created attachment 554475 [details] [diff] [review]
possible patch for extensions update dialog

I started with the extensions update dialog. Is this the right idea?

I'm also unsure how I should test this. I was looking around for a possible place to add a test in the existing extensions update test code, but I didn't see an obvious place. Dave, do you know if there's an easy way to add a test for this?
Assignee: nobody → margaret.leibovic
Status: NEW → ASSIGNED
Attachment #554475 - Flags: feedback?(tglek)
Attachment #554475 - Flags: feedback?(dtownsend)
(Reporter)

Comment 8

6 years ago
Comment on attachment 554475 [details] [diff] [review]
possible patch for extensions update dialog

This is exactly what I'm looking for. Please use Cc/Ci.
Attachment #554475 - Flags: feedback?(tglek) → feedback+
Comment on attachment 554475 [details] [diff] [review]
possible patch for extensions update dialog

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

::: toolkit/components/startup/public/nsIAppStartup.idl
@@ +138,5 @@
> +
> +    /**
> +     * True if startup was interrupted by an interactive process.
> +     */
> +    attribute boolean interrupted;

Seem to be missing an implementation for this method, however I'm not sure I agree that nsIAppStartup is the right place for this. This seems like something very specific to the telemetry system so I'd be much more inclined to put it there. Was there some other reason you thought of putting it here Taras?

::: toolkit/mozapps/extensions/content/update.js
@@ +85,5 @@
>        document.documentElement.currentPage = document.getElementById("versioninfo");
> +
> +    let appService = Components.classes["@mozilla.org/toolkit/app-startup;1"].
> +                     getService(Components.interfaces.nsIAppStartup);
> +    appService.interrupted = true;

I would probably do this in XPIProvider.jsm where we open this as there are now two different dialogs there that can be opened on startup.
Attachment #554475 - Flags: feedback?(dtownsend) → feedback-
(Reporter)

Comment 10

6 years ago
(In reply to Dave Townsend (:Mossop) from comment #9)
> Comment on attachment 554475 [details] [diff] [review]
> possible patch for extensions update dialog
> 
> Review of attachment 554475 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/startup/public/nsIAppStartup.idl
> @@ +138,5 @@
> > +
> > +    /**
> > +     * True if startup was interrupted by an interactive process.
> > +     */
> > +    attribute boolean interrupted;
> 
> Seem to be missing an implementation for this method, however I'm not sure I
> agree that nsIAppStartup is the right place for this. This seems like
> something very specific to the telemetry system so I'd be much more inclined
> to put it there. Was there some other reason you thought of putting it here
> Taras?

It makes sense to me because .interrupted is needed to make sure of .firstPaint and .sessionRestored numbers provided nsIAppStartup.
(In reply to Taras Glek (:taras) from comment #10)
> (In reply to Dave Townsend (:Mossop) from comment #9)
> > Comment on attachment 554475 [details] [diff] [review]
> > possible patch for extensions update dialog
> > 
> > Review of attachment 554475 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: toolkit/components/startup/public/nsIAppStartup.idl
> > @@ +138,5 @@
> > > +
> > > +    /**
> > > +     * True if startup was interrupted by an interactive process.
> > > +     */
> > > +    attribute boolean interrupted;
> > 
> > Seem to be missing an implementation for this method, however I'm not sure I
> > agree that nsIAppStartup is the right place for this. This seems like
> > something very specific to the telemetry system so I'd be much more inclined
> > to put it there. Was there some other reason you thought of putting it here
> > Taras?
> 
> It makes sense to me because .interrupted is needed to make sure of
> .firstPaint and .sessionRestored numbers provided nsIAppStartup.

Ah I see, that makes more sense. I'm fine with that then.
(Assignee)

Comment 12

6 years ago
Created attachment 555543 [details] [diff] [review]
patch attempt 2

I addressed the comments. I still don't have any tests, so if you have ideas about that, I'd love to hear :)
Attachment #554475 - Attachment is obsolete: true
Attachment #555543 - Flags: feedback?(dtownsend)
(Reporter)

Comment 13

6 years ago
Do any of the dialogs that interrupt startup have tests?
Comment on attachment 555543 [details] [diff] [review]
patch attempt 2

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

Isn't this patch also meant to be doing something when nsIAppStartup.interrupted is true, like not including the perf numbers?
(In reply to Taras Glek (:taras) from comment #13)
> Do any of the dialogs that interrupt startup have tests?

Yes, the xpcshell tests check that the add-ons manager attempts to open those dialog so they could also test that the startup timing numbers no longer exist in those cases
(Reporter)

Comment 16

6 years ago
(In reply to Dave Townsend (:Mossop) from comment #15)
> (In reply to Taras Glek (:taras) from comment #13)
> > Do any of the dialogs that interrupt startup have tests?
> 
> Yes, the xpcshell tests check that the add-ons manager attempts to open
> those dialog so they could also test that the startup timing numbers no
> longer exist in those cases

The plan is to still report startup numbers(at least for telemetry). We'd also send a .startupInterrupted metric along that would reflect value of nsIAppStartup.interrupted. I was planning to do that patch in a follow up.

Margaret, please add do_check_true(nsIAppStartup.interrupted) to xpcshell tests for addon manager.
(Assignee)

Comment 17

6 years ago
Created attachment 555794 [details] [diff] [review]
patch w/ test

Gavin suggested I add startup to Services.jsm, so I did that.
Attachment #555543 - Attachment is obsolete: true
Attachment #555794 - Flags: review?(dtownsend)
Attachment #555543 - Flags: feedback?(dtownsend)
(Reporter)

Comment 18

6 years ago
Comment on attachment 555794 [details] [diff] [review]
patch w/ test

By the way, this only takes care of addon manager, what about profile chooser?

There is also the windows UAC upgrade prompt(I can add that).
Looks basically ok, one thought I'd like to hear about though is whether we can make this more automatic. If nsAppStartup listened for the window observer notificatons (I think it already does) then it could detect windows opening during startup and just set the interrupted flag manually. This would guarantee we caught every case straight off the bat and allow us to make that property readonly. Makes testing marginally more difficult but not by much.
Comment on attachment 555794 [details] [diff] [review]
patch w/ test

Discussed over IRC, probably not worth the automated approach for the small number of dialogs we have. Maybe we'll revisit that if there are many that we're forgetting.
Attachment #555794 - Flags: review?(dtownsend) → review+
(Assignee)

Comment 21

6 years ago
(In reply to Taras Glek (:taras) from comment #18)
> By the way, this only takes care of addon manager, what about profile
> chooser?

Sorry, I was just trying to start with the most common dialog, then lap back to covering others. I can do that now.
(Assignee)

Comment 22

6 years ago
Created attachment 557626 [details] [diff] [review]
patch for profile manager

I wrote this patch for the profile manager, but I'm not sure if it's what we want.

I used dump statements near my changes to make sure they were actually running, but when I manually checked the value of Services.startup.interrupted after the browser launched, it wasn't set to true. I think the browser restarts after a profile is selected, so I don't think that time spent interacting with this dialog would be counted in startup time. Am I right about this? However, is the startup of just the profile manager still being tracked by some telemetry probes?
(Assignee)

Comment 23

6 years ago
I landed my first patch in fx-team, since it's ready:
http://hg.mozilla.org/integration/fx-team/rev/8ef08b003e21

Taras, any insight into what I ran into in comment 22?
Whiteboard: [fixed-in-fx-team]
(Reporter)

Comment 24

6 years ago
(In reply to Margaret Leibovic [:margaret] from comment #22)
> Created attachment 557626 [details] [diff] [review]
> patch for profile manager
> 
> I wrote this patch for the profile manager, but I'm not sure if it's what we
> want.
> 
> I used dump statements near my changes to make sure they were actually
> running, but when I manually checked the value of
> Services.startup.interrupted after the browser launched, it wasn't set to
> true. I think the browser restarts after a profile is selected, so I don't
> think that time spent interacting with this dialog would be counted in
> startup time. Am I right about this? However, is the startup of just the
> profile manager still being tracked by some telemetry probes?

Yeah, I suspect you are right. The way I deal with this is set MOZ_APP_RESTART in nsAppStartup. You could add another env variable or commandline flag or something.
(Assignee)

Comment 25

6 years ago
Landed the first patch on mozilla-central:
https://hg.mozilla.org/mozilla-central/rev/8ef08b003e21
Whiteboard: [fixed-in-fx-team]

Comment 26

6 years ago
Mozilla/5.0 (X11; Linux i686; rv:9.0a1) Gecko/20110915 Firefox/9.0a1

(In reply to Taras Glek (:taras) from comment #24)
> (In reply to Margaret Leibovic [:margaret] from comment #22)
> > Created attachment 557626 [details] [diff] [review]
> > patch for profile manager
> > 
> > I wrote this patch for the profile manager, but I'm not sure if it's what we
> > want.
> > 
> > I used dump statements near my changes to make sure they were actually
> > running, but when I manually checked the value of
> > Services.startup.interrupted after the browser launched, it wasn't set to
> > true. I think the browser restarts after a profile is selected, so I don't
> > think that time spent interacting with this dialog would be counted in
> > startup time. Am I right about this? However, is the startup of just the
> > profile manager still being tracked by some telemetry probes?
> 
> Yeah, I suspect you are right. The way I deal with this is set
> MOZ_APP_RESTART in nsAppStartup. You could add another env variable or
> commandline flag or something.

1. At the profile manager, pick a profile really quickly.
2. Check about:startup.
3. Quit and start again.
4. Wait 30 seconds at the profile manager and then pick a profile.
5. Check about:startup.

Step 2 gives:

main    sessionRestored firstPaint      version appBuildID
1534    4597            6348            9.0a1   20110915030845

Step 5 gives:

main    sessionRestored firstPaint      version appBuildID
29569   32474           32750           9.0a1   20110915030845

Comment 27

6 years ago
https://developer.mozilla.org/en/nsIAppStartup should be updated.
Keywords: dev-doc-needed
Version: unspecified → Trunk
(Reporter)

Updated

6 years ago
Blocks: 690585
(Assignee)

Updated

6 years ago
Depends on: 691774
(Assignee)

Comment 28

6 years ago
I haven't had time to work on this lately, so I'm just going to close this bug. I filed bug 691774 as a follow-up.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
(Reporter)

Updated

6 years ago
Blocks: 691979
Documented:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIAppStartup

Also mentioned on Firefox 9 for developers.
Keywords: dev-doc-needed → dev-doc-complete
(Reporter)

Updated

6 years ago
Blocks: 701872
(Reporter)

Updated

5 years ago
Blocks: 725401
You need to log in before you can comment on or make changes to this bug.