Closed Bug 748517 Opened 12 years ago Closed 12 years ago

use JSON for the persistent telemetry save format

Categories

(Toolkit :: Telemetry, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: froydnj, Assigned: froydnj)

References

Details

Attachments

(6 files, 1 obsolete file)

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.
Blocks: 748520
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.
Attachment #628409 - Flags: review?(taras.mozilla)
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.
Attachment #628410 - Flags: review?(taras.mozilla)
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.
Attachment #628412 - Flags: review?(taras.mozilla)
Now that we're sending the same information between normal and persistent pings, we should be checking that during tests as well.
Attachment #628413 - Flags: review?(taras.mozilla)
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.
Attachment #628414 - Flags: review?(taras.mozilla)
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.
Attachment #628416 - Flags: review?(taras.mozilla)
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 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
Attachment #628409 - Flags: review?(taras.mozilla) → review-
(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.
Attachment #628410 - Flags: review?(taras.mozilla) → review+
Comment on attachment 628412 [details] [diff] [review]
part 3 - small cleanups to ping test

seems ok
Attachment #628412 - Flags: review?(taras.mozilla) → review+
(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.
Attachment #628413 - Flags: review?(taras.mozilla) → review+
Attachment #628414 - Flags: review?(taras.mozilla) → review+
Attachment #628416 - Flags: review?(taras.mozilla) → review+
(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.
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.
Attachment #628409 - Attachment is obsolete: true
Attachment #631054 - Flags: review?(taras.mozilla)
Blocks: 763113
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.
Attachment #631054 - Flags: review?(taras.mozilla) → review+
Blocks: 763524
Blocks: 763525
Blocks: 763526
(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
Assignee: nobody → nfroyd
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla16
Blocks: 764513
No longer blocks: 763524
You need to log in before you can comment on or make changes to this bug.