Closed Bug 717061 Opened 9 years ago Closed 9 years ago

Turn on compression for telemetry xmlhttprequest

Categories

(Toolkit :: Telemetry, defect)

x86
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: taras.mozilla, Assigned: froydnj)

Details

Attachments

(3 files, 8 obsolete files)

1.09 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
5.19 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
8.71 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
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.
This actually looks surprisingly hard to do sanely.
Attached patch part 1 - fix nsDeflateConverter (obsolete) — Splinter Review
nsDeflateConvert has been broken for gzip compression for...well, forever.

I picked mwu for review, but really, the bug is obvious.
Attachment #591677 - Flags: review?(mwu)
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?
Attachment #591679 - Flags: review?(taras.mozilla)
Also, do we know that our server is setup to handle gzip packets?
(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 on attachment 591677 [details] [diff] [review]
part 1 - fix nsDeflateConverter

Don't know this code but I bet Mossop does.
Attachment #591677 - Flags: review?(mwu) → review?(dtownsend)
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
Attachment #591677 - Flags: review?(dtownsend) → review+
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.
Attachment #591679 - Flags: review?(taras.mozilla) → review+
Tests, as requested.
Attachment #596753 - Flags: review?(dtownsend+bugmail)
People pointed out that I could use nsIStreamLoader to streamline accumulation.  Carrying over r+.
Attachment #591679 - Attachment is obsolete: true
Attachment #596755 - Flags: review+
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
Attachment #596753 - Flags: review?(dtownsend+bugmail) → review+
(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.
Fix reviewer; merely cosmetic changes.
Attachment #591677 - Attachment is obsolete: true
Attachment #599194 - Flags: review+
Add a charset=UTF-8 parameter to Content-Type; requires less fiddling with the tests and explicit is better than implicit.
Attachment #596755 - Attachment is obsolete: true
Attachment #599196 - Flags: review+
Tweak tests to avoid copyright problems.
Attachment #596753 - Attachment is obsolete: true
Attachment #599197 - Flags: review+
Status: NEW → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [autoland-try]
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → ASSIGNED
Whiteboard: [autoland-try] → [autoland-in-queue]
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
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
Whiteboard: [autoland-in-queue]
Doh, I fail at exporting patches that include new binary files.
Attachment #599197 - Attachment is obsolete: true
Attachment #599569 - Flags: review+
Re-autolanding with just xpcshell tests, since the previous try run was green otherwise.
Whiteboard: [autoland-try:-b do -p all -u xpcshell -t none]
Whiteboard: [autoland-try:-b do -p all -u xpcshell -t none] → [autoland-in-queue]
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
(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.
backed out
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
Whiteboard: [autoland-in-queue]
Attachment #599569 - Attachment is obsolete: true
Attachment #599682 - Flags: review+
Whiteboard: [autoland-try:-b do -p all -u xpcshell -t none]
Whiteboard: [autoland-try:-b do -p all -u xpcshell -t none] → [autoland-in-queue]
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
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
Whiteboard: [autoland-in-queue]
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+.
Attachment #599682 - Attachment is obsolete: true
Attachment #599978 - Flags: review+
Whiteboard: [autoland-try:-b do -p win32 -u xpcshell -t none]
Whiteboard: [autoland-try:-b do -p win32 -u xpcshell -t none] → [autoland-in-queue]
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
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
Whiteboard: [autoland-in-queue]
Works a lot better when you add this. ...
Attachment #599978 - Attachment is obsolete: true
Attachment #600091 - Flags: review+
Whiteboard: [autoland-try:-b do -p win32 -u xpcshell -t none]
Whiteboard: [autoland-try:-b do -p win32 -u xpcshell -t none] → [autoland-in-queue]
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
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!
Keywords: checkin-needed
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
Whiteboard: [autoland-in-queue]
https://hg.mozilla.org/mozilla-central/rev/71f16a6905eb
https://hg.mozilla.org/mozilla-central/rev/9c63c2df5687
https://hg.mozilla.org/mozilla-central/rev/75c291e1c27e
Status: ASSIGNED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.