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

VERIFIED FIXED in Firefox 34

Status

Hello (Loop)
Client
P1
normal
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: RT, Assigned: Paolo)

Tracking

unspecified
mozilla35
Points:
1
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-moztrap -
qe-verify -

Firefox Tracking Flags

(firefox34 fixed, firefox35 verified)

Details

(Whiteboard: [loop-uplift][fig:wontverify])

User Story

As a product manager I want to know daily the number of created URLs on Firefox desktop so that I know the drop-off rate between generated and shared URLs.

This will be using Telemetry and will be correlated to the data on bug 1015988 (daily number of shared URLs via Telemetry).

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Comment 1

3 years ago
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.
Blocks: 1015988
Summary: Client needs to report number of generated URLs via Telemetry → Client needs to report number of generated URLs via Telemetry on Desktop
(Assignee)

Updated

3 years ago
Depends on: 1059754
(Assignee)

Comment 2

3 years ago
Created attachment 8480645 [details] [diff] [review]
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.
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Attachment #8480645 - Flags: feedback?(adam)
Hi Paolo, can you provide a point value and if the bug should be marked as qe-verify+ or qe-verify- for verification.
Iteration: --- → 34.3
Flags: qe-verify?
Flags: needinfo?(paolo.mozmail)
Flags: firefox-backlog+
(Assignee)

Updated

3 years ago
Points: --- → 1
Flags: qe-verify?
Flags: qe-verify+
Flags: needinfo?(paolo.mozmail)
QA Contact: anthony.s.hughes
(Reporter)

Comment 4

3 years ago
Updating US to remove FxOS from the scope.
User Story: (updated)
No longer depends on: 1059754
Depends on: 1059754

Comment 5

3 years ago
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.
Attachment #8480645 - Flags: feedback?(adam) → feedback+

Updated

3 years ago
Iteration: 34.3 → 35.1

Updated

3 years ago
Priority: -- → P1
Whiteboard: [loop-uplift]
Target Milestone: mozilla34 → mozilla35
(Assignee)

Comment 6

3 years ago
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?
Flags: needinfo?(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.
Flags: needinfo?(dmose)
(Assignee)

Comment 8

3 years ago
Created attachment 8489415 [details] [diff] [review]
Patch with tests

https://tbpl.mozilla.org/?tree=Try&rev=c7d6d487ae5d
Attachment #8480645 - Attachment is obsolete: true
Attachment #8489415 - Flags: review?(MattN+bmo)
(Assignee)

Updated

3 years ago
Depends on: 1067845
(Assignee)

Comment 9

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

Updated

3 years ago
Iteration: 35.1 → 35.2
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.
(Assignee)

Comment 11

3 years ago
(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 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?
Attachment #8489415 - Flags: review?(MattN+bmo) → review+
(Assignee)

Comment 13

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

Comment 14

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/8a6e14c73d94
https://hg.mozilla.org/mozilla-central/rev/8a6e14c73d94
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
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.
Flags: qe-verify+ → qe-verify-
QA Contact: anthony.s.hughes
Whiteboard: [loop-uplift] → [loop-uplift][fig:wontverify]
(Assignee)

Comment 17

3 years ago
(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 on attachment 8489415 [details] [diff] [review]
Patch with tests

Approval Request Comment
Uplift request for patches staged and tested on Fig
Attachment #8489415 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/releases/mozilla-aurora/rev/2d38bdaef1d4
https://hg.mozilla.org/releases/mozilla-aurora/rev/bc811bbf346d

Updated

3 years ago
status-firefox34: --- → fixed
status-firefox35: --- → fixed
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.
Attachment #8489415 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified in https://bugzilla.mozilla.org/show_bug.cgi?id=1015988#c43
Status: RESOLVED → VERIFIED
status-firefox35: fixed → verified
Flags: in-moztrap-
You need to log in before you can comment on or make changes to this bug.