Closed Bug 1253319 Opened 4 years ago Closed 4 years ago

Add search counts measurements

Categories

(Firefox for Android :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 49
Tracking Status
firefox49 --- fixed

People

(Reporter: gfritzsche, Assigned: mcomella)

References

(Blocks 2 open bugs)

Details

Attachments

(3 files)

Search counts are a requested "core" metric.
We have an open question on how to best implement them though.
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/
Flags: needinfo?(mark.finkle)
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.
Flags: needinfo?(mark.finkle)
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.
Assignee: nobody → michael.l.comella
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 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 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 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.
Attachment #8751526 - Flags: review?(s.kaspari) → review+
Comment on attachment 8751527 [details]
MozReview Request: Bug 1253319 - Call incrementSearch in BrowserApp. r=sebastian

https://reviewboard.mozilla.org/r/52071/#review49409
Attachment #8751527 - Flags: review?(s.kaspari) → review+
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
Attachment #8751528 - Flags: review?(s.kaspari) → review+
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.
You need to log in before you can comment on or make changes to this bug.