Check ping size after compression

RESOLVED FIXED in Firefox 62

Status

()

P4
normal
RESOLVED FIXED
2 years ago
8 months ago

People

(Reporter: katejimmelon, Assigned: joberts.ff, Mentored)

Tracking

unspecified
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

(Whiteboard: [good next bug][lang=js])

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
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.
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.
Depends on: 1318297, 1367094
Priority: -- → P4
(Assignee)

Comment 3

9 months ago
You can assign this to me. I'll give it a try.

Comment 4

9 months ago
Let me know if you have any questions!
Assignee: nobody → joberts.ff
Status: NEW → ASSIGNED
(Assignee)

Comment 5

9 months ago
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)
(Assignee)

Comment 6

9 months ago
Created attachment 8971842 [details]
Changes that I made.

Comment 7

9 months 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)
(Assignee)

Comment 8

9 months ago
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

9 months 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

9 months 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

9 months 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

9 months 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.
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

9 months 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

9 months ago
I can take a look at this one 1430531, unless you have something else.
(Assignee)

Comment 18

9 months 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

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b507e044f169
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
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
Depends on: 1331445
You need to log in before you can comment on or make changes to this bug.