Closed Bug 756142 Opened 8 years ago Closed 8 years ago

Report shutdown times via telemetry

Categories

(Toolkit :: Telemetry, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: espindola, Assigned: froydnj)

References

Details

Attachments

(4 files)

No description provided.
OS: Mac OS X → All
Hardware: x86 → All
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.
Attachment #640619 - Flags: review?(benjamin)
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.
Attachment #640620 - Flags: review?(benjamin)
Now that all of the complicated stuff is out of the way, sending the bits is pretty simple.
Attachment #640621 - Flags: review?(taras.mozilla)
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
Attachment #640620 - Flags: review-
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.
Attachment #640621 - Flags: review?(taras.mozilla) → review+
Attachment #640619 - Flags: review?(benjamin) → review+
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).
Attachment #640620 - Flags: review?(benjamin)
Non-throwing version.
Attachment #640773 - Flags: review?(benjamin)
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
Attachment #640773 - Flags: review?(benjamin) → review+
https://hg.mozilla.org/mozilla-central/rev/c80ee711a679
https://hg.mozilla.org/mozilla-central/rev/a16a7d6e15f0
https://hg.mozilla.org/mozilla-central/rev/6756d15958fe
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Depends on: 779298
You need to log in before you can comment on or make changes to this bug.