Once we have addon and pref collection & change detection mechanisms, we need to populate the environment data per the spec. This will involve some migration of existing FHR & Telemetry code as well as some new collection.

Comment on attachment 8555871 [details] [diff] [review] part 1 - Add Build section to Environment data. Review of attachment 8555871 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/telemetry/TelemetryEnvironment.jsm @@ +183,5 @@ > + version: Services.appinfo.version, > + vendor: Services.appinfo.vendor, > + platformVersion: Services.appinfo.platformVersion, > + xpcomAbi: Services.appinfo.XPCOMABI, > + hotfixVersion: Preferences.get(PREF_HOTFIX_LASTVERSION, undefined), This can presumably be changed mid-session? Please verify whether hotfixes can take effect mid-session. If so, we need to trigger environment changes for this.

> This can presumably be changed mid-session? > Please verify whether hotfixes can take effect mid-session. If so, we need > to trigger environment changes for this. You're right, it appears that hotfixes can take effect mid-session. See [1]. [1] - https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/AddonManager.jsm#1401

This patch adds all the sections to Environment Data. What's missing: - CPU stepping, family - OS Locale - Change notification for addons/hotfix/plugins

I've split the addons part out and will be moving that to bug 1122050.

Comment on attachment 8557053 [details] [diff] [review] part 1 - Add Build, Profile and System sections to Environment data. Review of attachment 8557053 [details] [diff] [review]: ----------------------------------------------------------------- A quick pass on this. ::: toolkit/components/telemetry/TelemetryEnvironment.jsm @@ +31,5 @@ > +const PREF_DISTRIBUTOR = "app.distributor"; > +const PREF_DISTRIBUTOR_CHANNEL = "app.distributor.channel"; > +const PREF_E10S_ENABLED = "browser.tabs.remote.autostart"; > +const PREF_HOTFIX_LASTVERSION = "extensions.hotfix.lastVersion"; > +const PREF_PARTNER_BRANCH = "app.partner."; Let's name it PREF_APP_PARTNER_BRANCH to avoid confusion. @@ +37,5 @@ > +const PREF_TELEMETRY_ENABLED = "toolkit.telemetry.enabled"; > +const PREF_UPDATE_ENABLED = "app.update.enabled"; > +const PREF_UPDATE_AUTODOWNLOAD = "app.update.auto"; > + > +// Constant used by |truncateToDays| I don't think we need to document where it is used. @@ +42,5 @@ > +const MILLISECONDS_PER_DAY = 24 * 60 * 60 * 1000; > + > +/** > + * Turn a millisecond timestamp into a day timestamp. Taken from > + * the Health Report profile provider. Don't document where it came from. @@ +376,3 @@ > return { > + blocklistEnabled: Preferences.get(PREF_BLOCKLIST_ENABLED, false), > + isDefaultBrowser: this._isDefaultBrowser(), Document that this can be null in the environment spec. @@ +377,5 @@ > + blocklistEnabled: Preferences.get(PREF_BLOCKLIST_ENABLED, false), > + isDefaultBrowser: this._isDefaultBrowser(), > + e10sEnabled: Preferences.get(PREF_E10S_ENABLED, false), > + telemetryEnabled: Preferences.get(PREF_TELEMETRY_ENABLED, false), > + locale: getLocale(), I see that providers.jsm has exception handling around locale retrieval. Please check if that can throw and handle accordingly. Document what we end up setting it to in case of failure etc. in the environment spec. @@ +379,5 @@ > + e10sEnabled: Preferences.get(PREF_E10S_ENABLED, false), > + telemetryEnabled: Preferences.get(PREF_TELEMETRY_ENABLED, false), > + locale: getLocale(), > + update: { > + channel: UpdateChannel.get(), Same here. @@ +383,5 @@ > + channel: UpdateChannel.get(), > + enabled: Preferences.get(PREF_UPDATE_ENABLED, false), > + autoDownload: Preferences.get(PREF_UPDATE_AUTODOWNLOAD, false), > + }, > + userPrefs: this._getPrefData(), Please update the environment spec for the behavior we have here: * some prefs are recorded with value * for some prefs we only record they are present with value being ... @@ +392,5 @@ > + * Get the profile data in object form. > + * @return Object containing the profile data. > + */ > + _getProfile: Task.async(function* () { > + this._checkForShutdown(); We already did _checkForShutdown() at top-level in getEnvironmentData(). We should not need to check it in all the functions it calls as well. If this gets shutdown during environment collection, then something else went wrong already: we should collect the environment usually from TelemetryPing, which should not shut it the environment down while it builds the ping. @@ +400,5 @@ > + > + let creationDate = yield profileAccessor.created; > + let resetDate = yield profileAccessor.reset; > + > + return Promise.resolve({ Just |return {...};| @@ +402,5 @@ > + let resetDate = yield profileAccessor.reset; > + > + return Promise.resolve({ > + creationDate: truncateToDays(creationDate), > + resetDate: (resetDate) ? truncateToDays(resetDate) : undefined, JSON.stringify({x:undefined}) is "{}". Let's use null or change the approach so we can do something like: if (resetDate) { data.resetDate = ... Document the behavior in the environment spec. @@ +431,5 @@ > + for (name in partnerNamePrefs) { > + partnerNames.push(name); > + } > + > + partnerData.partnerNames = partnerNames; Why not just |partnerData.partnerNames = partnerBranch.getChildList("")|? @@ +447,5 @@ > + > + let sysinfo = Services.sysinfo; > + > + let cpuData = { > + count: getSystemProperty("cpucount", null), Document the null-behavior in the spec. @@ +449,5 @@ > + > + let cpuData = { > + count: getSystemProperty("cpucount", null), > + //vendor: Services.sysinfo.getProperty("vendor"), > + /*family: Services.sysinfo.getProperty("family"), Not done yet? @@ +479,5 @@ > + /** > + * Get the device information, if we are on a portable device. > + * @return Object containing the device information data. > + */ > + _getDeviceData: function () { This is mobile-only, we don't need it on other platforms. @@ +486,5 @@ > + > + let sysinfo = Services.sysinfo; > + > + return { > + model: getSystemProperty("device", null), // the "device" from FHR We don't need to comment on FHR behavior here. Document the null-behavior in the spec. @@ +508,5 @@ > + let servicePack = null; > + > + // Try to get service pack information. > + try { > + servicePack = getServicePack(); We definitely should make sure to check that this contains sensible data in tests running on windows. @@ +591,5 @@ > + > + this._log.trace("_getGFXData - Two display adapters detected."); > + > + gfxData.adapters.push({ > + description: getGfxField("adapterDescription2", null), Except isGPU2Active the field names are the same except for the "2" suffix. This looks like it could be unified? @@ +617,5 @@ > + let sysinfo = Services.sysinfo; > + > + return { > + memoryMB: getSystemProperty("memsize", null), > + isWow64: getSystemProperty("isWow64", null), // Windows-only So let's not add this on non-windows platforms. @@ +643,2 @@ > "settings": () => this._getSettings(), > + "profile": this._getProfile.bind(this), Let's keep this uniform, whatever way we go with. @@ +655,2 @@ > } catch (e) { > + this._log.error("getEnvironmentData - There was an exception collecting " + s + e); This has |"foo" + s + e| without separators. Log.jsm supports additional object and error params, so just do _log.error("foo", error).

I've addressed your comments in the previous feedback.

(In reply to Georg Fritzsche [:gfritzsche] from comment #6) > @@ +449,5 @@ > > + > > + let cpuData = { > > + count: getSystemProperty("cpucount", null), > > + //vendor: Services.sysinfo.getProperty("vendor"), > > + /*family: Services.sysinfo.getProperty("family"), > > Not done yet? > @@ +508,5 @@ > > + let servicePack = null; > > + > > + // Try to get service pack information. > > + try { > > + servicePack = getServicePack(); > > We definitely should make sure to check that this contains sensible data in > tests running on windows. What do you mean? > @@ +643,2 @@ > > "settings": () => this._getSettings(), > > + "profile": this._getProfile.bind(this), > > Let's keep this uniform, whatever way we go with. I had to go with |this._get*| since fat arrows do not support generator functions (I think). Is there any other way around that?

- Added preference record policy to only watch prefs. - Partners values are null on failure. - Uniformed function bindings in |getEnvironmentData|

(In reply to Alessio Placitelli [:Dexter] from comment #8) > > @@ +508,5 @@ > > > + let servicePack = null; > > > + > > > + // Try to get service pack information. > > > + try { > > > + servicePack = getServicePack(); > > > > We definitely should make sure to check that this contains sensible data in > > tests running on windows. > > What do you mean? We should have windows-specific test-coverage for the service-pack info to ensure that getServicePack() is working sanely. > > > @@ +643,2 @@ > > > "settings": () => this._getSettings(), > > > + "profile": this._getProfile.bind(this), > > > > Let's keep this uniform, whatever way we go with. > > I had to go with |this._get*| since fat arrows do not support generator > functions (I think). Is there any other way around that? Have you tried it? I don't understand the problem. _getProfile() is a function that returns a promise. That it nests a generator function doesn't matter here, so there should be no problem with using arrow functions.

This patch removes GFX Info and System Info from TelemetrySession, as they are in the Environment already. It also adds more test coverage for Environment data.

Comment on attachment 8557870 [details] [diff] [review] part 1 - Add Build, Profile and System sections to Environment data (v4). I'll wait for the rebased patches.

Comment on attachment 8557870 [details] [diff] [review] part 1 - Add Build, Profile and System sections to Environment data (v4). Review of attachment 8557870 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/telemetry/TelemetryEnvironment.jsm @@ +407,2 @@ > return { > + blocklistEnabled: Preferences.get(PREF_BLOCKLIST_ENABLED, false), This defaults to true: https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/nsBlocklistService.js#303 @@ +412,5 @@ > + locale: getLocale(false), > + update: { > + channel: updateChannel, > + enabled: Preferences.get(PREF_UPDATE_ENABLED, false), > + autoDownload: Preferences.get(PREF_UPDATE_AUTODOWNLOAD, false), Can you cross-check what we actually default to for these two if the prefs are not set and adjust the default accordingly?

Rebased the patch and cross checked the default values as requested in comment 16. This is the result of the cross check: * e10sEnabled: Firefox profile default is false [1] * telemetryEnabled: should we default to true? We're sending a ping using telemetry. * update.enabled: FHR defaults to false in case of errors [2], Firefox defaults to true [3] * update.autoDownload: Same as above. [1] - https://dxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.js#1180 [2] - https://dxr.mozilla.org/mozilla-central/source/services/healthreport/providers.jsm#403 [3] - https://dxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.js#155

Rebased the patch.

Rebased the patch.

(In reply to Alessio Placitelli [:Dexter] from comment #17) > * e10sEnabled: Firefox profile default is false [1] We are interested in the default if the pref is not set at all - i.e. what default value we should pass to Preferences.get(). > * telemetryEnabled: should we default to true? We're sending a ping using > telemetry. This defaults to false per TelemetryPing.jsm. The FHR pref will soon be the "do we send data?" pref, with the telemetry pref controlling whether we send additional data. > * update.enabled: FHR defaults to false in case of errors [2], Firefox > defaults to true [3] > * update.autoDownload: Same as above. We should default to what the code does.

Thanks for the clarification. I've changed the TelemetryEnabled default back to false. The update services one are |true| as reported in [1] and [2]. [1] - https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsUpdateService.js#3875 [2] - https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsUpdateService.js#2871

Comment on attachment 8562779 [details] [diff] [review] part 1 - Add Build, Profile and System sections to Environment data (v6). Review of attachment 8562779 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/telemetry/TelemetryEnvironment.jsm @@ +55,5 @@ > + * @param [aGetOSLocale] If true, gets the underlying OS locale. Otherwise, get the > + * browser locale. > + * @return a string with the global locale or null on failure. > + */ > +function getLocale(aGetOSLocale = false) { Those are totally different behaviors below. Can't we just make it two functions? @@ +70,5 @@ > + } > +} > + > +/** > + * Safely get a system property and return its value. If the property is not "sysinfo property"? @@ +77,5 @@ > + * @param aPropertyName the property name to get. > + * @param aDefault the value to return if aPropertyName is not available. > + * @return The property value, if available, or aDefault. > + */ > +function getSystemProperty(aPropertyName, aDefault) { getSysinfoProperty? @@ +82,5 @@ > + let value = aDefault; > + try { > + // |getProperty| may throw if |aPropertyName| does not exist. > + value = Services.sysinfo.getProperty(aPropertyName); > + } catch (e) {} Nit: No need for |value|: try { return Services ... } ... return aDefault; @@ +98,5 @@ > + */ > +function getGfxField(aPropertyName, aDefault) { > + let gfxInfo = Cc["@mozilla.org/gfx/info;1"].getService(Ci.nsIGfxInfo); > + > + let value = aDefault; Nit: No need for |value|. @@ +116,5 @@ > + * > + * @param aPropertySuffix A suffix to add to the properties names. > + * @return An object containing the adapter properties. > + */ > +function getGfxAdapter(aPropertySuffix = "") { aPropertySuffix is unused. This means we also lack test-coverage for >1 gfx adapters. @@ +179,5 @@ > + return { > + major: winVer.wServicePackMajor, > + minor: winVer.wServicePackMinor, > + }; > + } finally { Can we add a catch here with error logging and returning a fallback? @@ +197,5 @@ > > // Policy to use when saving preferences. Exported for using them in tests. > RECORD_PREF_STATE: 1, // Don't record the preference value > RECORD_PREF_VALUE: 2, // We only record user-set prefs. > + RECORD_PREF_NOTHING: 3, // Record nothing, just notify of changes. RECORD_PREF_NOTIFY_ONLY maybe? @@ +358,5 @@ > + * Get the build data in object form. > + * @return Object containing the build data. > + */ > + _getBuild: function () { > + this._log.trace("_getBuild"); Less tracing. @@ +392,5 @@ > + try { > + shellService = Cc["@mozilla.org/browser/shell-service;1"] > + .getService(Ci.nsIShellService); > + } catch (ex) { > + this._log.warn("_isDefaultBrowser - Could not obtain shell service."); _log.error(..., ex)? @@ +403,5 @@ > + try { > + // This uses the same set of flags used by the pref pane. > + isDefault = shellService.isDefaultBrowser(false, true) ? true : false; > + } catch (ex) { > + this._log.warn("_isDefaultBrowser - Could not determine if default browser"); _log.error(..., ex)? @@ +416,5 @@ > * Get the settings data in object form. > * @return Object containing the settings. > */ > _getSettings: function () { > this._log.trace("_getSettings"); Less tracing. @@ +427,2 @@ > return { > + blocklistEnabled: Preferences.get(PREF_BLOCKLIST_ENABLED, true), Update the in-tree docs to the changes here. @@ +431,5 @@ > + telemetryEnabled: Preferences.get(PREF_TELEMETRY_ENABLED, false), > + locale: getLocale(false), > + update: { > + channel: updateChannel, > + enabled: Preferences.get(PREF_UPDATE_ENABLED, true), Update docs. @@ +432,5 @@ > + locale: getLocale(false), > + update: { > + channel: updateChannel, > + enabled: Preferences.get(PREF_UPDATE_ENABLED, true), > + autoDownload: Preferences.get(PREF_UPDATE_AUTODOWNLOAD, true), Update docs. @@ +443,5 @@ > + * Get the profile data in object form. > + * @return Object containing the profile data. > + */ > + _getProfile: Task.async(function* () { > + this._log.trace("_getProfile"); Less tracing. @@ +466,5 @@ > + * Get the partner data in object form. > + * @return Object containing the partner data. > + */ > + _getPartner: function () { > + this._log.trace("_getPartner"); Less tracing. @@ +488,5 @@ > + * Get the CPU information. > + * @return Object containing the CPU information data. > + */ > + _getCpuData: function () { > + this._log.trace("_getCpuData"); Less tracing. @@ +492,5 @@ > + this._log.trace("_getCpuData"); > + > + let cpuData = { > + count: getSystemProperty("cpucount", null), > + }; Missing: vendor, family, model, stepping. @@ +506,5 @@ > + Services.sysinfo.getProperty(ext); > + // If it doesn't throw, add it to the list. > + availableExts.push(ext); > + } catch (e) { > + continue; Nit: Redundant continue. @@ +521,5 @@ > + * Get the device information, if we are on a portable device. > + * @return Object containing the device information data. > + */ > + _getDeviceData: function () { > + this._log.trace("_getDeviceData"); Less tracing. @@ +523,5 @@ > + */ > + _getDeviceData: function () { > + this._log.trace("_getDeviceData"); > + > + let sysinfo = Services.sysinfo; Unused. @@ +539,5 @@ > + * Get the OS information. > + * @return Object containing the OS data. > + */ > + _getOSData: function () { > + this._log.trace("_getOSData"); Less tracing. @@ +541,5 @@ > + */ > + _getOSData: function () { > + this._log.trace("_getOSData"); > + > + let sysinfo = Services.sysinfo; Unused. @@ +550,5 @@ > + // Try to get service pack information. > + try { > + servicePack = getServicePack(); > + } catch (e) { > + this._log.trace("_getOSData - No service pack information."); _log.error(..., e) @@ +559,5 @@ > + name: getSystemProperty("name", null), > + version: getSystemProperty("version", null), > +#if defined(MOZ_WIDGET_GONK) || defined(MOZ_WIDGET_ANDROID) > + kernelVersion: getSystemProperty("kernel_version", null), > +#elifdef XP_WIN From quickly skimming C & C++ standards, i don't think thats a standard thing? Also, consistency. Just use #elif defined(XP_WIN)? @@ +572,5 @@ > + * Get the HDD information. > + * @return Object containing the HDD data. > + */ > + _getHDDData: function () { > + this._log.trace("_getHDDData"); Less tracing. @@ +574,5 @@ > + */ > + _getHDDData: function () { > + this._log.trace("_getHDDData"); > + > + let sysinfo = Services.sysinfo; Unused. @@ +597,5 @@ > + * Get the GFX information. > + * @return Object containing the GFX data. > + */ > + _getGFXData: function () { > + this._log.trace("_getGFXData"); Less tracing. @@ +630,5 @@ > + * Get the system data in object form. > + * @return Object containing the system data. > + */ > + _getSystem: function () { > + this._log.trace("_getSystem"); Less tracing. @@ +632,5 @@ > + */ > + _getSystem: function () { > + this._log.trace("_getSystem"); > + > + let sysinfo = Services.sysinfo; Unused. @@ +700,3 @@ > } catch (e) { > + this._log.error("getEnvironmentData - Exception while collecting " + s, e); > + continue; Redundant continue. ::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js @@ +5,5 @@ > > Cu.import("resource://gre/modules/TelemetryEnvironment.jsm", this); > Cu.import("resource://gre/modules/Preferences.jsm", this); > Cu.import("resource://gre/modules/PromiseUtils.jsm", this); > +Cu.import("resource://gre/modules/XPCOMUtils.jsm", this); This doesn't get used? ::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment_ServicePack.js @@ +44,5 @@ > + > + Assert.ok(Number.isFinite(osData[SERVICE_PACK_MAJOR]), > + "ServicePackMajor must be a number."); > + Assert.ok(Number.isFinite(osData[SERVICE_PACK_MINOR]), > + "ServicePackMajor must be a number."); ServicePackMinor

Comment on attachment 8562647 [details] [diff] [review] part 2 - Remove duplicated data from TelemetrySession (v2). Review of attachment 8562647 [details] [diff] [review]: ----------------------------------------------------------------- r+ with the below fixed. Please f?vladan for heads-up after you updated it. ::: toolkit/components/telemetry/TelemetrySession.jsm @@ +539,5 @@ > let ai = Services.appinfo; > let ret = { > reason: reason, > OS: ai.OS, > + appVersion: ai.version, // Already in Environment, but needed for |submissionPath| Can you add "TODO: " for those comments? @@ +540,5 @@ > let ret = { > reason: reason, > OS: ai.OS, > + appVersion: ai.version, // Already in Environment, but needed for |submissionPath| > + appName: ai.name, // Not in Environment Oh, that got lost. We should move this to Environment.build.applicationName.

Comment on attachment 8562779 [details] [diff] [review] part 1 - Add Build, Profile and System sections to Environment data (v6). Review of attachment 8562779 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/telemetry/TelemetryEnvironment.jsm @@ +9,5 @@ > ]; > > const {classes: Cc, interfaces: Ci, utils: Cu} = Components; > > +Cu.import("resource://gre/modules/ctypes.jsm"); We only need this on Windows, right? Maybe just make it lazy?

Oops, not sure how i ended up resolving this.

Thank you for reviewing this. In this patch most of the traces are removed and I've dealt with the other suggestions. > +Cu.import("resource://gre/modules/XPCOMUtils.jsm", this); You're right, this is not used in part 1, but in part 3, through |createAppInfo|. I think we should be lazy loading this in head.js, as createAppInfo uses it. Other tests in the suite simply import XPCOMUtils.jsm.

This patch adds the "TODO" in the comments for the duplicated values in both Environment and SessionPing that we need to keep (in this patch).

Comment on attachment 8562649 [details] [diff] [review] part 3 - Improve test coverage for environment data (v2). Review of attachment 8562649 [details] [diff] [review]: ----------------------------------------------------------------- I think there is three bigger points here: I'd like us to not only check for presence of fields, but also for them having an expected type and possibly a sane value. This is a little tedious (and we should probably integrate a JSON schema checker on this later), but would make us a lot safer. I want us to try a bit more to test with controlled data. We already fake appinfo, lets also fake at least sysinfo. Finally, lets move the environment data checks out of separate tests so that we can easily run all of them on one environment data object, e.g.: function checkEnvironmentData(data) { checkBuildSection(data); ... That way we can easily e.g. change appinfo values or other things and then run the checks again. ::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js @@ +105,5 @@ > + } > + > + // Make sure architecture and hotfixVersion are in the environment. > + Assert.ok("architecture" in environmentData.build); > + Assert.ok("hotfixVersion" in environmentData.build); We can fake those too (fake sysinfo and set the approppriate pref) and add them to expectedInfo. We will probably need to start faking sysinfo anyway, at least for the environment tests. @@ +107,5 @@ > + // Make sure architecture and hotfixVersion are in the environment. > + Assert.ok("architecture" in environmentData.build); > + Assert.ok("hotfixVersion" in environmentData.build); > + if ("@mozilla.org/xpcom/mac-utils;1" in Cc) { > + Assert.ok("architecturesInBinary" in environmentData.build); This is only present if also |macUtils && macUtils.isUniversalBinary|. Also, lets at least check for this being a string and not empty. @@ +116,5 @@ > + > +add_task(function* test_checkSettingsSection() { > + const EXPECTED_FIELDS = [ > + "blocklistEnabled", "isDefaultBrowser", "e10sEnabled", "telemetryEnabled", "locale", "update", "userPrefs", > + ]; Nit: line-wrapping, break after telemetryEnabled or so? Can we test them for expected values please where sensible? Where not sensible (e.g. isDefaultBrowser presumably), lets at least type-check. @@ +136,5 @@ > + yield TelemetryEnvironment.shutdown(); > +}); > + > +add_task(function* test_checkProfileSection() { > + const CREATION_DATE_FIELD = "creationDate"; Can't we just do profile.creationDate etc. directly? I'm not sure what we win by the const. Can we maybe easily fake & test a reset too? @@ +160,5 @@ > + > + yield TelemetryEnvironment.init(); > + let environmentData = yield TelemetryEnvironment.getEnvironmentData(); > + > + Assert.ok("profile" in environmentData, "There must be a profile section in Environment."); s/profile/partner/ @@ +163,5 @@ > + > + Assert.ok("profile" in environmentData, "There must be a profile section in Environment."); > + > + for (let f of EXPECTED_FIELDS) { > + Assert.ok(f in environmentData.partner); Can we check for sane values and types here? @@ +169,5 @@ > + > + yield TelemetryEnvironment.shutdown(); > +}); > + > +add_task(function* test_checkSystemSection() { This is missing a "device" test. @@ +177,5 @@ > + const EXPECTED_ADAPTER_FIELDS = [ > + "description", "vendorID", "deviceID", "subsysID", "RAM", "driver", "driverVersion", > + "driverDate", "GPUActive", > + ]; > + let isWindows = ("@mozilla.org/windows-registry-key;1" in Cc); You could just introduce top-level |const gIsWindows| & |const gIsMac|. @@ +186,5 @@ > + Assert.ok("system" in data, "There must be a system section in Environment."); > + > + // Make sure we have all the top level sections and fields. > + for (let f of EXPECTED_FIELDS) { > + Assert.ok(f in data.system); Lets check for sane values and types? @@ +198,5 @@ > + let cpuData = data.system.cpu; > + Assert.ok(Number.isFinite(cpuData.count), "CPU count must be a number."); > + Assert.ok("extensions" in cpuData, "CPU extensions must be available."); > + > + // Service pack is covered in a separate test. Lets just move those lines of 4 checks over here in a |if (isWindows)| block. @@ +201,5 @@ > + > + // Service pack is covered in a separate test. > + let osData = data.system.os; > + Assert.ok("name" in osData); > + Assert.ok("version" in osData); We can check for proper values here when faking sysinfo. @@ +205,5 @@ > + Assert.ok("version" in osData); > + > + for (let disk of EXPECTED_HDD_FIELDS) { > + Assert.ok("model" in data.system.hdd[disk]); > + Assert.ok("revision" in data.system.hdd[disk]); Lets check for sane values and types? @@ +210,5 @@ > + } > + > + let gfxData = data.system.gfx; > + for (let f of EXPECTED_GFX_FIELDS) { > + Assert.ok(f in gfxData); Lets check for sane values and types? @@ +213,5 @@ > + for (let f of EXPECTED_GFX_FIELDS) { > + Assert.ok(f in gfxData); > + } > + > + Assert.ok(gfxData.adapters.length > 0, "There must be at least a GFX adapter."); Nit: At least one. @@ +216,5 @@ > + > + Assert.ok(gfxData.adapters.length > 0, "There must be at least a GFX adapter."); > + for (let adapter of gfxData.adapters) { > + for (let f of EXPECTED_ADAPTER_FIELDS) { > + Assert.ok(f in adapter); Lets check for sane values and types?

Comment on attachment 8562918 [details] [diff] [review] part 1 - Add Build, Profile and System sections to Environment data (v7). Review of attachment 8562918 [details] [diff] [review]: ----------------------------------------------------------------- r+ with the below issues resolved. ::: toolkit/components/telemetry/TelemetryEnvironment.jsm @@ +139,5 @@ > +/** > + * Gets the service pack information on Windows platforms. This was copied from > + * nsUpdateService.js. > + * > + * @throws An error if the underlying OS API call fails. I don't think this throws anymore? @@ +186,5 @@ > + major: winVer.wServicePackMajor, > + minor: winVer.wServicePackMinor, > + }; > + } catch (e) { > + kernel32.close(); Lets still do the kernel32.close() in a finally block instead of repeating it. @@ +392,5 @@ > + try { > + shellService = Cc["@mozilla.org/browser/shell-service;1"] > + .getService(Ci.nsIShellService); > + } catch (ex) { > + this._log.error("_isDefaultBrowser - Could not obtain shell service.", ex); Nit: No period - error("foo", exc) is printed something like "foo: [exception stuff]". @@ +425,2 @@ > return { > + blocklistEnabled: Preferences.get(PREF_BLOCKLIST_ENABLED, true), I'm missing the doc updates requested in comment 22. Can you please land them as part of this patch that implements the changes? Stacking on bug 1124212 should be conflict-free. ::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment_ServicePack.js @@ +44,5 @@ > + > + Assert.ok(Number.isFinite(osData[SERVICE_PACK_MAJOR]), > + "ServicePackMajor must be a number."); > + Assert.ok(Number.isFinite(osData[SERVICE_PACK_MINOR]), > + "ServicePackMinor must be a number."); The actual test part is basically the four Assert.ok()s here. Lets just move that over into test_TelemetryEnvironment.js and kill this test. Side-note: I don't think we win anything really from the |const SERVICE_PACK_*| here either.

Thanks for the review. This patch changes |getServicePack| and updates the docs.

Whoops, forgot to remove the ServicePack test from the patch.

Changed |_isDefaultBrowser| to remove |let isDefault =...|.

Fix: GPU memory is now sent as a number (per spec).

Thanks Georg. This adds type checking to environment data and refactors the code as suggested. The Gfx test was broken and has been fixed as well.

Comment on attachment 8562920 [details] [diff] [review] part 2 - Remove duplicated data from TelemetrySession (v3). This patch removes data from TelemetrySession which was moved to TelemetryEnvironment. A few fields were kept, even if duplicated, due to them being used in |submissionPath|. Tests were updated accordingly.

Comment on attachment 8562920 [details] [diff] [review] part 2 - Remove duplicated data from TelemetrySession (v3). Thank you for the heads up. If you land this patch as-is, you'll break the Telemetry backend as it expects some of the fields you're removing. I'll make these changes next week. I think you could actually land this patch as-is if you just add back the "arch" and "version" fields. You'd still break the population-distribution dash (http://telemetry.mozilla.org/distribution/) but that's ok.

Comment on attachment 8563474 [details] [diff] [review] part 1 - Add Build, Profile and System sections to Environment data (v10) Review of attachment 8563474 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/telemetry/TelemetryEnvironment.jsm @@ +365,5 @@ > + */ > + _getBuild: function () { > + let buildData = { > + applicationId: Services.appinfo.ID, > + applicationName: Services.appinfo.name, This needs to be added to the docs (environment.rst).

Comment on attachment 8563477 [details] [diff] [review] part 3 - Improve test coverage for environment data (v3) Review of attachment 8563477 [details] [diff] [review]: ----------------------------------------------------------------- Looks good except issues noted below. ::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js @@ +51,5 @@ > + * @param aPrefs A map of preferences and their values. > + */ > +function setPreferencesValues(aPrefs) { > + for (let pref in aPrefs) { > + Preferences.set(pref, aPrefs[pref]); Why don't we just use Preferences.set() directly below? This approach doesn't seem really shorter. @@ +64,5 @@ > + reset: PROFILE_RESET_DATE_MS > + }); > +} > + > +function spoofPreferences() { spoofPartnerInfo() would be much more descriptive. We could move the one non-partner pref somewhere like run_test() for now. @@ +73,5 @@ > + prefsToSpoof["app.distributor.channel"] = DISTRIBUTOR_CHANNEL; > + prefsToSpoof["app.partner.test"] = PARTNER_NAME; > + prefsToSpoof["mozilla.partner.id"] = PARTNER_ID; > + prefsToSpoof["extensions.hotfix.lastVersion"] = APP_HOTFIX_VERSION; > + Nit: whitespace. @@ +174,5 @@ > + Assert.equal(typeof data.settings[f], EXPECTED_FIELDS_TYPES[f]); > + } > + > + // Check "isDefaultBrowser" separately, as it can either be null or boolean. > + Assert.ok("isDefaultBrowser" in data.settings); checkNullOrBool? @@ +180,5 @@ > + Assert.equal(typeof data.settings.isDefaultBrowser, "boolean"); > + } > + > + // Check "channel" separately, as it can either be null or string. > + let update = data.settings.update; checkNullOrString? @@ +195,5 @@ > + Assert.ok("profile" in data, "There must be a profile section in Environment."); > + > + Assert.ok(Number.isFinite(data.profile.creationDate), > + "Creation date must be a number."); > + Assert.equal(data.profile.creationDate, truncateToDays(PROFILE_CREATION_DATE_MS)); I think its enough to assert equality to a number here, the Number.isFinite seems redundant? @@ +197,5 @@ > + Assert.ok(Number.isFinite(data.profile.creationDate), > + "Creation date must be a number."); > + Assert.equal(data.profile.creationDate, truncateToDays(PROFILE_CREATION_DATE_MS)); > + > + Assert.ok(Number.isFinite(data.profile.resetDate), Redundant? @@ +234,5 @@ > + driverVersion: "string", > + driverDate: "string", > + GPUActive: "boolean", > + }; > + Whitespace. @@ +237,5 @@ > + }; > + > + for (let f in EXPECTED_ADAPTER_FIELDS_TYPES) { > + Assert.ok(f in data); > + Whitespace. @@ +264,5 @@ > + } > + > + let cpuData = data.system.cpu; > + Assert.ok(Number.isFinite(cpuData.count), "CPU count must be a number."); > + Assert.ok("extensions" in cpuData, "CPU extensions must be available."); Check that its an array? @@ +266,5 @@ > + let cpuData = data.system.cpu; > + Assert.ok(Number.isFinite(cpuData.count), "CPU count must be a number."); > + Assert.ok("extensions" in cpuData, "CPU extensions must be available."); > + > + // We don't check for Gonk here, as tests are skipped when Gonk is used. Right now they are, i think i mentioned somewhere else already that this seems to be due to manifest parsing issues. @@ +272,5 @@ > + let deviceData = data.system.device; > + Assert.ok(checkNullOrString(deviceData.model)); > + Assert.ok(checkNullOrString(deviceData.manufacturer)); > + Assert.ok(checkNullOrString(deviceData.hardware)); > + Assert.ok(checkNullOrBool(deviceData.model)); s/model/isTablet/ @@ +285,5 @@ > + Assert.ok(Number.isFinite(osData["servicePackMajor"]), > + "ServicePackMajor must be a number."); > + Assert.ok(Number.isFinite(osData["servicePackMinor"]), > + "ServicePackMinor must be a number."); > + } Missing checks: * kernelVersion * locale @@ +289,5 @@ > + } > + > + for (let disk of EXPECTED_HDD_FIELDS) { > + Assert.ok(checkNullOrString(data.system.hdd[disk].model)); > + Assert.ok(checkNullOrString(data.system.hdd[disk].revision)); I think we should expect those to be non-null? @@ +295,5 @@ > + > + let gfxData = data.system.gfx; > + Assert.ok(checkNullOrBool(gfxData.D2DEnabled)); > + Assert.ok(checkNullOrBool(gfxData.DWriteEnabled)); > + Assert.ok(checkNullOrString(gfxData.DWriteVersion)); Those should be non-null on windows? @@ +301,5 @@ > + > + Assert.ok(gfxData.adapters.length > 0, "There must be at least one GFX adapter."); > + for (let adapter of gfxData.adapters) { > + checkGfxAdapter(adapter); > + } Also, check for the first adapter also having GPUActive==true

Added |applicationName| to the docs.

Added the missing kernelVersion and locale checks, removed the redundant profile checks, removed the trailing whitespaces, added |gIsGonk|.

Comment on attachment 8563986 [details] [diff] [review] part 3 - Improve test coverage for environment data (v4) Review of attachment 8563986 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js @@ +269,5 @@ > + } else if (gIsAndroid || gIsGonk) { > + Assert.ok(checkString(osData.kernelVersion)); > + } > + > + for (let disk of EXPECTED_HDD_FIELDS) { This still fails for me on Mac. I think we discovered before that this is only really implemented for Windows.

Sorry, I forgot to attach the updated patch last time.

Rebase.

Pushed in: https://hg.mozilla.org/integration/fx-team/pushloghtml?changeset=b83c3fa8c80f

https://hg.mozilla.org/mozilla-central/rev/e0ebc1a3ec38 https://hg.mozilla.org/mozilla-central/rev/22df39a6c1fc https://hg.mozilla.org/mozilla-central/rev/c87290474968