[Metrics] FxOS Collection of number of web searches

RESOLVED FIXED in 2.2 S7 (6mar)

Status

enhancement
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: rdandu, Assigned: thills)

Tracking

unspecified
2.2 S7 (6mar)
ARM
Gonk (Firefox OS)
Dependency tree / graph
Bug Flags:
in-moztrap +

Firefox Tracking Flags

(blocking-b2g:2.2+, b2g-v2.2 fixed, b2g-master fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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.
(Reporter)

Updated

4 years ago
Assignee: nobody → hkoka
(Reporter)

Updated

4 years ago
Depends on: 1125131
(Reporter)

Comment 1

4 years ago
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?
Flags: needinfo?(dale)
(Reporter)

Comment 2

4 years ago
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.
Flags: needinfo?(marshall)
> 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
Flags: needinfo?(dale)
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?
Flags: needinfo?(marshall)
(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
(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)

Updated

4 years ago
Depends on: 1037495
(Reporter)

Comment 7

4 years ago
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.
blocking-b2g: --- → 2.2+

Comment 8

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

4 years ago
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
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
     },...
   }
}
(Assignee)

Updated

4 years ago
Assignee: hkoka → thills
Status: NEW → ASSIGNED
Flags: in-moztrap?(slyu)
(Assignee)

Comment 11

4 years ago
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
Flags: needinfo?(marshall)
I'm adding MNC/MCC already as part of the patch for Bug 1109422 :)
Flags: needinfo?(marshall)

Updated

4 years ago
Severity: normal → enhancement

Updated

4 years ago
Target Milestone: --- → 2.2 S6 (20feb)

Updated

4 years ago
Depends on: 1131315

Updated

4 years ago
No longer depends on: 1131315

Comment 13

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

Comment 14

4 years ago
Posted patch WIP for feedback (obsolete) — Splinter Review
Hi Marshall,

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

Thanks,

-tamara
Attachment #8562263 - Flags: feedback?(marshall)
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
Attachment #8562263 - Flags: feedback?(marshall) → feedback+
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.
(Assignee)

Comment 17

4 years ago
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
Flags: needinfo?(rdandu)
(Assignee)

Comment 19

4 years ago
Hi Marshall,

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

Thanks,
-tamara
Attachment #8562263 - Attachment is obsolete: true
Attachment #8565638 - Flags: review?(marshall)
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?
Attachment #8565638 - Flags: review?(marshall) → review+
(Reporter)

Comment 21

4 years ago
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.
Flags: needinfo?(thills)
(Reporter)

Updated

4 years ago
Flags: needinfo?(rdandu)
(Assignee)

Comment 22

4 years ago
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
Flags: needinfo?(thills)
(Assignee)

Comment 23

4 years ago
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
Flags: needinfo?(dale)
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
Flags: needinfo?(dale)
(Assignee)

Comment 25

4 years ago
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
Flags: needinfo?(dale)
(Assignee)

Comment 26

4 years ago
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
Attachment #8569617 - Flags: review?(marshall)
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.
Attachment #8569617 - Flags: review?(dale)
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
Attachment #8569617 - Flags: review?(marshall) → review-
BTW, feel free to squash and rebase when addressing my comments above :)
> 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
Flags: needinfo?(dale)
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
Attachment #8569617 - Flags: review?(dale)
(Assignee)

Comment 32

4 years ago
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
Flags: needinfo?(dale)
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)
Flags: needinfo?(dale)
(Assignee)

Comment 34

4 years ago
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
Attachment #8565626 - Flags: review?(marshall)
(Assignee)

Comment 35

4 years ago
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
Attachment #8565626 - Flags: review?(marshall)
(Assignee)

Comment 36

4 years ago
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
Attachment #8565626 - Flags: review?(marshall)
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 :).
Attachment #8565626 - Flags: review?(marshall) → review+
(Assignee)

Updated

4 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
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
Flags: needinfo?(thills)
(Assignee)

Comment 41

4 years ago
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.
Flags: needinfo?(thills)
Attachment #8565626 - Flags: approval-gaia-v2.2?(release-mgmt)
Attachment #8565626 - Flags: approval-gaia-v2.2?(release-mgmt) → approval-gaia-v2.2+
(Assignee)

Comment 43

4 years ago
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}}}}
Depends on: 1141891
Depends on: 1141896
Flags: in-moztrap?(slyu) → in-moztrap+
(Assignee)

Updated

4 years ago
Blocks: 1226182
(Assignee)

Updated

4 years ago
No longer blocks: 1226182
You need to log in before you can comment on or make changes to this bug.