Closed Bug 1381460 Opened 7 years ago Closed 6 years ago

Create an "activity-stream-newtab" source for Search Telemetry

Categories

(Firefox :: New Tab Page, defect, P2)

defect

Tracking

()

RESOLVED WONTFIX
Tracking Status
firefox57 --- wontfix
firefox58 --- affected

People

(Reporter: andreio, Assigned: andreio)

References

Details

User Story

The work for this bug is also tracked in Activity Stream Github repo https://github.com/mozilla/activity-stream/issues/2729

Attachments

(1 file, 5 obsolete files)

The search component in Activity Stream [0] currently reuses "newtab" events to prevent tests from failing. We need to add a new SEARCH_SOURCE [1].

[0] https://github.com/mozilla/activity-stream/blob/e19a42b505ae919901be658aeed99bbf2176d24f/system-addon/content-src/components/Search/Search.jsx#L26-L32
[1] https://dxr.mozilla.org/mozilla-central/source/browser/modules/BrowserUsageTelemetry.jsm#42
User Story: (updated)
We need to make people aware of this new event source and be prepared for the telemetry/metrics changes that come with it.
Ideally this will make it in 57 so that we can turn on Activity Stream in nightly.
Flags: needinfo?(gfritzsche)
:harter can judge the impact on the pipeline side.
:florian or :mak should be good to review the patch for the new search source.

The main tests for BrowserUsageTelemetry are in browser/modules/test/browser/browser_UsageTelemetry*.js
There are also some docs for the module here: https://gecko.readthedocs.io/en/latest/browser/browser/BrowserUsageTelemetry.html
Flags: needinfo?(gfritzsche)
Is there anything else besides:
* adding a new event https://dxr.mozilla.org/mozilla-central/source/browser/modules/BrowserUsageTelemetry.jsm#42
* handling it in a similar fashion to newtab https://dxr.mozilla.org/mozilla-central/source/browser/modules/BrowserUsageTelemetry.jsm#438
* updating/adding tests
Flags: needinfo?(rharter)
FWIW, adding a new source means we end up with e.g. new SEARCH_COUNTS keys, like "google.activitystream".
We'll need to whitelist the new source here [1]. Besides that, I don't see any other pipeline impact. I'm CC'ing a few people to make sure I'm not missing anything.

Mike, will this cause any problems with the follow-on search probe[2]? I didn't see any whitelist in the code, so it looks like we're fine.

Anthony, am I missing anything? Also, props for refactoring this whitelist out into a shared constant. 

Dave, FYI.

[1] https://github.com/mozilla/python_mozetl/blob/master/mozetl/constants.py
[2] https://github.com/mozilla/followonsearch
Flags: needinfo?(rharter)
Flags: needinfo?(mozilla)
Flags: needinfo?(amiyaguchi)
Adding the new source the whitelist in mozetl should be the only necessary step from the perspective of the pipeline. It affects all search related datasets, as far as I know.
Flags: needinfo?(amiyaguchi)
I don't see any problems with the follow-in search probe.
Flags: needinfo?(mozilla)
Attached patch 1381460.patch (obsolete) — Splinter Review
Attachment #8887637 - Flags: review?(florian)
Assignee: nobody → andrei.br92
Comment on attachment 8887637 [details] [diff] [review]
1381460.patch

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

In the future, please include 8 lines of context in patches.

This looks good to me overall, but I'm not very comfortable reviewing this code, I would suggest Dexter to review the next version of the patch.

::: browser/modules/test/browser/browser_UsageTelemetry_content.js
@@ +2,4 @@
>  
>  const BASE_PROBE_NAME = "browser.engagement.navigation.";
>  const SCALAR_CONTEXT_MENU = BASE_PROBE_NAME + "contextmenu";
> +const SCALAR_ABOUT_NEWTAB = BASE_PROBE_NAME + "activitystream";

This patch is adding things to BrowserUsageTelemetry for activity stream, but replacing about_newtab test coverage. Should some of this test be duplicated to cover both about_newtab and activitystream, adjusting the value of browser.newtabpage.activity-stream.enabled as needed?
Attachment #8887637 - Flags: review?(florian) → feedback+
Attached patch 1381460.patch (obsolete) — Splinter Review
This patch adds an activitystream specific event for new tab searches. We need this to differentiate telemetry events.

