1057613 - Telemetry for search widget buttons

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Search Activity, defect, P1)

Description

[:Margaret Leibovic]

We should add telemetry probes for users hitting the various search widget buttons.

Comment 2

[Tristan Pollitt]

Hi Margaret, I'd like to take on this bug if possible. Could you point me in the right direction as to which classes I'll need to be looking at and changing?

Comment 3

[:Margaret Leibovic]

Hi Tristan! You should look at the SearchWidget class:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/search/java/org/mozilla/search/SearchWidget.java

However, our workflow for this search activity part of the tree is a bit different than just writing a patch directly in mozilla-central, since we also have a fennec-search github repo that allows us to work on the search activity independently of the rest of fennec.

So, you should clone this repo, and make a pull request with your changes there:
https://github.com/mozilla/fennec-search

Then I can help you generate a patch for us to land on mozilla-central as well.

For more details, see this mailing list post:
https://mail.mozilla.org/pipermail/mobile-firefox-dev/2014-August/000844.html

For an example of adding telemetry probes, you can look at bug 1042956. In general we try to use existing Event/Method names as much as possible. However, we may want to add a new WIDGET method to indicate that something came from the widget. Perhaps all 3 of these buttons should be Event LAUNCH with Method WIDGET, and then we can use the extras field to specify which button was hit.

mfinkle, what do you think?

Comment 4

[Tristan Pollitt]

Hi Margaret, thanks for your help! I've made some changes and submitted a pull request on GitHub https://github.com/mozilla/fennec-search/pull/78

Comment 5

[:Margaret Leibovic]

(In reply to Tristan Pollitt from comment #4)
> Hi Margaret, thanks for your help! I've made some changes and submitted a
> pull request on GitHub https://github.com/mozilla/fennec-search/pull/78

Thanks for the pull request! I left some comments there, but it looks good.

In order to generate a patch for mozilla-central, from your fennec-search repo, you should run `grunt export --tree <path to your m-c tree>`, and that will apply the changes to mozilla-central. Then you can follow the regular steps for generating a mercurial patch from that:
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

You can attach that patch to this bug and flag me for review. Then I'll review it and help you land it.

Sorry for the extra steps, but hopefully it shouldn't be too hard!

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Margaret and I chatted on IRC. Looks good to me.

Comment 7

[Tristan Pollitt]

I've made the changes you mentioned in the pull request, and executed 'grunt export --tree...' to bring those accross to my mozilla-central repo. I then ran a 'hg diff', and it's only showing the changes I've made to SearchWidget.java, but not the change I made to TelemetryContract.java. Any idea why?

Comment 8

[:Margaret Leibovic]

(In reply to Tristan Pollitt from comment #7)
> I've made the changes you mentioned in the pull request, and executed 'grunt
> export --tree...' to bring those accross to my mozilla-central repo. I then
> ran a 'hg diff', and it's only showing the changes I've made to
> SearchWidget.java, but not the change I made to TelemetryContract.java. Any
> idea why?

Oh, that's because we just have a copy of TelemetryContract.java in the fennec-search repo. The real version lives in mozilla-central, but we keep of copy of that class (and some other stubbed out classes) in fennec-search so that it will still build. That's confusing, I'm sorry!

So you'll just have to make the change manually to the TelemetryContract.java file in mozilla-central, then generate your patch.

Comment 9

[Tristan Pollitt]

Attachment: Added telemetry probes for search widget buttons

Thanks for clarifying, makes sense now. Here's the patch.

Comment 10

[:Margaret Leibovic]

Comment on attachment 8483756 [details] [diff] [review]
Added telemetry probes for search widget buttons

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

Looks great, thanks! I can land this patch for you. You should also update your pull request, and I'll merge that.

Comment 11

[Tristan Pollitt]

Thanks Margaret! I've updated the pull request.

Comment 12

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/b33d49538c4c
https://github.com/mozilla/fennec-search/commit/746d7f730504c9b43c5ba1596ebc3fce6a63b3f9

Thanks for the patch! Let me know if you need help finding more bugs to work on. If you're interested in more search activity work, we have some "shovel-ready" bugs listed here:
https://wiki.mozilla.org/Mobile/Projects/Search_activity_v1

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/b33d49538c4c

Comment 14

[Flaviu Cos]

Verified as fixed in build 35.0a1 (2014-09-08);
Device Google Nexus 7 (Android 4.4.4);