1126524 - [Metrics] FxOS Collection of number of web searches

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, enhancement)

Description

[Ravi Dandu]

FxOS Metrics should track number of searches done. This will be used for Revenue phase of FxOS product metrics.
The following information will be needed:
1) Track number of searches, and search provider.
2) Record partner id. There will be an id associated with each partner, mostly operators.

Comment 1

[Ravi Dandu]

Partner/operator IDs storage for Search is being reworked in  https://bugzilla.mozilla.org/show_bug.cgi?id=1125131. 

Marshall Culpepper comments on email: To track the searches for a specific provider, we’d need to know a few things:
1) Either specific integration in the search code, or a DOM event we can plug into for a new search that was transmitted
2) The search provider ID

Do we do anything special to the search URL or HTTP headers to let the provider’s server know that the search is coming from FirefoxOS?

Comment 2

[Ravi Dandu]

Marshall, can you comment on where the changes will be needed to achieve this. eg: System/Settings/RIL/layers in GAIA/GECKO. This info will help other teams plan and contribute.

Comment 3

[Dale Harvey (:daleharvey)]

> Do we do anything special to the search URL or HTTP headers to let the provider’s
> server know that the search is coming from FirefoxOS?

We can use and put whatever we want in the url to indicate it is coming from fxos, some of them do this in some way already - https://github.com/mozilla-b2g/gaia/blob/master/shared/js/search_providers.json#L6

This lets providers know where the searches come from, Mozilla wont know any more. 

If Mozilla need to know what searches have been performed it would be fine to modify the search code in order to notify someone but its not something I know a lot about / have seen specs about at this point

Comment 4

[Marshall Culpepper [:marshall_law]]

If we want to collect this info on our telemetry server, at least two things will need to happen:

1.  Hooks for notifying the system app when a search is performed inside the Gaia search code, probably starting somewhere around: https://github.com/mozilla-b2g/gaia/blob/master/apps/search/js/search.js#L233

2. A new collector / submitter that ferries this data off to the Telemetry backend, in the vein of FTU ping or AppUsageMetrics, making use of the new TelemetryRequest code that will land in Bug 1037495

Does the wording for our current metrics opt-in/opt-out setting as seen in FTU already cover search?

Comment 5

[Kevin Grandon :kgrandon]

