841074 - Measurements should statically declare fields

Status: RESOLVED FIXED

(Firefox Health Report Graveyard :: Client: Desktop, defect)

Description

[Gregory Szorc [:gps]]

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

[Richard Newman [:rnewman]]

Isn't that how we started? :D

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

Comment 2

[Gregory Szorc [:gps]]

Attachment: Statically declare fields, v1

I think this should do it!

Comment 3

[Gregory Szorc [:gps]]

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

[Richard Newman [:rnewman]]

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…

Comment 5

[Gregory Szorc [:gps]]

(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.

Comment 6

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5054f997ef77

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5054f997ef77

Comment 8

[Gregory Szorc [:gps]]

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.

Comment 9

[bhavana bajaj [:bajaj]]

Comment on attachment 715784 [details] [diff] [review]
Statically declare fields, v1

approving the low risk patch for uplift to avoid suffering from bitrot later

Comment 10

[Gregory Szorc [:gps]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/530ef18baee6