1141891 - [Metrics] Search through suggestions are not counted

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System, defect)

Description

[Shing Lyu [:shinglyu]]

*** Description
Search by clicking the search suggestions are not counted in the app usage ping.

*** Steps to Reproduce
Pre-requisite: the search suggestion is enabled.

1. Click the rocket bar
2. Type "Mozilla"
3. Click the first search suggestions 
4. Wait for the App Usage ping to reach the metrics server

*** Expected Results
Default search provider has a search count of 1

*** Actual Results
Search count = 0

*** Other Notes
Only the searchs performed by clicking the "magnifier glass" button on the keyboard are counted. Users are equally probable to use both methods, so the search count will be underestimated

*** Reproduction Frequency


*** Build

Comment 1

[Shing Lyu [:shinglyu]]

*** Reproduction Frequency
3/3

*** Build
  Gaia-Rev        3f032238a52f08e4c2f68a47ad065a96eb22d470
  Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/9ae4bcc9b5f2
  Build-ID        20150309162503
  Version         37.0
  Device-Name     flame
  FW-Release      4.4.2
  FW-Incremental  eng.cltbld.20150309.201258
  FW-Date         Mon Mar  9 20:13:09 EDT 2015
  Bootloader      L1TC000118D0

Comment 2

[bhavana bajaj [:bajaj]]

Looks like its a broken functionality from a new metrics related feature, so blocking on this

Comment 3

[GitHub Autolander Bot]

Attachment: [gaia] tamarahills:bugfix/1141891-add-suggestion-search > mozilla-b2g:master

Comment 4

[howie [:howie]]

Hi Tamara, so you are working on this? thanks.

Comment 5

[Tamara Hills [:thills]]

Comment on attachment 8576401 [details] [review]
[gaia] tamarahills:bugfix/1141891-add-suggestion-search > mozilla-b2g:master

Hi Marshall, sorry for the confusion.  I originally inadvertently posted this patch on the wrong bug.  I was wondering why the autolander didn't show up :)

Comment 6

[Tamara Hills [:thills]]

Hi Howie,

Yes, sorry I forgot to set to assigned.  I have posted a PR as well.

Thanks,
-tamara

Comment 7

[Marshall Culpepper [:marshall_law]]

Comment on attachment 8576401 [details] [review]
[gaia] tamarahills:bugfix/1141891-add-suggestion-search > mozilla-b2g:master

Looks great! We should probably have someone that works on search take a look too..

Comment 8

[Tamara Hills [:thills]]

Comment on attachment 8576401 [details] [review]
[gaia] tamarahills:bugfix/1141891-add-suggestion-search > mozilla-b2g:master

Hi Dale,

Would you be able to have a look?

Thanks,

-tamara

Comment 9

[Dale Harvey (:daleharvey)]

Comment on attachment 8576401 [details] [review]
[gaia] tamarahills:bugfix/1141891-add-suggestion-search > mozilla-b2g:master

This looks great, cheers

Comment 10

[Tamara Hills [:thills]]

https://treeherder.mozilla.org/#/jobs?repo=gaia-try&revision=903e6f09d2db
https://github.com/mozilla-b2g/gaia/commit/db48e2353effeb3e3f74dda7b9103ee7ce8bbfde

Comment 11

[Tamara Hills [:thills]]

Comment on attachment 8576401 [details] [review]
[gaia] tamarahills:bugfix/1141891-add-suggestion-search > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Search count metrics feature
[User impact] if declined: Suggested searches will not be counted and we will significantly undercount the metrics if we don't have this
[Testing completed]: yes
[Risk to taking this patch] (and alternatives if risky): low-medium.  Should be easy to backout as it's small.
[String changes made]:no

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Reverted for Gaia unit test failures.
v2.2: https://github.com/mozilla-b2g/gaia/commit/fd7fa0c8f7ca575aafc3568262247e529323f31f