(In reply to Marshall Culpepper [:marshall_law] from comment #4)
> If we want to collect this info on our telemetry server, at least two things
> will need to happen:
> 
> 1.  Hooks for notifying the system app when a search is performed inside the
> Gaia search code, probably starting somewhere around:
> https://github.com/mozilla-b2g/gaia/blob/master/apps/search/js/search.js#L233

The system app already knows about searches as it hits rocketbar.js before being dispatched to the search frame. Perhaps we could use this as a hook? https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/rocketbar.js#L513

Comment 6

[Marshall Culpepper [:marshall_law]]

(In reply to Kevin Grandon :kgrandon from comment #5)
> The system app already knows about searches as it hits rocketbar.js before
> being dispatched to the search frame. Perhaps we could use this as a hook?
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/rocketbar.
> js#L513

Ah, even better! It looks like handleSubmit() might be even more well suited to this task, assuming we only want to count submitted searches (and not suggestions / real time rocketbar changes)

Comment 7

[Ravi Dandu]

This is proposed for 2.2.  Important for us to be able to track number of searches since that enables us to make data driven decisions on how much revenue can be generated by User activities, how to structure search partnerships, and how to divide the generated revenue generated among partners.

Comment 8

[Hema Koka [:hema]]

Update: Checking to see if Tamara Hills could help with this new feature ask for 2.2. We had initial meetings on it and she is currently digging into the code to assess feasibility.

Thanks
Hema

Comment 9

[Hema Koka [:hema]]

Ravi, 

Please note that this is a feature ask not a bug in a feature that exists. So, please use the feature flags for nominating so it falls in the radar of 2.2 release owner (Thomas Ho) and EPMs (Kevin Hu) tracking it. 

Thanks
Hema

Comment 10

[Marshall Culpepper [:marshall_law]]

It seems like the best way forward for Metrics reporting of searches in 2.2 would be not to rely (only) on the new partner codes defined in Bug 1125131, but start first with just MCC/MNC. We could incorporate the partner code too, if there is no SIM card in the device, but I suspect it will be a while before we see any builds with those partner codes actually set..

Based on our conversation, we'll report a new map in AppUsageMetrics that includes the provider string as the key, and the number of searches performed as the value. How this will look exactly in the payload is TBD, but something along the lines of..

{
  "ver": 1,
  "info": {
    "searches":
      "yahoo": 1,
      "google": 2
     },...
   }
}

Comment 11

[Tamara Hills [:thills]]

Hi Marshall,

Quick question.  Should the MCC/MNC be part of the new map in AppUsageMetrics Map or at a higher level so that it can be used for all AppUsage payloads?

Thanks,

-tamara

Comment 12

[Marshall Culpepper [:marshall_law]]

I'm adding MNC/MCC already as part of the patch for Bug 1109422 :)

Comment 13

[Michael:mtreese]

added: "blocks: 1131315: to enable tracking on 2.2 dashboard (https://wiki.mozilla.org/Firefox_OS/TwoDotTwoTracking/Dashboard#FxOS_Metrics_.28Michael_Treese.29)

Comment 14

[Tamara Hills [:thills]]

Attachment: WIP for feedback

Hi Marshall,

Just wanted to put something up for feedback to get your thoughts on this approach.  

Thanks,

-tamara

Comment 15

[Marshall Culpepper [:marshall_law]]

Comment on attachment 8562263 [details] [diff] [review]
WIP for feedback

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

This is a great start! Don't forget to add some tests :)

Can you also categorize the searches by day (the same way we do with app usage data?) That could help us see more details in the future.. something like:

{
  searches: {
    $provider: {
      20150101: 1,
      20150102: 2,
      ..
    }
  }
}

You can reuse the function |getDayKey()| to fill the keys for each day in the patch here:
https://github.com/mozilla-b2g/gaia/pull/27286/files#diff-cc8fa85f73ada24b3dea2bce82e86e92R756

Comment 16

[Shing Lyu [:shinglyu]]

If a user performs a search without Internet connection, will the search be counted and should it be counted? 

We know that the user is intended to do a web search, but from a search provider's view, no revenue is generated.

Comment 17

[Tamara Hills [:thills]]

Hi Shing Lyu,

I think this is a good point.  My $.02 is that we should not count it. I'm NI to Ravi just to be sure.

I'm making change to check if we are online.

Thanks,

-tamara

Comment 18

[GitHub Autolander Bot]

Attachment: [gaia] tamarahills:bugfix/1126524-collect-search-metrics > mozilla-b2g:master

Comment 19

[Tamara Hills [:thills]]

Attachment: PR for search metrics

Hi Marshall,

Here's a patch with some tests as well.  It's all merged in with the latest update you landed.

Thanks,
-tamara

Comment 20

[Marshall Culpepper [:marshall_law]]

Comment on attachment 8565638 [details] [review]
PR for search metrics

Looks great, thanks for the new unit tests! See my comments in github.. I have a few questions about the provider key being used internally.

Could you also add a unit test into rocketbar for the new event being fired?

Comment 21

[Ravi Dandu]

Tamara, 
1) Does the offline search return any local results?
2) Lets only count searches which are online for now. They contribute to revenue. We can have a future requirement, where we can mark how many are online, and offline. That can be used to compute the user engagement with the search feature.

Comment 22

[Tamara Hills [:thills]]

Hi Ravi,

1) the offline search will return some local results.  For example, if you look for 'music', you will see the music app

2) Right now, this is the way I have it.  Any local search would not be counted as it's offline.

Thanks,

-tamara

Comment 23

[Tamara Hills [:thills]]

Hi Dale,

I'm a bit stuck on this part of the SearchProvider object.  Here's the line in my PR where I'm calling it.

I've tried calling the ready() like in [2], but it never seems to return.  The really odd thing is that I print out console statements from SearchProvider.providersLoaded at [3] and it actually prints out 'Google'.  Then when I subsequently call my line in [1], i get back a false.  

In addition, I also tried mimicking what is done in [4], but that didn't seem to work either.

The really odd thing is that after I do a make reset-gaia and then reboot, everything works fine.  There is also this comment at [5], but I'm not sure if this has anything to do with it.  

Thanks in advance for any guidance you might have.

[1] https://github.com/mozilla-b2g/gaia/pull/28275/files#diff-5699b1000bc807368718bfddf7ef58c3R574
[2] https://github.com/tamarahills/gaia/blob/master/apps/settings/js/panels/search/search.js#L13
[3] https://github.com/tamarahills/gaia/blob/master/shared/js/search_provider.js#L71
[4] https://github.com/tamarahills/gaia/blob/master/apps/search/js/providers/suggestions.js#L12
[5] https://github.com/tamarahills/gaia/blob/master/shared/js/search_provider.js#L66

