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)
Firefox
New Tab Page
Tracking
()
RESOLVED
WONTFIX
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
Assignee | ||
Updated•7 years ago
|
User Story: (updated)
Assignee | ||
Comment 1•7 years ago
|
||
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)
Comment 2•7 years ago
|
||
: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)
Assignee | ||
Comment 3•7 years ago
|
||
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)
Comment 4•7 years ago
|
||
FWIW, adding a new source means we end up with e.g. new SEARCH_COUNTS keys, like "google.activitystream".
Comment 5•7 years ago
|
||
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)
Comment 6•7 years ago
|
||
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)
Comment 7•7 years ago
|
||
I don't see any problems with the follow-in search probe.
Flags: needinfo?(mozilla)
Assignee | ||
Comment 8•7 years ago
|
||
Assignee | ||
Comment 9•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d65d27f7c114a551b5d78b0028434691e84b92c9
Assignee | ||
Updated•7 years ago
|
Attachment #8887637 -
Flags: review?(florian)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → andrei.br92
Comment 10•7 years ago
|
||
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+
Assignee | ||
Comment 11•7 years ago
|
||
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 12•7 years ago
|
||
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+
Updated•7 years ago
|
Flags: needinfo?(alessio.placitelli)
Assignee | ||
Comment 13•7 years ago
|
||
Updated with the review comments.
Attachment #8888337 -
Attachment is obsolete: true
Attachment #8889074 -
Flags: review?(alessio.placitelli)
Comment 14•7 years ago
|
||
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+
Assignee | ||
Comment 15•7 years ago
|
||
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 16•7 years ago
|
||
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+
Comment 17•7 years ago
|
||
(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)
Comment 18•7 years ago
|
||
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)
Comment 19•7 years ago
|
||
(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.
Comment 20•7 years ago
|
||
Let's document the keys please.
Assignee | ||
Comment 21•7 years ago
|
||
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 22•7 years ago
|
||
Comment on attachment 8889520 [details] [diff] [review] 1381460.patch data-r=me
Attachment #8889520 -
Flags: review?(benjamin) → review+
Comment 23•7 years ago
|
||
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
Updated•7 years ago
|
Keywords: checkin-needed
Updated•7 years ago
|
Attachment #8889520 -
Attachment is obsolete: true
Comment 24•7 years ago
|
||
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
I had to back this out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=117187523&repo=mozilla-inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/956fc7cc7ce837c134d93ef4c8931c333cd4b575
Flags: needinfo?(andrei.br92)
Comment 26•7 years ago
|
||
(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 hidden (mozreview-request) |
Assignee | ||
Comment 28•7 years ago
|
||
Patch updated. Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=52f6830cac41d01e579fd07770b1b682b742d607
Flags: needinfo?(andrei.br92)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 31•7 years ago
|
||
mozreview-review |
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+
Assignee | ||
Updated•7 years ago
|
Attachment #8889651 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 32•7 years ago
|
||
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
Comment 33•7 years ago
|
||
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)
Comment 34•7 years ago
|
||
This also causes a browser-chrome test failure: https://treeherder.mozilla.org/logviewer.html#?job_id=124601345&repo=autoland
Assignee | ||
Updated•7 years ago
|
Attachment #8896512 -
Attachment is obsolete: true
Flags: needinfo?(andrei.br92)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 36•7 years ago
|
||
Try run https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e7213c40b19b0b86fb05821dd80f1f8a6a566bc
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 37•7 years ago
|
||
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
Backed out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=125569713&repo=autoland
Flags: needinfo?(andrei.br92)
Comment 39•7 years ago
|
||
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 40•7 years ago
|
||
mozreview-review |
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
Assignee | ||
Comment 41•7 years ago
|
||
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)
Comment 42•7 years ago
|
||
(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)
Updated•7 years ago
|
Updated•7 years ago
|
Updated•7 years ago
|
Assignee | ||
Comment 43•6 years ago
|
||
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
Updated•5 years ago
|
Component: Activity Streams: Newtab → New Tab Page
You need to log in
before you can comment on or make changes to this bug.
Description
•