Last Comment Bug 748517 - use JSON for the persistent telemetry save format
: use JSON for the persistent telemetry save format
Status: RESOLVED FIXED
:
Product: Toolkit
Classification: Components
Component: Telemetry (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Nathan Froyd [:froydnj]
:
: Georg Fritzsche [:gfritzsche]
Mentors:
Depends on:
Blocks: 715299 742539 748520 763113 763525 763526 764513
  Show dependency treegraph
 
Reported: 2012-04-24 13:26 PDT by Nathan Froyd [:froydnj]
Modified: 2012-07-03 07:33 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1 - use JSON for the persistent save format (11.65 KB, patch)
2012-05-30 12:00 PDT, Nathan Froyd [:froydnj]
taras.mozilla: review-
Details | Diff | Splinter Review
part 2 - delete detritus from ping test (2.10 KB, patch)
2012-05-30 12:02 PDT, Nathan Froyd [:froydnj]
taras.mozilla: review+
Details | Diff | Splinter Review
part 3 - small cleanups to ping test (3.53 KB, patch)
2012-05-30 12:03 PDT, Nathan Froyd [:froydnj]
taras.mozilla: review+
Details | Diff | Splinter Review
part 4 - more extensive testing of persistent pings (3.43 KB, patch)
2012-05-30 12:04 PDT, Nathan Froyd [:froydnj]
taras.mozilla: review+
Details | Diff | Splinter Review
part 5 - add load/save notifications (4.38 KB, patch)
2012-05-30 12:06 PDT, Nathan Froyd [:froydnj]
taras.mozilla: review+
Details | Diff | Splinter Review
part 6 - async load/save tests (7.77 KB, patch)
2012-05-30 12:07 PDT, Nathan Froyd [:froydnj]
taras.mozilla: review+
Details | Diff | Splinter Review
part 1 - use JSON for the persistent save format (12.53 KB, patch)
2012-06-07 11:24 PDT, Nathan Froyd [:froydnj]
taras.mozilla: review+
Details | Diff | Splinter Review

Description Nathan Froyd [:froydnj] 2012-04-24 13:26:15 PDT
The binary data format has caused problems in the past; it'd be easier to use JSON for this sort of thing.

It'd also be easier to add one-off things to the save format (e.g. saving slow SQL queries and the like) with JSON.
Comment 1 Nathan Froyd [:froydnj] 2012-05-30 12:00:20 PDT
Created attachment 628409 [details] [diff] [review]
part 1 - use JSON for the persistent save format

Here's the first patch; it's strictly about the save format.  We write the whole thing with JSON and the format is as simple as can be, just an array of serialized ping packets.  The array-ness is to make it simpler to save multiple pings at some future point and not mess with a slightly changed save format.

Tests work, but there's a number of things that we can do better that will be addressed in future patches.
Comment 2 Nathan Froyd [:froydnj] 2012-05-30 12:02:10 PDT
Created attachment 628410 [details] [diff] [review]
part 2 - delete detritus from ping test

gTestUUID isn't used any more, just delete it.  (Hmm, SAVED_PATH doesn't get used either, odd...)  I can roll this into the previous one if you'd rather.
Comment 3 Nathan Froyd [:froydnj] 2012-05-30 12:03:38 PDT
Created attachment 628412 [details] [diff] [review]
part 3 - small cleanups to ping test

This patch just shuffles a few things around and adds some new functions for clarity.  No functional changes, but we will need getSavedHistogramFile, at least, later.
Comment 4 Nathan Froyd [:froydnj] 2012-05-30 12:04:51 PDT
Created attachment 628413 [details] [diff] [review]
part 4 - more extensive testing of persistent pings

Now that we're sending the same information between normal and persistent pings, we should be checking that during tests as well.
Comment 5 Nathan Froyd [:froydnj] 2012-05-30 12:06:10 PDT
Created attachment 628414 [details] [diff] [review]
part 5 - add load/save notifications

For testing purposes, it'd be nice to know when loading and saving of persistent telemetry completes.  Doing it through the observer service seemed like the nicest option.
Comment 6 Nathan Froyd [:froydnj] 2012-05-30 12:07:41 PDT
Created attachment 628416 [details] [diff] [review]
part 6 - async load/save tests

This patch makes sure that we have testing for async load/store.  In the previous incarnation of persistent telemetry, async load/store was provided by Telemetry itself.  Since the new functionality is now hidden within TelemetryPing, this is where we should test it.  It's a little messy, but it does work.
Comment 7 Nathan Froyd [:froydnj] 2012-05-30 12:08:46 PDT
Those patches are not quite everything; nothing in this series turns persistent telemetry back on and of course, there's a pile of code to be deleted from Telemetry.cpp and friends.  Nevertheless, I thought I might start reviews now, since what's left is pretty trivial to review once it gets written.
Comment 8 (dormant account) 2012-05-30 13:33:03 PDT
Comment on attachment 628409 [details] [diff] [review]
part 1 - use JSON for the persistent save format

