1059186 - Client needs to report number of generated URLs via Telemetry on Desktop

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect, P1)

Comment 1

[:Paolo Amadini]

The user Story on the bug mentions Firefox OS and Firefox Desktop, but the implementations will be separate and this bug, that uses Telemetry, will be specific to Desktop.

Comment 2

[:Paolo Amadini]

Attachment: The patch

This adds the telemetry histogram LOOP_CLIENT_CALL_URL_REQUESTS_SUCCESS.
 - LOOP_CLIENT_: The prefix I chose for Loop client histograms.
 - CALL_URL_REQUESTS: Indicates we're monitoring Call URL network requests.
 - _SUCCESS: Suffix commonly used for success/failures count histograms.

There is a new telemetry recording function on navigator.mozLoop since many cases in unprivileged code will need to report telemetry (the "share" and "copy" button clicks will be the next), and it doesn't make sense to add a separate privileged API for each of them.

Comment 3

[Marco Mucci [:MarcoM]]

Hi Paolo, can you provide a point value and if the bug should be marked as qe-verify+ or qe-verify- for verification.

Comment 4

[Romain Testard [:RT]]

Updating US to remove FxOS from the scope.

Comment 5

[Adam Roach [:abr]]

Comment on attachment 8480645 [details] [diff] [review]
The patch

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

This looks good to me. Note that you'll need someone from the desktop team to sign off on the changes to MozLoopAPI.jsm.

Comment 6

[:Paolo Amadini]

I'm dealing with a testing problem which seems to me derives from an attempt to have self-contained code and tests, that however are not quite so.

The code I'm trying to test, from the patch in this bug, is located in "client.js". Now, I tried adding a case to "client_test.js":

      it("should update success telemetry", function(done) {
        client.requestCallUrl("foo", function callback(err) {
          expect(err).to.be.null;

          sinon.assert.calledOnce(mozLoop.telemetryAdd);
          sinon.assert.calledWith(mozLoop.telemetryAdd,
                                  "LOOP_CLIENT_CALL_URL_REQUESTS_SUCCESS",
                                  true);

          done();
        });
      });

Unfortunately, the "hawkRequest" function in the mock mozLoop API injected into the client is just a stub, and it never calls its callback. Even with a more intelligent stub, I'm not sure about which parameters that function would be expected to return. I'm not even sure that it matters, as this function is only trying to test telemetry, so even if the actual hawkRequest return values change, the test should still succeed.

Moreover, this still isn't testing the telemetry itself, just unit testing the client API. So, for instance, if the histogram gets accidentally deleted from Histograms.json, we don't detect it.

On the other hand, "client_test.js" seems to be the only place in tests where the client API is used, other tests seem to mock it or follow other routes.

Real testing might require writing a complete browser-chrome test from scratch, based more or less on the FxA tests.

Dan, is this related to our conversation for which we should probably land this without tests given the current state of things, and only execute a manual test?

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to :Paolo Amadini from comment #6)
> 
> Real testing might require writing a complete browser-chrome test from
> scratch, based more or less on the FxA tests.
> 
> Dan, is this related to our conversation for which we should probably land
> this without tests given the current state of things, and only execute a
> manual test?

It sounds to make there is some complexity here that can be sorted out by splitting this up into multiple types of tests.  

In particular, client_test.js is really just for unit tests.  It should be testing that this unit of code is behavior correctly, both in terms of inputs and outputs, as well as how it drives the APIs of other modules (eg mozLoop.hawkRequest).  With that in mind, I think the thing you've written above really wants to be testing something like

it("should call mozLoop.telemetry add with the correct arguments")