This patch includes Florian's feedback: we now have two tests that turn Activity Stream on and run the search event test and off and run the test as usually.
 
An addition to this is the fact that browser newtab preloading is influencing newtab. If you toggle activity stream on or off via the pref you will still get the old newtab the first time you open because it was preloaded so we close the first one after the pref change and open a second. 

Alessio would you be the right person to review?
Attachment #8887637 - Attachment is obsolete: true
Flags: needinfo?(alessio.placitelli)
Comment on attachment 8888337 [details] [diff] [review]
1381460.patch

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

This looks good overall, thanks for the patch (and thanks :florian for the good feedback)! I just have a few questions, I left some comments below.

Please note that you should also add "activity_stream" to the event's object list, here: http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/toolkit/components/telemetry/Events.yaml#3

Did you check that the searches from activity stream don't get double counted in other scalars/events as well (e.g. about_newtab or about_home)? It might be worthwhile doing a manual QA run on it just to be safe and avoid problems in the future.

::: browser/modules/BrowserUsageTelemetry.jsm
@@ +48,3 @@
>  ];
>  
>  const KNOWN_ONEOFF_SOURCES = [

Do you also need to track & distinguish one-off searches being done in activity stream? Are they being tracked already or being counted in the wrong scalar?

::: browser/modules/test/browser/browser_UsageTelemetry_content.js
@@ +102,5 @@
>    let search_hist = getSearchCountsHistogram();
>  
>    let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, "about:newtab", false);
> +  // If Activity Stream was enabled then browser.newtab.preload would have loaded an extra AS newtab in the background.
> +  await BrowserTestUtils.removeTab(tab);

Is this really needed? We're changing the pref before opening the new tab a couple of lines above.
In case there's a reason to keep this, should we protect this with |if (ACTIVITY_STREAM_ENABLED) |?

@@ +171,5 @@
> +
> +  // Also check events.
> +  let events = Services.telemetry.snapshotBuiltinEvents(Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN, false);
> +  events = (events.parent || []).filter(e => e[1] == "navigation" && e[2] == "search");
> +  checkEvents(events, [["navigation", "search", "about_newtab", "enter", {engine: "other-MozSearch"}]]);

Mh, I would have expected this to be (note "activity_stream" instead of "about_newtab"):

checkEvents(events, [["navigation", "search", "activity_stream", "enter", {engine: "other-MozSearch"}]]);

Why isn't this the case?
Attachment #8888337 - Flags: feedback+
Flags: needinfo?(alessio.placitelli)
Attached patch 1381460.patch (obsolete) — Splinter Review
Updated with the review comments.
Attachment #8888337 - Attachment is obsolete: true
Attachment #8889074 - Flags: review?(alessio.placitelli)
Comment on attachment 8889074 [details] [diff] [review]
1381460.patch

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

Looks good, thanks! Please note that that since you're adding a new scalar and a new entry in the Events file, you will need a data-review before landing this patch.
The list of reviewers is available at: https://wiki.mozilla.org/Firefox/Data_Collection
Attachment #8889074 - Flags: review?(alessio.placitelli) → review+
This patch adds a separate search event to distinguish between searches from newtab and searches from activity stream. Everything else is handled exactly the same as newtab searches.
Flags: needinfo?(benjamin)
Comment on attachment 8889074 [details] [diff] [review]
1381460.patch

I'm a little worried about the scalar name: browser.engagement.navigation.activitystream doesn't really indicate that this is search-specific. So perhaps it would be better to call this activitystream.search?

