Last Comment Bug 717061 - Turn on compression for telemetry xmlhttprequest
: Turn on compression for telemetry xmlhttprequest
Status: RESOLVED FIXED
:
Product: Toolkit
Classification: Components
Component: Telemetry (show other bugs)
: unspecified
: x86 Windows 7
: -- normal (vote)
: mozilla13
Assigned To: Nathan Froyd [:froydnj]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-10 15:04 PST by (dormant account)
Modified: 2012-02-26 16:17 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1 - fix nsDeflateConverter (1.09 KB, patch)
2012-01-25 18:39 PST, Nathan Froyd [:froydnj]
dtownsend: review+
Details | Diff | Splinter Review
part 2 - compress telemetry ping packets (5.05 KB, patch)
2012-01-25 18:48 PST, Nathan Froyd [:froydnj]
taras.mozilla: review+
Details | Diff | Splinter Review
part 3 - add tests for uncompressed->gzip conversion (11.96 KB, patch)
2012-02-13 12:41 PST, Nathan Froyd [:froydnj]
dtownsend: review+
Details | Diff | Splinter Review
part 2 - compress telemetry ping packets (4.58 KB, text/plain)
2012-02-13 12:41 PST, Nathan Froyd [:froydnj]
nfroyd: review+
Details
part 1 - fix nsDeflateConverter (1.09 KB, patch)
2012-02-21 08:35 PST, Nathan Froyd [:froydnj]
nfroyd: review+
Details | Diff | Splinter Review
part 2 - compress telemetry ping packets (5.19 KB, patch)
2012-02-21 08:36 PST, Nathan Froyd [:froydnj]
nfroyd: review+
Details | Diff | Splinter Review
part 3 - add tests for uncompressed->gzip conversion (6.69 KB, patch)
2012-02-21 08:37 PST, Nathan Froyd [:froydnj]
nfroyd: review+
Details | Diff | Splinter Review
part 3 - add tests for uncompressed->gzip conversion (8.33 KB, patch)
2012-02-22 04:32 PST, Nathan Froyd [:froydnj]
nfroyd: review+
Details | Diff | Splinter Review
part 3 - add tests for uncompressed->gzip conversion (8.33 KB, patch)
2012-02-22 10:22 PST, Nathan Froyd [:froydnj]
nfroyd: review+
Details | Diff | Splinter Review
part 3 - add tests for uncompressed->gzip conversion (8.71 KB, patch)
2012-02-23 06:43 PST, Nathan Froyd [:froydnj]
nfroyd: review+
Details | Diff | Splinter Review
part 3 - add tests for uncompressed->gzip conversion (8.71 KB, patch)
2012-02-23 10:55 PST, Nathan Froyd [:froydnj]
nfroyd: review+
Details | Diff | Splinter Review

Description (dormant account) 2012-01-10 15:04:19 PST
Our packet sizes are around 25KB now. By conservative estimates they will double within 6months, we should turn on compression to give us some breathing space before we need to switch to a more efficient packet format.
Comment 1 Nathan Froyd [:froydnj] 2012-01-25 07:16:19 PST
This actually looks surprisingly hard to do sanely.
Comment 2 Nathan Froyd [:froydnj] 2012-01-25 18:39:02 PST
Created attachment 591677 [details] [diff] [review]
part 1 - fix nsDeflateConverter

nsDeflateConvert has been broken for gzip compression for...well, forever.

I picked mwu for review, but really, the bug is obvious.
Comment 3 Nathan Froyd [:froydnj] 2012-01-25 18:48:07 PST
Created attachment 591679 [details] [diff] [review]
part 2 - compress telemetry ping packets

This is the ugly part; it's crazy how much work it is to compress and uncompress strings.

Couple of things to note:

