867401 - remove hash checksums from saved pings

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Nathan Froyd [:froydnj]]

After finally picking out the right message from the spew of messages in the console, I see why trying to save pings in profile-before-change2 was not working well.  We save sha<mumble> hashes of ping payloads as checksums in the saved pings.  Works great, except that the bits of security/manager/ssl/ responsible for implementing that hash shuts down at profile-before-change.  So if we're saving pings in profile-before-change2, we ask for an instance of nsICryptoHash and an exception is thrown, which totally kills the process of saving pings.

We're not really using the checksum for anything besides a quick check that the saved ping didn't get corrupted, so we could ditch it without loss of functionality.

Comment 1

[Nathan Froyd [:froydnj]]

FWIW, the READ_SAVED_PING_SUCCESS histogram indicates a 99.995% success rate for reading saved histograms across *all* channels.  We don't have any numbers on whether that's due to bad checksums or something else, but that's good enough that I would feel comfortable removing any checksumming and just letting the JSON syntax checker catch problems.

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: don't generate checksums for saved pings

Generating checksums means relying on nsICryptoHash, which is
unavailable after profile-before-change.  We'd like to save pings after
profile-before-change.  Hence, don't checksum.  Over 99.99% of our pings
are read successfully across all channels, so checksumming isn't really
buying us anything.

Comment 3

[(dormant account)]

can we just compress them instead?

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to Taras Glek (:taras) from comment #3)
> can we just compress them instead?

Compress what?  The entire saved ping?  How is that relevant to this bug?

Comment 5

[Vladan Djeric (:vladan)]

Comment on attachment 744112 [details] [diff] [review]
don't generate checksums for saved pings

Looks good to me. Can you just make sure Metrics isn't using the checksum field for anything?

Comment 6

[(dormant account)]

(In reply to Vladan Djeric (:vladan) from comment #5)
> Comment on attachment 744112 [details] [diff] [review]
> don't generate checksums for saved pings
> 
> Looks good to me. Can you just make sure Metrics isn't using the checksum
> field for anything?

it isn't.


(In reply to Nathan Froyd (:froydnj) from comment #4)
> (In reply to Taras Glek (:taras) from comment #3)
> > can we just compress them instead?
> 
> Compress what?  The entire saved ping?  How is that relevant to this bug?

09:47 < vladan> froydnj: i think Taras is saying use "compression as a checksum that will be available late during shutdown"

We should reduce disk footprint of pings too. 2birds, 1 stone sort of thing

Comment 7

[Nathan Froyd [:froydnj]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6fe5cab777ab

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/6fe5cab777ab