Comment 24

[Dale Harvey (:daleharvey)]

Hey Tamara

It looks like rocketbar.js is the wrong place for this, the system app side is only supposed to deal with the UX whereas the search application is what handles the provider logic in general.

https://github.com/mozilla-b2g/gaia/blob/master/apps/search/js/search.js#L251 is where the searches get triggered from (there is some filtering for urls in there that rocketbar doesnt do) and it already has searchProvider in scope and initialised so shouldnt be any problems

Comment 25

[Tamara Hills [:thills]]

Hi Dale,

Thanks for your response.  That was helpful.  I'm able to work with the SearchProvider fine now.  I had another question for you.  Now that I'm putting this logic into the search app, I believe the window.dispatchEvent doesn't work across the app, (but wanted to confirm that for sure with you).  I'm looking at using postMessage capability to get the message from search->system app.  Just want to check if that is the ok/preferred approach.

Thanks again,

-tamara

Comment 26

[Tamara Hills [:thills]]

Attachment: Updated PR using search app and PostMessage

Hi Marshall,

Here's a version of the patch that takes Dale's comment about using search.js as opposed to rocketbar due to the initialization concerns we found with the |make reset-gaia|.  As a result of that I had to use a postMessage so there are some additional changes due to this.

Please let me know if you'd like me to squash the patch.  Right now, it's two separate commits which can be confusing because some of the rocketbar.js and rocketbar_test.js changes will still show up in the second patch(but it's really just undoing them).  Let me know how you'd prefer.

Thanks,

-tamara

Comment 27

[Kevin Grandon :kgrandon]

Comment on attachment 8569617 [details] [review]
Updated PR using search app and PostMessage

Dale - are you sure you want to do this in the search app and use IAC? Rocketbar.js already knows about the input and when the user submits something, so it feels less hacky to me to do it in rocketbar.js.

We can also tell when the user clicks on a suggestion or navigates the search app in the system app, which should give us enough metrics.

Comment 28

[Marshall Culpepper [:marshall_law]]

Comment on attachment 8569617 [details] [review]
Updated PR using search app and PostMessage

Great work Tamara! I like the deeper integration with the search app, but I have some important follow up questions in github that I want to make sure are addressed before giving r+. Assuming those are fixed, I think we are in pretty good shape

Comment 29

[Marshall Culpepper [:marshall_law]]

BTW, feel free to squash and rebase when addressing my comments above :)

Comment 30

[Dale Harvey (:daleharvey)]

> Dale - are you sure you want to do this in the search app and use IAC? Rocketbar.js already 
> knows about the input and when the user submits something, so it feels less hacky to me 
> to do it in rocketbar.js.

It seems like the metrics should be done in the same place the actual search is performed if that is what we are measuring otherwise we miss or duplicate functionality (like not counting urls as searches), if we moved the metrics back to the search app, seems like the actual navigation should be as well

Comment 31

[Dale Harvey (:daleharvey)]

Comment on attachment 8569617 [details] [review]
Updated PR using search app and PostMessage

Why are we maintaining another port as opposed to reusing the existing one? We already have the connection and coupling between rocketbar(system) and search app, it seems easier to keep it to a single coupling but that possibly depends on how other app metrics are being collected, is this the only metric we can / are collecting outside the system app?

If carry on doing it this way I would like to see the connection handled in a seperate file so the change to search.js is just 

  Metrics.report(type: 'search', provider: 'goog'});

(free to bikeshed on that)

Will clear review, agree with a few of marshalls comments too

Comment 32

[Tamara Hills [:thills]]

Hi Dale,

What I had originally thought about doing was to piggyback on the iac-search-results vs creating iac-app-metrics, but my reason for not doing this was 1) To get the message to the AppUsageMetrics module, you need a port.postMessage to RocketBar and then an additional dispatchMessage from RocketBar to .  2) I was thinking we might reuse this iac-app-metrics port in the future when we want to send metrics.  For example, I would think dialer/callscreen/SMS and other apps could use this if we track things like calls made, etc.  I believe all these could be iac as well.

Let me know if this makes sense.

Thanks,

-tamara

Comment 33

[Dale Harvey (:daleharvey)]

That makes sense, then as I mentioned into review I think it would be a good idea to move the iac logic into a shared module so the search app (and possibly future dialer etc) just need to AppMetric.post(data)

Comment 34

[Tamara Hills [:thills]]

Comment on attachment 8565626 [details] [review]
[gaia] tamarahills:bugfix/1126524-collect-search-metrics > mozilla-b2g:master

