The default bug view has changed. See this FAQ.

use JSON for the persistent telemetry save format

RESOLVED FIXED in mozilla16

Status

()

Toolkit
Telemetry
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

unspecified
mozilla16
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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)

Updated

5 years ago
Blocks: 748520
(Assignee)

Comment 1

5 years ago
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.
Attachment #628409 - Flags: review?(taras.mozilla)
(Assignee)

Comment 2

5 years ago
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.
Attachment #628410 - Flags: review?(taras.mozilla)
(Assignee)

Comment 3

5 years ago
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.
Attachment #628412 - Flags: review?(taras.mozilla)
(Assignee)

Comment 4

5 years ago
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.
Attachment #628413 - Flags: review?(taras.mozilla)
(Assignee)

Comment 5

5 years ago
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.
Attachment #628414 - Flags: review?(taras.mozilla)
(Assignee)

Comment 6

5 years ago
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.
Attachment #628416 - Flags: review?(taras.mozilla)
(Assignee)

Comment 7

5 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

5 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

5 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

5 years ago
Attachment #628410 - Flags: review?(taras.mozilla) → review+

Comment 10

5 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

5 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

5 years ago
Attachment #628413 - Flags: review?(taras.mozilla) → review+

Updated

5 years ago
Attachment #628414 - Flags: review?(taras.mozilla) → review+

Updated

5 years ago
Attachment #628416 - Flags: review?(taras.mozilla) → review+
(Assignee)

Comment 12

5 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

5 years ago
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.
Attachment #628409 - Attachment is obsolete: true
Attachment #631054 - Flags: review?(taras.mozilla)
(Assignee)

Updated

5 years ago
Blocks: 763113

Comment 14

5 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)

Updated

5 years ago
Blocks: 763524
(Assignee)

Updated

5 years ago
Blocks: 763525
(Assignee)

Updated

5 years ago
Blocks: 763526
(Assignee)

Comment 15

5 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
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Blocks: 764513
(Assignee)

Updated

5 years ago
No longer blocks: 763524
You need to log in before you can comment on or make changes to this bug.