1) No, I don't know why I have to make an nsIStringInputStream to send in the XMLHttpRequest rather than just passing the string itself.  Sending the stream does the right thing; sending the string results in random bogosity in the sent string.  Suspect this has something to do with the binary-ness of the string.
2) The content-type of the request is changed, as the test in test_TelemetryPing.js shows; there's no charset=UTF-8 parameter appended.  Not sure if we need this or not.  I think it was being tacked on by XMLHttpRequest itself; we may have to provide it manually.
3) I assume that I don't have to mess with the mime type or the content-type; content-encoding should be all that I need, right?
Comment 4 Nathan Froyd [:froydnj] 2012-01-25 19:14:54 PST
Also, do we know that our server is setup to handle gzip packets?
Comment 5 (dormant account) 2012-01-25 21:45:12 PST
(In reply to Nathan Froyd (:froydnj) from comment #4)
> Also, do we know that our server is setup to handle gzip packets?

Xavier, it should work, right?
Comment 6 Michael Wu [:mwu] 2012-01-25 22:17:48 PST
Comment on attachment 591677 [details] [diff] [review]
part 1 - fix nsDeflateConverter

Don't know this code but I bet Mossop does.
Comment 7 Dave Townsend [:mossop] 2012-01-26 11:10:40 PST
Comment on attachment 591677 [details] [diff] [review]
part 1 - fix nsDeflateConverter

Obviously bad, can you add a test for this too though, you can see an example test for the deflate conversion here: http://mxr.mozilla.org/mozilla-central/source/modules/libjar/zipwriter/test/unit/test_bug399727.js
Comment 8 (dormant account) 2012-01-27 12:44:27 PST
Comment on attachment 591679 [details] [diff] [review]
part 2 - compress telemetry ping packets

gzipCompressString() -> gzip() is self-explanatory

+      this.buffer = "";
+    }
+
+    GzipListener.prototype = {
+      buffer: null,

that's redundant, use buffer: ""

Before landing this please confirm that telemetry server successfully receives one of these packets.
Comment 9 Nathan Froyd [:froydnj] 2012-02-13 12:41:06 PST
Created attachment 596753 [details] [diff] [review]
part 3 - add tests for uncompressed->gzip conversion

Tests, as requested.
Comment 10 Nathan Froyd [:froydnj] 2012-02-13 12:41:51 PST
Created attachment 596755 [details]
part 2 - compress telemetry ping packets

People pointed out that I could use nsIStreamLoader to streamline accumulation.  Carrying over r+.
Comment 11 Dave Townsend [:mossop] 2012-02-14 13:02:48 PST
Comment on attachment 596753 [details] [diff] [review]
part 3 - add tests for uncompressed->gzip conversion

Review of attachment 596753 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/libjar/zipwriter/test/unit/data/test_bug717061.html
@@ +145,5 @@
> +        <hr>
> +        <center>
> +        <div class="smallText">
> +        The views expressed within this site pretty much represent those of the author.<br>
> +        Copyright (c) 2002 <a href="http://tk-jk.net/">Terry Kearns</a>. All rights reserved.

This page seems to be copyrighted, I'd be concerned with including it in our test suite. Please make up something public domain

::: modules/libjar/zipwriter/test/unit/test_bug717061.js
@@ +33,5 @@
> + * the provisions above, a recipient may use your version of this file under
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK *****
> + */

Can you use a public domain header for tests please, like this one: http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpcshell/test_migrate2.js?force=1
Comment 12 Nathan Froyd [:froydnj] 2012-02-21 08:00:48 PST
(In reply to Taras Glek (:taras) from comment #8)
> Before landing this please confirm that telemetry server successfully
> receives one of these packets.

Sending a very minimal gzipped ping results in getting back:

Content-Type: plain/text
Connection: close
Content-Length: 36

2bc8fdb8-0a7b-4c02-8c54-e5300fb44125

I think this implies the server can handle gzip-encoded requests.
Comment 13 Nathan Froyd [:froydnj] 2012-02-21 08:35:51 PST
Created attachment 599194 [details] [diff] [review]
part 1 - fix nsDeflateConverter

Fix reviewer; merely cosmetic changes.
Comment 14 Nathan Froyd [:froydnj] 2012-02-21 08:36:53 PST
Created attachment 599196 [details] [diff] [review]
part 2 - compress telemetry ping packets

Add a charset=UTF-8 parameter to Content-Type; requires less fiddling with the tests and explicit is better than implicit.
Comment 15 Nathan Froyd [:froydnj] 2012-02-21 08:37:29 PST
Created attachment 599197 [details] [diff] [review]
part 3 - add tests for uncompressed->gzip conversion

Tweak tests to avoid copyright problems.
Comment 16 Mozilla RelEng Bot 2012-02-21 10:50:34 PST
Autoland Patchset:
	Patches: 599194, 599196, 599197
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=de7e4cb681c5
Try run started, revision de7e4cb681c5. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=de7e4cb681c5
Comment 17 Mozilla RelEng Bot 2012-02-21 22:46:05 PST
Try run for de7e4cb681c5 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=de7e4cb681c5
Results (out of 215 total builds):
    exception: 6
    success: 161
    warnings: 32
    failure: 16
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-de7e4cb681c5
Comment 18 Nathan Froyd [:froydnj] 2012-02-22 04:32:50 PST
Created attachment 599569 [details] [diff] [review]
part 3 - add tests for uncompressed->gzip conversion

Doh, I fail at exporting patches that include new binary files.
Comment 19 Nathan Froyd [:froydnj] 2012-02-22 04:33:46 PST
Re-autolanding with just xpcshell tests, since the previous try run was green otherwise.
Comment 20 Mozilla RelEng Bot 2012-02-22 04:37:24 PST
Autoland Patchset:
	Patches: 599194, 599196, 599569
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=8e0a581f470a
Try run started, revision 8e0a581f470a. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=8e0a581f470a
Comment 21 Dão Gottwald [:dao] 2012-02-22 04:39:50 PST
This landed, but I assume it's going to cause failures? Could you please remove checkin-needed when you don't want stuff to land?

http://hg.mozilla.org/integration/mozilla-inbound/rev/9a5b898000b5
http://hg.mozilla.org/integration/mozilla-inbound/rev/599a1a0c5fbe
http://hg.mozilla.org/integration/mozilla-inbound/rev/02d79cbf942c
Comment 22 Nathan Froyd [:froydnj] 2012-02-22 04:52:46 PST
(In reply to Dão Gottwald [:dao] from comment #21)
> This landed, but I assume it's going to cause failures? Could you please
> remove checkin-needed when you don't want stuff to land?

Yes, this will cause failures, as the new binary file in the third patch didn't get picked up.  Sorry, I've been checkin-needing and [autoland-try]'ing at the same time, assuming that folks will wait to see what the try run looks like before committing.

Try picked up the new file in the third patch, but it looks like the m-i push did not.  hg import appears to DTRT; I don't know what you used.
Comment 23 Dão Gottwald [:dao] 2012-02-22 05:08:18 PST
backed out
Comment 24 Mozilla RelEng Bot 2012-02-22 09:02:35 PST
Try run for 8e0a581f470a is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=8e0a581f470a
Results (out of 28 total builds):
    success: 14
    warnings: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-8e0a581f470a
Comment 25 Nathan Froyd [:froydnj] 2012-02-22 10:22:58 PST
Created attachment 599682 [details] [diff] [review]
part 3 - add tests for uncompressed->gzip conversion
Comment 26 Mozilla RelEng Bot 2012-02-22 10:29:13 PST
Autoland Patchset:
	Patches: 599194, 599196, 599682
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=89bb8b386d0f
Try run started, revision 89bb8b386d0f. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=89bb8b386d0f
Comment 27 Mozilla RelEng Bot 2012-02-22 18:02:45 PST
Try run for 89bb8b386d0f is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=89bb8b386d0f
Results (out of 28 total builds):
    success: 22
    warnings: 6
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-89bb8b386d0f
Comment 28 Nathan Froyd [:froydnj] 2012-02-23 06:43:15 PST
Created attachment 599978 [details] [diff] [review]
part 3 - add tests for uncompressed->gzip conversion

If you look at the tests, they failed on Windows systems only with:

TEST-UNEXPECTED-FAIL | C:/talos-slave/test/build/xpcshell/tests/modules/libjar/zipwriter/test/unit/test_bug717061.js | Invalid value 11 at offset 9, should have been 3 - See following stack:

As explained in the comment inserted in the test:

      // The byte at offset 9 is the OS byte (see RFC 1952, section 2.3), which
      // can legitimately differ when the source is compressed on different
      // operating systems.  The actual .gz for this test was created on a Unix
      // system, but we want the test to work correctly everywhere.  So ignore
      // the byte at offset 9.

Carrying over r+.
Comment 29 Mozilla RelEng Bot 2012-02-23 06:48:12 PST
Autoland Patchset:
	Patches: 599194, 599196, 599978
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=1dce48abed65
Try run started, revision 1dce48abed65. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=1dce48abed65
Comment 30 Mozilla RelEng Bot 2012-02-23 09:46:05 PST
Try run for 1dce48abed65 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=1dce48abed65
Results (out of 6 total builds):
    exception: 4
    success: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-1dce48abed65
Comment 31 Nathan Froyd [:froydnj] 2012-02-23 10:55:39 PST
Created attachment 600091 [details] [diff] [review]
part 3 - add tests for uncompressed->gzip conversion

Works a lot better when you add this. ...
Comment 32 Mozilla RelEng Bot 2012-02-23 11:00:12 PST
Autoland Patchset:
	Patches: 599194, 599196, 600091
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=ea9d35e02497
Try run started, revision ea9d35e02497. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=ea9d35e02497
Comment 33 Nathan Froyd [:froydnj] 2012-02-23 20:24:53 PST
Please be sure to use hg import rather than just plain patch; the third patch contains a new binary file that patch won't pick up, but import will.  Thanks!
Comment 34 Mozilla RelEng Bot 2012-02-23 22:31:11 PST
Try run for ea9d35e02497 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=ea9d35e02497
Results (out of 6 total builds):
    success: 6
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-ea9d35e02497

Note You need to log in before you can comment on or make changes to this bug.