Closed
Bug 841074
Opened 12 years ago
Closed 12 years ago
Measurements should statically declare fields
Categories
(Firefox Health Report Graveyard :: Client: Desktop, defect)
Firefox Health Report Graveyard
Client: Desktop
Tracking
(firefox20 unaffected, firefox21 fixed)
RESOLVED
FIXED
Firefox 22
Tracking | Status | |
---|---|---|
firefox20 | --- | unaffected |
firefox21 | --- | fixed |
People
(Reporter: gps, Assigned: gps)
References
Details
Attachments
(1 file)
(deleted),
patch
|
rnewman
:
review+
bajaj
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
Currently the Measurement API has a configureStorage() that the measurement implementation implements to call into storage (typically registerStorageField()). I think it makes more sense for measurements to statically define their fields as part of their prototype. e.g.
MyMeasurement.prototype = {
FIELDS: {
"foo": Measurement.FIELD_DAILY_COUNTER,
"bar": Measurement.FIELD_LAST_TEXT,
}
}
This more loosely couples the storage backend from the measurement implementation. So, if we want to do crazy things like e.g. swap out storage backends or have transient backends that store data in prefs or IndexedDB until SQLite comes online and/or flushes, we could do that easier.
rnewman: What are your thoughts?
Comment 1•12 years ago
|
||
Isn't that how we started? :D
Yeah, broadly concur, if all of our methods simply declare fields.
Assignee | ||
Comment 2•12 years ago
|
||
I think this should do it!
Assignee | ||
Comment 3•12 years ago
|
||
It's worth noting that I made the field descriptors objects so the door is open for future additions. Notably, I'm thinking of things like a flushing policy (e.g. wait at most 60s before flushing to storage) and "allow preference storage."
Comment 4•12 years ago
|
||
Comment on attachment 715784 [details] [diff] [review]
Statically declare fields, v1
Review of attachment 715784 [details] [diff] [review]:
-----------------------------------------------------------------
I think this is fine. If tests pass...
::: services/metrics/dataprovider.jsm
@@ +32,5 @@
> *
> * This type provides the primary interface for storing, retrieving, and
> * serializing data.
> *
> + * Each measurements consists of a set of named fields. Each field is primarily
s/ments/ment/
@@ +171,5 @@
> return entry[0];
> },
>
> fieldType: function (name) {
> + let entry = this._fields[name];
This will throw until configureStorage has been called. That's a change in behavior. Perhaps initialize _fields to {}, or throw about configureStorage not having been run?
@@ +368,5 @@
> _serializeJSONDay: function (data) {
> let result = {"_v": this.version};
>
> for (let [field, data] of data) {
> + if (!(field in this._fields)) {
Shouldn't happen, but this will quietly skip over fields if we haven't yet been configured…
Attachment #715784 -
Flags: review?(rnewman) → review+
Assignee | ||
Comment 5•12 years ago
|
||
(In reply to Richard Newman [:rnewman] from comment #4)
> @@ +368,5 @@
> > _serializeJSONDay: function (data) {
> > let result = {"_v": this.version};
> >
> > for (let [field, data] of data) {
> > + if (!(field in this._fields)) {
>
> Shouldn't happen, but this will quietly skip over fields if we haven't yet
> been configured…
So it will. My intention with this patch is to pave the road for a slightly different storage world where SQLite isn't always available and/or we perform periodic flushing and flushing on shutdown. Until then, I'm OK with the code being in a slightly weird state.
Assignee | ||
Comment 6•12 years ago
|
||
Target Milestone: --- → mozilla22
Comment 7•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Assignee | ||
Comment 8•12 years ago
|
||
Comment on attachment 715784 [details] [diff] [review]
Statically declare fields, v1
[Approval Request Comment]
Bug caused by (feature/regressing bug #): None
User impact if declined: None
Testing completed (on m-c, etc.): It's baked for a while.
Risk to taking this patch (and alternatives if risky): Low risk.
String or UUID changes made by this patch: None
I am requesting uplift of a number of FHR patches to Aurora. This patch, while not technically required, will cause significant bit rot on future patches that will be uplifted (specifically those involved with bug 849947). While we could work around this bit rot, we would essentially be shipping a new, untested configuration on Aurora/21. I'm confident this would work. But, I'd feel much better if we had an as-similar code configuration between 21 and 22 as possible.
Attachment #715784 -
Flags: approval-mozilla-aurora?
Comment 9•12 years ago
|
||
Comment on attachment 715784 [details] [diff] [review]
Statically declare fields, v1
approving the low risk patch for uplift to avoid suffering from bitrot later
Attachment #715784 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 10•12 years ago
|
||
status-firefox20:
--- → unaffected
status-firefox21:
--- → fixed
Updated•12 years ago
|
Component: Metrics and Firefox Health Report → Client: Desktop
Flags: in-testsuite+
Product: Mozilla Services → Firefox Health Report
Target Milestone: mozilla22 → Firefox 22
Updated•6 years ago
|
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•