Closed Bug 867401 Opened 7 years ago Closed 7 years ago

remove hash checksums from saved pings

Categories

(Toolkit :: Telemetry, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: froydnj, Unassigned)

References

Details

Attachments

(1 file)

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.
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.
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.
Attachment #744112 - Flags: review?(vdjeric)
can we just compress them instead?
(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?
Flags: needinfo?(taras.mozilla)
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?
Attachment #744112 - Flags: review?(vdjeric) → review+
(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
Flags: needinfo?(taras.mozilla)
https://hg.mozilla.org/mozilla-central/rev/6fe5cab777ab
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.