Closed
Bug 867401
Opened 11 years ago
Closed 11 years ago
remove hash checksums from saved pings
Categories
(Toolkit :: Telemetry, defect)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla23
People
(Reporter: froydnj, Unassigned)
References
Details
Attachments
(1 file)
4.26 KB,
patch
|
vladan
:
review+
|
Details | Diff | Splinter Review |
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•11 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•11 years ago
|
||
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•11 years ago
|
||
can we just compress them instead?
Reporter | ||
Comment 4•11 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 5•11 years ago
|
||
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•11 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•11 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/6fe5cab777ab
Comment 8•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6fe5cab777ab
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in
before you can comment on or make changes to this bug.
Description
•