Measurements should statically declare fields

RESOLVED FIXED in Firefox 21

Status

Firefox Health Report
Client: Desktop
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: gps, Assigned: gps)

Tracking

unspecified
Firefox 22
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox20 unaffected, firefox21 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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?
Isn't that how we started? :D

Yeah, broadly concur, if all of our methods simply declare fields.
(Assignee)

Comment 2

5 years ago
Created attachment 715784 [details] [diff] [review]
Statically declare fields, v1

I think this should do it!
Assignee: nobody → gps
Status: NEW → ASSIGNED
Attachment #715784 - Flags: review?(rnewman)
(Assignee)

Comment 3

5 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."
(Assignee)

Updated

5 years ago
Blocks: 830492
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

5 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

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/5054f997ef77
Target Milestone: --- → mozilla22
https://hg.mozilla.org/mozilla-central/rev/5054f997ef77
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED

Updated

5 years ago
Blocks: 846082
(Assignee)

Comment 8

5 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 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

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/530ef18baee6
status-firefox20: --- → unaffected
status-firefox21: --- → fixed

Updated

5 years ago
Component: Metrics and Firefox Health Report → Client: Desktop
Flags: in-testsuite+
Product: Mozilla Services → Firefox Health Report
Target Milestone: mozilla22 → Firefox 22
You need to log in before you can comment on or make changes to this bug.