which frames this test more concretely in terms of the unit of code under test.  (I've often found asking myself about the boundaries of the unit of code or behavior under test helpful in figuring out how to frame things crisply).

With that in mind, I think the test as written, with more complete stubbing, as you suggested, should be fine.  <http://sinonjs.org/docs/#stubs> has "good enough" documentation about how to do the stubbing you'll need, and there should also be plenty of examples in our other tests of more complex stubbing.

Separately from that, looking at the patch, it really touches two modules: the client.js code, that you've already found the tests for, and MozLoopAPI.jsm.  All the existing tests for MozLoopAPI are in browser/components/loop/test/mochitest.  Many of them are integration tests, there may well be unit tests mixed in there too.  In any case, you probably want to add an appropriate unit or integration test here.

Now, above and beyond that, as you point, you could also write an integration or functional test using browser-chrome or (later) marionette, that tests cross-module interaction.  I'm somewhat on the fence about whether that would be worth the effort given the relatively low level of code complexity here, so I personally probably wouldn't bother.

Comment 8

[:Paolo Amadini]

Attachment: Patch with tests

https://tbpl.mozilla.org/?tree=Try&rev=c7d6d487ae5d

Comment 9

[:Paolo Amadini]

Filed bug 1067845 for the failure when the test ran in the full suite. New tryserver build:

https://tbpl.mozilla.org/?tree=Try&rev=c5200aeee906

Comment 10

[Matthew N. [:MattN]]

I wonder why we don't just do this on the server and fix more important clients bugs…

It might be good to take bug 1065153 into account (at least for bitrot reasons) and/or to record the two types of call URLs separately.

Comment 11

[:Paolo Amadini]

(In reply to Matthew N. [:MattN] from comment #10)
> I wonder why we don't just do this on the server and fix more important
> clients bugs…

We actually do both of those things already :-)

This bug contains foundation work for all the other telemetry bugs.

> It might be good to take bug 1065153 into account (at least for bitrot
> reasons) and/or to record the two types of call URLs separately.

It's not clear to me whether bug 1065153 should block this one, or we can just go ahead and commit this first. This bug is currently tagged for uplift and finished while the other is not tagged and is in progress, so it might be easier to just check this in first. This also had a successful tryserver build already, after we fixed all the dependent bugs found while working on it:

https://tbpl.mozilla.org/?tree=Try&rev=db2699276624

Comment 12

[Matthew N. [:MattN]]

Comment on attachment 8489415 [details] [diff] [review]
Patch with tests

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

(In reply to :Paolo Amadini from comment #11)
> (In reply to Matthew N. [:MattN] from comment #10)
> > I wonder why we don't just do this on the server and fix more important
> > clients bugs…
> 
> We actually do both of those things already :-)

If we have the data on the server already, why do we also need it from the client for 34?

::: browser/components/loop/content/js/client.js
@@ +204,5 @@
> +    _telemetryAdd: function(histogramId, value) {
> +      try {
> +        this.mozLoop.telemetryAdd(histogramId, value);
> +      } catch (err) {
> +        console.log("Error recording telemetry", err);

console.error?

Comment 13

[:Paolo Amadini]

(In reply to Matthew N. [:MattN] from comment #12)
> If we have the data on the server already, why do we also need it from the
> client for 34?

One reason may be to calculate the ratio of generated and shared URLs. I believe bug 1015988 has more context here.

Comment 14

[:Paolo Amadini]

https://hg.mozilla.org/integration/fx-team/rev/8a6e14c73d94

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8a6e14c73d94

Comment 16

[u279076]

It's unclear to me if this is high priority for verification so I'm untracking this for now. I'd be happy to add a Moztrap test for this if we think it needs ongoing manual test coverage. However, I'll need clearer steps to test this if that's desired.

Comment 17

[:Paolo Amadini]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #16)
> It's unclear to me if this is high priority for verification so I'm
> untracking this for now. I'd be happy to add a Moztrap test for this if we
> think it needs ongoing manual test coverage. However, I'll need clearer
> steps to test this if that's desired.

I put detailed steps in bug 1015988, so that all telemetry can be verified when it is resolved.

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8489415 [details] [diff] [review]
Patch with tests

Approval Request Comment
Uplift request for patches staged and tested on Fig

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2d38bdaef1d4
https://hg.mozilla.org/releases/mozilla-aurora/rev/bc811bbf346d

Comment 20

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8489415 [details] [diff] [review]
Patch with tests

I worked with Randell and Maire on uplifting a large number of Loop bugs at once. All of the bugs have been staged on Fig and tested by QE before uplift to Aurora. As well, all of the bugs are isolated to the Loop client. Randell handled the uplift with my approval. I am adding approval to the bug after the fact for bookkeeping.

Comment 21

[Paul Silaghi, QA [:pauly]]

Verified in https://bugzilla.mozilla.org/show_bug.cgi?id=1015988#c43