data-r=me
Flags: needinfo?(benjamin)
Attachment #8889074 - Flags: review+
(In reply to Benjamin Smedberg [:bsmedberg] from comment #16)
> I'm a little worried about the scalar name:
> browser.engagement.navigation.activitystream doesn't really indicate that
> this is search-specific. So perhaps it would be better to call this
> activitystream.search?

I think we should keep "browser.engagement.navigation.activitystream" for consistency with the other probes in the "browser.engagement.navigation.*" group: the names and the structure were discussed in bug 1295932 comment 1, bug 1303333 comment 6 (and some other related bugs). The idea is for the scalar name to represent the "component" / "data source" and for the key to give the context about what happened.

This bug enables the "search_enter" key for "browser.engagement.navigation.activitystream", which basically tells us that a "plain text search was performed in the activity stream page". In the future, that could be expanded to count "one-off searches" in a different key, or count other things.

Given the context, do you feel strongly about changing the name?
Flags: needinfo?(benjamin)
I don't feel super-strongly about this, but I don't think this is consistent. The other members of browser.engagement.navigation count all navigations from the thing in question: browser.engagement.navigation.urlbar counts all navigation from the urlbar. browser.engagement.navigation.searchbar counts all navigation triggered from the searchbar.

This one, however, only counts searches, and not all navigations from activity stream. So the behavior is strangely inconsistent with the name.

Actually though, are the "key"s documented somewhere for any of these? When originally reviewing this I didn't realize it was a keyed scalar, and I don't see documentation for what the key values could be.
Flags: needinfo?(benjamin) → needinfo?(andrei.br92)
(In reply to Benjamin Smedberg [:bsmedberg] from comment #18)
> I don't feel super-strongly about this, but I don't think this is
> consistent. The other members of browser.engagement.navigation count all
> navigations from the thing in question: browser.engagement.navigation.urlbar
> counts all navigation from the urlbar.
> browser.engagement.navigation.searchbar counts all navigation triggered from
> the searchbar.
> 
> This one, however, only counts searches, and not all navigations from
> activity stream. So the behavior is strangely inconsistent with the name.

That's the same behaviour that we have for the "browser.engagement.navigation.about_newtab": right now it's only tracking simple searches ("search_enter" key), but the plan was to eventually track other search types too (e.g. historical, suggested, one-off, etc.). I don't think that happened though.

> Actually though, are the "key"s documented somewhere for any of these? When
> originally reviewing this I didn't realize it was a keyed scalar, and I
> don't see documentation for what the key values could be.

The keys are not documented at the moment.
Let's document the keys please.
Attached patch 1381460.patch (obsolete) — Splinter Review
I've updated the scalar description to mention that it tracks searches originating from the Activity Stream Page

"Count search events originating from Activity Stream. This is currently only tracking simple searches and saving them in the 'search_enter' key."
Attachment #8889074 - Attachment is obsolete: true
Flags: needinfo?(andrei.br92)
Attachment #8889520 - Flags: review?(benjamin)
Comment on attachment 8889520 [details] [diff] [review]
1381460.patch

data-r=me
Attachment #8889520 - Flags: review?(benjamin) → review+
Attached patch for checkin (obsolete) — Splinter Review
Updated attachment 8889520 [details] [diff] [review] commit message to be:

Bug 1381460 - Add telemetry search event for Activity Stream. f=florian r=Dexter data-r=bsmedberg
Keywords: checkin-needed
Attachment #8889520 - Attachment is obsolete: true
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/839d8a60a2ea
Add telemetry search event for Activity Stream. f=florian, r=Dexter, data-r=bsmedberg
Keywords: checkin-needed
(In reply to Wes Kocher (:KWierso) from comment #25)
> https://treeherder.mozilla.org/logviewer.html#?job_id=117187523&repo=mozilla-inbound
6:19 AM i think these are actual failures browser_bing_behavior.js | search URL was loaded - Got https://www.bing.com/search?q=foo&pc=MOZI&form=MOZSBR, expected https://www.bing.com/search?q=foo&pc=MOZI&form=MOZTSB 
6:19 AM https://searchfox.org/mozilla-central/source/browser/locales/searchplugins/bing.xml#22
6:20 AM MOZTSB is used when the purpose is "newtab"
6:20 AM i think only one of the "newtab", "newtab" should be set to "activitystream"
6:20 AM one is for telemetry. one is for search engine

Looking at the comment block in activity-stream.bundle that was removed in the patch (probably should be kept), the former is for "BrowserUsageTelemetry" and the latter is for search engine plugin.
Comment on attachment 8896512 [details]
Bug 1381460 - Add telemetry search event for Activity Stream.  data-r=bsmedberg

https://reviewboard.mozilla.org/r/167766/#review174868
Attachment #8896512 - Flags: review?(alessio.placitelli) → review+
Attachment #8889651 - Attachment is obsolete: true
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ed1101844eea
Add telemetry search event for Activity Stream. r=Dexter data-r=bsmedberg
Keywords: checkin-needed
Backed out for eslint failure at browser/modules/test/browser/browser_UsageTelemetry_content.js:148: 'getSearchCountsHistogram' is not defined:

https://hg.mozilla.org/integration/autoland/rev/b59cc16b8d8b02617ed3ca3f28549c22462ff836

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=ed1101844eea0f2d7d6c1369306971383db6827d&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=124597068&repo=autoland
> TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/browser/modules/test/browser/browser_UsageTelemetry_content.js:148:21 | 'getSearchCountsHistogram' is not defined. (no-undef)
Flags: needinfo?(andrei.br92)
Attachment #8896512 - Attachment is obsolete: true
Flags: needinfo?(andrei.br92)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/69f853076a0f
Add telemetry search event for Activity Stream. r=Dexter data-r=bsmedberg
Keywords: checkin-needed
Backout by kwierso@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/607635026c14
Backed out changeset 69f853076a0f for failures in browser_UsageTelemetry_content.js a=backout
Comment on attachment 8896512 [details]
Bug 1381460 - Add telemetry search event for Activity Stream.  data-r=bsmedberg

https://reviewboard.mozilla.org/r/167766/#review177620

::: browser/extensions/activity-stream/data/content/activity-stream.bundle.js
(Diff revision 4)
>      if (input) {
> -      // The first "newtab" parameter here is called the "healthReportKey" and needs
> +      this.controller = new ContentSearchUIController(input, input.parentNode, "activitystream", "newtab");
> -      // to be "newtab" so that BrowserUsageTelemetry.jsm knows to handle events with
> -      // this name, and can add the appropriate telemetry probes for search. Without the
> -      // correct name, certain tests like browser_UsageTelemetry_content.js will fail (See
> -      // github ticket #2348 for more details)

It would seem that this comment block shouldn't be removed given the test failures with browser_UsageTelemetry_content.js
Blocks: 1394533
Blocks: 1394777
Blocks: 1396272
No longer blocks: 1394777
I've tested the patch on a Ubuntu 64 debug build which is the build failing on try but I was not able to reproduce. I've experimented with different waiting conditions including waiting for "content.wrappedJSObject.gSearch / gContentSearchController" which are references to the instances of ContentSearchUIController but it didn't change the failure.
Finally I logged the state of the scalars which are failing the assertions. The old newtab page seems to not record the search at all [0] while AS newtab shows both scalar events [1] but I don't know what to really make of this result. Simply running FFx with this patch only records 1 (the correct) event in the about:telemetry page so I'm still looking for a problem in the test.

[0]: https://treeherder.mozilla.org/logviewer.html#?job_id=128701264&repo=try&lineNumber=9426
[1]: https://treeherder.mozilla.org/logviewer.html#?job_id=128701264&repo=try&lineNumber=9545-9552
Flags: needinfo?(edilee)
(In reply to Andrei Oprea [:andreio] from comment #41)
> The old newtab page seems to not record the search at all [0] while AS
> newtab shows both scalar events [1] but I don't know what to really make of
> this result.
This would seem to indicate the "browser.engagement.navigation.about_newtab" entry gets added during the activitystream test. Maybe try printing out the scalars just before/after each await to see when it appears. Potentially this will also help point towards why the newtab test is checking too early and what to wait on instead.

I suppose another sanity check is to use `await pushPrefEnv` and verifying that the desired about:newtab page is being loaded (as both pages have "newtab-search-text")
Flags: needinfo?(edilee)
Priority: -- → P2
This hasn't landed in time for 57. I'm closing this as won't fix unless someone decides we still need it.
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: needinfo?(andrei.br92)
Resolution: --- → WONTFIX
Component: Activity Streams: Newtab → New Tab Page
You need to log in before you can comment on or make changes to this bug.