717061 - Turn on compression for telemetry xmlhttprequest

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[(dormant account)]

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]]

This actually looks surprisingly hard to do sanely.

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: 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]]

Attachment: 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]]

Also, do we know that our server is setup to handle gzip packets?

Comment 5

[(dormant account)]

(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]]

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]]

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)]

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]]

Attachment: part 3 - add tests for uncompressed->gzip conversion

Tests, as requested.

Comment 10

[Nathan Froyd [:froydnj]]

Attachment: 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]]

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]]

(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]]

Attachment: part 1 - fix nsDeflateConverter

Fix reviewer; merely cosmetic changes.

Comment 14

[Nathan Froyd [:froydnj]]

Attachment: 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]]

Attachment: part 3 - add tests for uncompressed->gzip conversion

Tweak tests to avoid copyright problems.

Comment 16

[Mozilla RelEng Bot]

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]

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/[e-mail]-de7e4cb681c5

Comment 18

[Nathan Froyd [:froydnj]]

Attachment: part 3 - add tests for uncompressed->gzip conversion

Doh, I fail at exporting patches that include new binary files.

Comment 19

[Nathan Froyd [:froydnj]]

Re-autolanding with just xpcshell tests, since the previous try run was green otherwise.

Comment 20

[Mozilla RelEng Bot]

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]]

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]]

(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]]

backed out

Comment 24

[Mozilla RelEng Bot]

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/[e-mail]-8e0a581f470a

Comment 25

[Nathan Froyd [:froydnj]]

Attachment: part 3 - add tests for uncompressed->gzip conversion

Comment 26

[Mozilla RelEng Bot]

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]

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/[e-mail]-89bb8b386d0f

Comment 28

[Nathan Froyd [:froydnj]]

Attachment: 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]

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]

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/[e-mail]-1dce48abed65

Comment 31

[Nathan Froyd [:froydnj]]

Attachment: part 3 - add tests for uncompressed->gzip conversion

Works a lot better when you add this. ...

Comment 32

[Mozilla RelEng Bot]

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]]

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]

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/[e-mail]-ea9d35e02497

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/71f16a6905eb
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c63c2df5687
https://hg.mozilla.org/integration/mozilla-inbound/rev/75c291e1c27e

Comment 36

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/71f16a6905eb
https://hg.mozilla.org/mozilla-central/rev/9c63c2df5687
https://hg.mozilla.org/mozilla-central/rev/75c291e1c27e