shutdownduration is not reported

RESOLVED FIXED in mozilla17

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: taras.mozilla, Assigned: froydnj)

Tracking

unspecified
mozilla17
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Snappy])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/TelemetryPing.js#138 is landed yet,
there is no .shutdownDuration in my saved-telemetry-pings dir.

I also have Telemetry.ShutdownTime.txt, so the flaw is somewhere in bug 756142. There is also no data coming through telemetry dashboard, so it's likely I'm not the only one affected.
(Reporter)

Updated

6 years ago
Whiteboard: [Snappy]
(Assignee)

Comment 1

6 years ago
What are the contents of your Telemetry.ShutdownTime.txt file?
(Reporter)

Comment 2

6 years ago
(In reply to Nathan Froyd (:froydnj) from comment #1)
> What are the contents of your Telemetry.ShutdownTime.txt file?

"10249"
(Assignee)

Comment 3

6 years ago
Created attachment 647974 [details] [diff] [review]
part 1 - store file in the correct location

For whatever reason, we were storing the file in the prefs directory, rather than the profile directory.  I don't fully understand the ins and outs here, as the two directories are the same (?), but switching makes it more consistent with the rest of the codebase and enables us to actually test this stuff.

I think I can harass Doug as a Toolkit peer for review on this one.
Attachment #647974 - Flags: review?(doug.turner)
(Assignee)

Comment 4

6 years ago
Created attachment 647975 [details] [diff] [review]
part 2 - fix fetching the value

...and the thinko part: it's not the startupInfo that holds the field, it's the actual service.  Fix that, and add a test to try and ensure we don't screw up later.
Attachment #647975 - Flags: review?(taras.mozilla)
(Assignee)

Updated

6 years ago
OS: Windows 7 → All
Hardware: x86_64 → All

Comment 5

6 years ago
Comment on attachment 647974 [details] [diff] [review]
part 1 - store file in the correct location

Yup.  Caused by bug 756142.  Nice catch. :)
Attachment #647974 - Flags: review?(doug.turner) → review+
(Reporter)

Comment 6

6 years ago
Comment on attachment 647975 [details] [diff] [review]
part 2 - fix fetching the value

What about the fact that my shutdowntime.txt contains 10249 VS 1919251566 value returned by lastShutdownDuration
(Reporter)

Comment 7

6 years ago
Comment on attachment 647975 [details] [diff] [review]
part 2 - fix fetching the value

+  do_check_true(payload.simpleMeasurements.shutdownDuration == SHUTDOWN_TIME);
that should be do_check_eq
Attachment #647975 - Flags: review?(taras.mozilla) → review+
(Reporter)

Comment 8

6 years ago
Comment on attachment 647974 [details] [diff] [review]
part 1 - store file in the correct location

I actually think we should store the file in saved-telemetry-pings directory. Would make sense to group all of the weird telemetry-motivated data together.
(Assignee)

Comment 9

6 years ago
(In reply to Taras Glek (:taras) from comment #6)
> What about the fact that my shutdowntime.txt contains 10249 VS 1919251566
> value returned by lastShutdownDuration

That's just weird.  I don't know what's going on with your machine.  Your machine is Windows?  Should do a try push tomorrow just to make sure this isn't going to break something.

(In reply to Taras Glek (:taras) from comment #8)
> I actually think we should store the file in saved-telemetry-pings
> directory. Would make sense to group all of the weird telemetry-motivated
> data together.

Not a bad idea, but it would require some tweaking to how we enumerate pings and I'd rather not do that here.  If you want to file a followup bug, we can discuss.
(Assignee)

Comment 10

6 years ago
For reasons I do not understand, the new test is consistently failing everywhere:

https://tbpl.mozilla.org/?tree=Try&rev=5d487fda7200

even though it passes just fine locally.
(Assignee)

Comment 11

6 years ago
Even after being a little more careful with safe file output, things are still falling over:

https://tbpl.mozilla.org/?tree=Try&rev=4ea59ed6b766

Notice especially the WinXP log, which seems to show problems similar to comment 6.

This test also seems to make bug 764030 happen quite a bit more often.
OS: All → Windows 7
Hardware: All → x86_64
(Assignee)

Updated

6 years ago
OS: Windows 7 → All
Hardware: x86_64 → All
(Assignee)

Comment 12

6 years ago
Created attachment 652438 [details] [diff] [review]
part 1 - store file in the correct location; brownbag fix

Testing revealed that I had forgotten to initialize the new member variables added in bug 756142.  I think this is obvious/brownbaggish enough that I'm just rolling that fix into part 1, sans re-review.
Attachment #647974 - Attachment is obsolete: true
Attachment #652438 - Flags: review+
bah, s/&onlyunstarred=1//
(Assignee)

Comment 16

6 years ago
Created attachment 652483 [details] [diff] [review]
fix test bustage

I believe this patch fixes the bustage seen on Inbound; it's currently running on Try and seems plausible enough.  I forgot that M-oth tests touch Telemetry and should have been exercised prior to committing.

The problem is that recording the timestamp assumes that if the filename of the timestamp file has been computed, then that must mean that we recorded a start time.  However, if we have:

- a build without telemetry enabled;
- we access lastShutdownDuration (thereby computing the filename to try and read the file;
- we shutdown;

then we try to compute the different between TimeStamp::Now() and the start time.  Except that the start time is a null TimeStamp, because we never set it earlier.  And kaboom on debug builds.

This patch is one way to fix that.  The other is adding an explicit flag to say "yes, we recorded a start time" rather than relying on the filename to serve as such.
Attachment #652483 - Flags: review?(doug.turner)
(Reporter)

Updated

6 years ago
Attachment #652483 - Flags: review?(doug.turner) → review+
https://hg.mozilla.org/mozilla-central/rev/28944cefdd3c
https://hg.mozilla.org/mozilla-central/rev/497806930eb6
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.