The default bug view has changed. See this FAQ.

Turn on compression for telemetry xmlhttprequest

RESOLVED FIXED in mozilla13

Status

()

Toolkit
Telemetry
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: (dormant account), Assigned: froydnj)

Tracking

unspecified
mozilla13
x86
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 8 obsolete attachments)

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
(Reporter)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
This actually looks surprisingly hard to do sanely.
(Assignee)

Comment 2

5 years ago
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.
Attachment #591677 - Flags: review?(mwu)
(Assignee)

Comment 3

5 years ago
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?
Attachment #591679 - Flags: review?(taras.mozilla)
(Assignee)

Comment 4

5 years ago
Also, do we know that our server is setup to handle gzip packets?
(Reporter)

Comment 5

5 years ago
(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

5 years ago
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+
(Reporter)

Comment 8

5 years ago
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+
(Assignee)

Comment 9

5 years ago
Created attachment 596753 [details] [diff] [review]
part 3 - add tests for uncompressed->gzip conversion

Tests, as requested.
Attachment #596753 - Flags: review?(dtownsend+bugmail)
(Assignee)

Comment 10

5 years ago
Created attachment 596755 [details]
part 2 - compress telemetry ping packets

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+
(Assignee)

Comment 12

5 years ago
(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.
(Assignee)

Comment 13

5 years ago
Created attachment 599194 [details] [diff] [review]
part 1 - fix nsDeflateConverter

Fix reviewer; merely cosmetic changes.
Attachment #591677 - Attachment is obsolete: true
Attachment #599194 - Flags: review+
(Assignee)

Comment 14

5 years ago
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.
Attachment #596755 - Attachment is obsolete: true
Attachment #599196 - Flags: review+
(Assignee)

Comment 15

5 years ago
Created attachment 599197 [details] [diff] [review]
part 3 - add tests for uncompressed->gzip conversion

Tweak tests to avoid copyright problems.
Attachment #596753 - Attachment is obsolete: true
Attachment #599197 - Flags: review+
(Assignee)

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [autoland-try]
(Assignee)

Updated

5 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

5 years ago
Status: REOPENED → ASSIGNED

Updated

5 years ago
Whiteboard: [autoland-try] → [autoland-in-queue]

Comment 16

5 years ago
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

5 years ago
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

Updated

5 years ago
Whiteboard: [autoland-in-queue]
(Assignee)

Comment 18

5 years ago
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.
Attachment #599197 - Attachment is obsolete: true
Attachment #599569 - Flags: review+
(Assignee)

Comment 19

5 years ago
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]

Updated

5 years ago
Whiteboard: [autoland-try:-b do -p all -u xpcshell -t none] → [autoland-in-queue]

Comment 20

5 years ago
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
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
Keywords: checkin-needed
(Assignee)

Comment 22

5 years ago
(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

Comment 24

5 years ago
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

Updated

5 years ago
Whiteboard: [autoland-in-queue]
(Assignee)

Comment 25

5 years ago
Created attachment 599682 [details] [diff] [review]
part 3 - add tests for uncompressed->gzip conversion
Attachment #599569 - Attachment is obsolete: true
Attachment #599682 - Flags: review+
(Assignee)

Updated

5 years ago
Whiteboard: [autoland-try:-b do -p all -u xpcshell -t none]

Updated

5 years ago
Whiteboard: [autoland-try:-b do -p all -u xpcshell -t none] → [autoland-in-queue]

Comment 26

5 years ago
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

5 years ago
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

Updated

5 years ago
Whiteboard: [autoland-in-queue]
(Assignee)

Comment 28

5 years ago
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+.
Attachment #599682 - Attachment is obsolete: true
Attachment #599978 - Flags: review+
(Assignee)

Updated

5 years ago
Whiteboard: [autoland-try:-b do -p win32 -u xpcshell -t none]

Updated

5 years ago
Whiteboard: [autoland-try:-b do -p win32 -u xpcshell -t none] → [autoland-in-queue]

Comment 29

5 years ago
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

5 years ago
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

Updated

5 years ago
Whiteboard: [autoland-in-queue]
(Assignee)

Comment 31

5 years ago
Created attachment 600091 [details] [diff] [review]
part 3 - add tests for uncompressed->gzip conversion

Works a lot better when you add this. ...
Attachment #599978 - Attachment is obsolete: true
Attachment #600091 - Flags: review+
(Assignee)

Updated

5 years ago
Whiteboard: [autoland-try:-b do -p win32 -u xpcshell -t none]

Updated

5 years ago
Whiteboard: [autoland-try:-b do -p win32 -u xpcshell -t none] → [autoland-in-queue]

Comment 32

5 years ago
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
(Assignee)

Comment 33

5 years ago
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

Comment 34

5 years ago
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

Updated

5 years ago
Whiteboard: [autoland-in-queue]
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
Keywords: checkin-needed
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
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.