763524 - give each saved ping its own file under a telemetry holding directory

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Comment 1

[Nathan Froyd [:froydnj]]

Bug 753238 is adding some ASCII encoding stuff that would be mighty useful here, so waiting until that goes in.

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: part 1 - pass entire ping to doPing

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: part 2 - separate finishRequest out into its own function

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: part 3 - save pings to unique files

I freely note here that we do lose the saved old-format ping in the sense that we no longer send it or read it in.  I don't think it's worth keeping old code around and this is a small one-time loss of data, so I wasn't terribly concerned about it.  But if you're concerned, let me know and I'll adjust.

Comment 5

[(dormant account)]

(In reply to Nathan Froyd (:froydnj) from comment #4)
> Created attachment 640595 [details] [diff] [review]
> part 3 - save pings to unique files
> 
> I freely note here that we do lose the saved old-format ping in the sense
> that we no longer send it or read it in.  I don't think it's worth keeping
> old code around and this is a small one-time loss of data, so I wasn't
> terribly concerned about it.  But if you're concerned, let me know and I'll
> adjust.

I'm not concerned. We should delete it, and do nothing else.

Comment 6

[(dormant account)]

Comment on attachment 640591 [details] [diff] [review]
part 1 - pass entire ping to doPing

I like easy patches like this

Comment 7

[(dormant account)]

Comment on attachment 640592 [details]
part 2 - separate finishRequest out into its own function

+      if (file.exists()) {
+        file.remove(true);
+      }

Don't use this pattern. Just .remove() directly and deal with exceptions if any.

This almost doubles IO overhead with no gain.

Comment 8

[(dormant account)]

Comment on attachment 640595 [details] [diff] [review]
part 3 - save pings to unique files

addToPendingPings needs a comment describing what it does.

+      this._pendingPings.push(data);
+      this._pingLoadsCompleted++;

these also need documenting at declarations site

s/ensurePingDirectoryExists/ensurePingDirectory/

+      // Note that addToPendingPings relies on the callback(s) here from
+      // multiple calls to loadHistogram being called all on the same
+      // thread.

Code is less confusing without this comment

+    file.append(checksum);

Use uuid for filename to avoid redundant io.

r- because this patch is creating too many files

There should only ever be one pending ping to save which would overwrite previously saved ping.

Comment 9

[Nathan Froyd [:froydnj]]

Attachment: part 3 - save pings to unique files

Comment 10

[(dormant account)]

Comment on attachment 641102 [details] [diff] [review]
part 3 - save pings to unique files

+  verifyPingChecksum: function verifyPingChecksum(ping) {
+    if (!ping.checksum) {
+      return true;
+    }
+
+    let checksumNow = this.hashString(ping.payload);
+    return ping.checksum == checksumNow;
   },

This needs a comment on why sometimes there is no checksum

Why would you have multiple pending pings? Is this just in preparation for future work?

Comment 11

[Nathan Froyd [:froydnj]]

(In reply to Taras Glek (:taras) from comment #10)
> Why would you have multiple pending pings? Is this just in preparation for
> future work?

It is unlikely that we'd have them now (the examples of unreachable metrics servers, either via downtime or lack of network connectivity, won't supply multiple pending pings until another bug is fixed), but an (contrived?) example goes something like:

- User starts Firefox;
- User shuts down Firefox  in < 5 minutes, so the initial idle ping never gets sent;
- We therefore save statistics from that session;
- Repeat as often as you like; we continue to save pings at shutdown;
- User finally runs a session where the initial ping gets sent, and so sends off multiple pings from previous sessions.

I thought this was the sort of motivating example for doing this work, since telemetry suggested we have users starting and stopping Firefox in short bursts.  The save-ping-to-prearranged-file approach we had would therefore lose data in such a scenario.

If you find such a scenario unconvincing, then yes, you can think of it as preparation for future work.  But I don't understand why it's a problem now, since pendingPings has been an array to prepare for this sort of an eventuality for a while now.

Comment 12

[(dormant account)]

Comment on attachment 641102 [details] [diff] [review]
part 3 - save pings to unique files

Thanks for the explanation. 
Please comment verifyPingChecksum.

Comment 13

[Nathan Froyd [:froydnj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9233c05bd8b0
https://hg.mozilla.org/integration/mozilla-inbound/rev/93b4fb5855a8
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8e672f4fc77

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9233c05bd8b0
https://hg.mozilla.org/mozilla-central/rev/93b4fb5855a8
https://hg.mozilla.org/mozilla-central/rev/b8e672f4fc77

Comment 15

[Dave Miller [:justdave]]

FYI this doesn't work.  The directory is created mode 0600, which makes it inaccessible, so it can't write to it.  I discovered this because my profile backups are complaining that they can't read it to back it up every night.  The directory should be mode 0700.