We need to make sure org.mozilla.searches [1] data is added to Telemetry: * org.mozilla.searches.engines - SEARCH_DEFAULT_ENGINE ? * org.mozilla.searches.counts - Probably covered by the keyed histogram SEARCH_COUNTS [1] https://ci.mozilla.org/job/mozilla-central-docs/Tree_Documentation/services/healthreport/healthreport/dataformat.html

See updated user story. SEARCH_COUNTS is already correct; we want the default to be part of the environment.

(Benjamin Smedberg [:bsmedberg] on bug 1160220, comment #2) > We should probably remove SEARCH_DEFAULT_ENGINE when we add that to the > environment in bug 1138503.

This patch adds the default search engine to TelemetryEnvironment and notifies environment changes when the default search engine is changed. I've kept the logic behind |_getDefaultSearchEngine| the same as the old |recordDefaultSearchEngine|, so it returns: 1) "NONE" if no default search engine is available; 2) "UNDEFINED" if there's a default search engine set, but we're unable to get both its identifier and name; 3) <identifier> (e.g. "yahoo") - if there's a default search engine set and we have its identifier; 4) "other"-<name> (e.g. "other-someengine") - if there's a default engine set and we don't have its identifier, but we have its name. I think cases 1 & 2 could probably be collapsed and we could just be reporting a null default search engine, as we do with other stuff in Telemetry's environment. What do you think? Also, I noticed that we're not stopping preferences watching when shutting down and we're not setting the _shutdown flag to true. I'm filing a separate bug for that.

