If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

remove hash checksums from saved pings

RESOLVED FIXED in mozilla23

Status

()

Toolkit
Telemetry
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: froydnj, Unassigned)

Tracking

unspecified
mozilla23
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

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

Comment 1

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

Comment 2

5 years ago
Created attachment 744112 [details] [diff] [review]
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.
Attachment #744112 - Flags: review?(vdjeric)

Comment 3

5 years ago
can we just compress them instead?
(Reporter)

Comment 4

5 years ago
(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+

Comment 6

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

Comment 7

5 years ago
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6fe5cab777ab
https://hg.mozilla.org/mozilla-central/rev/6fe5cab777ab
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.