Closed
Bug 1369049
Opened 7 years ago
Closed 6 years ago
Check ping size after compression
Categories
(Toolkit :: Telemetry, enhancement, P4)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla62
Tracking | Status | |
---|---|---|
firefox62 | --- | fixed |
People
(Reporter: katejimmelon, Assigned: joberts.ff, Mentored)
References
Details
(Whiteboard: [good next bug][lang=js])
Attachments
(2 files)
Ping size is checked before compression https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/TelemetrySend.jsm#1160. It seems that it is better to check TelemetryStorage.MAXIMUM_PING_SIZE after compression.
Comment 1•7 years ago
|
||
This is a good point; the size after compression is what will actually be meaningful for upload success etc. We should probably look into this after we have an understanding (and monitoring) of ping size vs. upload success via bug 1318297 and bug 1367094.
Updated•7 years ago
|
Priority: -- → P4
Comment 2•6 years ago
|
||
Here's an updated link: https://searchfox.org/mozilla-central/rev/a30a60eadcff99d2a043241955d215dbeccad876/toolkit/components/telemetry/TelemetrySend.jsm#1217
Mentor: chutten
Whiteboard: [good next bug][lang=js]
Comment 4•6 years ago
|
||
Let me know if you have any questions!
Assignee: nobody → joberts.ff
Status: NEW → ASSIGNED
Ok, so I moved the code that checks the ping size to after the ping is compressed, this caused the test_discardBigPings to fail. I believe this failure is do to sending and checking a 2MB ping, and since we are now checking the ping after compression the size could be smaller then 2MB. So, I changed the test and sent a 4MB ping and the test passed. 1. Do we need to add a MAXIMUM_COMPRESSED_PING_SIZE property to TelemetryStorage.jsm, and what should that size be? 2. Other than running the test, how do I test my code changes? I would like to see what the ping is compressed to.
Flags: needinfo?(chutten)
Comment 7•6 years ago
|
||
Thank you for your patch! I can't quite tell from the text file what has been changed and what has remained the same. Can you follow the instructions on https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch for how to format a patch for review? Having test_discardBigPings fail is probably a good sign that your code is working properly. If that 2MB ping compresses to under the 1MB limit, then of course it should be sent :) > 1. Do we need to add a MAXIMUM_COMPRESSED_PING_SIZE property to > TelemetryStorage.jsm, and what should that size be? No, this 1MB limit should be compared to the compressed size, this is fine. > 2. Other than running the test, how do I test my code changes? I would like > to see what the ping is compressed to. You could run your build (./mach run) and check about:telemetry. There are histograms TELEMETRY_SUCCESSFUL_SEND_PINGS_SIZE_KB and TELEMETRY_FAILED_SEND_PINGS_SIZE_KB which record the compressed size of the pings we attempted to send (in KB). Would that give you the information you need?
Flags: needinfo?(chutten)
Should I go ahead and submit the patch for review without changing the test "test_discardBigPings"? I assume the patch would fail review if the test did not pass. Does another bug need to be created to fix the test? I could not find histograms TELEMETRY_SUCCESSFUL_SEND_PINGS_SIZE_KB or TELEMETRY_FAILED_SEND_PINGS_SIZE_KB after running the tests, could be that I'm looking in the wrong place(I was looking at the parent thread).
Flags: needinfo?(chutten)
Comment 9•6 years ago
|
||
As a general rule you should fix the test in the same bug you changed the behaviour under test (in other words: this one :D ). Change the test to make it work again, add it to the change to check sizes after compression, and you can put it up for my review. As for those histograms... it's possible we aren't actually sending pings from your build and are just pretending. (Generally we don't care about usage stats from dev builds. They can be _weird_). We could try to make it send the pings, or we can trust our test coverage for sending pings (we have that machinery fairly well covered).
Flags: needinfo?(chutten)
Comment hidden (mozreview-request) |
Comment 11•6 years ago
|
||
mozreview-review |
Comment on attachment 8973365 [details] Bug 1369049 - Moved code that checks for big pings after the ping is compressed. Had to update test_discardBigPings to send a 4MB ping due to the ping being compressed; https://reviewboard.mozilla.org/r/241828/#review247940 This is looking pretty good, I have one question: ::: toolkit/components/telemetry/TelemetrySend.jsm:1220 (Diff revision 1) > + .createInstance(Ci.nsIStringInputStream); > + startTime = Utils.monotonicNow(); > + payloadStream.data = gzipCompressString(utf8Payload); > + > // Check the size and drop pings which are too big. > - const pingSizeBytes = utf8Payload.length; > + const compressedPingSizeBytes = Math.floor(payloadStream.data.length); Why do we need to floor the input stream length?
Attachment #8973365 -
Flags: review?(chutten)
Comment hidden (mozreview-request) |
Comment 13•6 years ago
|
||
mozreview-review |
Comment on attachment 8973365 [details] Bug 1369049 - Moved code that checks for big pings after the ping is compressed. Had to update test_discardBigPings to send a 4MB ping due to the ping being compressed; https://reviewboard.mozilla.org/r/241828/#review248022 Looks good, thanks!
Attachment #8973365 -
Flags: review?(chutten) → review+
Assignee | ||
Comment 14•6 years ago
|
||
(In reply to Chris H-C :chutten from comment #11) > Comment on attachment 8973365 [details] > Bug 1369049 - Moved code that checks for big pings after the ping is > compressed. Had to update test_discardBigPings to send a 4MB ping due to > the ping being compressed; > > https://reviewboard.mozilla.org/r/241828/#review247940 > > This is looking pretty good, I have one question: > > ::: toolkit/components/telemetry/TelemetrySend.jsm:1220 > (Diff revision 1) > > + .createInstance(Ci.nsIStringInputStream); > > + startTime = Utils.monotonicNow(); > > + payloadStream.data = gzipCompressString(utf8Payload); > > + > > // Check the size and drop pings which are too big. > > - const pingSizeBytes = utf8Payload.length; > > + const compressedPingSizeBytes = Math.floor(payloadStream.data.length); > > Why do we need to floor the input stream length? It must have been left over from copying, and I just missed. Should be fixed now.
Comment 15•6 years ago
|
||
I have pushed this to autoland which should start the patch on the way to being integrated into Firefox. Thank you for your contribution! Did you have anything in mind for what you'd like to work on next? There's always more work to do :)
Comment 16•6 years ago
|
||
Pushed by chutten@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b507e044f169 Moved code that checks for big pings after the ping is compressed. Had to update test_discardBigPings to send a 4MB ping due to the ping being compressed; r=chutten
Assignee | ||
Comment 17•6 years ago
|
||
I can take a look at this one 1430531, unless you have something else.
Assignee | ||
Comment 18•6 years ago
|
||
I don't now after reviewing bug 1430531, it might be more than I can take on right now. Do you have anything else?
Comment 19•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b507e044f169
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Comment 20•6 years ago
|
||
Ah, sorry for not seeing this earlier. Are you okay to continue with bug 1430531 anyway? If not, a simpler one may be bug 1451813
You need to log in
before you can comment on or make changes to this bug.
Description
•