The core metrics data ping module is responsible for managing the scheduling of submission, sending events for observers to receive, gathering the metrics generated by the observers, and posting the data to the web service Patches will be attached by Monday EOD.

Comment on attachment 589048 [details] [diff] [review] Part 10 - Main Metrics.jsm module (data collection) >+ // Extract a subset of fields >+ let addonFields = ["id", "userDiabled", "appDisabled", "version", >+ "installDate", "updateDate"]; >+ ret.addons = []; >+ for each (addon in gAddons) { >+ let addonSubset = {}; >+ for each (f in addonFields) { >+ addonSubset[f] = addon[f]; >+ } >+ ret.addons.push(addonSubset); >+ } >+ } I use custom personas and have in the past written extensions that I know nobody else is using... I consider this personally identifiable information, since the ids will point to my home page.

(In reply to Dão Gottwald [:dao] from comment #11) > I use custom personas and have in the past written extensions that I know > nobody else is using... I consider this personally identifiable information, > since the ids will point to my home page. Very good point and very difficult problem. :) Having IDs for extensions is pretty important because discovering add-ons that cause performance or stability problems in our user-base is a major benefit of this feature. We can't really have a checking service that determines if an ID should be masked or not because that would just move the identifying "ping". We could theoretically have the service send data to the client that contains a list of the top N add-ons, but that would have to be maintained and it would prevent us from being able to find add-ons that caused problems but weren't popular enough. I have a couple of other ideas that might help with situations like this, but nothing that would be straight-forward enough to accomplish in the first version of this feature. Other than opting out because you know you are an outlier, do you have any ideas that would help us mitigate this issue without hampering our ability to study add-ons and improve Firefox for the majority of our userbase?

Note that I'm not actually interested in my particular case... I already enabled telemetry and know what's being sent. More interesting are cases of users that would disable this if they had to make an informed decision but won't disable it because they don't know about the ping in the first place or trust us about not sending personally identifiable data. I have no ideas how to solve this, other than not sending such data without the user's consent.

But even with user consent, is it reasonable to think that the user has inspected the ping to look for PIIs? is user consent obtained once? or for every ping? what happens when for new Firefox downloads with no add-ons, the user with consent wont see any PII. I wouldn't be surprised that most users who provide consent "trust us". I think the key thing here is that they can *easily* turn the feature off.

Have you y'all (Metrics) talked with the Privacy folks (Alex Fowler et al)? This will need a privacy review anyway, so going over the code and specific bits of data is kinda premature until Privacy is onboard with the big-picture stuff. Just want to make sure we don't sink a bunch of time into something that has to radically change at the last minute!

Yes, we have spent the last few months discussing and working hrough the strategy, purpose, and need for this feature with Privacy and the User Data Council. These bugs are the result of that work.

(In reply to Saptarshi Guha from comment #14) > But even with user consent, is it reasonable to think that the user has > inspected the ping to look for PIIs? Probably not... So yes, making sure the decision is an informed one is another problem, but no good reason for doing it without consent. > I think the key thing here is that they can *easily* turn the feature off. I don't think privacy works like this on a large scale. We can't expect everyone who happens to be identifiable to make a self-motivated decision. People trust Mozilla not to leak data like that by default.

Comment on attachment 589045 [details] [diff] [review] Part 7 - Main Metrics.jsm module (base + utilities) + +XPCOMUtils.defineLazyGetter(this, "gPref", function bls_gPref() { + return Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefService) + .QueryInterface(Ci.nsIPrefBranch2); +}); + +XPCOMUtils.defineLazyServiceGetter(this, "gConsoleService", + "@mozilla.org/consoleservice;1", + "nsIConsoleService"); do not reinvent https://developer.mozilla.org/en/JavaScript_code_modules/Services.jsm +// For testing the day-timer. +//const SECONDS_PER_DAY = 300; do not leave testing code What's addMerge for? My r+ is not sufficient to land this. You need to get relevant module owners to review respective patches.

Comment on attachment 589048 [details] [diff] [review] Part 10 - Main Metrics.jsm module (data collection) Review of attachment 589048 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/telemetry/Metrics.jsm @@ +1181,5 @@ > + //ret.addons = gAddons; > + > + // Extract a subset of fields > + let addonFields = ["id", "userDiabled", "appDisabled", "version", > + "installDate", "updateDate"]; We'd also like to collect the hasBinaryComponents property (important for addons that aren't on AMO).

(In reply to Dão Gottwald [:dao] from comment #17) > (In reply to Saptarshi Guha from comment #14) > > But even with user consent, is it reasonable to think that the user has > > inspected the ping to look for PIIs? > > Probably not... So yes, making sure the decision is an informed one is > another problem, but no good reason for doing it without consent. > > > I think the key thing here is that they can *easily* turn the feature off. > > I don't think privacy works like this on a large scale. We can't expect > everyone who happens to be identifiable to make a self-motivated decision. > People trust Mozilla not to leak data like that by default. ping?

Comment on attachment 589045 [details] [diff] [review] Part 7 - Main Metrics.jsm module (base + utilities) > +const PREF_METRICS_PING_UUID = "toolkit.metrics.ping.uuid"; I object to that. See bug 718066 comment 4.

(In reply to Dão Gottwald [:dao] from comment #20) > (In reply to Dão Gottwald [:dao] from comment #17) > > (In reply to Saptarshi Guha from comment #14) > > > But even with user consent, is it reasonable to think that the user has > > > inspected the ping to look for PIIs? > > > > Probably not... So yes, making sure the decision is an informed one is > > another problem, but no good reason for doing it without consent. > > > > > I think the key thing here is that they can *easily* turn the feature off. > > > > I don't think privacy works like this on a large scale. We can't expect > > everyone who happens to be identifiable to make a self-motivated decision. > > People trust Mozilla not to leak data like that by default. > > ping? Going to reply to this in the mea bug 718066 so we can keep this bug's comments as much about the code as possible.

Incorporated feedback from Taras, changed the timer between gathering data and submission to use a normal timer instead of waiting for idle.

Incorporated feedback from Taras, changed the timer between gathering data and submission to use a normal timer instead of waiting for idle.

Wraps the ShellService calls in a try/catch to fix submission errors in Xubuntu (and possibly other platforms)

(In reply to Taras Glek (:taras) from comment #18) > Comment on attachment 589045 [details] [diff] [review] > Part 7 - Main Metrics.jsm module (base + utilities) > > + > +XPCOMUtils.defineLazyGetter(this, "gPref", function bls_gPref() { > + return > Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefService) > + .QueryInterface(Ci.nsIPrefBranch2); > +}); > + > +XPCOMUtils.defineLazyServiceGetter(this, "gConsoleService", > + "@mozilla.org/consoleservice;1", > + "nsIConsoleService"); > > do not reinvent > https://developer.mozilla.org/en/JavaScript_code_modules/Services.jsm These have been removed and replaced with Services.prefs and Services.console respectively. > +// For testing the day-timer. > +//const SECONDS_PER_DAY = 300; > do not leave testing code Removed. > What's addMerge for? addMerge is used for the Event aggregation in Part 8 - it merges 2 aggregate event logs by adding the value of any common keys.

As noted in email, I have concerns about the general plan and some specifics, so I am going to postpone any reviews here until documents about the general plan as well as specifics about privacy and data have been posted publicly.

This group of patch updates moves the cumulative collection of data into the client, and changes from a static UUID to a per-submission Document ID as discussed in the security review from Feb. 2nd - https://wiki.mozilla.org/Security/Reviews/MetricsDataPing Some changes with respect to Addons: - We no longer collect specific information about addons of type "theme", since they provide little analytic value and are more likely to be personally identifying. This partially addresses Comment 11. - Add "hasBinaryComponents" for addons that contain a value for it per Comment 19.

Small update to truncate Addon install/update dates to "day" precision

(In reply to Mark Reid from comment #35) > This group of patch updates moves the cumulative collection of data into the > client, and changes from a static UUID to a per-submission Document ID as > discussed in the security review from Feb. 2nd - > https://wiki.mozilla.org/Security/Reviews/MetricsDataPing Since this sends the previous ID along with the current ID and relies on the server side to merge the submissions in some privacy-sensitive manner, isn't this technically the same as a static UUID?

(In reply to Dão Gottwald [:dao] from comment #37) > (In reply to Mark Reid from comment #35) > > This group of patch updates moves the cumulative collection of data into the > > client, and changes from a static UUID to a per-submission Document ID as > > discussed in the security review from Feb. 2nd - > > https://wiki.mozilla.org/Security/Reviews/MetricsDataPing > > Since this sends the previous ID along with the current ID and relies on the > server side to merge the submissions in some privacy-sensitive manner, isn't > this technically the same as a static UUID? The server doesn't merge, the server uses the previous ID to delete that document. Several of the details are discussed in the security review meeting notes linked above. I understand that it does not relieve every worry about the possibility of the process being subverted and somehow being used to identify people, but that wasn't the primary purpose for making the change. The change was made because it reduced the surface area on the server for potential attacks or abuse, and because it provides significant privacy and security gains over the static UUID method. The current implementation still has a requirement to allow the user to remove the data associated with one of their Firefox profiles/installations from our servers, and that means we need some sort of document identifier to enable that.

(In reply to Daniel Einspanjer :dre [:deinspanjer] from comment #38) > The server doesn't merge, the server uses the previous ID to delete that > document. That's better. Could this be separated somehow to avoid the possibility to link the two IDs? The user would still need to trust Mozilla to actually delete the data when requested, of course...

Because the server supports removing the data, a delete request can be sent by itself. However, if the two IDs are not sent together, then only one of them might succeed. If the client manages the transaction itself, it would have to either send the delete, wait for it to suceed, then send the new document (for which there is no guarantee of success), or it could send the new document then send the delete and hope that the delete request went through properly. In either case, the server could be left in an inconsistent state where we have bad statistics, and worse, potentially could leave an orphaned document where the client doesn't keep the ID and can't send a delete request in the future. Finally, if the client sent two requests, it would only be effective if they were sent at the same time. That would not actually make any substantial improvement to user security or privacy as far as trusting Mozilla to do what is expected because we could always use the two requests if we decided to change the source code to implement linking. Obviously, that is not the goal which is why we have mitigating factors such as privacy policies, open source server code, and visibility into the data as primary requirements for the project. No one will argue that those factors create a perfect situation where trust is not needed, but we are trying hard to stay responsive to feedback and demonstrate our intent through making code improvements and being as transparent as possible.

Removed some unused code, and use UTC times for formatting Day-precision timestamps.

Removed some unused code

Removed the "fetchServerData" function which is no longer needed.

Truncate timestamps to 1-day resolution, and track version changes at the top level, rather than putting the version in each day's data point.

Comment on attachment 595863 [details] [diff] [review] Part 10 - Main Metrics.jsm module (data collection) Review of attachment 595863 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/telemetry/Metrics.jsm @@ +378,5 @@ > + //Metrics.log("Gathering async metrics..."); > + this.gatherPlacesData(); > + > + // asynchronously get addon count. > + AddonManager.getAllAddons(function(addons){ Getting all addons means you'll also get 3rd party addon types - eg, those provided by Scriptish (the new Greasemonkey) and Stylish. I suspect those won't be very useful to analyze, but they could be used for fingerprinting. I assume what you really want is just extensions and plugins: AddonManager.getAddonsByTypes(["extension", "plugin"], function(addons) { ... }); That also saves you from filtering themes and lightweight themes (aka personas/backgrounds) later on. As an aside, bug 520124 will split themes and lightweight themes into separate types ("theme" and "lwtheme" respectively) - not sure when that will land, though. It means you'll need to count themes separately (I assume that's what gAddonCount is trying to indirectly do), but the way it's currently being counted is also counting all other extension types too, so isn't very useful IMO. If we do want a count, it should be explicitly only counting addons of type "theme" (and "lwtheme", once that exists). @@ +1137,5 @@ > + var addons = []; > + if (gAddonCount != -1) { > + // Extract a subset of fields > + let addonFields = ["id", "userDisabled", "appDisabled", "version", "type", > + "hasBinaryComponents"]; I know I already commented on this field list, but... I think it'd be useful to also include the "scope" and "foreignInstall" properties here. "scope" describes which scope the addon is installed in - Firefox profile, application directory, etc. And "foreignInstall" describes whether the addon was installed by a 3rd party. That would finally give us concrete data on 3rd party addon installs.

Clearing the nomination flag for ff12 as this is not currently in aurora.

Removed preprocessor stuff for the javascript files and rebased to latest. Carrying forward the r+ from netzen.

Removed commented-out code and fixed an extraneous comma.

rebased to latest code

Renamed some prefs

Renamed some prefs

Changed to ignore search events in offline mode.

Updates submission and deletion to use Document ID, and removes obsolete "fetchServerData" code.

Changed the payload structure, fixed crash counting, updated addon counts (by type instead of overall number) and added the two extra addon data fields per Comment 45.

AIUI the current status of this, there is going to be a more detailed/comprehensive proposal posted. I would like to refrain from technical review until that information is finalized.

Comment on attachment 603376 [details] [diff] [review] Part 9 - Main Metrics.jsm module (server communication) Taking this out of my queue for the moment, per comment 55.

Comment on attachment 603363 [details] [diff] [review] Part 2 - Generate search notifications likewise, please re-request review when the docs are finalized.

Comment on attachment 603371 [details] [diff] [review] Part 7 - Main Metrics.jsm module (base + utilities) likewise per previous couple of comments

This group of patch updates deals with some bitrot/cleanup and adds support for sampling and gzip compression of payloads.

This group of patch updates fixes some more bitrot and uses the common UpdateChannel code per bug 721165. It also adds a locale placeholder to the about:metrics URL for improved i18n / l10n support.

Comment on attachment 663774 [details] [diff] [review] Part 9 - Main Metrics.jsm module (server communication) Review of attachment 663774 [details] [diff] [review]: ----------------------------------------------------------------- Drive-by review... Pretty good for a first draft. Some general nits: * Always use "let" instead of "var" in chrome JS. * Some trailing whitespace. ::: toolkit/components/telemetry/Metrics.jsm @@ +238,5 @@ > + var responseStatus = request.channel.QueryInterface(Ci.nsIHttpChannel).responseStatus; > + if (responseStatus == 404) { > + success = true; > + Metrics.log("Success: There was no server data to delete"); > + } Logging the status code might be useful. @@ +249,5 @@ > + if (success) { > + Metrics.log("Successfully deleted server data"); > + Metrics.log("Response: " + request.responseText); > + } else { > + Metrics.log("Error deleting server data."); I'd just handle this in the above block. That makes the logic easier to follow, IMO. @@ +255,5 @@ > + > + if (aCallback) { > + aCallback(success); > + } > + } Perhaps this function should be moved into module scope? I personally find code hard to read when large closures pollute function bodies. If nothing else, moving it to module scope makes it easier to unit test the response handling as you can just mock the response instead of sending a request all the way through. @@ +307,5 @@ > + .createInstance(Ci.nsIXMLHttpRequest); > + request.mozBackgroundRequest = true; > + request.open("POST", url, true); > + request.overrideMimeType("text/plain"); > + request.setRequestHeader("Content-Type", "application/json"); Always set charset for textual data. Yes, RFC 4627 says the default encoding for application/json is UTF-8. But, if you know what the encoding actually is, remove the guesswork from the receiving agent. @@ +348,5 @@ > + MetricsEventRecorder._persist("previous_submission", payload); > + } else { > + Metrics.log("Error submitting Metrics data."); > + // TODO: schedule a retry, and set a "retrying" flag so we don't double > + // up on attempts. This should be a P0. This code will quickly become unwieldy. What happens if the client is restarted? Is the data lost? Also, there needs to be a backoff here to avoid the thundering herd problem. @@ +350,5 @@ > + Metrics.log("Error submitting Metrics data."); > + // TODO: schedule a retry, and set a "retrying" flag so we don't double > + // up on attempts. > + } > + } Another long closure that could probably be uplifted to module scope. @@ +360,5 @@ > + // Log payload to console > + Metrics.log(JSON.stringify(payload)); > + > + // Send to the server > + request.send(JSON.stringify(payload)); Save serialization to a variable to avoid redundant work. ::: toolkit/components/telemetry/MetricsPing.js @@ +334,4 @@ > // Since we don't start watching the preference again, switching > // it back on won't have any effect until the next restart. > } > + } else if (aData == "submit.override") { Perhaps the blocks of this switch statement should be separated into functions. If nothing else that should make it a little easier to unit test. @@ +369,5 @@ > + // then just reset the pref upon success? > + Services.prefs.setBoolPref(PREF_DEL_REQUESTED, true); > + Services.prefs.setBoolPref(PREF_DEL_SUCCEEDED, false); > + > + Metrics.deleteServerData(server, function(success) { Name function.

Comment on attachment 663775 [details] [diff] [review] Part 10 - Main Metrics.jsm module (data collection) Review of attachment 663775 [details] [diff] [review]: ----------------------------------------------------------------- I'm not sure how I feel about Metrics.jsm containing all this domain-specific functionality. I think I'd favor an architecture where there was a "core" metrics component that handled I/O, service interaction, buffered storage, etc. Then, all the domain-specific bits focused on collection would live elsewhere. The main reason for this is that metrics collections will obviously have variations across different applications and platforms. I don't think it's scalable to have all of this in one file (Metrics.jsm). We either have lots of code that doesn't apply to the current application or we litter the file with #ifdef. I don't like either of those. I would favor an architecture where metrics-providing subsystems registered with the main metrics collector. Each application would only register the collectors relevant to it. Ideally this code would live close to the subsystem it is involved with. e.g. the Places metrics collecting code would live next to Places under toolkit/components/places/. In short, I think we should build a generic metrics collection service.

Comment on attachment 663775 [details] [diff] [review] Part 10 - Main Metrics.jsm module (data collection) Review of attachment 663775 [details] [diff] [review]: ----------------------------------------------------------------- Shotgun review to illustrate some style points. Generally, we need to split this into a few different component modules: * Data collection * Data persistence * Payload composition * Payload formatting * Envelope persistence * Envelope submission * System orchestration (that is, registering data collectors, asking for payloads, forming envelopes, and queueing them for upload). Right now it's all pretty smushed together, which is going to make testing, extending, and porting it a pain. ::: toolkit/components/telemetry/Metrics.jsm @@ +146,5 @@ > return fullTimestamp.slice(1, 11); > }, > > + // Calculate a date 'numDays' days prior to the specified date. > + getDaysAgo: function Metrics_getDaysAgo(aDate, numDays) { Lose the function name prefixes. Backtraces already tell you the file. @@ +170,5 @@ > > + // Build the data payload > + getPayload: function Metrics_getPayload(aReason, aPingEndDate) { > + // What time range does this payload cover? > + let lastUpdateTimePref = getPref("getIntPref", PREF_METRICS_PING_LASTUPDATE, 0); Testability will be improved if you aim for pure functions. The time range should be an input to this function. @@ +173,5 @@ > + // What time range does this payload cover? > + let lastUpdateTimePref = getPref("getIntPref", PREF_METRICS_PING_LASTUPDATE, 0); > + let lastUpdateTime = new Date(lastUpdateTimePref * 1000); > + > + // TODO: expose "payload" during the "gather async" stage so that Nit: trailing whitespace throughout. @@ +180,5 @@ > + schemaVersion: PAYLOAD_VERSION, > + lastPingTime: Metrics.formatDay(lastUpdateTime), > + thisPingTime: Metrics.formatDay(aPingEndDate), > + reason: aReason, > + addons: getAddons() Further to Greg's point: there's no reason why a function named "getPayload" should know anything about addons. Formatting an envelope, submitting that envelope, and constructing the contents are three separate tasks. @@ +192,5 @@ > + if (gEvents.currentSession.startTime) { > + // Calculate current session duration relative to "now", not > + // relative to aPingEndDate since activeTime is up-to-date. > + payload.currentSessionTime = Math.round((new Date() > + - gEvents.currentSession.startTime) / 1000); You should consider using Date.now() for this kind of thing. @@ +199,5 @@ > + payload.currentSessionActiveTime = gEvents.currentSession.activeTime; > + } > + } > + > + Metrics.refreshSimpleMeasurements(); Formatting a payload shouldn't have any side-effects. I should be able to call getPayload() twice and get the same output. @@ +318,5 @@ > + /** > + * Insert metadata about the application and playform into > + * the specified payload. > + */ > + insertAppInfo: function Metrics_insertAppInfo(aPayload) { Generally, prefer non-mutating approaches to this. If you have an "appInfo" member for the payload, you could just have this method return a single opaque blob… @@ +415,5 @@ > + } > + } > + }, > + pagesCount: { > + sql: "SELECT count(*) FROM moz_places", Firstly, this shouldn't live here. It's going to do bad, bad things on Android, and probably B2G, too. Secondly, this is the kind of thing that should live in a tested abstraction layer.

Comment on attachment 663772 [details] [diff] [review] Part 7 - Main Metrics.jsm module (base + utilities) Review of attachment 663772 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/telemetry/Metrics.jsm @@ +126,5 @@ > + // Browser started to the about:sessionrestore page > + SESSION_RESTORE_FROM_CRASH: 21 > +}; > + > +let Metrics = { I really don't like singletons. Singletons are a special case of globals and globals are evil because they enforce your way on the world. I would prefer there to exist a MetricsCollector type (or similar - naming is hard). We can, of course, instantiate an instance of this type as a Gecko service and/or make it available under a special global variable. Why would we want to do this? Well, what happens when we want to instantiate multiple instances of the metrics collector? We can't do this unless it is a type (not a singleton). Why would we want to instantiate multiple instances? Testing is a good example. Create an instance, modify its state, test it, throw it away, move on to the next test. With singletons, now you have to clean up the singleton's state after testing or you have to start a new process. That's a lot of extra work and IMO discourages thorough testing. Also, there might be legitimate use cases for multiple instances. Perhaps an add-on wishes to provide its own metrics collection system outside of what Mozilla may provide via sharing of the data we collect. Well, if this were a type not a singleton, add-ons could instantiate an instance, register their own collectors, point it at their own server, and they get a framework for collecting data without having to write a lot of code! What about B2G? If carriers wanted a subset of metrics data, we could instantiate a separate metrics collector instance that pointed to the carrier's server and it could co-exist with Mozilla's. This could all be done without having to touch a single line in Metrics.jsm. In summary, singletons/globals limit flexibility and re-usability of code. Build (generic) libraries that can be more easily tested and used to solve tomorrow's problems. @@ +147,5 @@ > + gProfileTimestamp = -1; > + gPlacesPagesCount = -1; > + gPlacesBookmarksCount = -1; > + gAddonCounts = {}; > + gAddons = []; When this is refactored to not be a singleton, referenced state should be passed into the constructor.

Comment on attachment 663772 [details] [diff] [review] Part 7 - Main Metrics.jsm module (base + utilities) Review of attachment 663772 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/telemetry/Metrics.jsm @@ +155,5 @@ > + generateUUID: function Metrics_generateUUID() { > + let str = Cc["@mozilla.org/uuid-generator;1"] > + .getService(Ci.nsIUUIDGenerator).generateUUID().toString(); > + // trim '{' and '}' > + return str.substring(1, str.length - 1); There are privacy implications here. Version 1 UUIDs contain the MAC address of the host machine. I doubt this is something we'd like to go over the wire because it can be used to track users (UUIDs created with this function are part of the data being sent to the server). We probably want version 4 UUIDs (random) instead. Unfortunately, https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIUUIDGenerator does not state which UUID version is produced nor does it allow us to specify one. The implementation (xpcom/base/nsUUIDGenerator.cpp) calls out to CoCreateGuid on Windows and CFUUIDCreate on OS X. Neither doc page states which UUID version is returned AFAICT. I interpret that to mean it is undefined and thus prone to change at any time and possibly varying on different run-time platforms. If not on Windows or OS X, our in-line implementation appears to generate version 4 UUIDs. Although, it does call out to random() unless on Android, so it might not be cryptographically secure. I defer to a security expert on whether we care. I'm going to assert that we don't want to take a chance that version 1 UUIDs are used because they leak (unnecessary?) information to Mozilla. If we are relying on version 1 UUIDs to correlate uploads from the same device, nsIUUIDGenerator doesn't seem to guarantee behavior, so I don't think we can rely on it for this behavior. Either way, more info is needed.

UuidCreate (on Windows) does guarantee a fully random GUID, and CoCreateGuid documents that it forwards to UuidCreate. I think we should just fix the interface/impl to have the guarantees we need, instead of trying to code around it in odd ways.

(In reply to Gregory Szorc [:gps] from comment #85) > Comment on attachment 663772 [details] [diff] [review] > Part 7 - Main Metrics.jsm module (base + utilities) > > Review of attachment 663772 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: toolkit/components/telemetry/Metrics.jsm > @@ +155,5 @@ > > + generateUUID: function Metrics_generateUUID() { > > + let str = Cc["@mozilla.org/uuid-generator;1"] > > + .getService(Ci.nsIUUIDGenerator).generateUUID().toString(); > > + // trim '{' and '}' > > + return str.substring(1, str.length - 1); > > There are privacy implications here. I don't pretend to understand the rest of your comment (where is this version 4 uuid generator, for one thing...?) but this code is exactly what Telemetry already uses for its session identifiers. So either we need to fix Telemetry posthaste, or this code is just fine.

(In reply to Nathan Froyd (:froydnj) from comment #87) > I don't pretend to understand the rest of your comment (where is this > version 4 uuid generator, for one thing...?) but this code is exactly what > Telemetry already uses for its session identifiers. So either we need to > fix Telemetry posthaste, or this code is just fine. Read https://en.wikipedia.org/wiki/Universally_unique_identifier and learn about different variants and versions of UUIDs. If Telemetry is posting UUIDs created with nsIUUIDGenerator.generateUUID() and you are under the assumption that those UUIDs are not traceable, than I think you have a problem. I think Gecko should expose UUID generation functions that explicitly declare what version of UUIDs are returned. I will file a bug.

(In reply to Gregory Szorc [:gps] from comment #88) > (In reply to Nathan Froyd (:froydnj) from comment #87) > > I don't pretend to understand the rest of your comment (where is this > > version 4 uuid generator, for one thing...?) but this code is exactly what > > Telemetry already uses for its session identifiers. So either we need to > > fix Telemetry posthaste, or this code is just fine. > > Read https://en.wikipedia.org/wiki/Universally_unique_identifier and learn > about different variants and versions of UUIDs. Thanks. FWIW, the online docs for CFUUIDCreate on Mac seem to indicate that the MAC address gets mixed in somehow, though I'd like to think that Apple was smart enough to not stick it into the UUID directly. If somebody has a Mac and wants to play around with what generateUUID gives us, that would be interesting too.

(In reply to Benjamin Smedberg [:bsmedberg] from comment #86) > UuidCreate (on Windows) does guarantee a fully random GUID, and CoCreateGuid > documents that it forwards to UuidCreate. I think we should just fix the > interface/impl to have the guarantees we need, instead of trying to code > around it in odd ways. You are correct. I was originally thrown off because the MSDN docs were mentioning things like "ethernet or token-ring hardware address." However, the remarks section says it cannot be traced to the ethernet address. So, I guess they are doing something funky. (In reply to Nathan Froyd (:froydnj) from comment #89) > (In reply to Gregory Szorc [:gps] from comment #88) > > (In reply to Nathan Froyd (:froydnj) from comment #87) > > > I don't pretend to understand the rest of your comment (where is this > > > version 4 uuid generator, for one thing...?) but this code is exactly what > > > Telemetry already uses for its session identifiers. So either we need to > > > fix Telemetry posthaste, or this code is just fine. > > > > Read https://en.wikipedia.org/wiki/Universally_unique_identifier and learn > > about different variants and versions of UUIDs. > > Thanks. FWIW, the online docs for CFUUIDCreate on Mac seem to indicate that > the MAC address gets mixed in somehow, though I'd like to think that Apple > was smart enough to not stick it into the UUID directly. If somebody has a > Mac and wants to play around with what generateUUID gives us, that would be > interesting too. The very definition of version 1 UUIDs contains *both* a time component and the MAC address. Apple would be following the specification if they incorporated the MAC address. I created a simple Core Foundation program that used CFUUIDCreate and it produced the UUID 013EE4A0-AF53-4469-B887-72C4C4F5601E. Using Python: >>> uuid.UUID(hex='013EE4A0-AF53-4469-B887-72C4C4F5601E').version 4 Also, I filed bug 801950 for proper Gecko APIs regarding UUID generation. Maybe it will be a WONTFIX since Windows and Mac appear to not incorporate MAC addresses.

David: I'm interested in using OS.File for this work. However, I need to backport the code to current Aurora. What's the state of OS.File there? If there are fixes/features not supported in Aurora a) what are they b) do you think they could get backported without much complaint from release drivers?

Most of OS.File is present in Aurora. As far as I remember, there are no fixes that need to be backported yet, although I am currently tracking a bug that may require one such fix. I will try and tell you more about this by Tuesday.

This is carried over from bug 802914 with some minor naming changes and additions of core testing code. This bug is the more appropriate place for it. This patch creates toolkit/healthreport. It will contain code centered around Firefox Health Report. While some of this code might constitute generic metrics gathering that could be used outside of FHR, I'm content with landing everything in toolkit/healthreport today and bikeshedding over naming tomorrow. After all, not all the code is written yet and any naming/should-this-module-go-here discussion is silly until we have a complete understanding of what things will look like. rnewman gets primary review. Mossop provides toolkit sign-off.

Comment on attachment 675187 [details] [diff] [review] Part 1: Create healthreport skeleton in toolkit, v1 Review of attachment 675187 [details] [diff] [review]: ----------------------------------------------------------------- I realise we've flip-flopped over names and yes renaming is relatively cheap later, but I want it to go in initially as toolkit/metrics for a couple of reasons. People keep saying this will include some generic metrics work in addition to the health report specifics bits. I also don't want to use productized names in the source in general since they could well change before release (see personas). ::: toolkit/healthreport/modules-testing/common.jsm @@ +7,5 @@ > +const EXPORTED_SYMBOLS = ["initConsoleLogging"]; > + > +const {utils: Cu} = Components; > + > +Cu.import("resource://services-common/log4moz.js"); log4moz doesn't ship in all apps so you shouldn't use it from here.

(In reply to Dave Townsend (:Mossop) from comment #94) > (see personas). and Weave... http://mxr.mozilla.org/mozilla-central/search?string=weave

(In reply to Dave Townsend (:Mossop) from comment #94) > ::: toolkit/healthreport/modules-testing/common.jsm > @@ +7,5 @@ > > +const EXPORTED_SYMBOLS = ["initConsoleLogging"]; > > + > > +const {utils: Cu} = Components; > > + > > +Cu.import("resource://services-common/log4moz.js"); > > log4moz doesn't ship in all apps so you shouldn't use it from here. What apps doesn't it ship with because I'm pretty sure it ships with all of them (at least the ones we care about that are in-tree). See bug 800154.

(In reply to Gregory Szorc [:gps] from comment #96) > (In reply to Dave Townsend (:Mossop) from comment #94) > > ::: toolkit/healthreport/modules-testing/common.jsm > > @@ +7,5 @@ > > > +const EXPORTED_SYMBOLS = ["initConsoleLogging"]; > > > + > > > +const {utils: Cu} = Components; > > > + > > > +Cu.import("resource://services-common/log4moz.js"); > > > > log4moz doesn't ship in all apps so you shouldn't use it from here. > > What apps doesn't it ship with because I'm pretty sure it ships with all of > them (at least the ones we care about that are in-tree). See bug 800154. Doesn't look like Thunderbird has it, nor is it included in XULRunner.

(In reply to Dave Townsend (:Mossop) from comment #97) > Doesn't look like Thunderbird has it, nor is it included in XULRunner. That can easily be corrected (if we need to). My intent is to move log4moz.js and other pieces from services/common to toolkit. So, I think you should consider it part of toolkit today. It just happens to have a different import URI for the time being.

Sync services are at a higher level than toolkit and have no meaningful connection with Marionette. What happened in bug 800154 is a hack that works around messed up dependencies. :/ > My intent is to move log4moz.js and other pieces from services/common to > toolkit. So, I think you should consider it part of toolkit today. I don't think you can take for granted that it will make it into toolkit. File a bug on it, ask for review, and either make it block this bug or avoid using it here for the time being?

(In reply to Dão Gottwald [:dao] from comment #99) > Sync services are at a higher level than toolkit and have no meaningful > connection with Marionette. What happened in bug 800154 is a hack that works > around messed up dependencies. :/ Nothing in services/common is Sync specific. It is essentially our team's "toolkit" because we didn't feel like going through Toolkit's module review process at the time. > > My intent is to move log4moz.js and other pieces from services/common to > > toolkit. So, I think you should consider it part of toolkit today. > > I don't think you can take for granted that it will make it into toolkit. > File a bug on it, ask for review, and either make it block this bug or avoid > using it here for the time being? I'm trying to land things for B2G/Aurora 19. I am trying to minimize the number of changes necessary. This means sacrificing some purity and putting things off to follow-up bugs.

(In reply to Gregory Szorc [:gps] from comment #100) > (In reply to Dão Gottwald [:dao] from comment #99) > > Sync services are at a higher level than toolkit and have no meaningful > > connection with Marionette. What happened in bug 800154 is a hack that works > > around messed up dependencies. :/ > > Nothing in services/common is Sync specific. It is essentially our team's > "toolkit" because we didn't feel like going through Toolkit's module review > process at the time. Do you mean code review process? That's exciting but perhaps something to be discussed elsewhere. > > > My intent is to move log4moz.js and other pieces from services/common to > > > toolkit. So, I think you should consider it part of toolkit today. > > > > I don't think you can take for granted that it will make it into toolkit. > > File a bug on it, ask for review, and either make it block this bug or avoid > > using it here for the time being? > > I'm trying to land things for B2G/Aurora 19. I am trying to minimize the > number of changes necessary. This means sacrificing some purity and putting > things off to follow-up bugs. This doesn't mean you get to ship code that is blatantly broken for other toolkit consumers. If log4moz really hasn't undergone code review then we should do that regardless but that wouldn't hold up moving it into toolkit so I don't see why we wouldn't just do that. If that is actually not possible then we should be making sure this code only ships for apps where it will work.

Everything in services/ has been reviewed and should be safe to use from toolkit without sacrificing "purity."

(In reply to Gregory Szorc [:gps] from comment #102) > Everything in services/ has been reviewed and should be safe to use from > toolkit without sacrificing "purity." Then let's move it to toolkit.

(In reply to Dave Townsend (:Mossop) from comment #103) > (In reply to Gregory Szorc [:gps] from comment #102) > > Everything in services/ has been reviewed and should be safe to use from > > toolkit without sacrificing "purity." > > Then let's move it to toolkit. That is tracked in bug 451283. I agree we should do it! That being said, I am opposed to doing it as a prerequisite of this bug because assuming we do it properly, we'd have to update Sync, AITC, others (https://mxr.mozilla.org/mozilla-central/search?string=log4moz) and these changes would need to be backported to Aurora because this code will need to support B2G. I would prefer to not abuse Release Drivers with this needless overhead and risk (not to mention breaking imports on 3rd party Sync engines, other consumers). The hacky solution is to just copy log4moz.js to toolkit. But, then we have 3 copies of log4moz in the tree and possible redundant JS compartments constituting wastage. I think consuming log4moz from services-common until after this lands is the least hacky solution. Long term we can move log4moz to Toolkit and consolidate all in-tree users to use it. This should be done on m-c and should ride the trains.

On the bigger-picture "where should this code live?" question, after discussions involving both gps and Mossop we've agreed that we'll land the initial work in /services as a Firefox-only piece. We'll revisit where it should live once we've baked things for longer and we have time to explore the various tradeoffs. If there are further questions/concerns/comments on that front, those can go elsewhere to this bug.

Now creating things in services/metrics. I could conceivably split things into {metrics, healthreport}. But, I'm going to hold out until we know where the logical boundaries are going to be. The build system changes are trivial and are in-line with things there currently. IMO an explicit sign-off from a build peer is not necessary. (Besides, I am a build peer and I like to think I know what I'm doing here.) The only part of this patch I really question is the set of applications I enabled the component for. I enabled it for browser, mobile/xul, and b2g. This just hooks up build system integration. We'll still need per-application prefs to enable the feature (provided in later patches). rnewman: also note that I am installing things to services/metrics instead of services-metrics. That's in-line with our desire to install modules consistently with the rest of Gecko. (That's tracked elsewhere but I can't find the bug right now. I don't want to make the larger transition a prerequisite because of backporting concerns.)

(In reply to Gregory Szorc [:gps] from comment #106) > The only part of this patch I really question is the set of applications I > enabled the component for. I enabled it for browser, mobile/xul, and b2g. > This just hooks up build system integration. We'll still need > per-application prefs to enable the feature (provided in later patches). This list looks fine to me. After we get v1 out the door, we can engage with mobile/android and figure out how to carve up the bits they need to implement there.

Here is my plan for the internal data model. Instead of generic JS objects (essentially mappings), we have a dedicated type to hold our collected data. This is called `MetricsDataSet`. It consists of metadata and fields. Fields are currently a string name, a well-defined type (stronger than what JS provides), and the JS value. Essentially the FHR payload will consist of some generic metadata plus a set of `MetricsDataSet`. e.g. ApplicationInfo, AddonsInfo, UpgradeInfo, etc. Client and server will have some central spec that defines what the names and versions are. This will in turn define what fields are expected. The definition of a type is not required by the current implementation. However, Xavier, Mark, and I agree that there is a strong possibility that we will want to have the option of efficient binary encoding of FHR payloads because A) this saves precious mobile bandwidth B) saves resources on Mozilla's end (less hardware resources to deal with binary). We can make the transition to efficient binary encoding easier if we have strong type guarantees. If we fail to record the types today, we'll have to add strong guarantees later and I think that will be more effort. Having types may also help about:metrics, as it could potentially use this for richer data presentation. The second type in this patch is `MetricsDataProvider`. It is an base type for entities that provide metrics data. Currently, it only has one method, `collectConstantData`, but this will change with time to support new collection needs. Each Gecko component providing metrics data will have its own `MetricsDataProvider`-derived type. These will be registered with a central `MetricsDataCollector` (or similar) service or singleton. The `MetricsDataCollector` will periodically query registered `MetricsDataProvider` instances for new data. It will take the returned data and save it to disk, aggregate it for upload, etc. `MetricsDataProvider` likely needs a lot of work. For example, it should probably have a way of saying "data not ready." I'll figure out specifics as I implement more of the P0 probes. I'm asking for feedback to bless my general approach here. I think it's consistent with what we've talked about offline. I think it strikes a good balance between simple enough to land rapidly but robust enough to scale out to support our needs of tomorrow.

So for the initial version, would you expect that objects of this type could be easily serialized into a JSON object for submission? Would we have to develop the full binary route from source through submission to sink in order to release v1? Would the about:metrics page still be able to deal with the payload as a JSON/JavaScript object or would it have to transform the binary payload itself?

(In reply to Daniel Einspanjer :dre [:deinspanjer] from comment #109) > So for the initial version, would you expect that objects of this type could > be easily serialized into a JSON object for submission? Yes. `DataProviderSet` has a toJSON() method so we can just JSON.stringify() the instance. > Would we have to develop the full binary route from source through > submission to sink in order to release v1? No. That can be added later. > Would the about:metrics page still be able to deal with the payload as a > JSON/JavaScript object or would it have to transform the binary payload > itself? Yes to JSON. `DataProviderSet` instances could be encoded/serialized to whatever form makes sense for the consumer. This is a major benefit of having a dedicated JS type to represent the data rather than an anonymous {}. Likewise, if some "web purist" insisted on JSON for the upload rather than binary, we could probably expose that via a pref. As long as the server understands the Content-Type, we should be free to send whatever makes sense.

Example implementation of a data provider. This one collects basic application info. Patch is for curiosity piquing only. Not asking for any kind of review.

As I started implementing more, I realized that we really need the ability to capture a lot of rich metadata during the course of metrics gathering. This could be used to report partial data capture or errors encountered when capturing. This would lead to identification of client-side bugs. So, FHR could be used to find bugs in itself. Cool! Anyway, the big change now is that we have a dedicated type for capturing the results of a data collection request. It currently captures some metadata. I'm sure I'll think of more useful things for it to do as I code more.

As I wrote more code today, I finally found a good dividing line between generic metrics code and the FHR code. It has been split into services/metrics and services/healthreport in my local tree. I refactored this patch to include the creation of the healthreport skeleton as well.

Comment on attachment 675765 [details] [diff] [review] Part 2: Define generic data provider types, v2 Overall I like this approach. It seems flexible enough to cover current and foreseeable needs. One suggestion: >+ aggregate: function aggregate(other) { ... >+ for (let [name, data] of other.dataSets) { >+ if (name in this.dataSets) { >+ throw new Error("Incoming data has same data set as us: " + name); >+ } >+ >+ this.dataSets.set(name, data); >+ } >+ >+ for (let [missing, reason] of other.missingDataSets) { >+ this.missingDataSets.set(missing, reason); >+ } When you're adding dataSets from other, you should remove them from this.missingDataSets, and vice-versa - only add to this.missingDataSets if it's not already in this.dataSets. You don't want to end up with a name in both dataSets and missingDataSets.

Comment on attachment 675921 [details] [diff] [review] Part 1: Create skeletons for services/{metrics,healthreport} Review of attachment 675921 [details] [diff] [review]: ----------------------------------------------------------------- I'm no Makefile wizard, but this all looks cheerfully boilerplate to me. ::: mobile/android/confvars.sh @@ +19,5 @@ > # Enable getUserMedia > MOZ_MEDIA_NAVIGATOR=1 > > +MOZ_SERVICES_COMMON=1 > +MOZ_SERVICES_HEALTHREPORT=1 I'd like to see this split to be more granular (not necessarily now, but eventually). If we're going to be submitting through a background Android service in a few months, we need *two* healthreport definitions: collection and submission. The latter would be *off* for mobile/android. We could do this at runtime, but no sense shipping unnecessary code in the product….

(In reply to Richard Newman [:rnewman] from comment #115) > I'd like to see this split to be more granular (not necessarily now, but > eventually). > > If we're going to be submitting through a background Android service in a > few months, we need *two* healthreport definitions: collection and > submission. So, there are the JS modules and the XPCOM services. This patch is just build system integration for the JS modules. There's a minimal (albeit non-zero) cost to shipping unused JS modules. The XPCOM services can be enabled on a per app UUID basis. You haven't seen the patches yet, but current I'm planning on a single XPCOM service. This service does collection and data submission. It has separate prefs to control each. We could conceivably split that into multiple XPCOM services. Although, I'm not sure that gains us much. I'm all for crossing that bridge later.

(In reply to Gregory Szorc [:gps] from comment #116) > I'm all for crossing that bridge later. That works for me.

Submitting all the code I have so rnewman can start paging it in.

The type is minimal now. Essentially this is a coordinator type for metrics gathering. Eventually it will have persistence. It is arguably not necessary yet.

This implements the XPCOM service for the Firefox Health Reporter. Implementation is mostly there. Still lacking some test coverage. But, it does work (last I tried, anyway).

Refreshing patch.

Comment on attachment 675765 [details] [diff] [review] Part 2: Define generic data provider types, v2 Review of attachment 675765 [details] [diff] [review]: ----------------------------------------------------------------- I know you're about to upload another version, so have a review fragment! ::: services/metrics/dataprovider.jsm @@ +35,5 @@ > + * (string) Name of this data set. > + * @param version > + * (Number) Integer version of the data in this set. > + */ > +function MetricsDataSet(name) { I don't think DataSet is the right name here. A "dataset" (in common use) is a collection of measurements in the sense of multiple measurements of the same value: an array of these instances with the same fields would be a dataset. This is not that: it's a set of measurements taken at the same instant in time, right? (It's not even really a 'set', in that it's a map.) The word "sample" is tempting, but that's too confusing when it's close to the statistical definition of a selected subset of a population. I think the word you're looking for is "Measurement" (a correlation of a name and a value), pluralized. @@ +39,5 @@ > +function MetricsDataSet(name) { > + this.name = name; > + > + this._version = null; > + this._fields = new Map(); I would be inclined to have a fixed set of read-only fields supplied in the constructor. That offers some structure, some supplied meaning, and -- in theory -- will aid longitudinal processing of multiple measurements, because they can be assumed to have the same fields.

Comment on attachment 676901 [details] [diff] [review] Part 2: Metrics data providers, v3 Review of attachment 676901 [details] [diff] [review]: ----------------------------------------------------------------- ::: services/metrics/dataprovider.jsm @@ +33,5 @@ > + * > + * @param name > + * (string) Name of this data set. > + * @param version > + * (Number) Integer version of the data in this set. Doc and code disagree. I think the doc should be correct, and the version should be a constructor parameter. @@ +77,5 @@ > + * (string) Name of the field being set. > + * @param value > + * (Number) Value to set the field to. > + */ > + addUInt32Field: function addUInt32Field(name, value) { I think this might be backwards. What you've done here is allowed this: // Construct two things that should have the same shape. let measurementA = new MetricsDataSet("foo"); let measurementB = new MetricsDataSet("foo"); // Set values that are not congruent. measurementA.addUInt32Field("bar", 5); measurementB.addStringField("bar", "noo"); I think what you want is this: let fooFields = {"bar": UINT32}; // These two should be produced by a factory. let measurementA = new MetricsDataSet("foo", fooFields); let measurementB = new MetricsDataSet("foo", fooFields); measurementA.setValue("bar", 5); // This'll throw because bar should be a UINT32. measurementB.setValue("bar", "noo"); Then you can validate field contents in a stronger sense than "I called the right method" (even going so far as to attach validators to a field spec), verify that a measurement has the correct fields, etc.

Comment on attachment 676901 [details] [diff] [review] Part 2: Metrics data providers, v3 Review of attachment 676901 [details] [diff] [review]: ----------------------------------------------------------------- ::: services/metrics/dataprovider.jsm @@ +77,5 @@ > + * (string) Name of the field being set. > + * @param value > + * (Number) Value to set the field to. > + */ > + addUInt32Field: function addUInt32Field(name, value) { And note that these field specs can be as rich as you want: {"bar": {"type": UINT32, "min": 6, "max": 100, "validator": isEven, "optional": true}} This will prevent you from having to scatter this validation logic around the callsites. @@ +86,5 @@ > + if (value < 0) { > + throw new Error("addUIntField got a negative integer: " + value); > + } > + > + this._fields.set(name, [this.TYPE_UINT32, value]); To continue on the same vein: I suspect that the *field* should have a typespec ("percentage"); the value should just be a value. But I'm open to discussion on that. @@ +186,5 @@ > + } > + > + this.name = name; > + > + this.dataSets = new Map(); this.measurements feels kinda right. @@ +195,5 @@ > +MetricsDataCollectionResult.prototype = { > + /** > + * Add a `MetricsDataSet` to this result. > + */ > + addDataSet: function addDataSet(data) { contributeMeasurement. @@ +247,5 @@ > + this.dataSets.set(name, data); > + } > + > + for (let [missing, reason] of other.missingDataSets) { > + this.missingDataSets.set(missing, reason); This is incorrect, surely? If I have one result: R1: measurements: {A: 5} missing: {B: "borked"} and another: R2: measurements: {B: "yay"} missing: {A: "lame"} then aggregating R2 into R1 should yield: R3: measurements: {A: 5, B: "yay"} missing: {} not missing: {A: "lame", B: "borked"}

Comment on attachment 676894 [details] [diff] [review] Part 3: MetricsCollector Review of attachment 676894 [details] [diff] [review]: ----------------------------------------------------------------- I'm sure there'll be more comments as later patches arrive. ::: services/metrics/collector.jsm @@ +13,5 @@ > +Cu.import("resource://gre/modules/services/metrics/dataprovider.jsm"); > + > + > +/** > + * Handles and coordinates the collections of metrics data from providers. collections or collection? @@ +39,5 @@ > + throw new Error("argument must be a MetricsDataProvider instance."); > + } > + > + for (let p of this._providers) { > + if (p.provider == provider) { indexOf. @@ +46,5 @@ > + } > + > + this._providers.push({ > + provider: provider, > + constantsCollected: false, What's the behavior if *some* providers have contributed their constant data, and others have not? @@ +69,5 @@ > + if (provider.constantsCollected) { > + continue; > + } > + > + promises.push(provider.provider.collectConstantData()); You're never setting constantsCollected (so the test I suggest you write will fail). @@ +79,5 @@ > + /** > + * Handles promises returned by the collect* functions. > + * > + * This consumes the data resolved by the promises and returns a new promise > + * that will be resolved once all promises have been resolved. Oh hey look! It's one of my favorite interview questions! @@ +102,5 @@ > + }.bind(this); > + > + for (let promise of promises) { > + promise.then(onResult, onError); > + } Do we not already have a promise library function to chain in this way? ::: services/metrics/tests/xpcshell/test_data_collector.js @@ +24,5 @@ > + let collector = new MetricsDataCollector(); > + let dummy = new DummyProvider(); > + > + collector.registerProvider(dummy); > + do_check_eq(collector._providers.length, 1); Add a do_check_length function alongside do_check_attribute_count in common/tests/unit/head_helpers.js? @@ +48,5 @@ > + > + collector.collectConstantData().then(function onResult() { > + do_check_eq(collector._collectionResults.size(), 1); > + > + run_next_test(); Add a test that collectConstantData obeys its API contract: that it doesn't collect more than once.

Comment on attachment 676896 [details] [diff] [review] Part 5: HealthReporter service, v1 Review of attachment 676896 [details] [diff] [review]: ----------------------------------------------------------------- ::: services/healthreport/healthreporter.jsm @@ +3,5 @@ > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +"use strict"; > + > +this.EXPORTED_SYMBOLS = ["FirefoxHealthReporter"]; I'm probably not the person to make this determination, but is there a reason to call this FirefoxHealthReporter rather than HealthReporter? @@ +16,5 @@ > +Cu.import("resource://gre/modules/services/metrics/manager.jsm"); > +Cu.import("resource://gre/modules/services/healthreport/policy.jsm"); > + > + > +this.FirefoxHealthReporter = function FirefoxHealthReporter(branch) { I'm guessing this should come with a pretty thorough comment explaining what it's for. Here's my guess from reading the code: this is the desktop browser implementation of a coordinator, smushing together the roles of data aggregation (talking to the collector to grab results), serialization (turning results into JSON), and submission (Bagheera upload). On Android we'd have a different implementation, one which would aggregate, serialize, and not upload. Is that right? @@ +51,5 @@ > + return CommonUtils.getDatePref(this._prefs, "lastPingTime", 0, this, 2012); > + }, > + > + set lastPingDate(value) { > + CommonUtils.setDatePref(this._prefs,"lastPingTime", value, Unwrap, and space after comma. @@ +160,5 @@ > + for (let [name, provider] of this._collector.collectionResults) { > + o.providers[name] = provider; > + } > + > + return JSON.stringify(o); It feels a little wrong to me for this entity to be so intimately aware of the format of collection results, and their serialization. Where's the separation for us to introduce other submitters that don't call into JavaScript? I guess this ties into my overarching question above. I would rather see the policy handler chain together an aggregator (which is writing out a chunk of JSON on command) and an uploader (which is taking a 'recipe' -- a payload, a MIME type, and a destination -- and sending the content somewhere). You might be envisioning the upload as something so trivial that it doesn't need its own component, and so this is just the Firefox version of the aggregator+uploader combined, but that does imply that we're doing a bunch of redundant 'pull' work if the upload fails, no? @@ +167,5 @@ > + //----------------------------- > + // HealthReportPolicy listeners > + //----------------------------- > + > + onRequestDataSubmission: function onRequestDataSubmission(request) { This might be a little too expedient. Doesn't this conflate the creation of a snapshot document and uploading it? On desktop it's best to conflate the two for minor efficiency reasons. On mobile it's not -- the lifecycles of the component which can write the document (Fennec) and the component which can upload it (the background service) are not likely to overlap. On mobile we want to write out the document as soon as we can after our policy-determined period expires, and upload it whenever we have background data capability. I wouldn't ordinarily object, but this is really an issue with the policy. Is this API call lenient enough to support this separation?

(In reply to Richard Newman [:rnewman] from comment #126) Most of your comments stem from a concern that the implementation is overly simplified and not loosely coupled enough. I wholeheartedly agree. I think new modules and new coupling for the distinct steps will evolve as new features are added. For now, I want to land something that will serve the needs of today while not incurring /too/ much engineering debt. I /think/ by eliminating whole areas of separation/coupling (like persistence and serialization) while having robust solutions in place for the "distributed" problem of data representation/gathering, I have struck a good balance.

Regarding part 5: yes, absolutely. Consider those questions as questions, rather than blocking comments; so long as we have a good foundation of terminology, I'm happy for the capture and upload part to start simple and accrue abstraction as it goes. Hence the f+!

rnewman was complaining about an empty review queue. So... This patch incorporates some of the review feedback. I'm relatively happy with the API changes. I still need to update some tests. I figure rnewman can nit the API while I continue coding.

Comment on attachment 677224 [details] [diff] [review] Part 2: Metrics data providers, v4 Review of attachment 677224 [details] [diff] [review]: ----------------------------------------------------------------- ::: services/metrics/dataprovider.jsm @@ +29,5 @@ > + * This type is meant to be an abstract base type. Child types should define > + * a `fields` property which is a mapping of field names to metadata describing > + * that field. This field constitutes the "schema" of the measurement/type. > + * > + * Data is added to instances by calling `addField()`. Values are validated You use `setValue` in code, which I think is the right name. @@ +80,5 @@ > + * An unsigned integer field stored in 32 bits. > + * > + * This holds values from 0 to 2^32 - 1. > + */ > + TYPE_UINT32: 1, Wouldn't it be exciting if these were objects: TYPE_UINT32: { toString: function toString() { return "TYPE_UINT32"; }, validate: function validate(value) { if (!Number.isInteger(value)) { throw new Error("UINT32 field expects an integer. Got " + value); } … } }, and your setValue method could just be: setValue: function setValue(name, value) { if (!(name in this.fields)) { throw new Error("Attempting to set unknown field: " + name); } let fieldSpec = this.fields[name]; if (!fieldSpec) { throw new Error("Unhandled field type: " + field.type); } fieldSpec.validate(value); this.values.set(name, value); } ? That way you don't need to modify the definition of setValue every time you define a new type. @@ +110,5 @@ > + } > + > + let field = this.fields[name]; > + > + if (field.type == this.TYPE_UINT32) { For future reference: this is is a perfect opportunity to use a `switch`, but a great red flag that you're writing C code rather than using object-orientation. (At least when it's a set that isn't fixed!)

I am satisfied with the test coverage and I'm pretty sure I've hit all points of feedback.

Added more configurable options for Mock objects. Changed some minor naming.

I think I hit all points of feedback. Added more tests. Changed APIs slightly. I think I'm pretty happy with this.

Comment on attachment 676895 [details] [diff] [review] Part 4: MetricsManager This abstraction is silly at this time.

I think part 3 is a good cut-off point for this bug. All the code through part 3 is just generic metrics gathering base types. There is no code that obtains any data of useful value. There are no references to "health report" (aside from the build system skeleton in part 1 - this could be deferred to a new patch easily enough). So, proposed plan: land parts 1-3 as soon as they get review then clone this bug to track the Firefox Health Report patches.

+ TYPE_UINT32: { + validate: function validate(value) { + if (!Number.isInteger(value)) { + throw new Error("UINT32 field expects an integer. Got " + value); + } Don't think this check is complete, based on the name of the type (assuming it's a 32 bit unsigned integer?). It will return true for values such as 1e300, which is a double. Think the more complete version would be: + TYPE_UINT32: { + validate: function validate(value) { + if (!Number.isInteger(value) && value <= 0xFFFFFFFF) { + throw new Error("UINT32 field expects an integer. Got " + value); + }

Comment on attachment 677491 [details] [diff] [review] Part 2: Measurement, Provider, and Result types, v6 Review of attachment 677491 [details] [diff] [review]: ----------------------------------------------------------------- ::: services/metrics/dataprovider.jsm @@ +120,5 @@ > + * @param value > + * The value to set the field to. > + */ > + setValue: function setValue(name, value) { > + if (!(name in this.fields)) { This still leaves the possibility of a bad dereference in line 128. Maybe just (!this.fields[name])?

Typo in my previous. Should be `value < 0xFFFFFFFF`. Richard, wouldn't `(!this.fields[name])` fail if the return value is falsey (e.g. if a UINT32_TYPE is equal to 0).

(In reply to Trevor Norris from comment #138) > Richard, wouldn't `(!this.fields[name])` fail if the return value is falsey > (e.g. if a UINT32_TYPE is equal to 0). `fields` is a mapping of names to fieldspecs, not keys to values. Yes, it will fail if the return value is falsy, and that's exactly why I suggested the change: if an entry is present in `fields` with a non-truthy spec (e.g., someone defines a subclass with fields: {"foo": undefined}, then tries to add a "foo" value), then we would have an error in the subsequent line when we try to extract the "type" attribute from the fieldspec: + let type = this.fields[name].type; -- [13:27:50.017] this.fields["foo"].type [13:27:50.019] TypeError: this.fields.foo is undefined I'd rather make that error explicit.

gps: are you going to obsolete Part 5 and move it to another bug, or do you have another version forthcoming?

Comment on attachment 676896 [details] [diff] [review] Part 5: HealthReporter service, v1 This will have life in another bug.

Updating bug summary to reflect new nature of bug. FHR service will be tracked in a to-be-created bug.

https://hg.mozilla.org/projects/larch/rev/d6fad7552de4 https://hg.mozilla.org/projects/larch/rev/9768f36643b0 https://hg.mozilla.org/projects/larch/rev/be25a4cfa6b4

Comment on attachment 675921 [details] [diff] [review] Part 1: Create skeletons for services/{metrics,healthreport} Bulk-setting approval flags for FHR landing for FxOS ADU ping (Bug 788894).

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e58a9949ced https://hg.mozilla.org/integration/mozilla-inbound/rev/1a386fa321b2 https://hg.mozilla.org/integration/mozilla-inbound/rev/9abaed804094

https://hg.mozilla.org/mozilla-central/rev/1e58a9949ced https://hg.mozilla.org/mozilla-central/rev/1a386fa321b2 https://hg.mozilla.org/mozilla-central/rev/9abaed804094

Comment on attachment 675921 [details] [diff] [review] Part 1: Create skeletons for services/{metrics,healthreport} Approving as this work blocks B2G ADI pings, and is pref'd off & disabled in Desktop/Mobile.