We need to collect longitudinal data for FHR. There are essentially two classes of longitudinal data: * The value of something at a particular time (likely only the per-day value of something) * A history of the frequency of events (e.g. how many times X occurred on a particular day) This patch implements the latter. I intend to implement the former in this patch as well. I just started with daily counters just because. Currently this patch doesn't address saving data and applying new data on top of old saved data. We'll likely bubble that up to the responsibility of whoever instantiates a MetricsCollector instance (I'd prefer MetricsCollector to be in-memory data only). I'm mostly interested in API feedback at this juncture. Docs should be improved before this lands. I'm still working things out...

Comment on attachment 682596 [details] [diff] [review] Collect longitudinal data, v1 Review of attachment 682596 [details] [diff] [review]: ----------------------------------------------------------------- Call me crazy, but this doesn't do any kind of persistence, right? How do you plan to address that? ::: services/metrics/collector.jsm @@ +25,5 @@ > + addCount: function addCount(date) { > + let key = this._keyFromDate(date); > + > + if (this.days.has(key)) { > + this.days.set(key, this.days.get(key) + 1); This depends on nobody else writing into this.days. Either note that assumption, or guard here. @@ +32,5 @@ > + } > + }, > + > + _keyFromDate: function _keyFromDate(date) { > + return date.toISOString().substr(0, 10); The Long Now Foundation are preparing letters at this very moment. Needlessly pedantic, but I'd rather see this use `substring(0, s.indexOf("T"))`, or compose the string directly using `getUTC*`. @@ +195,5 @@ > return deferred.promise; > }, > + > + _onPushedMeasurements: function _onPushedMeasurements(provider, entry, m) { > + entry.pushedMeasurementedRegistered = true; s/ted/ts ::: services/metrics/dataprovider.jsm @@ +309,5 @@ > + > + /** > + * Push data corresponding to a counter increment. > + * > + * This should be called by the provider when implementation when it Rephrase.

Comment on attachment 682596 [details] [diff] [review] Collect longitudinal data, v1 Clearing feedback flag until I can see how this ties into persistence.

Here's a very rough initial draft of persistent storage of data. Most of the work so far has been on counters. I'm relatively happy with those APIs. I kinda stapled measurements on a few minutes ago. I think we can do better than JSON in a database (at least in some scenarios). The goal is for there to eventually be one "storage" instance that handles volatile per-app-instance measurements (constant measurements in provider land), counters, and periodically retrieved measurements. Whether constant measurements should be volatile or persisted with last write wins is a discussion we should have. I don't think it matters much on desktop. Android may have other considerations. We should also discuss how to store longitudinal data in the database. I need to go over all the probes again and see if I can come up with a decent db schema that can store everything (I hate storing blobs with structure in relational databases out of principle). I may or may not significantly alter things from the first patch in this series. Will require more thought and likely some proof-of-concept code before I am satisfied with APIs.

Comment on attachment 690708 [details] [diff] [review] Persistent storage of collected data, v1 I'll look at this when gps is done thinkin'.

More robust implementation. We still need 1 more form of storage to accommodate things like add-ons metadata. There are a few ways we could model this. I don't think I like any of them. For feedback, I'm mostly interested in your thoughts on the schema and general API design. Some areas to consider: * Should we preserve the division between {provider, measurement, field} or should we attempt to merge them? * Should we store the type of each field rather than resorting to dynamic behavior? * Should {provider, measurement, field} IDs be explicitly tracked or should I implement SQLite trigger magic to auto-insert upon missing? I'm assuming redundant storage of the string names in each row is a non-starter for perf reasons. Feel free to challenge. * ... (this patch is susceptible to plenty of *-shedding - DB schema design is hard) We can merge provider and measurement easily. That would certainly make the implementation a little simpler. The main identifier would thus be "measurement name" and would probably consist of the provider and original measurement name's concatenated. Fields on measurements are already strongly typed. It seems to make sense to extend that to the storage layer as well. At the same time, making the storage layer adaptable is nice (we won't have to write code for dealing with type changes).

I temporarily merged both patches to make it easier for me to work. At this point of implementation, if there are any large architectural changes, they could take a long time to refactor. Therefore, I'd like feedback ASAP. The docs could be better. I hope you can follow along. I /think/ things are more or less what we discussed on Monday. Call me if you have any questions.

Ignore the code about pushed measurements/data. It's left over from an intermediate refactor that didn't pan out.

Comment on attachment 694582 [details] [diff] [review] Persistent storage of collected data, v3 Review of attachment 694582 [details] [diff] [review]: ----------------------------------------------------------------- I have read this, but I don't claim to have fully wrapped it in my brain and squeezed out the juice — a large bottle of beer got to me first. I think this is a good direction. I'm happy for this to co-evolve with the first provider to use it, if that will help with its evolution. ::: services/metrics/Metrics.jsm @@ +14,5 @@ > +this.Metrics = { > + Provider: Provider, > + Measurement: Measurement, > + Storage: MetricsStorageBackend, > +}; Interesting pattern! ::: services/metrics/storage.jsm @@ +29,5 @@ > + > +/** > + * Represents a collection of per-day values. > + * > + * This is a proxy around a Map which can transparently round Date to their "Date instances" @@ +30,5 @@ > +/** > + * Represents a collection of per-day values. > + * > + * This is a proxy around a Map which can transparently round Date to their > + * appropriate key. Document that the iterator yields truncated timestamp Dates as the key. @@ +65,5 @@ > + > + if (this._days.has(key)) { > + this._days.get(key).push(value); > + } else { > + this._days.set(key, [value]); Just `return this._days.get…` rather than else. @@ +79,5 @@ > + * We use a SQLite database as the backend for persistent storage of metrics > + * data. > + * > + * Every piece of recorded data is associated with a measurement. A measurement > + * is an entity with a name and version associated with a provider. "version. Each measurement is associated with a provider." @@ +128,5 @@ > + * number of days since UNIX epoch, not Julian. > + */ > + > +/** > + * All of our SQL statements are stored in a central table so they can easily I don't think you mean 'table' here. @@ +136,5 @@ > + // Create the providers table. > + createProvidersTable: > + "CREATE TABLE providers (" + > + "id INTEGER PRIMARY KEY AUTOINCREMENT, " + > + "name TEXT" + I think you want `name` to be unique, right? @@ +176,5 @@ > + "name TEXT, " + > + "value_type INTEGER , " + > + "UNIQUE (measurement_id, name), " + > + "FOREIGN KEY (measurement_id) REFERENCES measurements(id) ON DELETE CASCADE " + > + "FOREIGN KEY (value_type) REFERENCES types(id) ON DELETE CASCADE " + I don't think you want type deletions to cascade. @@ +196,5 @@ > + "WHERE " + > + "fields.measurement_id = measurements.id " + > + "AND measurements.provider_id = providers.id " + > + "AND fields.value_type = types.id " + > + "", Lose that. @@ +691,5 @@ > + * > + * This returns a promise that resolves to the storage ID for this > + * measurement. > + * > + * If the measurement exists, this is a no-op. If not, then the measurement No-op: so what's the return value? @@ +817,5 @@ > + > + return deferred.promise; > + }, > + > + _init: function() { I think this would benefit from a high-level overview of the steps it takes. @@ +820,5 @@ > + > + _init: function() { > + let self = this; > + return Task.spawn(function initTask() { > + // TODO can schemas be altered in a transaction? As far as I know, yes. @@ +828,5 @@ > + if (schema == 0) { > + self._log.info("Creating database schema."); > + > + for (let k of self._SCHEMA_STATEMENTS) { > + yield self._connection.execute(SQL[k]); Careful. Do the lookup first. @@ +836,5 @@ > + } > + }); > + > + for (let k of self._NAMED_STATEMENTS) { > + self._connection.registerStatement(k, SQL[k]); Likewise. @@ +899,5 @@ > + * @param date > + * (Date) Old data threshold. > + */ > + pruneDataBefore: function (date) { > + let self = this; Just capture PRUNE_STATEMENTS instead. @@ +905,5 @@ > + let days = dateToDays(date); > + > + let params = {days: days}; > + for (let name of self._PRUNE_STATEMENTS) { > + yield conn.execute(SQL[name], params); I think we need a utility for safely executing named statements. I'm concerned that there's a bug opportunity between names and SQL[]. @@ +919,5 @@ > + */ > + _ensureFieldType: function (id, type) { > + let info = this._fieldsByID.get(id); > + > + if (!info) { Check for array-ness, too. @@ +930,5 @@ > + type); > + } > + }, > + > + queueOperation: function queueOperation (func) { `enqueueOperation`, please. Queue is ambiguously nouny. (Also, no need for name.) @@ +985,5 @@ > + //--------------------------------------------------------------------------- > + // Low-level storage operations > + // > + // These will be performed immediately (or at least as soon as the underlying > + // connection allows them to be. Close paren. @@ +1020,5 @@ > + this._ensureFieldType(id, this.FIELD_DAILY_COUNTER); > + > + let self = this; > + return Task.spawn(function fetchCounterDays() { > + let rows = yield self._connection.executeNamed("getDailyCounterCountsFromFieldID", Can we lift these query names into a `let` right under the function name? Don't want them to get disassociated. @@ +1026,5 @@ > + > + let result = new DailyValues(); > + for (let row of rows) { > + let days = row.getResultByIndex(0); > + let counter = row.getResultByIndex(1); Ah, the horror of constant query strings somewhere else. @@ +1112,5 @@ > + {field_id: field}); > + > + if (!rows.length) { > + throw new Task.Result(null); > + } OK, there's way too much boilerplate here. I will take any kind of ugliness to avoid 500 lines of this stuff. Let's have some parameterized functions: get rid of as much duplication as you can.

(In reply to Richard Newman [:rnewman] from comment #8) > @@ +176,5 @@ > > + "name TEXT, " + > > + "value_type INTEGER , " + > > + "UNIQUE (measurement_id, name), " + > > + "FOREIGN KEY (measurement_id) REFERENCES measurements(id) ON DELETE CASCADE " + > > + "FOREIGN KEY (value_type) REFERENCES types(id) ON DELETE CASCADE " + > > I don't think you want type deletions to cascade. Well, fields with unknown types would be an undefined state, no? And, there is no mechanism to delete types. So... > @@ +196,5 @@ > > + "WHERE " + > > + "fields.measurement_id = measurements.id " + > > + "AND measurements.provider_id = providers.id " + > > + "AND fields.value_type = types.id " + > > + "", > > Lose that. Huh? > @@ +820,5 @@ > > + > > + _init: function() { > > + let self = this; > > + return Task.spawn(function initTask() { > > + // TODO can schemas be altered in a transaction? > > As far as I know, yes. I searched the internets and SQLite (unlike the big RDMS I am familiar with) does seem to support rollback of DDLs in transactions! > @@ +828,5 @@ > > + if (schema == 0) { > > + self._log.info("Creating database schema."); > > + > > + for (let k of self._SCHEMA_STATEMENTS) { > > + yield self._connection.execute(SQL[k]); > > Careful. Do the lookup first. execute() performs argument validation. > @@ +1020,5 @@ > > + this._ensureFieldType(id, this.FIELD_DAILY_COUNTER); > > + > > + let self = this; > > + return Task.spawn(function fetchCounterDays() { > > + let rows = yield self._connection.executeNamed("getDailyCounterCountsFromFieldID", > > Can we lift these query names into a `let` right under the function name? > Don't want them to get disassociated. What are you suggesting? Breaking up SQL into multiple properties? Or, should we inline the SQL in the functions now that we switched the API to executeCached()? I'm a big fan of having a unified data structure of all the SQL. While the SQL and code that uses it are not together, I find it greatly improves understanding the database schema. That being said, meh, it's easy enough to change as we go.

(In reply to Gregory Szorc [:gps] from comment #9) > > I don't think you want type deletions to cascade. > > Well, fields with unknown types would be an undefined state, no? And, there > is no mechanism to delete types. So... I would rather see a foreign key constraint violation if you try to delete a type that's in use, rather than it quietly deleting rows elsewhere. > > > + "", > > > > Lose that. > > Huh? Unnecessary empty string that contains a close paren in some other functions. > > > + let self = this; > > > + return Task.spawn(function fetchCounterDays() { > > > + let rows = yield self._connection.executeNamed("getDailyCounterCountsFromFieldID", > > > > Can we lift these query names into a `let` right under the function name? > > Don't want them to get disassociated. > > What are you suggesting? Breaking up SQL into multiple properties? Or, > should we inline the SQL in the functions now that we switched the API to > executeCached()? I mean literally: getDailyCounterCountsFromFieldID: function (...) { let sqlName = "getDailyCounterCountsFromFieldID"; ... ... let rows = yield self._connection.executeNamed(sqlName, ...) If we can't achieve DRY, at least repeat yourself in the adjacent line.

I haven't performed my customary last pass through the code to look for obvious gotchas. But, time is short. I'm sure you'll find things during review. For parts where there is a delta, it's probably best to ignore the diff and just look at the new code, as the API broke in many ways.

Started working on rebasing FHR code on top of the new API. Patch is far from complete. But, the providers and tests have mostly been ported to the new APIs.

With this patch, all FHR unit tests pass again. Still needs a little work before review.

Comment on attachment 696829 [details] [diff] [review] Persistent storage of collected data, v4 Review of attachment 696829 [details] [diff] [review]: ----------------------------------------------------------------- ::: services/metrics/dataprovider.jsm @@ +165,4 @@ > */ > + serialize: function (data, format, day) { > + if (format == this.SERIALIZE_JSON) { > + return this._serializeJSON(data, day); Per discussion and prior admission (:D), please change this. We should be scarfing stuff off the disk grouped appropriately (measurement, date), then handing in field-value maps to serialize, gluing them together into the output however the caller wishes.

Code isn't perfect but seems to work. We lost the ability to report provider errors. I may or may not add this back by Monday.

Not sure where else to put this patch. If we had a shared location for generic testing-only JSM's, this module would go there. Someone remind me to create /testing/js-modules someday... Anyway, we now have an API to easily manipulate the current app's info. I'm asking for bsmedberg's feedback on the component registration foo. I /think/ I'm doing it right. Multiple registration does work, so that must count for something.

Comment on attachment 698094 [details] [diff] [review] Part 3: API for registering app info, v1 Review of attachment 698094 [details] [diff] [review]: ----------------------------------------------------------------- Looks fine, and I imagine any problems will be immediately obvious… ::: services/healthreport/modules-testing/utils.jsm @@ +39,5 @@ > +/** > + * Update the current application info. > + * > + * If the argument is defined, it will be the object used. Else, > + * APP_INFO (via getAppInfo) is used. Comment kinda lies — it does not call getAppInfo. Just kill that parenthetical. @@ +53,5 @@ > + let registrar = Components.manager.QueryInterface(Ci.nsIComponentRegistrar); > + > + // Unregister an existing factory if one exists. > + try { > + let existing = Components.manager.getClassObjectByContractID(cid, Ci.nsIFactory); Candidate for `Cm`, I think.

Comment on attachment 698071 [details] [diff] [review] Part 1: Persistent storage of collected data, v5 Review of attachment 698071 [details] [diff] [review]: ----------------------------------------------------------------- I'm pretty happy with this, at least as far as my brain can stretch around a patch this big! Top concerns: * Date rigor and tests. * Serialization, as we discussed the other day. * Handling of DB upgrades. * Indices for SQL tables (which I'd like to land with this to avoid upgrades). As requested, I've included some suggestions, but take them with a pinch of salt -- they're optimization without measuring, and indices add a cost to insertions and deletions, so I'm assuming that the joins and date-range queries for six months will cost more. There is no substitute for timing with real data. ::: services/metrics/dataprovider.jsm @@ +72,3 @@ > * > + * One role of a `Measurement` is to interface with the storage backend. The > + * `Measurement` registers strongly-typed named fields that will be associated “A compound modifier – two or more words that express a single concept – precedes a noun, use hyphens to link all the words in the compound except the adverb very and all adverbs that end in -ly” http://editingmonks.blogspot.com/2007/05/this-is-from-ap.html @@ +72,4 @@ > * > + * One role of a `Measurement` is to interface with the storage backend. The > + * `Measurement` registers strongly-typed named fields that will be associated > + * with each `Measurement` type. For example, there can be daily-aggregated “daily-aggregated” is fine, IMO; it's a compound. @@ +115,2 @@ > */ > + configureStorage: function () { This is a big one: your life will be much easier if this is versioned. configureStorage should be provided with the current version as last configured, and should return the version to which we've just configured the DB for this measurement. That way you have a place to hang upgrade code. @@ +290,5 @@ > */ > + _serializeJSON: function (data, day) { > + if (day) { > + return this._serializeJSONDay(data, day); > + } Don't do this dispatch here. serializeJSONDay doesn't even care about what the day is! All you're really doing is providing two *identical* serialization functions that include different sets of acceptable types. And those type filters won't catch real type errors (numeric instead of text, say), and don't signal an error if something really weird happens, and you get a daily counter passed into a non-daily measurement. So they're of no value. Either: * Have a *single* serializeJSON method which combines the two methods, and omit `day`, or * Have serializeJSONDay and serializeJSONSingular (and corresponding serializeDay/serializeSingular), and have the caller invoke the right one. It already knows whether it's handling daily or singular data. ::: services/metrics/storage.jsm @@ +25,5 @@ > +} > + > +function daysToDate(days) { > + return new Date(days * MILLISECONDS_PER_DAY); > +} These need tests. @@ +29,5 @@ > +} > + > +/** > + * Represents a collection of per-day values. > + * This is a good place to note that we work in UTC days. @@ +36,5 @@ > + * > + * This emulates Map by providing .size and iterator support. Note that keys > + * from the iterator are Date instances corresponding to midnight of the start > + * of the day. get(), has(), and set() are modeled as getDay(), hasDay(), and > + * setDay(), respectively. Without having looked at the tests yet: please make sure that you have tests covering leap seconds, leap years, year rollover, etc. I know that Date tracks milliseconds since epoch internally, but I think the onus is on your documentation to explain what happens when you div+floor across a leap boundary…. Also, note that it's "midnight UTC". @@ +144,5 @@ > +/** > + * All of our SQL statements are stored in a central mapping so they can easily > + * be audited for security, perf, etc. > + */ > +const SQL = { I would like this to be SQL.v1, and for you to use the DB schema version (see storage/test/unit/test_storage_connection.js) to track the overall version. I know this will be a little bit of extra work, but schema migration code is not something you can bolt on after you ship. @@ +223,5 @@ > + "AND fields.value_type = types.id", > + > + createDailyCountersTable: > + "CREATE TABLE daily_counters (" + > + "field_id INTEGER, " + You will *probably* benefit from an index on field_id for each of these tables. @@ +224,5 @@ > + > + createDailyCountersTable: > + "CREATE TABLE daily_counters (" + > + "field_id INTEGER, " + > + "day INTEGER, " + And in this case, definitely an index on `day`. @@ +283,5 @@ > + "FROM providers, measurements, fields, daily_discrete_numeric " + > + "WHERE " + > + "daily_discrete_numeric.field_id = fields.id " + > + "AND fields.measurement_id = measurements.id " + > + "AND measurements.provider_id = providers.id " + Indices for these three. @@ +339,5 @@ > + "FROM providers, measurements, fields, last_numeric " + > + "WHERE " + > + "last_numeric.field_id = fields.id " + > + "AND fields.measurement_id = measurements.id " + > + "AND measurements.provider_id = providers.id " + Wherever you see this kind of thing, you're going to want to establish some indices for the joined values that aren't already primary keys. I suggest setting up data for 180 days, then timing your queries. @@ +387,5 @@ > + "measurements.version AS measurement_version, " + > + "fields.id AS field_id, " + > + "fields.name AS field_name, " + > + "daily_last_numeric.day AS day, " + > + "dailY_last_numeric.value AS value, " + Accidentally capitalized 'Y'. @@ +474,5 @@ > + pruneOldDailyDiscreteText: "DELETE FROM daily_discrete_text WHERE day < :days", > + pruneOldDailyLastNumeric: "DELETE FROM daily_last_numeric WHERE day < :days", > + pruneOldDailyLastText: "DELETE FROM daily_last_text WHERE day < :days", > + pruneOldLastNumeric: "DELETE FROM last_numeric WHERE day < :days", > + pruneOldLastText: "DELETE FROM last_text WHERE day < :days", Presumably these are all infrequent enough and on small enough tables that an index is not worthwhile. @@ +634,5 @@ > + * > + * Instances of this should be obtained by calling MetricsStorageConnection(). > + * > + * The current implementation will not work if the database is mutated by > + * multiple connections because of the way we cache primary keys. Is there an option to openConnection which will error in this state? @@ +1206,5 @@ > + * singular -- Map of field names to values. This holds all fields that > + * don't have a temporal component. > + * > + * This returns a promise that resolves to a Map. The keys in the map will > + * be field names. Values are different depending on the type of field. The This comment is confusing, because it has two mutually exclusive paragraphs that begin "This returns a promise that...". Please fix! @@ +1242,5 @@ > + handleResult(dailyLast); > + handleResult(dailyDiscrete); > + > + for (let [field, value] of last) { > + result.singular.set(field, value); Let's save the N*M `result.days`/`result.singular` lookups. Keep `days` and `singular` in locals, build `result` *at the end*. @@ +1261,5 @@ > + // > + // These will be performed immediately (or at least as soon as the underlying > + // connection allows them to be.) It is recommended to call these from within > + // a function added via `enqueueOperation()`. > + // --------------------------------------------------------------------------- Or else? ::: services/metrics/tests/xpcshell/test_metrics_provider.js @@ +237,5 @@ > + yield m.setLastTextValue("last-text", "hello", now); > + > + let data = yield provider.storage.getMeasurementValues(m.id); > + > + let formatted = m.serialize(data.singular, m.SERIALIZE_JSON, null); See how we're already aware of whether this is a day or non-day measurement?

(In reply to Richard Newman [:rnewman] from comment #19) > @@ +115,2 @@ > > */ > > + configureStorage: function () { > > This is a big one: your life will be much easier if this is versioned. > configureStorage should be provided with the current version as last > configured, and should return the version to which we've just configured the > DB for this measurement. > > That way you have a place to hang upgrade code. Except that measurements aren't currently upgradeable. This is a known (and now commented) deficiency in the current API. The workaround is to create a new `Measurement` type for the new version. The old version is read only (provider doesn't populate any new data). That way the FHR payload contains the old and new data. Application downgrades will still work (new data gets added to old measurement on old code). I think this is satisfactory for an initial release. > @@ +290,5 @@ > > */ > > + _serializeJSON: function (data, day) { > > + if (day) { > > + return this._serializeJSONDay(data, day); > > + } > > Don't do this dispatch here. serializeJSONDay doesn't even care about what > the day is! > > All you're really doing is providing two *identical* serialization functions > that include different sets of acceptable types. > > And those type filters won't catch real type errors (numeric instead of > text, say), and don't signal an error if something really weird happens, and > you get a daily counter passed into a non-daily measurement. So they're of > no value. Ugh. I didn't use my brain when I changed the API semantics to only pass in relevant data. I will do something sane. > > @@ +144,5 @@ > > +/** > > + * All of our SQL statements are stored in a central mapping so they can easily > > + * be audited for security, perf, etc. > > + */ > > +const SQL = { > > I would like this to be SQL.v1, and for you to use the DB schema version > (see storage/test/unit/test_storage_connection.js) to track the overall > version. I think we can change this later. We also kinda, sorta have schema migration baked in. We don't have a terrific story for the downgrade case (we just throw - which will prevent FHR from initializing). I /think/ this is acceptable for the initial release. We can always do something more robust later, no? > @@ +223,5 @@ > > + "AND fields.value_type = types.id", > > + > > + createDailyCountersTable: > > + "CREATE TABLE daily_counters (" + > > + "field_id INTEGER, " + > > You will *probably* benefit from an index on field_id for each of these > tables. I concur. > > @@ +224,5 @@ > > + > > + createDailyCountersTable: > > + "CREATE TABLE daily_counters (" + > > + "field_id INTEGER, " + > > + "day INTEGER, " + > > And in this case, definitely an index on `day`. There is already a UNIQUE(field_id, day) constraint on this table. UNIQUE table constraints result in indexes. I don't believe we ever perform any operations on this table that don't use both {field_id, day}. > > @@ +283,5 @@ > > + "FROM providers, measurements, fields, daily_discrete_numeric " + > > + "WHERE " + > > + "daily_discrete_numeric.field_id = fields.id " + > > + "AND fields.measurement_id = measurements.id " + > > + "AND measurements.provider_id = providers.id " + > > Indices for these three. I /think/ a foreign key constraint sets up some kind of index if the foreign key is indexed in the foreign table. We only SELECT from these views when creating the payload. I'm not too concerned about perf for data retrieval. If we need an index, I think we can add one in a later release. Or, if I find time to run EXPLAIN before Monday... > @@ +474,5 @@ > > + pruneOldDailyDiscreteText: "DELETE FROM daily_discrete_text WHERE day < :days", > > + pruneOldDailyLastNumeric: "DELETE FROM daily_last_numeric WHERE day < :days", > > + pruneOldDailyLastText: "DELETE FROM daily_last_text WHERE day < :days", > > + pruneOldLastNumeric: "DELETE FROM last_numeric WHERE day < :days", > > + pruneOldLastText: "DELETE FROM last_text WHERE day < :days", > > Presumably these are all infrequent enough and on small enough tables that > an index is not worthwhile. Correct. These run once per day. > @@ +634,5 @@ > > + * > > + * Instances of this should be obtained by calling MetricsStorageConnection(). > > + * > > + * The current implementation will not work if the database is mutated by > > + * multiple connections because of the way we cache primary keys. > > Is there an option to openConnection which will error in this state? No. That would be a potential feature for SQLite.openConnection. For now, I suppose we could create a temporary table or insert some other semaphore in the DB. I think both ways are followups.

Incorporated review feedback. I also made some minor API changes and optimizations. The biggest optimization is getMeasurementValues() is now intelligent enough to only issue queries against tables where we have fields in them. This significantly reduces the number of queries at health report generation time.

Landed part 3 as part 0. https://hg.mozilla.org/services/services-central/rev/4ac2babbafec

(In reply to Gregory Szorc [:gps] from comment #20) > Application downgrades will still work (new data gets > added to old measurement on old code). > > I think this is satisfactory for an initial release. Yeah, works for me. > I think we can change this later. Let's at least do the rudiments of DB migration now, where "rudiments" is broadly equivalent to "pick future-proof names and think about it". > We also kinda, sorta have schema migration > baked in. We don't have a terrific story for the downgrade case (we just > throw - which will prevent FHR from initializing). I /think/ this is > acceptable for the initial release. We can always do something more robust > later, no? Maybe. Remember that there's a "downgrade horizon" -- you can't downgrade back to the code before you landed version-awareness. We can only ship code that requires a schema upgrade once we're satisfied with the market saturation of downgrade code. Given that version-awareness will probably only get shipped when we need to do an upgrade (because lazy), we'll be screwing some percentage of our users (market saturation = 0). This is why I encourage you to ship version-handling code if you can manage it, or at the very least leave enough markers in the data that you can recognize "version 0" without a pile of hacks. > There is already a UNIQUE(field_id, day) constraint on this table. UNIQUE > table constraints result in indexes. For future reference: compound indices are only useful in isolation for the first value, not the second. That is, that index is useful for field_id, but not for day. > I don't believe we ever perform any > operations on this table that don't use both {field_id, day}. Yeah, fair enough. (But perhaps we should? Would reduce the number of queries run…) > > Indices for these three. > > I /think/ a foreign key constraint sets up some kind of index if the foreign > key is indexed in the foreign table. The parent key *must* be covered by a unique index. But the child benefits: "Indices are not required for child key columns but they are almost always beneficial." "So, in most real systems, an index should be created on the child key columns of each foreign key constraint. The child key index does not have to be (and usually will not be) a UNIQUE index." http://www.sqlite.org/draft/foreignkeys.html#fk_indexes > We only SELECT from these views when creating the payload. I'm not too > concerned about perf for data retrieval. SQLite also selects from these on deletion from any table referenced through a foreign key constraint. Not a big deal in this case, because our set of providers/fields/etc. is mostly static. I'll only be unconcerned about perf for data retrieval if you can show me that we're not doing a large number of table walks over six months' worth of data! Remember that we have plenty of users who gripe that Sync hangs their browser for *ten seconds* to sync bookmarks. This DB will eventually be bigger than their places DB, and sod's law says that we'll be building a payload a minute after they start reading their morning news…. > If we need an index, I think we can > add one in a later release. Or, if I find time to run EXPLAIN before > Monday... Heh. That works for me. > > Is there an option to openConnection which will error in this state? > > No. That would be a potential feature for SQLite.openConnection. For now, I > suppose we could create a temporary table or insert some other semaphore in > the DB. I think both ways are followups. Yup.

Integrated new XPCOM service code. Moved some settings to prefs (yay easier hacking). Also heavily documented the startup and shutdown behavior. I'd like to write some mochitests or possibly marionette tests for init/shutdown behavior. Otherwise, we're relying on manual testing for XPCOM service behavior. That scares me.

Comment on attachment 698348 [details] [diff] [review] Part 1: Persistent storage of collected data, v6 Review of attachment 698348 [details] [diff] [review]: ----------------------------------------------------------------- Next version should be a rubberstamp. ::: services/metrics/Metrics.jsm @@ +18,5 @@ > + Measurement: Measurement, > + Provider: Provider, > + Storage: MetricsStorageBackend, > + dateToDays: dateToDays, > + daysToDate: daysToDate, Note that you don't need to export these in order to test them: you can use the backstage pass to do so. ::: services/metrics/dataprovider.jsm @@ +122,5 @@ > + * @param data > + * (varies) All data from this measurement from storage. > + * @param dataType > + * (string) The type of data being passed. One of {"daily", > + * "singular"} You're building a string-driven interpreter, here. Let's make the steps explicit (and also save on work when we're serializing 100 daily samples). let serializer = measurement.serializer(format); serializer.daily(data) serializer.singular(data) So: _serializers: {}, … // Or a prototype-based equivalent; whatever. _serializers[this.SERIALIZE_JSON] = { "daily": this._serializeJSONDay, "singular": this._serializeJSONSingular, }; … serializer: function (format) { if (!(format in this._serializers)) { throw new Error("Don't know…"); } return this._serializers[format]; },

Comment on attachment 698365 [details] [diff] [review] Part 2: Rebase FHR, v5 Review of attachment 698365 [details] [diff] [review]: ----------------------------------------------------------------- Next version should be a rubberstamp. Want to see the rebase on top of recommended changes. ::: services/healthreport/HealthReportService.js @@ +15,2 @@ > const BRANCH = "healthreport."; > +const DEFAULT_LOAD_DELAY_INTERVAL = 10 * 1000; // Milliseconds. Just name it DEFAULT_LOAD_DELAY_INTERVAL_MSEC. @@ +62,5 @@ > + * until the reporter completes shutdown. We need to spin the event loop > + * because if we fail to block shutdown, there is a race condition between > + * reporter shutdown and XPCOM shutdown. If XPCOM shutdown occurs before > + * the reporter has completed shutdown, Storage may assert due to a) attempting > + * to perform a new query after shutdown b) a connection not being closed. Excellent. @@ +101,5 @@ > os.removeObserver(this, "final-ui-startup"); > os.addObserver(this, "quit-application", true); > + os.addObserver(this, "profile-before-change", true); > + > + let delayInterval = this._prefs.get("service.loadDelayInterval") Similarly, this might be more informative as "service.loadDelayIntervalMsec", or "service.loadDelayMsec". @@ +102,5 @@ > os.addObserver(this, "quit-application", true); > + os.addObserver(this, "profile-before-change", true); > + > + let delayInterval = this._prefs.get("service.loadDelayInterval") > + || DEFAULT_LOAD_DELAY_INTERVAL; || at end of line, align. ::: services/healthreport/healthreport-prefs.js @@ +5,5 @@ > pref("healthreport.documentServerURI", "https://data.mozilla.com/"); > pref("healthreport.documentServerNamespace", "metrics"); > +pref("healthreport.service.loadDelayInterval", 10000); > +pref("healthreport.service.enabled", true); > +pref("healthreport.service.providerCategories", "healthreport-js-provider"); Move these below .policy, and sort them by name. ::: services/healthreport/healthreporter.jsm @@ +371,3 @@ > let o = { > version: 1, > + thisPingDate: this._formatDate(now), Capture the date outside this, and log that we're producing a payload for that date. @@ +394,5 @@ > + data = yield this._storage.getMeasurementValues(measurement.id); > + } catch (ex) { > + this._log.warn("Error obtaining data for measurement: " + > + name + ": " + CommonUtils.exceptionStr(ex)); > + // TODO record in payload. File a bug or do it. @@ +418,5 @@ > + > + try { > + let serialized = measurement.serialize(data.days.getDay(date), > + "daily", > + measurement.SERIALIZE_JSON); This'll be lovely with my suggested change. let serializer; try { serializer = measurement.serializer(measurement.SERIALIZE_JSON); } catch (ex) { this._log.error("Hey look, we can explicitly catch this!"); … } … if (data.singular.size) { … let serialized = serializer.singular(data.singular); … } … let days = data.days; for (…) { try { let serialized = serializer.daily(days.getDay(date)); … See what I mean about it not having to do string-based interpreting driven by "daily" vs "singular"? And we don't have to do that serializer format lookup every time… or runtime function lookup… ::: services/healthreport/profile.jsm @@ +10,5 @@ > ]; > > const {utils: Cu, classes: Cc, interfaces: Ci} = Components; > > +const DEFAULT_PROFILE_MEASUREMENT_NAME = "age"; If you're concerned about compartment overhead, feel free to merge this file into providers.jsm. It was only separate so I could work on it in parallel… ::: services/healthreport/providers.jsm @@ +68,5 @@ > + configureStorage: function () { > + let self = this; > + return Task.spawn(function configureStorage() { > + for (let field of self.LAST_TEXT_FIELDS) { > + yield self.registerStorageField(field, self.storage.FIELD_LAST_TEXT); Man, are we ever going hard on Task.spawn. "Hey Greg, wanna spawn a tea task, then a code review task?" ::: services/healthreport/tests/xpcshell/test_healthreporter.js @@ +161,4 @@ > > + let now = new Date(); > + let m = provider.getMeasurement("DummyMeasurement", 1); > + for (let i = 0; i < 365; i++) { https://www.google.com/search?q=how+many+days+in+a+leap+year @@ +208,5 @@ > let request = new DataSubmissionRequest(deferred, new Date()); > reporter.onRequestDataUpload(request); > + yield deferred.promise; > + do_check_eq(request.state, request.SUBMISSION_SUCCESS); > + do_check_neq(reporter.lastPingDate.getTime(), 0); Greater than zero, plz.

Reimplemented service startup and shutdown logic. Again. Now part of HealthReporter. Seems to work. Haven't looked at most recent review comments yet.

Added indexes everywhere. FWIW, I did notice some of the unit tests now take maybe 25% longer to execute. But, the unit tests are doing a *lot* of rapid inserts (like a few hundred per second). I'm not too worried. I also incorporated the serializer change.

Incorporated all your review feedback (I think). Added an "idle-daily" observer to perform pruning of old data.

I appear to have broken test_profile.js. https://tbpl.mozilla.org/?tree=Try&rev=868deba6a819 I remember doing some weird things to this code, particularly the tests. I suspect I'm using the wrong promise somewhere. Doh.

Comment on attachment 698396 [details] [diff] [review] Part 2: Rebase FHR, v7 Review of attachment 698396 [details] [diff] [review]: ----------------------------------------------------------------- 'cept for whatever's wrong with test_profile, which we'll figure out out-of-band :) ::: services/healthreport/HealthReportService.js @@ +13,2 @@ > const BRANCH = "healthreport."; > +const DEFAULT_LOAD_DELAY_MSEC = 10 * 1000; // Milliseconds. Kill redundant comment. ::: services/healthreport/healthreporter.jsm @@ +97,5 @@ > if (!this.serverNamespace) { > throw new Error("No server namespace defined. Did you forget a pref?"); > } > + > + this._dbName = this._prefs.get("dbName") || "healthreport.sqlite"; DEFAULT_DATABASE_NAME, plz. @@ +645,5 @@ > + } > + > + for (let i = 0; i < DAYS_IN_PAYLOAD; i++) { > + let date = new Date(now.getTime() - i * MILLISECONDS_PER_DAY); > + if (!data.days.hasDay(date)) { Lift data.days into a local outside the loop.

https://hg.mozilla.org/services/services-central/rev/f3f08413e863 https://hg.mozilla.org/services/services-central/rev/ee9453c65e33 We'll use s-c as our try server :)

At some point it might be worth abstracting the concept of set up and tear down functions and use a stack or something. Follow-up.

https://hg.mozilla.org/services/services-central/rev/ba2d808f5634 Landed a tweak to profile age fetching per Yoric's suggestion. Let's see if this passes on Linux.

I wrote tests for shutdown! I uncovered a legitimate problem: Services.obs.removeObserver(this, "idle-daily") was throwing because apparently it throws if you try to remove an observer that doesn't exist.

dataprovider.jsm said init() was called serially. Providers were implemented with this assumption. collector.jsm didn't get the memo. In the ideal world we wouldn't need this. We should take a long hard look at the operating queueing model and decide if we want to perform a logic refactor to facilitate parallel initialization.

Comment on attachment 698533 [details] [diff] [review] Part 4: Don't parallelize provider initialization, v1 Review of attachment 698533 [details] [diff] [review]: ----------------------------------------------------------------- I don't see a purpose in parallel initialization, unless we can jam everything into a single DB call and improve efficiency. Serial takes longer wall-clock, but who cares if it takes longer? We want to minimize user impact, not get FHR up and running as fast as possible.

https://hg.mozilla.org/mozilla-central/rev/4ac2babbafec https://hg.mozilla.org/mozilla-central/rev/f3f08413e863 https://hg.mozilla.org/mozilla-central/rev/ee9453c65e33 https://hg.mozilla.org/mozilla-central/rev/66786ce0c5fb https://hg.mozilla.org/mozilla-central/rev/ba2d808f5634 https://hg.mozilla.org/mozilla-central/rev/e26105667116 https://hg.mozilla.org/mozilla-central/rev/d3c914f579dd

Unfortunately this was perma-orange on Linux32 debug xpcshell - due to timeouts during test_metrics_storage.js, but was presumably missed due to other xpcshell bustage at the time. m-c tip is now orange due to this - I can either just disable the test on Linux32, or else back out this bug (and no doubt a bunch of others from the latest services-central merge). You're asleep now, but hoping someone CCed might not be & can weigh in. If I don't hear anything shortly, I'll likely just go with the test disable for now.

(In reply to Ed Morley [:edmorley UTC+0] from comment #43) > Unfortunately this was perma-orange on Linux32 debug xpcshell - due to > timeouts during test_metrics_storage.js Disabled the test for now: https://hg.mozilla.org/mozilla-central/rev/b4fec0759b12

Hmm also seems broken on Windows PGO (those runs have only just completed) :-(

(In reply to Ed Morley [:edmorley UTC+0] from comment #45) > Hmm also seems broken on Windows PGO (those runs have only just completed) > :-( Disabled on Windows (don't believe we have a way to check for PGO): https://hg.mozilla.org/mozilla-central/rev/8f8ceea47a26

Log of failure on Windows: https://tbpl.mozilla.org/php/getParsedLog.php?id=18551328&tree=Firefox

To complete the set, also Linux32 pgo: https://tbpl.mozilla.org/php/getParsedLog.php?id=18551838&tree=Mozilla-Inbound To avoid spending all day playing whac-a-mole, disabled the test entirely: https://hg.mozilla.org/mozilla-central/rev/776346e8fcda

I take blame for not noticing this on s-c. So weird to see only failures on Linux32 debug + apparently PGO. I don't think I've ever managed that from an xpcshell test before! Anyway, disabling the test was the correct thing to do. This code is enabled in any shipping feature (yet), so the test can safely be disabled.

If there were a wall of shame, we both deserve to be on it for missing this.

qref strikes again.

Test was disabled after REOPEN. So, marking back resolved. Will file follow-up to deal with part 5 and uplift request.

Comment on attachment 698094 [details] [diff] [review] Part 3: API for registering app info, v1 Why are you bothering to unregister the old factory? I can't think of any reason you'd need to do that. Just map the contract ID to your new CID. It's a giant hack anyway, so I'm not sure I care much.

(In reply to Benjamin Smedberg [:bsmedberg] from comment #53) > Why are you bothering to unregister the old factory? I can't think of any > reason you'd need to do that. Just map the contract ID to your new CID. My XPCOM skills are weak. How do you do this?

Remove the call to registrar.unregisterFactory(id, existing). The call to registerFactory should work just by itself, assuming you made up a new CID and didn't reuse one.