838072 - APIs for periodic pull-based collection of data

Status: RESOLVED FIXED

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

Description

[Gregory Szorc [:gps]]

Currently the only pull-based API for collecting data for FHR is "collectConstantData." The intent was always to add new APIs as we needed them. Bug 828546 is forcing our hand.

I plan to add a collectHourlyData() and/or collectDailyData() API. We'll see what I do.

Comment 1

[Gregory Szorc [:gps]]

Attachment: Part 1: Make core collection code reusable, v1

Patch should speak for itself. There should be no functional code changes. Except for 1. We previously threw if the provider did not conform to the proper API. Now, we simply log an error and continue.

Comment 2

[Gregory Szorc [:gps]]

Attachment: Part 2: Add collectDailyData()

Just defining it. Will hook it up in part 3.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Is this related to bug 833794?

Comment 4

[Richard Newman [:rnewman]]

Comment on attachment 710055 [details] [diff] [review]
Part 1: Make core collection code reusable, v1

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

::: services/metrics/collector.jsm
@@ +175,3 @@
>        let collectPromise;
>        try {
> +        collectPromise = provider[fnProperty]();

I think you want

  collectPromise = provider[fnProperty].call(provider);

Comment 5

[Richard Newman [:rnewman]]

Comment on attachment 710056 [details] [diff] [review]
Part 2: Add collectDailyData()

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

::: services/metrics/dataprovider.jsm
@@ +528,5 @@
>      return Promise.resolve();
>    },
>  
>    /**
> +   * Collects data roughly every day.

The service walked briskly down the street, scanning the passers-by. He spotted his first victim. Same old, same old, he thought. These suckers don't know what they're doing.

"GIMME THE DATA", he bellowed, roughly snatching the bag from the provider. The provider fell against the bus stop.

"That's twice this week!"

@@ +530,5 @@
>  
>    /**
> +   * Collects data roughly every day.
> +   *
> +   * For long-running applications, this is called roughly every day. It may

Approximately.

@@ +533,5 @@
> +   *
> +   * For long-running applications, this is called roughly every day. It may
> +   * or may not be called every time the application is run. It also may be
> +   * called more than once a day. In other words, the guarantees are very
> +   * loose and the provider should take this into account.

By…?

Being efficient?

Comment 6

[Richard Newman [:rnewman]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #3)
> Is this related to bug 833794?

Nope (at least not directly). It's just a different collection pattern for FHR: some providers (e.g., Places) need to collect certain values at different intervals.

Comment 7

[Gregory Szorc [:gps]]

Attachment: Part 3: Call collectDailyData() from FHR, v1

Went with the easiest implementation of this. No state. Always collected at upload time (just in case).

Comment 8

[Richard Newman [:rnewman]]

Comment on attachment 710061 [details] [diff] [review]
Part 3: Call collectDailyData() from FHR, v1

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

::: services/healthreport/tests/xpcshell/test_healthreporter.js
@@ +217,5 @@
> +
> +  yield reporter.collectMeasurements();
> +  do_check_eq(provider.collectConstantCount, 1);
> +  do_check_eq(provider.collectDailyCount, 1);
> +

One more test:

  yield reporter.collectMeasurements();
  do_check_eq(provider.collectDailyCount, 1);   // Too soon.

@@ +221,5 @@
> +
> +  reporter._lastDailyDate = now.getTime() - MILLISECONDS_PER_DAY - 1;
> +  yield reporter.collectMeasurements();
> +  do_check_eq(provider.collectDailyCount, 2);
> +

One more test: 

  reporter._lastDailyDate = null;
  yield reporter.collectMeasurements();
  do_check_eq(provider.collectDailyCount, 3);

Comment 9

[Gregory Szorc [:gps]]

https://hg.mozilla.org/services/services-central/rev/41003e4da9ab
https://hg.mozilla.org/services/services-central/rev/f02317d480ee
https://hg.mozilla.org/services/services-central/rev/e0b8010e2eaa

Comment 10

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/41003e4da9ab
https://hg.mozilla.org/mozilla-central/rev/f02317d480ee
https://hg.mozilla.org/mozilla-central/rev/e0b8010e2eaa