1253319 - Add search counts measurements

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Georg Fritzsche [:gfritzsche]]

Search counts are a requested "core" metric.
We have an open question on how to best implement them though.

Comment 1

[Georg Fritzsche [:gfritzsche]]

The original design [0] just has this in the "core" ping.
This is an optional field, so it doesn't cost us data if we don't record anything.
Personally i lean towards just staying with that if that's

Putting it into the larger pings (main/saved-session) doesn't help us because we wanted to keep those opt-in.

Mark, can you expand on other options you considered?

0: https://docs.google.com/document/d/1z3kD4EBO5_xFke2NTar2GvlCIkJaTHxwz3IjRGpWC2U/

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

The main idea I had was this:
* We are collection 100% of event data on all channels
* We could add an event: (SEARCH, <method>, <engine>) -> where method is the how the search happened
* We'd do an SQL rollup for searches per day over a date range

This removes the need to track search separately. It also adds a nice event for searches, which is a unique subset of LOADURL events.

The downsides are: Event data is opt-in, and we probably want opt-out. Core ping is opt-out.

I guess we need to decide if this data is OK as opt-in or not. If there are other opt-out style event data, perhaps we should add an opt-out section of event data. We do that for Telemetry Histograms. I think that's how SEARCH_COUNT is being collected on desktop.

Comment 3

[Georg Fritzsche [:gfritzsche]]

For the needs of top-level dashboards and similar needs we'd want opt-out data.

We could move forward with taking this into the "core" ping now as well as adding an event.
If we have an other opt-out collection later, we could consider moving this to it.

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1253319 - Add SearchCountMeasurements & tests. r=sebastian

Review commit: https://reviewboard.mozilla.org/r/52069/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52069/

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1253319 - Call incrementSearch in BrowserApp. r=sebastian

Review commit: https://reviewboard.mozilla.org/r/52071/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52071/

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1253319 - getAndZero search counts in core ping builder. r=sebastian

Review commit: https://reviewboard.mozilla.org/r/52073/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52073/

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8751526 [details]
MozReview Request: Bug 1253319 - Add SearchCountMeasurements & tests. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52069/diff/1-2/

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8751527 [details]
MozReview Request: Bug 1253319 - Call incrementSearch in BrowserApp. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52071/diff/1-2/

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8751528 [details]
MozReview Request: Bug 1253319 - getAndZero search counts in core ping builder. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52073/diff/1-2/

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8751526 [details]
MozReview Request: Bug 1253319 - Add SearchCountMeasurements & tests. r=sebastian

https://reviewboard.mozilla.org/r/52069/#review49405

All those unit tests are great. I feel like having them and running them in automation has significantly improved our quality.

::: mobile/android/base/java/org/mozilla/gecko/telemetry/measurements/SearchCountMeasurements.java:77
(Diff revision 2)
> +    }
> +
> +    /**
> +     * Gets and zeroes search counts.
> +     *
> +     * We return ExtendedJSONObject for now because that's the format needed by the core telemetry ping.

This is okay; but your class javadoc states that this is a class that can be used independently from telemetry. You can still use it somehow; but maybe do not make this promise? :)

::: mobile/android/base/java/org/mozilla/gecko/telemetry/measurements/SearchCountMeasurements.java:84
(Diff revision 2)
> +    public static synchronized ExtendedJSONObject getAndZeroSearch(@NonNull final SharedPreferences prefs) {
> +        final ExtendedJSONObject out = new ExtendedJSONObject();
> +        final SharedPreferences.Editor editor = prefs.edit();
> +
> +        final Set<String> keysFromPrefs = prefs.getStringSet(PREF_SEARCH_KEYSET, Collections.<String>emptySet());
> +        for (final String engineWhereStr : keysFromPrefs) { // TODO: name

nit: Is this TODO a leftover or is it handled in one of the follow-up commits? Otherwise I don't know what is left to do. :)

::: mobile/android/base/java/org/mozilla/gecko/telemetry/measurements/SearchCountMeasurements.java:89
(Diff revision 2)
> +        for (final String engineWhereStr : keysFromPrefs) { // TODO: name
> +            final String key = getEngineSearchCountKey(engineWhereStr);
> +            out.put(engineWhereStr, prefs.getInt(key, 0));
> +            editor.remove(key);
> +        }
> +        editor.remove(PREF_SEARCH_KEYSET).apply();

nit: It took a while to find the call to "apply()" and I was pretty sure you forgot it. Maybe we can make it more explicit by moving it to a new line.

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8751527 [details]
MozReview Request: Bug 1253319 - Call incrementSearch in BrowserApp. r=sebastian

https://reviewboard.mozilla.org/r/52071/#review49409

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8751528 [details]
MozReview Request: Bug 1253319 - getAndZero search counts in core ping builder. r=sebastian

https://reviewboard.mozilla.org/r/52073/#review49411

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

I think the only thing I would have done differently is only passing the Context around and doing the SharedPreference handling inside the class (Hiding the usage of SharedPreferences as an implementation detail). However with all the static methods it's not easy to mock this in tests. In general this would have been a great use case for Dagger, where Dagger takes care of injecting the same SearchCountMeasurements instance into every class that needs it -> SearchCountMeasurements would not need to be all static.

Comment 14

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/integration/fx-team/rev/1f23135db4cec3ac7daf456f8945bee9283d5aa4
Bug 1253319 - Add SearchCountMeasurements & tests. r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/a47894b6ef587ffc13f783193338349593a53751
Bug 1253319 - Call incrementSearch in BrowserApp. r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/aac5cc71030702eb349c2d7b0c2509c8dcbc60c7
Bug 1253319 - getAndZero search counts in core ping builder. r=sebastian

Comment 15

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/integration/fx-team/rev/a7e57536b61c186973b8ef1a5efccf630e60d068
Bug 1253319 - Increment core ping version number. r=me

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1f23135db4ce
https://hg.mozilla.org/mozilla-central/rev/a47894b6ef58
https://hg.mozilla.org/mozilla-central/rev/aac5cc710307
https://hg.mozilla.org/mozilla-central/rev/a7e57536b61c