+    do {
+      let data = this.getSessionPayloadAndSlug(reason);
+      this.doPing(server, data.slug, data.payload, !data.previous);
+      previous = data.previous
+    } while (previous);
Yuck. use iterators. This is not C. yield from getSessionPayloadAndSlug...Should also do json parsing, etc, lazily too(might even be able to do io this way, ie via this ghetto continuation mechanism). There is a theoretical case of having hundreds of stale telemetry data files.

+    ostream.init(file, 0x02 | 0x08 | 0x20, 0600, ostream.DEFER_OPEN);
constants please, not magic numbers

rest of this i think is ok
Comment 9 Nathan Froyd [:froydnj] 2012-05-30 13:38:00 PDT
(In reply to Taras Glek (:taras) from comment #8)
> Comment on attachment 628409 [details] [diff] [review]
> part 1 - use JSON for the persistent save format
> 
> +    do {
> +      let data = this.getSessionPayloadAndSlug(reason);
> +      this.doPing(server, data.slug, data.payload, !data.previous);
> +      previous = data.previous
> +    } while (previous);
> Yuck. use iterators. This is not C. yield from
> getSessionPayloadAndSlug...Should also do json parsing, etc, lazily
> too(might even be able to do io this way, ie via this ghetto continuation
> mechanism). There is a theoretical case of having hundreds of stale
> telemetry data files.

I'm not planning on having hundreds of stale telemetry data files; there will forever and always be One Persistent Telemetry File.  If you don't think that's a good idea, we should have a discussion about that now.
Comment 10 (dormant account) 2012-05-30 14:36:53 PDT
Comment on attachment 628412 [details] [diff] [review]
part 3 - small cleanups to ping test

seems ok
Comment 11 (dormant account) 2012-05-31 21:14:48 PDT
(In reply to Nathan Froyd (:froydnj) from comment #9)
> 
> I'm not planning on having hundreds of stale telemetry data files; there
> will forever and always be One Persistent Telemetry File.  If you don't
> think that's a good idea, we should have a discussion about that now.

I would prefer to have a file per ping. This should in theory reduce memory usage during serialization/deserialization and minimize risk of blowing away existing pings while appending a newer ping.
Comment 12 Nathan Froyd [:froydnj] 2012-06-02 07:44:47 PDT
(In reply to Taras Glek (:taras) from comment #11)
> I would prefer to have a file per ping. This should in theory reduce memory
> usage during serialization/deserialization and minimize risk of blowing away
> existing pings while appending a newer ping.

I'm nervous about doing this because it introduces complexity: how do I find/enumerate these files (along with possible performance issues for opening/closing lots of small files during startup/shutdown), how do I make sure I delete the right files once the ping is sent, how do I select an unused filename for writing, etc. etc.

These are not insurmountable issues, of course (possible solutions, in order: separate profile-local directory for saved pings, pings know about what file they are/would be stored in, filename is a cryptographic hash of the JSON, etc.), but my gut feeling is that One Big File will be easier and sidestep many of these cases.  If it makes you feel better, we can do One Big File and collect telemetry on how big that file is, # of pings written, etc.  At least going from One Big File to directory-o'-files won't introduce file format version issues or anything like that.
Comment 13 Nathan Froyd [:froydnj] 2012-06-07 11:24:30 PDT
Created attachment 631054 [details] [diff] [review]
part 1 - use JSON for the persistent save format

JS iterators and no magic numbers, as requested.

This does not implement the directory of saved pings that we discussed.  I'd like to do that in a separate bug; the patch series for this bug is long enough.
Comment 14 (dormant account) 2012-06-09 21:52:12 PDT
Comment on attachment 631054 [details] [diff] [review]
part 1 - use JSON for the persistent save format


+      this._pendingPings = this._pendingPings.concat(data);
I think you should take that out of the try/catch. This only fails with good reason.

The following likely best in a followup bug:
I feel we should reverse the ping flow. Send current ping first, followed by old pings. We can then abort on the first failed ping instead of hammering an upset/missing server as we do now. Should also record success/fail with every ping.
Comment 15 Nathan Froyd [:froydnj] 2012-06-11 09:44:47 PDT
(In reply to Taras Glek (:taras) from comment #14)
> +      this._pendingPings = this._pendingPings.concat(data);
> I think you should take that out of the try/catch. This only fails with good
> reason.

I agree, but I think the flow is better this way.  Taking it out of the try/catch means moving the variable decl out of the try, then testing post-catch to see if we actually got data.  Unless there's a finally-but-no-exceptions-throw in JavaScript...

> The following likely best in a followup bug:

Several followup bugs filed.

Pushed to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e7f97d98b95
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca42cd911967
https://hg.mozilla.org/integration/mozilla-inbound/rev/970a56868001
https://hg.mozilla.org/integration/mozilla-inbound/rev/01aefb558bc8
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b953ebe26d8
https://hg.mozilla.org/integration/mozilla-inbound/rev/67e8adffcc49

Note You need to log in before you can comment on or make changes to this bug.