Closed
Bug 1126524
Opened 9 years ago
Closed 9 years ago
[Metrics] FxOS Collection of number of web searches
Categories
(Firefox OS Graveyard :: General, enhancement)
Tracking
(blocking-b2g:2.2+, b2g-v2.2 fixed, b2g-master fixed)
People
(Reporter: rdandu, Assigned: thills)
References
Details
Attachments
(4 files, 1 obsolete file)
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•9 years ago
|
Assignee: nobody → hkoka
Reporter | ||
Comment 1•9 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•9 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)
Comment 3•9 years ago
|
||
> 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)
Comment 4•9 years ago
|
||
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)
Comment 5•9 years ago
|
||
(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•9 years ago
|
||
(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)
Reporter | ||
Comment 7•9 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•9 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•9 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
Comment 10•9 years ago
|
||
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•9 years ago
|
Assignee: hkoka → thills
Status: NEW → ASSIGNED
Updated•9 years ago
|
Flags: in-moztrap?(slyu)
Assignee | ||
Comment 11•9 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)
Comment 12•9 years ago
|
||
I'm adding MNC/MCC already as part of the patch for Bug 1109422 :)
Flags: needinfo?(marshall)
Updated•9 years ago
|
Severity: normal → enhancement
Updated•9 years ago
|
Target Milestone: --- → 2.2 S6 (20feb)
Comment 13•9 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)
Assignee | ||
Comment 14•9 years ago
|
||
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 15•9 years ago
|
||
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
Updated•9 years ago
|
Attachment #8562263 -
Flags: feedback?(marshall) → feedback+
Comment 16•9 years ago
|
||
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•9 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)
Comment 18•9 years ago
|
||
Assignee | ||
Comment 19•9 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 20•9 years ago
|
||
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•9 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•9 years ago
|
Flags: needinfo?(rdandu)
Assignee | ||
Comment 22•9 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•9 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)
Comment 24•9 years ago
|
||
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•9 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•9 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 27•9 years ago
|
||
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 28•9 years ago
|
||
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-
Comment 29•9 years ago
|
||
BTW, feel free to squash and rebase when addressing my comments above :)
Comment 30•9 years ago
|
||
> 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 31•9 years ago
|
||
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•9 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)
Comment 33•9 years ago
|
||
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•9 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•9 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•9 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 37•9 years ago
|
||
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 | ||
Comment 38•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=gaia-try&revision=fa00f0803351
Assignee | ||
Comment 39•9 years ago
|
||
https://github.com/mozilla-b2g/gaia/commit/8c8069ecbc8d7490782abe468c0492d578fc9e56
Assignee | ||
Updated•9 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Comment 40•9 years ago
|
||
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•9 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)
Updated•9 years ago
|
Attachment #8565626 -
Flags: approval-gaia-v2.2?(release-mgmt) → approval-gaia-v2.2+
Comment 42•9 years ago
|
||
v2.2: https://github.com/mozilla-b2g/gaia/commit/0cd14e8b64d5e750588adacf4ad027aa90889648
status-b2g-v2.2:
--- → fixed
status-b2g-master:
--- → fixed
Target Milestone: 2.2 S6 (20feb) → 2.2 S7 (6mar)
Assignee | ||
Comment 43•9 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}}}}
Comment 44•9 years ago
|
||
Updated•9 years ago
|
Flags: in-moztrap?(slyu) → in-moztrap+
You need to log in
before you can comment on or make changes to this bug.
Description
•