Closed Bug 838072 Opened 9 years ago Closed 9 years ago

APIs for periodic pull-based collection of data

Categories

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

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 21

People

(Reporter: gps, Assigned: gps)

References

Details

Attachments

(3 files)

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.
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.
Assignee: nobody → gps
Status: NEW → ASSIGNED
Attachment #710055 - Flags: review?(rnewman)
Just defining it. Will hook it up in part 3.
Attachment #710056 - Flags: review?(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);
Attachment #710055 - Flags: review?(rnewman) → review+
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?
Attachment #710056 - Flags: review?(rnewman) → review+
(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.
Went with the easiest implementation of this. No state. Always collected at upload time (just in case).
Attachment #710061 - Flags: review?(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);
Attachment #710061 - Flags: review?(rnewman) → review+
https://hg.mozilla.org/mozilla-central/rev/41003e4da9ab
https://hg.mozilla.org/mozilla-central/rev/f02317d480ee
https://hg.mozilla.org/mozilla-central/rev/e0b8010e2eaa
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in services]
Target Milestone: --- → mozilla21
Component: Metrics and Firefox Health Report → Client: Desktop
Product: Mozilla Services → Firefox Health Report
Target Milestone: mozilla21 → Firefox 21
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.