Comment on attachment 8604046 [details] [diff] [review] part 1 - Add the default search engine to TelemetryEnvironment Review of attachment 8604046 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/telemetry/TelemetryEnvironment.jsm @@ +153,5 @@ > > const MILLISECONDS_PER_DAY = 24 * 60 * 60 * 1000; > > const EXPERIMENTS_CHANGED_TOPIC = "experiments-changed"; > +const SEARCH_ENGINE_TOPIC = "browser-search-engine-modified"; SEARCH_ENGINE_MODIFIED_TOPIC @@ +809,5 @@ > + > + _removeObservers: function() { > + // Remove the search engine change and service observers. > + Services.obs.removeObserver(this, SEARCH_ENGINE_TOPIC); > + Services.obs.removeObserver(this, SEARCH_SERVICE_TOPIC); You potentially double-remove this observer here. @@ +813,5 @@ > + Services.obs.removeObserver(this, SEARCH_SERVICE_TOPIC); > + }, > + > + observe: function (aSubject, aTopic, aData) { > + this._log.trace("observe - aTopic: " + aTopic); Log aData too? ::: toolkit/components/telemetry/docs/environment.rst @@ +33,5 @@ > }, > settings: { > blocklistEnabled: <bool>, // true on failure > isDefaultBrowser: <bool>, // null on failure, not available on Android > + defaultSearchEngine: <string>, // e.g. "yahoo" (engine identifier), "other" + <engine name> if no identifier is available, "NONE" if there is no default search engine or "UNDEFINED" if the default engine is available but its name and identifier cannot be determined I think that it's about time we start documenting the data format properly below. This part here should only serve as an example. We can do two levels of headings for now, "settings" -> "defaultSearchEngine". Then add the full description there and leave this comment as just "yahoo".

Comment on attachment 8604047 [details] [diff] [review] part 2 - Add test coverage. Review of attachment 8604047 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/telemetry/TelemetryEnvironment.jsm @@ +77,5 @@ > + > + // Testing method to reset changes throttling. > + _resetThrottling: function() { > + return getGlobal()._resetThrottling(); > + }, We could just extend fakeNow() to cover TelemetryEnvironment too.

Comment on attachment 8604048 [details] [diff] [review] part 3 - Remove the SEARCH_DEFAULT_ENGINE histogram Let's not land this until we cleared up the data analysis question.

Brendan, we are moving the measurement for the default search engine into the environment here, ping.environment.settings.defaultSearchEngine. Does that effect your FHR data equivalency verification yet or can we just remove the keyed histogram (SEARCH_DEFAULT_ENGINE)?

Nope, I haven't looked at search yet, you can go for it.

This patch makes use of fakeNow instead of introducing resetThrottling.

(In reply to Georg Fritzsche [:gfritzsche] from comment #6) > Comment on attachment 8604046 [details] [diff] [review] > part 1 - Add the default search engine to TelemetryEnvironment > > Review of attachment 8604046 [details] [diff] [review]: > ----------------------------------------------------------------- > @@ +809,5 @@ > > + > > + _removeObservers: function() { > > + // Remove the search engine change and service observers. > > + Services.obs.removeObserver(this, SEARCH_ENGINE_TOPIC); > > + Services.obs.removeObserver(this, SEARCH_SERVICE_TOPIC); > > You potentially double-remove this observer here. Right, I've removed the call from |observe|. I had cargo culted this from [1] but, after examining nsSearchService.js, it doesn't seem to be needed. [1] - https://hg.mozilla.org/mozilla-central/annotate/617dbce26726/browser/components/nsBrowserGlue.js#l484

Comment on attachment 8605156 [details] [diff] [review] part 2 - Add test coverage. - v2 Review of attachment 8605156 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js @@ +947,5 @@ > + // Register a new change listener and then wait for the search engine change to be notified. > + let deferred = PromiseUtils.defer(); > + // Set the clock in the future so our changes don't get throttled. > + gNow = futureDate(gNow, 10 * MILLISECONDS_PER_MINUTE); > + fakeNow(gNow); gNow = fakeNow(futureDate(...

Comment on attachment 8605187 [details] [diff] [review] part 1 - Add the default search engine to TelemetryEnvironment - v2 Review of attachment 8605187 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/telemetry/TelemetryEnvironment.jsm @@ +827,5 @@ > + if (aData != "init-complete") { > + return; > + } > + // Record the new default search choice. > + this._onSearchEngineChange(); Actually, the init shouldn't trigger an environment change. Also, given that we do lazy init for the environment data, we might miss the notification. @@ +942,5 @@ > blocklistEnabled: Preferences.get(PREF_BLOCKLIST_ENABLED, true), > #ifndef MOZ_WIDGET_ANDROID > isDefaultBrowser: this._isDefaultBrowser(), > #endif > + defaultSearchEngine: this._getDefaultSearchEngine(), Given that we have async init of the search service, we can't do this. We should update the search engine data similar to async addon & profile data collection. I think what we need to do is branch off Services.search.isInitialized: * if isInitialized, update the search data now * if !isInitialized, wait for SEARCH_SERVICE_TOPIC with data=="init-complete"

(In reply to Georg Fritzsche [:gfritzsche] from comment #15) > I think what we need to do is branch off Services.search.isInitialized: > * if isInitialized, update the search data now > * if !isInitialized, wait for SEARCH_SERVICE_TOPIC with data=="init-complete" Trying you gavin as Florian is on PTO - does that sound ok? I'm assuming triggering the search service async init here is undesirable for perf reasons.

(In reply to Georg Fritzsche [:gfritzsche] from comment #16) > (In reply to Georg Fritzsche [:gfritzsche] from comment #15) > > I think what we need to do is branch off Services.search.isInitialized: > > * if isInitialized, update the search data now > > * if !isInitialized, wait for SEARCH_SERVICE_TOPIC with data=="init-complete" > > Trying you gavin as Florian is on PTO - does that sound ok? Yep.

This patch takes care of nsSearchService async init.

I'm sorry Georg, I had to slightly change the tests: - It accounts for the "defaultSearchEngine" not being available if the search service is not initialized. - It initializes the search service in the environment tests. - It adds coverage for the expected search engine value if no default search envgine is found.

Comment on attachment 8605780 [details] [diff] [review] part 1 - Add the default search engine to TelemetryEnvironment - v3 Review of attachment 8605780 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/telemetry/TelemetryEnvironment.jsm @@ +865,5 @@ > + */ > + _updateSearchEngine: function () { > + this._log.trace("_updateSearchEngine - isInitialized: " + Services.search.isInitialized); > + if (!Services.search.isInitialized) { > + return; This means that settings.defaultSearchEngine is just not present in environment data collected before search service init. We should document that clearly in the in-tree docs. @@ +869,5 @@ > + return; > + } > + > + // Update the search engine entry in the current environment. > + this._currentEnvironment.settings.defaultSearchEngine = this._getDefaultSearchEngine(); We should either 1) make sure settings exist (e.g. |...settings = ...settings || {}|) or 2) very clearly document that this has to be called after _updateSettings. Preference on 1) as 2) is fragile. @@ +877,5 @@ > + * Update the default search engine value and trigger the environment change. > + */ > + _onSearchEngineChange: function () { > + this._log.trace("_onSearchEngineChange - isInitialized: " + Services.search.isInitialized); > + if (!Services.search.isInitialized) { I don't think we need this. This should only ever get called if the search service was initialized already. @@ +883,5 @@ > + } > + > + // Finally trigger the environment change notification. > + let oldEnvironment = Cu.cloneInto(this._currentEnvironment, myScope); > + this._currentEnvironment.settings.defaultSearchEngine = this._getDefaultSearchEngine(); Just call _updateSearchEngine()?

Comment on attachment 8605783 [details] [diff] [review] part 2 - Add test coverage. - v4 Review of attachment 8605783 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js @@ +947,5 @@ > + > + // Initialize the search service and disable geoip lookup, so we don't get unwanted > + // network connections. > + Preferences.set("browser.search.geoip.url", ""); > + yield new Promise(resolve => Services.search.init(resolve)); Add a check for |!("defaultSearchEngine" in data.settings)| before search.init(). @@ +969,5 @@ > + data = TelemetryEnvironment.currentEnvironment; > + checkEnvironmentData(data); > + > + const EXPECTED_SEARCH_ENGINE = "other-" + SEARCH_ENGINE_ID; > + Assert.equal(data.settings.defaultSearchEngine, EXPECTED_SEARCH_ENGINE); The test-cases for this data point are: * engine.identifier - not covered * engine.name - "other-...", covered * "NONE" - covered * "UNDEFINED" - not covered

Thanks Georg. This patch adds the coverage for "Search Engine Identifiers". I'm not sure how should be be able to test for the "UNDEFINED", as the Search Service does not allow to add engines without at least a name. While writing this test, I've also tripped on something interesting: the search service does not notify of a default-engine change when it gets removed (|Services.search.removeEngine(Services.search.defaultEngine)|). So, if the default engine is deleted from the search preferences panel, no event will be fired other than "engine-removed". The |SEARCH_DEFAULT_ENGINE| histogram isn't getting updated in that case, nor is Telemetry. Also, "about:healthreport" will show the wrong value (the old one) for the default search engine but will record the correct value when sending. I'm filing a bug for this.

Comment on attachment 8606293 [details] [diff] [review] part 2 - Add test coverage. - v5 Review of attachment 8606293 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js @@ +970,5 @@ > + for (let engine of Services.search.getEngines()) { > + Services.search.removeEngine(engine); > + } > + // The search service does not notify "engine-default" when removing a default engine. > + // Manually force the notification. This should have a TODO pointing to the bug on this. @@ +986,5 @@ > + > + // Register a new change listener and then wait for the search engine change to be notified. > + let deferred = PromiseUtils.defer(); > + // Set the clock in the future so our changes don't get throttled. > + gNow = fakeNow(futureDate(gNow, 10 * MILLISECONDS_PER_MINUTE)); Nit: This is not part of the registering/waiting, so could just come before the "// Register" line.

Thanks Georg. Here's the try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e4b7128dfbf

Comment on attachment 8606177 [details] [diff] [review] part 1 - Add the default search engine to TelemetryEnvironment - v4 Review of attachment 8606177 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/telemetry/docs/environment.rst @@ +199,5 @@ > +Contains the string identifier or name of the default search engine provider. This will not be present in environment data collected before the Search Service initialization. > + > +The special value ``NONE`` could occur if there is no default search engine. > + > +The special value ``UNDEFINED`` could occur if a default search engine exists but its identifier could not be determined. The word 'identifier' here is slightly confusing, I initially read it as meaning engine.identifier.

Comment on attachment 8606999 [details] [diff] [review] part 2 - Add test coverage. - v6 Review of attachment 8606999 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js @@ +952,5 @@ > + > + // Enable loading the engines definitions from JAR files: that's needed so that > + // the search provider reports an engine identifier. > + Preferences.set("browser.search.jarURIs", "chrome://testsearchplugin/locale/searchplugins/"); > + Preferences.set("browser.search.loadFromJars", true); My patch in bug 1162569 is changing the nsSearchService.js code to only take into account the default values of these preferences, and ignore user-set values. So this code will need to be changed to something like: Services.prefs.getDefaultBranch(null).setCharPref(... Note that in the future we are planning to completely remove these 2 preferences, but the bug for that isn't filed yet.

Thanks flo, this patch fixes, as you suggested, the test failing due to landing of your bug.

Whops, wrong patch attached.

New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=638c448802c1

https://hg.mozilla.org/integration/fx-team/rev/18c502f9c074 https://hg.mozilla.org/integration/fx-team/rev/1f42548ebc0e https://hg.mozilla.org/integration/fx-team/rev/0edb997f1e42

https://hg.mozilla.org/mozilla-central/rev/18c502f9c074 https://hg.mozilla.org/mozilla-central/rev/1f42548ebc0e https://hg.mozilla.org/mozilla-central/rev/0edb997f1e42

Comment on attachment 8606177 [details] [diff] [review] part 1 - Add the default search engine to TelemetryEnvironment - v4 Approval Request Comment [Feature/regressing bug #]: Unified Telemetry, https://wiki.mozilla.org/Unified_Telemetry This is part of the first (main) batch of uplifts to 40 to enable shipping on that train, see bug 1120356, comment 2. [User impact if declined]: Data & measurement insight projects delayed or blocked with direct impact on projects depending on this. [Describe test coverage new/current, TreeHerder]: We have good automation coverage of the feature. We also had manual tests of the main tasks as well as confirmation of correct behavior on Nightly for the patches here. [Risks and why]: Low-risk - these patches are rather isolated to Telemetry and have been on Nightly for a while with no bad reports. We intend to track on-going data quality and other issues during the 40 aurora & beta and flip the new behavior off when it doesn't meet the requirements. [String/UUID change made/needed]: The only string changes were to the about:telemetry page. We decided that we can live with missing translations on that page for a cycle as that page is not exactly user-facing.

Comment on attachment 8608119 [details] [diff] [review] part 2 - Add test coverage. - v7 Approval Request Comment [Feature/regressing bug #]: Unified Telemetry, https://wiki.mozilla.org/Unified_Telemetry This is part of the first (main) batch of uplifts to 40 to enable shipping on that train, see bug 1120356, comment 2. [User impact if declined]: Data & measurement insight projects delayed or blocked with direct impact on projects depending on this. [Describe test coverage new/current, TreeHerder]: We have good automation coverage of the feature. We also had manual tests of the main tasks as well as confirmation of correct behavior on Nightly for the patches here. [Risks and why]: Low-risk - these patches are rather isolated to Telemetry and have been on Nightly for a while with no bad reports. We intend to track on-going data quality and other issues during the 40 aurora & beta and flip the new behavior off when it doesn't meet the requirements. [String/UUID change made/needed]: The only string changes were to the about:telemetry page. We decided that we can live with missing translations on that page for a cycle as that page is not exactly user-facing.

Comment on attachment 8604048 [details] [diff] [review] part 3 - Remove the SEARCH_DEFAULT_ENGINE histogram Approval Request Comment [Feature/regressing bug #]: Unified Telemetry, https://wiki.mozilla.org/Unified_Telemetry This is part of the first (main) batch of uplifts to 40 to enable shipping on that train, see bug 1120356, comment 2. [User impact if declined]: Data & measurement insight projects delayed or blocked with direct impact on projects depending on this. [Describe test coverage new/current, TreeHerder]: We have good automation coverage of the feature. We also had manual tests of the main tasks as well as confirmation of correct behavior on Nightly for the patches here. [Risks and why]: Low-risk - these patches are rather isolated to Telemetry and have been on Nightly for a while with no bad reports. We intend to track on-going data quality and other issues during the 40 aurora & beta and flip the new behavior off when it doesn't meet the requirements. [String/UUID change made/needed]: The only string changes were to the about:telemetry page. We decided that we can live with missing translations on that page for a cycle as that page is not exactly user-facing.

Comment on attachment 8604048 [details] [diff] [review] part 3 - Remove the SEARCH_DEFAULT_ENGINE histogram Unified telemetry is an important new feature. It is blocking some other projects. Taking it.

https://hg.mozilla.org/releases/mozilla-aurora/rev/f6d297bd0d03 https://hg.mozilla.org/releases/mozilla-aurora/rev/af31924d3068 https://hg.mozilla.org/releases/mozilla-aurora/rev/e2080264b2e0