Closed Bug 1177099 Opened 9 years ago Closed 9 years ago

[Metrics] Compress AdvancedTelemetry Payload

Categories

(Firefox OS Graveyard :: Gaia::System, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(feature-b2g:2.5+)

VERIFIED FIXED
feature-b2g 2.5+

People

(Reporter: thills, Assigned: rnicoletti)

References

Details

Attachments

(2 files, 2 obsolete files)

The payload of the AdvancedTelemetry Ping needs to be gzip-compressed to save on the mobile data charges.

The server will automatically decompress the payload
QA Contact: thills
Blocks: 1180699
feature-b2g: --- → 2.5+
Attached patch gzip compress in a worker (obsolete) — Splinter Review
Hi David and Tamara, attached is a patch that uses a worker to gzip the telemetry payload before transmitting to the server.
Attachment #8661876 - Flags: feedback?(thills)
Attachment #8661876 - Flags: feedback?(dflanagan)
Comment on attachment 8661876 [details] [diff] [review]
gzip compress in a worker

Hi Russ,

It looks good.  My only question is about whether we are supposed to put this in shared or not.  I'm hoping David can weigh in on this.  In a future release, I'm thinking we can utilize this compression for AppUsagePing and FtuPing once we change those over to use the Unified Telemetry format.

One thing I would add is a debug statement before it gets compressed.  There is another debug statement of the payload that happens in telemetry.js, but that is after it gets compressed.

Thanks,

-tamara
Attachment #8661876 - Flags: feedback?(thills) → feedback+
Comment on attachment 8661876 [details] [diff] [review]
gzip compress in a worker

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

Russ,

My biggest concern with this is that I don't know if we can use GPL code without infecting the rest of Gaia with its copyleft.  Looks like there is an alternative here: https://github.com/imaya/zlib.js, so maybe you can use that.

Various other comments below. I think you have a bug in your code where reusing a single onmessage handler could cause problems

::: apps/system/js/advanced_telemetry.js
@@ +358,5 @@
>  
> +    // Offload the process of gzip'ing the payload to a worker: avoid the
> +    // memory hit of the gzip library until it is needed, let the worker's
> +    // process do the work instead of the system's app process.
> +    this.gzipHelper = new GzipHelper('shared/js/gzip/gzip_worker.js');

I'd suggest you hide the path inside the helper class. That should be an implementation detail.  (In fact, GzipHelper ought to be completely contained, so that users of it can't tell whether it is using a worker or not.)

