877607 - [Cost Control] In Data usage graph red and green dots are shown in wrong position

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Cost Control, defect)

Description

[Leo]

1. Title : In Data usage graph red and green dots are shown in wrong position
2. Precondition : In Usage app enable Mobile usage and Wi-Fi usage check boxes and browse internet using Wi-Fi and 3G/2G
3. Tester's Action : Go to Usage app and observe the usage graph.
4. Detailed Symptom (ENG.) :The Red and Green dots are not aligned with the vertical lines and shown in wrong position after next day.
6.Reproducibility: Y
1)Frequency Rate : 100%\
7.Gaia Master/v1-train : Reproduced
8.Gaia Revision: 95e0dfb5edb55502a02ff35712c291031d443754
9.Personal email id: [e-mail]

Comment 1

[Leo]

Attachment: Screen shot attached.

Comment 2

[Leo]

Hi Salva,

Please find the STR for this bug.

1.In FTU select/keep as it is American time zone.
2.Go to Usage app and pass the FTE with Reset report weekely.(for clear observation of bug in Chart)
3.Browser the3G and Wi-Fi data and check the Usage graph.

The Red and Green dots are not aligned with the vertical line of usage.

Comment 3

[Leo]

I found the root cause for this bug in code.

In datausage.js drawMobileGraphic and drawWifiGraphic functions having the below condition
if (today.getTime() === toMidnight(new Date(sample.date)).getTime() &&
          x >= model.originX) {

Comment 4

[Salvador de la Puente González [:salva]]

I imagined something related with that. As usual, try the solution in a separated branch, make a PR and I'll review it.

Thank you.

Comment 5

[Leo]

Hi Salva,

Are you able to reproduce this issue?
Sometimes in log sample.date is showing NaN.

Comment 6

[Salvador de la Puente González [:salva]]

I did not try. I have another higher priorities right now. Let's see if the bug is triaged soon.

Comment 7

[[e-mail]]

WIP:

https://github.com/mozilla-b2g/gaia/pull/10202
works in firefox nightly, but cannot lazyload correctly on device

need to figure out why lazyload callback is not run on device.

Comment 8

[[e-mail]]

Sorry comment 7 is the wrong post for other issue...

Comment 9

[Leo]

The data chunks returned by getNetworkStats() api having wrong date sometimes,if the Timezone is set wrongly.This is causing the getUsage() function returning undefined usage.(Its very random)

Comment 10

[John Hu [:johnhu][:johu][:醬糊小弟]]

There is a strange thing here. This bug only occurred while using timezone east of GMT (-01:00 to -11:30). It always gets one day shift between the dot and line.

If we print the usage data of yesterday and make some network usage, we will get:

Note date/time of Device is changed to Fri Jun 07 2013 17:22:22 GMT-0900 (HADT) for testing, and that is Jun 08 2013 02:22:22 in UTC.

Before usage:
today: Fri Jun 07 2013 17:22:22 GMT-0900 (HADT), data usage record: value = 59872601 @ Thu Jun 06 2013 15:00:00 GMT-0900 (HADT)
today: Fri Jun 07 2013 17:22:22 GMT-0900 (HADT), data usage record: value = undefined @ Fri Jun 07 2013 15:00:00 GMT-0900 (HADT)

After usage:
today: Fri Jun 07 2013 17:25:10 GMT-0900 (HADT), data usage record: value = 61877869 @ Thu Jun 06 2013 15:00:00 GMT-0900 (HADT)
today: Fri Jun 07 2013 17:25:10 GMT-0900 (HADT), data usage record: value = undefined @ Fri Jun 07 2013 15:00:00 GMT-0900 (HADT)


If we assume the NetworkStats API uses UTC time, the UTC time of Fri Jun 07 2013 17:22:22 GMT-0900 (HADT) will be the date of Jun 08 2013 (UTC), and the accumulation should be made to Jun 08 2013 00:00:00 (UTC) which is Fri Jun 07 2013 15:00:00 GMT-0900 (HADT). But it doesn't and this bug happens!!!

If we assume the NetworkStats API uses local time, it should accumulate the usage to Jun 07 2013 00:00:00 (HADT) instead of Jun 06 2013 15:00:00 (HADT) and this bug happens!!!

And assumes more about gaia

If we assume the NetworkStats API uses UTC time and accumulates data correctly to Jun 08 2013 (UTC), we will get data like:
today: Fri Jun 07 2013 17:25:10 GMT-0900 (HADT), data usage record: value = 59872601 @ Thu Jun 06 2013 15:00:00 GMT-0900 (HADT)
today: Fri Jun 07 2013 17:25:10 GMT-0900 (HADT), data usage record: value = 2005268 @ Fri Jun 07 2013 15:00:00 GMT-0900 (HADT)

The drawing will be correct, but users may not understand why network usage of Jun 07 starts at Jun 07 15:00:00.

But, if all of this happens at Fri Jun 07 2013 13:25:10 GMT-0900 (HADT) which is Jun 07 2013 22:25:10 in UTC, the data will like this:
today: Fri Jun 07 2013 13:25:10 GMT-0900 (HADT), data usage record: value = 61877869 @ Thu Jun 06 2013 15:00:00 GMT-0900 (HADT)
today: Fri Jun 07 2013 13:25:10 GMT-0900 (HADT), data usage record: value = undefined @ Fri Jun 07 2013 15:00:00 GMT-0900 (HADT)

The current implementation will fill usage to Jun 06 2013 but draw dot of today at Jun 07 2013, and this bug happens!!!

So, there may be two different causes to reproduce it, one in gaia, and one in gecko.

Salva, 

Those tests are made by (Leo, [e-mail], and I). Would you mind to tell us if this investigation correct??

No matter gaia part or gecko part, we all need the one who implements NetworkStats API to tell us what kind of time, UTC or local time, this API uses. So, I loops Albert in.

Alert, any ideas??

Comment 11

[Albert [:albert]]

In gecko, data usage samples are stored in UTC.

In my device the bug is happening and checking NetworkStats response it seems to be correct:

result: {"connectionType":"mobile","start":"2013-06-06T04:00:00.000Z","end":"2013-06-30T04:00:00.000Z","data":[{"rxBytes":3456,"txBytes":2741,"date":"2013-06-06T00:00:00.000Z"},{"rxBytes":5124,"txBytes":3260,"date":"2013-06-07T00:00:00.000Z"},{"date":"2013-06-08T00:00:00.000Z"},{"date":"2013-06-09T00:00:00.000Z"},{"date":"2013-06-10T00:00:00.000Z"},{"date":"2013-06-11T00:00:00.000Z"},{"date":"2013-06-12T00:00:00.000Z"} ....

In costcontrol application I see the horizontal line starting between to vertical lines and the dot as Leo said. I think horizontal line must start at the point of a vertical one, can you confirm Salva?

Comment 12

[John Hu [:johnhu][:johu][:醬糊小弟]]

Albert,

Thanks for your information. I agree UTC is the correct way to deal this kind of thing. 

But why does our test accumulate to wrong date when we read from getNetworkStats. Like:
Before usage:
today: Fri Jun 07 2013 17:22:22 GMT-0900 (HADT), data usage record: value = 59872601 @ Thu Jun 06 2013 15:00:00 GMT-0900 (HADT)
today: Fri Jun 07 2013 17:22:22 GMT-0900 (HADT), data usage record: value = undefined @ Fri Jun 07 2013 15:00:00 GMT-0900 (HADT)

After usage:
today: Fri Jun 07 2013 17:25:10 GMT-0900 (HADT), data usage record: value = 61877869 @ Thu Jun 06 2013 15:00:00 GMT-0900 (HADT)
today: Fri Jun 07 2013 17:25:10 GMT-0900 (HADT), data usage record: value = undefined @ Fri Jun 07 2013 15:00:00 GMT-0900 (HADT)

Fri Jun 07 2013 17:25:10 GMT-0900 (HADT) === Sat Jun 08 2013 02:25:10 (UTC)
Fri Jun 07 2013 15:00:00 GMT-0900 (HADT) === Sat Jun 08 2013 00:00:00 (UTC)

Comment 13

[Albert [:albert]]

I've seen an error when converting date:

  convertDate: function convertDate(aDate) {
    // Convert to UTC according to timezone and
    // filter timestamp to get SAMPLE_RATE precission
    let timestamp = aDate.getTime() - aDate.getTimezoneOffset() * 60 * 1000;
    timestamp = Math.floor(timestamp / SAMPLE_RATE) * SAMPLE_RATE;
    return timestamp;
  },

TimezoneOffset must be added instead of substracted.

Anyway, it should not affect to results returned because the same conversion is done when storing samples and when requesting.

Does it makes sense to you?

Comment 14

[Salvador de la Puente González [:salva]]

I saw the problem. In addition of what Albert said about the timezone offset (does not affect the fixing process actually), the main problem is related how the API returns the dates for samples. Start and end dates are returned converted into local time while the sample dates not.

So, the gray lines are drawn taking in count the time zone but the samples not and hence the gap between the gray line and the graphic.

Assigning to Albert. What do you think?

Comment 15

[Albert [:albert]]

You are right. During the day I will upload a patch to convert UTC timestamp of the samples to the local timestamp coming in the request.

Comment 16

[John Hu [:johnhu][:johu][:醬糊小弟]]

That's fine to reassign to albert.

Albert, I have different thought about "the same conversion is done when storing samples and when requesting".

But I don't know which direction of this convertDate is. In storing, the conversion is from local time to UTC which should use subtraction, like above. While requesting, which is from UTC to local time, it should use add.

If we make wrong direction on storing (use +), it will store data to yesterday. But if requesting uses wrong direction (use -) too, it will correct it back.

But if we make wrong direction on storing (use +) and use correct direction on requesting (use +), it be double add on timezone. In this case (-09:00), it will be Jun 07 2013 08:25:10 (from local to UTC for storing), and saving as Jun 07 2013 00:00:00 (discards hh:mm:ss for storing). While requesting, it gives Jun 06 2013 15:00:00 (from UTC to local with + operand).

How about this assumption??

Comment 17

[Albert [:albert]]

Attachment: Convert sample dates to local timezone when return.

Comment 18

[Albert [:albert]]

Salva, can you check if the patch fix the problem?

Comment 19

[Salvador de la Puente González [:salva]]

The patch, in addition of Gaia patch for bug 881866 works perfectly.

Comment 20

[Albert [:albert]]

Attachment: Tests

Comment 21

[Gene Lian (I already quit Mozilla)]

Comment on attachment 760079 [details] [diff] [review]
Convert sample dates to local timezone when return.

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

Looks good!

::: dom/network/src/NetworkStatsDB.jsm
@@ +233,5 @@
>      }, aResultCb);
>    },
>  
>    find: function find(aResultCb, aOptions) {
> +    let offset = aOptions.start.getTimezoneOffset() * 60 * 1000;

Using (new Date()).getTimezoneOffset() is sufficient. This value always reflects the current system timezone offset no matter what the Date object is. In this way, the followers won't be confused about why do you use |aOptions.start|'s timezone offset instead of |aOptions.end|'s.

@@ +238,2 @@
>      let start = this.convertDate(aOptions.start);
>      let end = this.convertDate(aOptions.end);

Since you're here, could you please do:

  s/convertDate/normalizeDate

where the |convertDate| is a bit implicit to me.

Also, you can do the same thing for .normalizeDate(...) as mentioned above:

  normalizeDate: function normalizeDate(aDate) {
    // Convert to UTC according to timezone and
    // filter timestamp to get SAMPLE_RATE precission
    let timestamp = aDate.getTime() - (new Date()).getTimezoneOffset() * 60 * 1000;
    timestamp = Math.floor(timestamp / SAMPLE_RATE) * SAMPLE_RATE;
    return timestamp;
  },

@@ +277,5 @@
>      }.bind(this), aResultCb);
>    },
>  
>    findAll: function findAll(aResultCb, aOptions) {
> +    let offset = aOptions.start.getTimezoneOffset() * 60 * 1000;

Ditto.

@@ +297,5 @@
>        if (!txn.result) {
>          txn.result = {};
>        }
>  
>        let request = store.index("timestamp").openCursor(range).onsuccess = function(event) {

Since you're here, please fold this line:

let request = store.index("timestamp").openCursor(range)
                                      .onsuccess = function(event) {

@@ +300,5 @@
>  
>        let request = store.index("timestamp").openCursor(range).onsuccess = function(event) {
>          var cursor = event.target.result;
>          if (cursor) {
> +          if (data.length > 0 && data[data.length - 1].date.getTime() == cursor.value.timestamp + offset) {

Please fold this:

if (data.length > 0 &&
    data[data.length - 1].date.getTime() == cursor.value.timestamp + offset)

Comment 22

[Gene Lian (I already quit Mozilla)]

Comment on attachment 761910 [details] [diff] [review]
Tests

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

::: dom/network/tests/test_networkstats_basics.html
@@ +50,5 @@
>    return;
>  }
>  
>  function checkDataDates(data, start, end, sampleRate){
> +  var offset = start.getTimezoneOffset() * 60 * 1000;

I'd prefer using (new Date()).getTimezoneOffset().

@@ +150,5 @@
>      // Get samplerate in millis
>      var sampleRate = netStats.sampleRate * 1000;
>      // Get date with samplerate's precision
> +    var offset = new Date().getTimezoneOffset() * 60 * 1000;
> +    var endDate = new Date(Math.floor((new Date().getTime() - offset) / sampleRate) * sampleRate + offset);

Please fold this line:

var endDate = new Date(Math.floor((new Date().getTime() - offset) / sampleRate)
                       * sampleRate + offset);

Well, I don't really like to be so picky. Since it's Mozilla's coding convention, so I have to stick to that. :)

@@ +181,5 @@
>      // Get samplerate in millis
>      var sampleRate = netStats.sampleRate * 1000;
>      // Get date with samplerate's precision
> +    var offset = new Date().getTimezoneOffset() * 60 * 1000;
> +    var endDate = new Date(Math.floor((new Date().getTime() - offset)/ sampleRate) * sampleRate + offset);

var endDate = new Date(Math.floor((new Date().getTime() - offset) / sampleRate)
                       * sampleRate + offset);

Note that you have to add a space before '/'.

Comment 23

[Gene Lian (I already quit Mozilla)]

r=gene after the above-mentioned nits are fixed. Thanks for the nice patch!

Comment 24

[Albert [:albert]]

Attachment: Convert sample dates to local timezone when return.

Changes from comment 21

Comment 25

[Albert [:albert]]

Attachment: Tests

Changes from comment 22

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/e5e4f78c1b28
https://hg.mozilla.org/projects/birch/rev/f100653c8996

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e5e4f78c1b28
https://hg.mozilla.org/mozilla-central/rev/f100653c8996

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/aba4c76e8382
https://hg.mozilla.org/releases/mozilla-b2g18/rev/c3a5f0dbb9c6

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/aba4c76e8382
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/c3a5f0dbb9c6

Comment 30

[sbogati]

Issue no longer reproduces on the Leo.
 
Build ID: 20130716070204
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/629020cf576b
Gaia: fb9362d34260771d4a00b9a0e10a6bbad397bd3b
Platform Version: 18.1

In Data usage graph red and green dots are shown as expected.