Closed
Bug 748517
Opened 13 years ago
Closed 13 years ago
use JSON for the persistent telemetry save format
Categories
(Toolkit :: Telemetry, defect)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla16
People
(Reporter: froydnj, Assigned: froydnj)
References
Details
Attachments
(6 files, 1 obsolete file)
(deleted),
patch
|
taras.mozilla
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
taras.mozilla
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
taras.mozilla
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
taras.mozilla
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
taras.mozilla
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
taras.mozilla
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•13 years ago
|
||
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)
Assignee | ||
Comment 2•13 years ago
|
||
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)
Assignee | ||
Comment 3•13 years ago
|
||
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)
Assignee | ||
Comment 4•13 years ago
|
||
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)
Assignee | ||
Comment 5•13 years ago
|
||
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)
Assignee | ||
Comment 6•13 years ago
|
||
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)
Assignee | ||
Comment 7•13 years ago
|
||
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•13 years ago
|
||
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-
Assignee | ||
Comment 9•13 years ago
|
||
(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.
Updated•13 years ago
|
Attachment #628410 -
Flags: review?(taras.mozilla) → review+
Comment 10•13 years ago
|
||
Comment on attachment 628412 [details] [diff] [review]
part 3 - small cleanups to ping test
seems ok
Attachment #628412 -
Flags: review?(taras.mozilla) → review+
Comment 11•13 years ago
|
||
(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.
Updated•13 years ago
|
Attachment #628413 -
Flags: review?(taras.mozilla) → review+
Updated•13 years ago
|
Attachment #628414 -
Flags: review?(taras.mozilla) → review+
Updated•13 years ago
|
Attachment #628416 -
Flags: review?(taras.mozilla) → review+
Assignee | ||
Comment 12•13 years ago
|
||
(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.
Assignee | ||
Comment 13•13 years ago
|
||
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)
Comment 14•13 years ago
|
||
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+
Assignee | ||
Comment 15•13 years ago
|
||
(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
Comment 16•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7e7f97d98b95
https://hg.mozilla.org/mozilla-central/rev/ca42cd911967
https://hg.mozilla.org/mozilla-central/rev/970a56868001
https://hg.mozilla.org/mozilla-central/rev/01aefb558bc8
https://hg.mozilla.org/mozilla-central/rev/5b953ebe26d8
https://hg.mozilla.org/mozilla-central/rev/67e8adffcc49
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•