@@ +365,4 @@
>      var data = JSON.stringify(this.packet);
> +    this.gzipHelper.gzip(data).then(function(gzipData) {
> +      // The gzip code is no longer needed, unload it.
> +      self.gzipHelper.unload();

Instead of requiring this to be explicitly called, GzipHelper could create the worker when needed, then kill it after 10 seconds of inactivity (or something like that).  If you do that, then the use of the worker really becomes completely an implementation detail.

@@ +366,5 @@
> +    this.gzipHelper.gzip(data).then(function(gzipData) {
> +      // The gzip code is no longer needed, unload it.
> +      self.gzipHelper.unload();
> +
> +      xhr.setRequestHeader('Content-type', 'application/x-gzip');

I'd think that you'd want:

Content-type: application/json
Content-encoding: gzip

But I'm not really familiar with HTTP, so I might be completely wrong about that.

@@ +370,5 @@
> +      xhr.setRequestHeader('Content-type', 'application/x-gzip');
> +      xhr.responseType = 'text';
> +
> +      var data = new Blob([gzipData], { type: 'application/x-gzip' });
> +      xhr.send(data);

You might be able to send the gzipData directly without creating the blob.

::: shared/js/gzip/gzip_helper.js
@@ +9,5 @@
> +  GzipHelper.prototype.gzip = function(payload) {
> +    var self = this;
> +    return new Promise(function(resolve, reject) {
> +
> +      if (self.worker) {

You test for the worker, but never handle the case where it doesn't exist.  If the user calls unload() before calling gzip(), then the returned promise will never resolve or reject and the app will stall out.

You could fix that by just adding an else reject clause.

But, I'd suggest that it would be better to simplify this api into a single gzip() function (Or a static GZip.gzip() function if you think you might want to add more to the GZip module in the future). gzip() can create the worker when needed, and terminate it on a timer, so that it stays alive when used frequently, but is killed when not used.

@@ +10,5 @@
> +    var self = this;
> +    return new Promise(function(resolve, reject) {
> +
> +      if (self.worker) {
> +        self.worker.onmessage = function(msg) {

There's a subtle bug here... If your code calls gzip twice, and the second call is made before the first one resolves, then the second onmessage handler will overwrite the first one, when the first gzip completes, it will resolve the second promise!

One way to avoid this is to simplify the API so that there is only one gzip() call, and it creates and destroys a separate worker for every call. Multiple calls would be inefficient, but that might be fine for your use case.

Or if you want to destroy the worker when it is idle, and allow multiple gzip requests on a single worker, then you may need to have some kind of id for each request and map the request id to the promise you returned for that request, so that you can then resolve the correct promise when you get a message from the worker.  (And the worker would have to pass the request id back)

::: shared/js/gzip/gzip_worker.js
@@ +8,5 @@
> +  if (cmd === 'gzip') {
> +
> +    var payload = message.data.payload;
> +
> +    // `deflate` returns a string containing the compressed payload

Hmm. A string. I assumed it would be an ArrayBuffer or Uint8Array.  So in addition to being under GPL, this library is dated since it is still using binary strings.

@@ +11,5 @@
> +
> +    // `deflate` returns a string containing the compressed payload
> +    var zippedPayload = RawDeflate.deflate(payload);
> +
> +    postMessage(zippedPayload);

For symmetry you might want to post an object here, and copy the cmd (and maybe requestID?) properties from the request message.

::: shared/js/gzip/readme.txt
@@ +5,5 @@
> +in an app.
> +
> +The gzip compression module, rawdeflate.js, is distrubuted under the
> +GNU General Public License, version 2 (GPL-2.0). The source code lives
> +here:  http://www.onicos.com/staff/iz/amuse/javascript/expert/deflate.txt

GPL might be problematic... doesn't it infect the rest of the code we ship it with?

We might not be able to use this library.
Attachment #8661876 - Flags: feedback?(dflanagan) → feedback-
(In reply to Tamara Hills [:thills] from comment #2)
> Comment on attachment 8661876 [details] [diff] [review]
> gzip compress in a worker
> 
> Hi Russ,
> 
> It looks good.  My only question is about whether we are supposed to put
> this in shared or not.  I'm hoping David can weigh in on this.  In a future
> release, I'm thinking we can utilize this compression for AppUsagePing and
> FtuPing once we change those over to use the Unified Telemetry format.
>
David seemed fine with putting it in 'shared', especially with the plan to have a wrapper around the lib that is also in 'shared'.
 
> One thing I would add is a debug statement before it gets compressed.  There
> is another debug statement of the payload that happens in telemetry.js, but
> that is after it gets compressed.

There is a debug statement in the `TelemetryRequest.getSettings` callback that logs the wrapper, which includes the payload. This does log the wrapper based on my testing so we're good.

> 
> Thanks,
> 
> -tamara
Assignee: nobody → rnicoletti
Status: NEW → ASSIGNED
I've updated the patch to address David's feedback. 

One minor issue I'm having is checking in the gzip lib code (gzip.min.js). There are many (hundreds) of jshint errors. I see a `build/eslint/xfail.lst` but no correspond jshint file. How does one suppress jshint errors without having to modify the source code itself?
Attachment #8661876 - Attachment is obsolete: true
Attachment #8663770 - Flags: feedback?(thills)
Attachment #8663770 - Flags: feedback?(dflanagan)
Comment on attachment 8663770 [details] [diff] [review]
bug-1177099.patch

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

Hi Russ,

It looks good.  I like this approach.  if you do the following at the top of the min.js, does it help?
// jshint ignore: start 

Thanks,
-ta,ara
Attachment #8663770 - Flags: feedback?(thills) → feedback+
(In reply to Tamara Hills [:thills] from comment #6)
> Comment on attachment 8663770 [details] [diff] [review]
> bug-1177099.patch
> 
> Review of attachment 8663770 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hi Russ,
> 
> It looks good.  I like this approach.  if you do the following at the top of
> the min.js, does it help?
> // jshint ignore: start 
> 
> Thanks,
> -ta,ara

Yes, that suppresses all the errors. Thanks for the tip.
Comment on attachment 8663770 [details] [diff] [review]
bug-1177099.patch

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

A couple of comments here, but overall, this looks like it is in good shape.

Note that your string encoding solution won't work, but you can use TextEncoder, I think.  (It works on desktop, anyway)

::: apps/system/js/advanced_telemetry.js
@@ +110,5 @@
>    AT.REPORT_URL = 'https://incoming.telemetry.mozilla.org/submit/telemetry';
>    // How often do we try to send the reports
>    // Can be overridden with metrics.advancedtelemetry.reportInterval setting.
> +  //AT.REPORT_INTERVAL = 24 * 60 * 60 * 1000;  // 1 DAY
> +  AT.REPORT_INTERVAL = 60 * 1000;  // 1 DAY

This looks like a change for testing. Did you mean to leave it in? 

Same question for the DEBUG and LOGINFO changes above.

@@ +357,4 @@
>        xhr.timeout = xhrAttrs.timeout;
>      }
>  
> +    this.gzipHelper = new GzipHelper();

Do you really need a class to instantiate here? Or could you just have a global GZipper.gzip() function

::: shared/js/gzip/gzip_helper.js
@@ +2,5 @@
> +
> +(function(exports) {
> +
> +  function GzipHelper() {
> +  }

I'm not convinced this needs to be a class.  Maybe just a single gzip() function or in a namespace like Gzipper.gzip()?

@@ +9,5 @@
> +
> +    // If a previous invocation has not completed, do not continue.
> +    if (this.worker) {
> +      return Promise.reject('gzip is in progress.');
> +    }

I don't think you need to do this. Just put the working in a local variable, so you have a different worker for each invocation.

@@ +16,5 @@
> +     * Create (and then destroy) the worker for each `gzip` invocation.
> +     * This simplified implementation is favored over creating a single
> +     * worker in the constructor that is shared by all the `gzip` invocations.
> +     * In that case, there is the possibility of a bug where invocation 2
> +     * completes before invocation 1; the second `onmessage` handler will have

iirc, the bug happened if invocation 2 started before invocation 1 resolved. That would overwrite the onmessage handler.

@@ +29,5 @@
> +
> +      // Protect against the worker failing for whatever reason.
> +      var workerTimeout = setTimeout(function() {
> +        reject('GzipWorker did not respond.');
> +      }, 2000);

I'm not sure you need this. If you do, consider doing a longer timeout.  And note that in this case you never terminate the worker.

@@ +31,5 @@
> +      var workerTimeout = setTimeout(function() {
> +        reject('GzipWorker did not respond.');
> +      }, 2000);
> +
> +      if (self.worker) {

I don't think you need this. I don't think that worker creation can fail. Or if it did, it would probably fail with an exception from the constructor, so you'd never get here anyway.  Using a local variable for the worker instead of a property simplifies this.

::: shared/js/gzip/gzip_worker.js
@@ +14,5 @@
> +    var buf = new ArrayBuffer(payload.length); // 1 byte for each char
> +    var uint8Data = new Uint8Array(buf);
> +    for (var i=0; i < payload.length; i++) {
> +      var charValue = payload.charCodeAt(i);
> +      if (charValue > 127) {

This isn't okay. If an app name has an accented character in it, you're going to fail to compress.

There's got to be a better way to utf8-encode a string into an array buffer. I'm pretty sure I've written that code by hand at some point.

Ah. See https://developer.mozilla.org/en-US/docs/Web/API/TextEncoder/encode
Attachment #8663770 - Flags: feedback?(dflanagan) → feedback+
Attachment #8664520 - Flags: review?(thills)
Attachment #8664520 - Flags: review?(dflanagan)
Comment on attachment 8664520 [details] [review]
[gaia] russnicoletti:Bug-1177099 > mozilla-b2g:master

r- only because I think you should have a test in apps/sharedtest/.  Just something that verifies that some known payload compresses to some known compressed bytes.

Consider adding a comment at the top of gzip.min.js to link to the original source, since you're already adding the jshint comment.

And consider renaming gzip_helper.js to just plain gzip.js

I don't think it is ideal for gzip.min.js to live in shared, but I can't really think of a better way to do it than what you're doing here, so you've got my r+ on that.
Attachment #8664520 - Flags: review?(dflanagan) → review-
Comment on attachment 8664520 [details] [review]
[gaia] russnicoletti:Bug-1177099 > mozilla-b2g:master

Hi Russ,

(I'm canceling my review as I'm not really a peer to review system)

The code looks good, but the unit tests are broken. I know David has asked you to write an integration test for this.  Since you are doing this, it might suffice to just fix up the unit tests and add a few test to advanced_telemetry_test.js.  I'm thinking the following tests:
1.  Make sure that GzipHelper.gzip gets called when send is called and
2.  Make sure that it retries when it fails.

Thanks,

-tamara
Attachment #8664520 - Flags: review?(thills)
QA Whiteboard: [COM=Telemetry]
Attachment #8664520 - Attachment is obsolete: true
Comment on attachment 8671708 [details] [review]
[gaia] russnicoletti:bug-1177099 > mozilla-b2g:master

Hi Marshall, my assumption is you will review the system app changes and djf will review the 'shared' changes. 

Regarding the advanced_telemetry.js changes, I introduced the promise chains for unit testing. I saw that was a pattern being used by other modules to test async code (e.g., settings/js/panels/improve_browser_os/panel.js).
Attachment #8671708 - Flags: review?(marshall)
Comment on attachment 8671708 [details] [review]
[gaia] russnicoletti:bug-1177099 > mozilla-b2g:master

Hi David, the shared worker unit test is working properly. It turned out to be very simple.

Please review the 'shared' changes. I have asked Marshall to review the system app changes.
Attachment #8671708 - Flags: review?(dflanagan)
Comment on attachment 8671708 [details] [review]
[gaia] russnicoletti:bug-1177099 > mozilla-b2g:master

I think the move to Promises in the request pipeline make sense here, but I have some concerns about general lack of error handling and fallback in this code. See some of my comments on github, but broadly speaking:

* The request code should fallback to plain text if for whatever reason the GZip encoding of the request content fails

* It isn't clear to me the behavior of the gzip.min.js library, but I assume there are several different error conditions that could occur when trying to compress data.. those should probably all be handled in the gzip worker, and bubbled up to the request code
Attachment #8671708 - Flags: review?(marshall) → review-
Comment on attachment 8671708 [details] [review]
[gaia] russnicoletti:bug-1177099 > mozilla-b2g:master

r+ from me. Thanks for getting the test working.

Marshall makes good points about error handling, and I'd like to see a try/catch in the worker where it calls gzip.encode() so that any errors the library throws get returned to gzip.js in a postMessage right away.

The r+ from me assumes that you'll have resolved any of my concerns in the process of resolving Marshall's concerns.
Attachment #8671708 - Flags: review?(dflanagan) → review+
Comment on attachment 8671708 [details] [review]
[gaia] russnicoletti:bug-1177099 > mozilla-b2g:master

Hi Marshall, thanks for your comments. Regarding your principle review comments:

I've added error handling around the gzip library. It doesn't explicitly throw or return any errors, but the error handling is there in case it throws an unexpected error. The corresponding test I've put in 'sharedtest'. The test is somewhat non-standard as you'll see. I'm open to improvements.

I've also updated the telemetry code so that it falls back to sending uncompressed payload in the event the compression fails.
Attachment #8671708 - Flags: review- → review?(marshall)
Hi Marshall,

Just looking for an update on when you think you'll be able to get to review.

Thanks!

-tamara
Flags: needinfo?(marshall)
Comment on attachment 8671708 [details] [review]
[gaia] russnicoletti:bug-1177099 > mozilla-b2g:master

The error handling here looks much better, thanks Russ!
Flags: needinfo?(marshall)
Attachment #8671708 - Flags: review?(marshall) → review+
Master: https://github.com/mozilla-b2g/gaia/commit/c0dd1d46fae3fc67f7f47918a33becf3ec637b19
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Verified@
Build ID               20151106013953
Gaia Revision          1fb7e4b7904d577a1859b91269a85b363ddc2836
Gaia Date              2015-11-06 02:30:49
Gecko Revision         https://hg.mozilla.org/integration/mozilla-inbound/rev/f073181a927d63ae7abac86e47cc3872ccac7275
Gecko Version          45.0a1
Device Name            aries
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.worker.20151106.005858
Firmware Date          Fri Nov  6 00:59:06 UTC 2015
Bootloader             s1

Verify result:
The payload of the AdvancedTelemetry has been compressed as gzip format, and it can be decompressed on server side.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: