Closed Bug 1042951 Opened 10 years ago Closed 10 years ago

Report search activity searches with FHR

Categories

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

All
Android
defect

Tracking

(firefox35 verified, fennec35+)

VERIFIED FIXED
Firefox 35
Tracking Status
firefox35 --- verified
fennec 35+ ---

People

(Reporter: Margaret, Assigned: rnewman, Mentored)

References

Details

(Whiteboard: [lang=java][terrible first bug][shovel-ready])

Attachments

(4 files, 1 obsolete file)

rnewman, any advice about how to approach this?
Flags: needinfo?(rnewman)
Blocks: search
Two work items here:

* Adding a new measurement type to FHR.
* Writing instances of that measurement into the FHR data store.

The former is straightforward: see, e.g., Bug 873496. So let's focus on the latter.


There are a few ways to go about this.

Half of the problem is how to determine the current environment hash. The other half is how to trigger a write.


One approach (if your code is in android-sync, or you can move it there, or snapshot your dependency on the FHR code for your out-of-mc development) is to do exactly what SubmissionsTracker does:

https://github.com/mozilla-services/android-sync/blob/develop/src/main/java/org/mozilla/gecko/background/healthreport/upload/AndroidSubmissionClient.java#L321


Another approach is to get the current environment from the browser (e.g., by having Fennec write it into a known shared pref), and use the FHR ContentProvider (which is somewhat bitrotted, but not too hard to get working for insertions) to store your data. This is totally decoupled: yay ContentProviders!


The final approach is to use a dead drop. Write your events somewhere, and periodically dump them into FHR from within BrowserHealthRecorder -- "oh hey, the user searched three times using the search activity; let me record those". This will be most straightforward, particularly if your activity searches will just be recorded alongside bartext/barsuggest/barkeyword.


I'm happy to spend a little time on video talking through thorny areas. Let me know.
Flags: needinfo?(rnewman)
Priority: -- → P1
Assignee: margaret.leibovic → eric.edens
Yesterday we talked about using a refactored version of BrowserHealthRecorder directly from the search activity. After thinking about that approach, I'm a bit concerned for two reasons.

 1. Introducing a strong compile-time dependency between the search activity and BrowserHealthRecorder. There's a lot of gecko detail leakage that search activity shouldn't have to care about, and it could make changing BrowserHealthRecorder more difficult in the future.
 2. Refactoring, and possibly causing fallout, in some very tricky code in BrowserHealthRecorder.

How about this: we create a service that accepts the FHR events. If gecko is available, the events get handled immediately. Otherwise, they get queued for later. This is pretty similar to your dead drop idea from above.
Flags: needinfo?(rnewman)
(In reply to Eric Edens [:eedens] from comment #2)

>  1. Introducing a strong compile-time dependency between the search activity
> and BrowserHealthRecorder. There's a lot of gecko detail leakage that search
> activity shouldn't have to care about, and it could make changing
> BrowserHealthRecorder more difficult in the future.

Part of the point of refactoring is to separate Gecko stuff from the recorder you'd use.

You'd essentially be making a read-only BHR; this would have no dependency on Gecko, just an implied dependency on another read-write BHR in order to get a PIC.

You'd have a compile-time dependency between the search activity and your recorder class, or an interface that it offers, but I don't see that as a huge obstacle.


>  2. Refactoring, and possibly causing fallout, in some very tricky code in
> BrowserHealthRecorder.

Where's your sense of adventure? ;P


> How about this: we create a service that accepts the FHR events. If gecko is
> available, the events get handled immediately. Otherwise, they get queued
> for later. This is pretty similar to your dead drop idea from above.

I think this is acceptable, but it accrues details elsewhere -- e.g., when to flush (is the browser running yet? how about now?), making sure you got the timestamps/envs right, avoiding accidental cross-process messaging, making sure that the BHR that just came up is using the right profile...

And the big ones: getting access to the current GeckoApp's BHR instance from a service, and making sure it's initialized. You'll end up running into the same intimidating aspects of BHR that you're seeing now -- after all, the service is basically half of what we were discussing.

In short: make sure that you're not just running into a separate pile of difficulties to avoid touching BHR. Talk it through with Margaret!
Flags: needinfo?(rnewman)
Priority: P1 → --
Status: NEW → ASSIGNED
Eric, how far did you get with this? Do you have a WIP to share? You can take this off your plate if you have too many other bugs to work on right now, but I just want to make sure we don't lose your work.
Flags: needinfo?(eric.edens)
Priority: -- → P1
(In reply to :Margaret Leibovic from comment #4)
> Eric, how far did you get with this? Do you have a WIP to share? You can
> take this off your plate if you have too many other bugs to work on right
> now, but I just want to make sure we don't lose your work.

No code; I know we talked about having an intermediary service that would queue events. It might be easier, however, to use the same approach as in bug 1042956 for telemetry.
Assignee: eric.edens → nobody
Flags: needinfo?(eric.edens)
This may be tricky, but there's nothing blocking us from starting on this.
Whiteboard: shovel-ready
Status: ASSIGNED → NEW
rnewman, do you want to take this bug?
Flags: needinfo?(rnewman)
Eventually, most likely :)
Mentor: rnewman
Flags: needinfo?(rnewman)
Whiteboard: shovel-ready → [lang=java][terrible first bug][shovel-ready]
The eventuality has arrived.
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
tracking-fennec: --- → 35+
This speculatively draws a distinction between the search activity and the widget.

Unfortunately, it bumps the version for the FHR measurement in order to define the new type. Expect a doc update to follow.

I have considered hacking around it, but we'll see.
Attachment #8500801 - Flags: review?(margaret.leibovic)
This adds a very simple static queue on BrowserHealthRecorder, and empties it in onResume.

That means that searches will be processed as soon as the user launches the browser.

We will lose searches that are performed from the activity, don't result in Fennec being launched, and aren't followed by Fennec being launched before being evicted from memory.

I consider that acceptable, but counter-arguments are welcome!
Attachment #8500802 - Flags: review?(margaret.leibovic)
This calls the new queuing method from SearchActivity.

As far as I can tell, this is the right place.

What I *don't* know is how to differentiate between the widget and the launched activity. Could you give me some pointers?
Attachment #8500804 - Flags: feedback?(margaret.leibovic)
This should do the trick from a doc standpoint.
Attachment #8500808 - Flags: review?(bcolloran)
(In reply to Richard Newman [:rnewman] from comment #12)

> What I *don't* know is how to differentiate between the widget and the
> launched activity. Could you give me some pointers?

Do we need to differentiate?
We don't need to, but someone will eventually ask.

I'm thinking this: if it's easy, let's do it. It gives us the information to decide whether anyone uses the widget, and we can still get accurate totals.

If it's hard, 0fg, I won't bother. Margaret will know.
Also note that we do have UI Telemetry on the widget
Then I won't feel bad if I don't end up bothering :)
Comment on attachment 8500808 [details] [diff] [review]
Part 4: document new search locations. v1

Looks good, thanks for staying on top of the docs Richard! :-)
Attachment #8500808 - Flags: review?(bcolloran) → review+
Comment on attachment 8500804 [details] [diff] [review]
Part 3: record searches from SearchActivity. v1

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

::: mobile/android/search/java/org/mozilla/search/SearchActivity.java
@@ +264,5 @@
> +            } catch (Exception e) {
> +                // This should never happen: it'll only throw if the
> +                // search location is wrong. But let's not tempt fate.
> +                Log.w(LOGTAG, "Unable to record search.");
> +            }

This will re-record the search if the activity is being restored:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/search/java/org/mozilla/search/SearchActivity.java#165

Would we want to count that twice?

If not, you could put this in onSearch, next to where we store the search query.
Attachment #8500804 - Flags: feedback?(margaret.leibovic) → feedback+
Comment on attachment 8500801 [details] [diff] [review]
Part 1: bump searches measurement version to include search activity sources. v1

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

r+, but you can get rid of the widget bit if we don't want to pursue that.

::: mobile/android/base/health/BrowserHealthRecorder.java
@@ +646,5 @@
>      public static final Set<String> SEARCH_LOCATIONS = Collections.unmodifiableSet(new HashSet<String>(Arrays.asList(new String[] {
> +        "barkeyword",    // A search keyword (e.g., "imdb star wars").
> +        "barsuggest",    // A suggestion picked after typing in the search bar.
> +        "bartext",       // Raw text in the search bar.
> +        "activity",      // The standalone search activity.

Technically speaking, this isn't "standalone". It's more "search activity, not browser URL bar".

@@ +647,5 @@
> +        "barkeyword",    // A search keyword (e.g., "imdb star wars").
> +        "barsuggest",    // A suggestion picked after typing in the search bar.
> +        "bartext",       // Raw text in the search bar.
> +        "activity",      // The standalone search activity.
> +        "widget",        // The search widget.

Yeah, I wouldn't worry too much about the widget, since it's just a different way to end up in the search activity, and we do have telemetry about it.
Attachment #8500801 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 8500802 [details] [diff] [review]
Part 2: allow for delayed recording of searches with FHR. v1

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

Seems reasonable to me, although I am a bit worried about dropping searches if we want to use these numbers to negotiate partnerships. Given that I myself will often find the answer I'm looking for in the search activity without launching Fennec, I think we could drop a non-trivial number of searches if we're just holding onto them in memory.

How hard would it be to store this on disk instead of in memory? Nothing in the UI depends on this, so could we just do it in some low-priority background thread kind of way?
Switched recording location per comment 19.
Attachment #8501257 - Flags: review?(margaret.leibovic)
Attachment #8500804 - Attachment is obsolete: true
(In reply to :Margaret Leibovic from comment #21)

> How hard would it be to store this on disk instead of in memory? Nothing in
> the UI depends on this, so could we just do it in some low-priority
> background thread kind of way?

What I'd probably do is flush the ConcurrentLinkedQueue to SharedPreferences. Let's see if I can write a patch…
Attachment #8501257 - Flags: review?(margaret.leibovic) → review+
Actually, let me punt that to a follow-up. I'd like to get something simple landed and tested before we add another iteration.
Comment on attachment 8500802 [details] [diff] [review]
Part 2: allow for delayed recording of searches with FHR. v1

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

Okay, r+, but file that follow-up! :)
Attachment #8500802 - Flags: review?(margaret.leibovic) → review+
Blocks: 1079584
"2014-10-08": { "5WVwbqBogt2zz5URGBj58Mz00lQ=": { "org.mozilla.searches.counts": { "activity": { "yahoo": 1 }, "_v": "6" } } },
Richard, do you need QE testing on this before we release Fennec 35?
Flags: qe-verify?
Flags: needinfo?(rnewman)
Yes. I'm confident in my manual testing, but this is in support of revenue, so it's worth double checking.

Test steps:

* Perform a search with the search activity.
* Click a result, so as to launch Fennec.
* Open about:healthreport, navigate to raw data, search for "activity". Verify that there's a count for the right engine.
* Repeat as needed with the same and different engines, ensuring that the count is correct.
* Data will be lost if you never open Fennec after a search.
Flags: qe-verify?
Flags: qe-verify+
Flags: needinfo?(rnewman)
Thank you Richard.

Ioana, please make sure this gets tested as part of your Fennec 35 cycle.
Flags: needinfo?(ioana.chiorean)
I verified this in Firefox 35 Beta 1 with Nexus 4 (Android 4.4.4) using steps from comment 30. 
I performed searches with different engines and the correct count is displayed in about:healthreport.
I will mark this as Verified fixed based on comment 32 and I will clear the needinfo since this was tested in Fennec 35 cycle.
Status: RESOLVED → VERIFIED
Flags: needinfo?(ioana.chiorean)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.