878303 - Implement and use DISCRETE_COUNTED type

Status: RESOLVED FIXED

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

Description

[Richard Newman [:rnewman]]

We currently record searches as discrete strings within named fields:

  "bartext": [
    "google", "google", "amazon"
  ]

I intended to roll these up into counts:

  "bartext": {
    "google": 2,
    "amazon": 1
  }

This bug will do exactly that. And I might morph it to include any data type I need for sessions, too.

Comment 1

[Richard Newman [:rnewman]]

https://github.com/mozilla-services/android-sync/pull/314

Comment 2

[Richard Newman [:rnewman]]

Attachment: Part 1 (from Git) v1

Comment 3

[Richard Newman [:rnewman]]

Attachment: Part 2. v1

Comment 4

[Richard Newman [:rnewman]]

Nick and I discussed that we likely also want to implement DAILY_LAST_DISCRETE, which operates as a combination of DAILY_LAST (one value per day, updated if a new one arrives) and DISCRETE (multiple values per day) -- like a DAILY_LAST with dynamic field names.

This would allow a provider to record multiple values in a day, updating individual values if they change -- for example, to record that a session completed.

Comment 5

[Nick Alexander :nalexander [he/him]]

Comment on attachment 756836 [details] [diff] [review]
Part 2. v1

Review of attachment 756836 [details] [diff] [review]:
-----------------------------------------------------------------

wfm.

::: mobile/android/base/health/BrowserHealthRecorder.java
@@ +583,5 @@
>                          // We're not using a counter, because the set of engine
>                          // identifiers is potentially unbounded, and thus our
>                          // measurement version would have to keep growing as
> +                        // fields changed. Instead we store discrete values, and
> +                        // accrete them into a counting map during processing.

Again, I feel like this should be "accumulate".  But you've been consistent with "accrete" which suggests a reason.  Enlighten me.

Comment 6

[Nick Alexander :nalexander [he/him]]

Comment on attachment 756835 [details] [diff] [review]
Part 1 (from Git) v1

Review of attachment 756835 [details] [diff] [review]:
-----------------------------------------------------------------

Reviewed on github at https://github.com/mozilla-services/android-sync/pull/314.

Comment 7

[Richard Newman [:rnewman]]

(In reply to Nick Alexander :nalexander from comment #5)

> Again, I feel like this should be "accumulate".  But you've been consistent
> with "accrete" which suggests a reason.  Enlighten me.

I just like words! In the util lib I used "accrue", and here I used "accrete". Changed to be more boring ;)

Comment 8

[Richard Newman [:rnewman]]

   https://hg.mozilla.org/services/services-central/rev/a31f790f0681
   https://hg.mozilla.org/services/services-central/rev/5ccd0592e074

Will land in inbound when the tree reopens.

Comment 9

[Richard Newman [:rnewman]]

   https://hg.mozilla.org/integration/mozilla-inbound/rev/88b09f11a45a
   https://hg.mozilla.org/integration/mozilla-inbound/rev/c6e087e836f9

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/88b09f11a45a
https://hg.mozilla.org/mozilla-central/rev/c6e087e836f9

Comment 11

[Richard Newman [:rnewman]]

https://hg.mozilla.org/mozilla-central/rev/a31f790f0681
https://hg.mozilla.org/mozilla-central/rev/5ccd0592e074

Comment 12

[Richard Newman [:rnewman]]

Comment on attachment 756835 [details] [diff] [review]
Part 1 (from Git) v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
  New work.

User impact if declined: 
  Incorrect search reporting => no FHR.

Testing completed (on m-c, etc.): 
  Baking for a while on m-c. Extensive manual testing.

Risk to taking this patch (and alternatives if risky):
  Slim (no DB upgrade if everything lands at once). 

String or IDL/UUID changes made by this patch:
  None.

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 756835 [details] [diff] [review]
Part 1 (from Git) v1

Approving as part of the body of work needed for FHR on FF23 Android.

Comment 14

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 756836 [details] [diff] [review]
Part 2. v1

Approving as part of the body of work needed for FHR on FF23 Android.

Comment 15

[Richard Newman [:rnewman]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/795a66deb9b9
https://hg.mozilla.org/releases/mozilla-aurora/rev/990c0e1d666b