Hi Marshall,

Sorry, this took me a bit longer as i had to wrangle with some of the tests a bit.  This has the following:

1. I kept the approach of having a separate port for Metrics that we can reuse
2. I tried to make that reusable for other apps that we might want to collect form in future and put it in shared.
3. Addressed the review comments.

Thanks,

-tamara

Comment 35

[Tamara Hills [:thills]]

Comment on attachment 8565626 [details] [review]
[gaia] tamarahills:bugfix/1126524-collect-search-metrics > mozilla-b2g:master

Hi Marshall,

I found an issue that I need to fix, so clearing the review flag.

Thanks,

-tamara

Comment 36

[Tamara Hills [:thills]]

Comment on attachment 8565626 [details] [review]
[gaia] tamarahills:bugfix/1126524-collect-search-metrics > mozilla-b2g:master

Hi Marshall,

I realized that I kind of created the issue that I found via development process.  I had switched branches to do another review and then it wrote the data without searches in there and then of course searches was undefined.

I added a check on the load for this... I guess we can take it or leave it because there is not a deployed base for AppUsage yet, right?

Resetting the review flag.

Thanks,

-tamara

Comment 37

[Marshall Culpepper [:marshall_law]]

Comment on attachment 8565626 [details] [review]
[gaia] tamarahills:bugfix/1126524-collect-search-metrics > mozilla-b2g:master

Looks great! I have one question about a logic block I left on github, assuming that gets addressed, this looks good to land :).

Comment 38

[Tamara Hills [:thills]]

https://treeherder.mozilla.org/#/jobs?repo=gaia-try&revision=fa00f0803351

Comment 39

[Tamara Hills [:thills]]

https://github.com/mozilla-b2g/gaia/commit/8c8069ecbc8d7490782abe468c0492d578fc9e56

Comment 40

[Shing Lyu [:shinglyu]]

It's now in master only. Are you going to request for merging this to v2.2? The blocking-b2g flag still says 2.2+.

(In reply to Tamara Hills [:thills] from comment #39)
> https://github.com/mozilla-b2g/gaia/commit/
> 8c8069ecbc8d7490782abe468c0492d578fc9e56

Comment 41

[Tamara Hills [:thills]]

Comment on attachment 8565626 [details] [review]
[gaia] tamarahills:bugfix/1126524-collect-search-metrics > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Search Feature
[User impact] if declined: We won't be able to track the searches.
[Testing completed]: yes
[Risk to taking this patch] (and alternatives if risky): Should be low-medium risk.  
[String changes made]:None.

Comment 42

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/0cd14e8b64d5e750588adacf4ad027aa90889648

Comment 43

[Tamara Hills [:thills]]

Here is some sample payload.  The searches information at the end is new:

 {"start":1425992500719,"apps":{"app://ftu.gaiamobile.org/manifest.webapp":{"20150310":{"usageTime":94,"invocations":1,"installs":0,"uninstalls":0,"activities":{}}},"app://verticalhome.gaiamobile.org/manifest.webapp":{"20150310":{"usageTime":48,"invocations":5,"installs":0,"uninstalls":0,"activities":{}}},"app://settings.gaiamobile.org/manifest.webapp":{"20150310":{"usageTime":57,"invocations":3,"installs":0,"uninstalls":0,"activities":{}}}},"searches":{"DuckDuckGo":{"20150310":{"count":2}},"Yahoo":{"20150310":{"count":1}},"Google":{"20150310":{"count":1}},"Bing":{"20150310":{"count":1}}}} 
I/GeckoConsole( 9219): Content JS LOG: [AppUsage] app data: {"start":1425992500719,"apps":{"app://ftu.gaiamobile.org/manifest.webapp":{"20150310":{"usageTime":94,"invocations":1,"installs":0,"uninstalls":0,"activities":{}}},"app://verticalhome.gaiamobile.org/manifest.webapp":{"20150310":{"usageTime":48,"invocations":5,"installs":0,"uninstalls":0,"activities":{}}},"app://settings.gaiamobile.org/manifest.webapp":{"20150310":{"usageTime":57,"invocations":3,"installs":0,"uninstalls":0,"activities":{}}}},"searches":{"DuckDuckGo":{"20150310":{"count":2}},"Yahoo":{"20150310":{"count":1}},"Google":{"20150310":{"count":1}},"Bing":{"20150310":{"count":1}}}}

Comment 44

[Shing Lyu [:shinglyu]]

Attachment: MozTrap Cases