756142 - Report shutdown times via telemetry

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: part 1 - more persistent shutdown filename

I'm choosing to expose the shutdown data through nsIAppStartup instead of groveling in the file directly in Telemetry.  This way we only need knowledge about the filename is one place, rather than widely separated places.  It does have the downside of being somewhat more complicated.  Feel free to r- and tell me to do it the dirt-simple way if you don't like the complexity.

The current way the shutdown filename is generated is only suitable for shutdown times; for exposing this data through telemetry, we need it to stick around longer.  Make that possible first.

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: part 2 - expose shutdown duration through nsIAppStartup

The caching is in case we request the time after the next shutdown timestamp has been written.  Don't know if it's possible for Telemetry to do that, but figured I'd better play it safe.

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: part 3 - send the shutdown duration

Now that all of the complicated stuff is out of the way, sending the bits is pretty simple.

Comment 4

[(dormant account)]

Comment on attachment 640620 [details] [diff] [review]
part 2 - expose shutdown duration through nsIAppStartup

I think it'd be better if this returns null on failure. Exposing throwy APIs is a recipe for someone to forget an exception handler

Comment 5

[(dormant account)]

Comment on attachment 640621 [details] [diff] [review]
part 3 - send the shutdown duration

Take out the try/catch. turn it into if(si.lastShutdownDurection)...

r+ with that...Alternatively is bsmedberg prefers exceptions(I hope he does not), this is ok too.

Comment 6

[Benjamin Smedberg]

Comment on attachment 640620 [details] [diff] [review]
part 2 - expose shutdown duration through nsIAppStartup

Yeah, Taras is right, return 0 on failure instead of throwing (and add that to the IDL docs).

Comment 7

[Nathan Froyd [:froydnj]]

Attachment: part 2 - expose shutdown duration through nsIAppStartup

Non-throwing version.

Comment 8

[Benjamin Smedberg]

Comment on attachment 640773 [details] [diff] [review]
part 2 - expose shutdown duration through nsIAppStartup

either mLastShutdownTime needs to be "int" instead of "PRUint32" or you need to use a temporary int and then assign it. r=me with that change

Comment 9

[Nathan Froyd [:froydnj]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/c80ee711a679
http://hg.mozilla.org/integration/mozilla-inbound/rev/a16a7d6e15f0
http://hg.mozilla.org/integration/mozilla-inbound/rev/6756d15958fe

Comment 10

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/c80ee711a679
https://hg.mozilla.org/mozilla-central/rev/a16a7d6e15f0
https://hg.mozilla.org/mozilla-central/rev/6756d15958fe