https://treeherder.mozilla.org/logviewer.html#?job_id=73272&repo=mozilla-b2g37_v2_2

Comment 14

[Tamara Hills [:thills]]

Hi Dale,

I'm having some trouble with my suggestions_test.js when it gets uplifted to 2.2 and realized that this line (SearchProvider.ready()) is removed in master, but still exists in 2.2

master:
https://github.com/mozilla-b2g/gaia/commit/323d773af7c579cc7f33fbe571cc5c8631156b15#diff-a72719dbedcb33e670a082ee4341d876L36

2.2:
https://github.com/mozilla-b2g/gaia/blob/v2.2/apps/search/js/providers/suggestions.js#L36

It looks like bug 1131966 removes the line, but that change doesn't seem to be in 2.2.  Just wanted to know if this will be uplifted before I redo the unit test to work differently with 2.2.

Thanks!

-tamara

Comment 15

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/3c5565bcdf0045261e0fd76e700eb32c4bbf3dc0

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Unfortunately, this is still hitting test failures on v2.2 even with bug 1136664 uplifted first :(. Backed out again.
v2.2: https://github.com/mozilla-b2g/gaia/commit/f52a2ac8f42489587e95a9254045683e0454b2e6

https://treeherder.mozilla.org/logviewer.html#?job_id=76315&repo=mozilla-b2g37_v2_2

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Ugh, I obviously can't read. I have no idea why I thought bug 1136664 was going to help here.

Comment 18

[Shing Lyu [:shinglyu]]

Hi Tamara, 

Can we uplift this to 2.2?

Comment 19

[Dale Harvey (:daleharvey)]

So somehow a line that was removed in master did not get removed in 2.2

https://github.com/mozilla-b2g/gaia/commit/323d773af7c579cc7f33fbe571cc5c8631156b15 is the master commit, https://github.com/mozilla-b2g/gaia/commit/df92a3c0d8ed757c04b81085c260da9234c59981 is the 2.2 one

The |SearchProvider.ready()| was removed in master but not in the 2.2 commit, a little confused as to how that can happen since it wasnt a branch patch but I am not a git expert, Ryan do you have any ideas?

Tamara is gonna do a branch patch to remove that and hopefully the existing test should work as is.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

No clue, sorry.

Comment 21

[GitHub Autolander Bot]

Attachment: [gaia] tamarahills:bugfix/1141891-add-suggestion-search-2.2-branch-patch > mozilla-b2g:v2.2

Comment 22

[Tamara Hills [:thills]]

Comment on attachment 8582392 [details] [review]
[gaia] tamarahills:bugfix/1141891-add-suggestion-search-2.2-branch-patch > mozilla-b2g:v2.2

Hi Marshall,

I talked with Dale and we decided best approach is to make a branch patch.  Essentially, there was a merge conflict from bug 1131966 where a merge didn't get correctly resolved on the uplift.  So, we decided I should remove this line in the branch patch.  

BTW, I'm not sure what the process is for branch patch review, so just adding a review flag on this.  It's the same patch as the 3.0, except with this line removed: https://github.com/mozilla-b2g/gaia/pull/29097/files#diff-a72719dbedcb33e670a082ee4341d876L36

For reference here is the merge commit difference on bug 1131966
master commit:  https://github.com/mozilla-b2g/gaia/commit/323d773af7c579cc7f33fbe571cc5c8631156b15

2.2 commit:  https://github.com/mozilla-b2g/gaia/commit/df92a3c0d8ed757c04b81085c260da9234c59981

Comment 23

[Marshall Culpepper [:marshall_law]]

Comment on attachment 8582392 [details] [review]
[gaia] tamarahills:bugfix/1141891-add-suggestion-search-2.2-branch-patch > mozilla-b2g:v2.2

Looks good, just had one quick question about a removed line of code

Comment 24

[Ryan VanderMeulen [:RyanVM]]

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