The data limit of alarms is cumulative. If you activate a data alert at "5M", when your mobile data traffic is not zero (i.e. 1.5M). The alarm is fired when your reached your initial traffic plus the 5M (i.e. when your reached 6.5M). STR. 1. Browser some page (i.e. 1,5M) 2.Set "Alert me when used" to "5M"; 3.Then browser some page; 4.It pop up message when the data has used 6.5M EXPECTED BEHAVIOUR: Alarms fires at "5M" ACTUAL BEHAVIOUR: Alarms fires at the sum of initial data traffic and "5M"

nominating to v1.3? since it is an issue related to NetworkStats API already uplifted to v1.3. Thanks!

netd does not work with absolute values for alerts. Instead of set an alarm to the desired threshold, we need to set a quota based on the current stats in order to achieve the desired threshold.

Updated tests and some fixes of some errors found.

Comment on attachment 8368521 [details] [diff] [review] Patch Review of attachment 8368521 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/network/src/NetworkStatsService.jsm @@ +1026,5 @@ > return; > } > > let threshold = { > + absoluteThreshold: result.rxTotalBytes + result.txTotalBytes + quota, You can simply do: absoluteThreshold: aAlarm.threshold without redundant calculation.

Comment on attachment 8368523 [details] [diff] [review] Update tests Review of attachment 8368523 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/network/tests/unit_stats/test_networkstats_service.js @@ +166,5 @@ > NetworkStatsService.updateStats(netId2); > do_check_eq(NetworkStatsService.updateQueue.length, 2); > do_check_eq(NetworkStatsService.updateQueue[0].callbacks.length, 1); > > + var i = 0; Add one more: var updateCount = 0; @@ +171,3 @@ > var callback = function(success, msg) { > + i++; > + if (i >= 2) { Where does the magic number come from? Can it be calculated? Or at least be defined? if (i >= updateCount) @@ +177,4 @@ > }; > > NetworkStatsService.updateStats(netId1, callback); > NetworkStatsService.updateStats(netId2, callback); If I don't misunderstand, we can do: NetworkStatsService.updateStats(netId1, callback); updateCount++; NetworkStatsService.updateStats(netId2, callback); updateCount++; Or just define updateCount = 2 before these 2 calls (not sure if callback would run immediately after the first call): updateCount = 2; NetworkStatsService.updateStats(netId1, callback); NetworkStatsService.updateStats(netId2, callback); Please correct me if I'm wrong. Thanks!

I'm not sure if we're still allowed to uplift for v1.3. Needs Fabrice's approval before uplifting.

Oh... this patch needs to be on top of the bug 964270. I'm on CNY holidays but I'll finish that review early next week if it's supposed to be V1.3.

Conditional approval waiting on bug 964270 which looks scarier.

Changes from comment 6. More testing revealed some errors with getCurrentStats function due to the introduction of serviceType in stats database.

Fixes from comment 7 and update according to the new version of the main patch.

Albert, Please help with user experience

The user impact is that alarms are fired at wrong threshold. As explained in comment 1 if the user adds an alarm at 2M and then set another one to 3M, the first alarm is going to be triggered when 2M of data are consumed but the second alarm is currently fired at 5M (3 + 2) instead of 3M.

Thanks Albert. 1.3+ this is providing wrong info to the user.

Can we get ETA to fix this bug? Thank you.

(In reply to Kevin Hu [:khu] from comment #16) > Can we get ETA to fix this bug? Thank you. We need to uplift bug 964270 before fixing this one.

Comment on attachment 8370027 [details] [diff] [review] Patch Review of attachment 8370027 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/network/src/NetworkStatsDB.jsm @@ +160,3 @@ > rxDiff = cursor.value.rxSystemBytes; > txDiff = cursor.value.txSystemBytes; > + counters[netId] = { I lost here. I'd like to see John's review before landing. @@ +522,5 @@ > sample.serviceType = ""; > sample.rxBytes = 0; > sample.txBytes = 0; > + sample.rxTotalBytes = 0; > + sample.txTotalBytes = 0; I lost here. I'd like to see John's review before landing. ::: dom/network/src/NetworkStatsService.jsm @@ +852,5 @@ > for (let i = 0; i < result.length; i++) { > let alarm = result[i]; > alarms.push({ id: alarm.id, > network: self._networks[alarm.networkId].network, > + threshold: alarm.absoluteThreshold, This is confusing, because we use threshold and absoluteThreshold over the codes, which can have different meanings. Can you s/threshold/absoluteThreshold for all the alarms? @@ +998,5 @@ > aCallback(null, true); > }); > }, > > + _getQuota: function _getQuota(aAlarm, aCallback) { s/_getQuota/_getAlarmQuota @@ +1019,5 @@ > aCallback("InvalidStateError", null); > return; > } > > + aAlarm.absoluteThreshold = aAlarm.threshold; Ditto. This is confusing. Can we just use the reasonable name in the first place? @@ +1021,5 @@ > } > > + aAlarm.absoluteThreshold = aAlarm.threshold; > + if (aAlarm.startTime) { > + aAlarm.absoluteThreshold = aAlarm.threshold; This line is redundant. @@ +1022,5 @@ > > + aAlarm.absoluteThreshold = aAlarm.threshold; > + if (aAlarm.startTime) { > + aAlarm.absoluteThreshold = aAlarm.threshold; > + aAlarm.threshold = result.rxTotalBytes + result.txTotalBytes + quota; I totally lost the difference between absoluteThreshold and threshold. Could you clarify me by adding more comments? I kind of have a feeling you should rename all the threshold to absoluteThreshold (if so, just simply use threshold). And the threshold you set here is something different from absoluteThreshold. Could you have a better naming for this? Or at least add comments to clarify it.

Comment on attachment 8370029 [details] [diff] [review] Update tests Review of attachment 8370029 [details] [diff] [review]: ----------------------------------------------------------------- The test looks good but the main patch is still not solid to me. ::: dom/network/tests/unit_stats/test_networkstats_service.js @@ +189,3 @@ > }); > > +add_test(function test_getQuota() { s/test_getQuota()/test_getAlarmQuota() @@ +192,3 @@ > let alarm = { networkId: wifiId, threshold: 10000 }; > > + NetworkStatsService._getQuota(alarm, function onSet(error, quota){ s/_getQuota/_getAlarmQuota

(In reply to Gene Lian [:gene] (CNY Jan. 29 ~ Feb. 5) from comment #18) > Comment on attachment 8370027 [details] [diff] [review] > Patch > > Review of attachment 8370027 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/network/src/NetworkStatsDB.jsm > @@ +160,3 @@ > > rxDiff = cursor.value.rxSystemBytes; > > txDiff = cursor.value.txSystemBytes; > > + counters[netId] = { > > I lost here. I'd like to see John's review before landing. counters array is only used to upgrade database total stats of appId 0, which are only used in alarms. I am going to request John's review. > @@ +522,5 @@ > > sample.serviceType = ""; > > sample.rxBytes = 0; > > sample.txBytes = 0; > > + sample.rxTotalBytes = 0; > > + sample.txTotalBytes = 0; > > I lost here. I'd like to see John's review before landing. The same, TotalBytes only affect alarms. > ::: dom/network/src/NetworkStatsService.jsm > @@ +852,5 @@ > > for (let i = 0; i < result.length; i++) { > > let alarm = result[i]; > > alarms.push({ id: alarm.id, > > network: self._networks[alarm.networkId].network, > > + threshold: alarm.absoluteThreshold, > > This is confusing, because we use threshold and absoluteThreshold over the > codes, which can have different meanings. Can you > s/threshold/absoluteThreshold for all the alarms? The main problem here is regarding alarms with start date. If we have alarms with no start date there is no problem. But alarms with start date make us to have two different thresholds, the one received in the addAlarm request and the internal one obtained from the stats since the start date. To be able to manage alarms and have them sorted we have to work with internal threshold because it is relative to the threhold of the alarms without start date. The point is that we have the database sorted by the 'threshold' index, so in the database the threshold holds the internal threshold and the absoluteThreshold holds the alarm threshold (used to be returned in the getalarms and to recalculate the internal threshold). I keep these names to avoid to modify the database. I can modify both alarmToRecord and recordToAlarm to modify the name of the variables and avoid confusion. > @@ +1019,5 @@ > > aCallback("InvalidStateError", null); > > return; > > } > > > > + aAlarm.absoluteThreshold = aAlarm.threshold; > > Ditto. This is confusing. Can we just use the reasonable name in the first > place? The same commented before > @@ +1022,5 @@ > > > > + aAlarm.absoluteThreshold = aAlarm.threshold; > > + if (aAlarm.startTime) { > > + aAlarm.absoluteThreshold = aAlarm.threshold; > > + aAlarm.threshold = result.rxTotalBytes + result.txTotalBytes + quota; > > I totally lost the difference between absoluteThreshold and threshold. Could > you clarify me by adding more comments? > > I kind of have a feeling you should rename all the threshold to > absoluteThreshold (if so, just simply use threshold). > > And the threshold you set here is something different from > absoluteThreshold. Could you have a better naming for this? Or at least add > comments to clarify it. As commented before, we have two options: - Use threshold and _threshold (for alarms with start date will be different than threshold) in NetworkStatsService and flip values in the NetworkStatsDb. - Modify the database index name, change 'threshold' index name by '_threshold', by now the alarms database is only being used by marina while integrating alarms in costcontrol. What do you think?

The |threshold| can now have different meaning between the |threshold| we want to expose (i.e. |absoluteThreshold|) and the |threshold| we use to maintain the DB, which would make followers really confused. I'd prefer changing the index name in DB from |threshold| to |relativeThreshold| (btw, _threshold is not a good name to me), which is aimed to sort alarms. I understand this makes more changes but it's worth it for future maintenance. In this way, we'll have |absoluteThreshold| and |relativeThreshold| for internal uses. We have to use |absoluteThreshold|/|relativeThreshold| in DB and service as possible as we can to clarify their purposes, and finally expose the name |threshold| to the content until really needed. Please refer to MobileMessageDB's upgradeSchema6, I think changing the index name is feasible in this case. Fortunately, the V1.3 and V1.4's DB schema are sync'ed, so the change can be safely uplifted. Does that make sense to you?

Comment on attachment 8370027 [details] [diff] [review] Patch Review of attachment 8370027 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/network/src/NetworkStatsDB.jsm @@ +160,3 @@ > rxDiff = cursor.value.rxSystemBytes; > txDiff = cursor.value.txSystemBytes; > + counters[netId] = { I see you are going to reset the total counter in each reboot time, but why didn't you do the same thing in _processSamplesDiff()? Or I may miss something :-)

(In reply to John Shih from comment #22) > Comment on attachment 8370027 [details] [diff] [review] > Patch > > Review of attachment 8370027 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/network/src/NetworkStatsDB.jsm > @@ +160,3 @@ > > rxDiff = cursor.value.rxSystemBytes; > > txDiff = cursor.value.txSystemBytes; > > + counters[netId] = { > > I see you are going to reset the total counter in each reboot time, but why > didn't you do the same thing in _processSamplesDiff()? Or I may miss > something :-) Oh! It is a mistake, I want to reset total counters when there is a clearInterfaceStats call, not in each reboot time. Thanks!

Minor changes from comment 19

Comment on attachment 8371435 [details] [diff] [review] Patch Review of attachment 8371435 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/network/src/NetworkStatsDB.jsm @@ +159,5 @@ > rxDiff = cursor.value.rxSystemBytes; > txDiff = cursor.value.txSystemBytes; > } > > + if ((cursor.valuexrxBytes == 0 && rxDiff > 0) || s/cursor.valuexrxBytes/cursor.value.rxBytes Also, I think you need to check rxDiff >=0 @@ +160,5 @@ > txDiff = cursor.value.txSystemBytes; > } > > + if ((cursor.valuexrxBytes == 0 && rxDiff > 0) || > + (cursor.value.txBytes == 0 && txDiff > 0)) { Ditto. @@ +161,5 @@ > } > > + if ((cursor.valuexrxBytes == 0 && rxDiff > 0) || > + (cursor.value.txBytes == 0 && txDiff > 0)) { > + // Stats cseared through clearInterfaceStats, so reset total counters. s/cseared/was cleared @@ +188,5 @@ > }; > > // Create object store for alarms. > objectStore = db.createObjectStore(ALARMS_STORE_NAME, { keyPath: "id", autoIncrement: true }); > + objectStore.createIndex("alarm", ['networkId','relativeThreshold'], { unique: false }); Another concern is that since you are changing original update schema, these change will not be executed if a device is already with the up-to-date db version. I'll let Gene have double check on it.

Comment on attachment 8371435 [details] [diff] [review] Patch Review of attachment 8371435 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/network/src/NetworkStatsDB.jsm @@ +161,5 @@ > } > > + if ((cursor.valuexrxBytes == 0 && rxDiff > 0) || > + (cursor.value.txBytes == 0 && txDiff > 0)) { > + // Stats cseared through clearInterfaceStats, so reset total counters. s/cseared/cleared I'll leave the review to John for all the counters stuff. @@ +188,5 @@ > }; > > // Create object store for alarms. > objectStore = db.createObjectStore(ALARMS_STORE_NAME, { keyPath: "id", autoIncrement: true }); > + objectStore.createIndex("alarm", ['networkId','relativeThreshold'], { unique: false }); This is impossible to work. We have to provide a schema updating process (currVersion == 6) to replace the 'threshold' by 'relativeThreshold'. Keep in mind that we've already had some records in the alarm DB. We need to make sure those alarms are still working after updating the DB. Please refer to MobileMessageDB's upgradeSchema6(). @@ +526,5 @@ > sample.serviceType = ""; > sample.rxBytes = 0; > sample.txBytes = 0; > + sample.rxTotalBytes = 0; > + sample.txTotalBytes = 0; Leave the review to John. ::: dom/network/src/NetworkStatsService.jsm @@ +931,5 @@ > > let newAlarm = { > id: null, > networkId: aNetId, > threshold: threshold, Can you also change this to absoluteThreshold and its related codes? As mentioned before, we had better to use |absoluteThreshold|/|relativeThreshold| as possible as we can in the internal codes. We use |threshold| only when: 1. Catch the input from the API (like set alarm). 2. Expose it to return the API (like get alarms). @@ +1022,5 @@ > aCallback(error, result); > return; > } > > + let quota = aAlarm.threshold - result.rxBytes - result.txBytes; s/threshold/absoluteThreshold? Is that possible? @@ +1030,5 @@ > aCallback("InvalidStateError", null); > return; > } > > + aAlarm.relativeThreshold = aAlarm.threshold; s/threshold/absoluteThreshold? Can we? @@ +1031,5 @@ > return; > } > > + aAlarm.relativeThreshold = aAlarm.threshold; > + if (aAlarm.startTime) { Also, I'd prefer: aAlarm.relativeThreshold = aAlarm.startTime ? result.rxTotalBytes + result.txTotalBytes + quota : aAlarm.absoluteThreshold; Instead of overriding the value for special case.

(In reply to John Shih from comment #26) > Comment on attachment 8371435 [details] [diff] [review] > Patch > > Review of attachment 8371435 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/network/src/NetworkStatsDB.jsm > @@ +159,5 @@ > > rxDiff = cursor.value.rxSystemBytes; > > txDiff = cursor.value.txSystemBytes; > > } > > > > + if ((cursor.valuexrxBytes == 0 && rxDiff > 0) || > > s/cursor.valuexrxBytes/cursor.value.rxBytes > > Also, I think you need to check rxDiff >=0 If rxDiff is equal to 0 it doesn't mean that a reset happened, reset occurs when systemBytes has incremented but bytes are 0. > @@ +188,5 @@ > > }; > > > > // Create object store for alarms. > > objectStore = db.createObjectStore(ALARMS_STORE_NAME, { keyPath: "id", autoIncrement: true }); > > + objectStore.createIndex("alarm", ['networkId','relativeThreshold'], { unique: false }); > > Another concern is that since you are changing original update schema, these > change will not be executed if a device is already with the up-to-date db > version. I'll let Gene have double check on it. I'm not sure about that, the name of the index is the same, I'll test it.

In theory, I wonder you have to update all the records based on the new atrrribute (|relativeThreshold|) so that the index table can be re-updated as well. Even if it's not necessary, you still need to change |threshold| to |relativeThreshold| for all the existing alarm records. Right? So that the |alarm| index can find it.

Modified alarm's schema, 'threshold' renamed to 'relativeTrheshold' in 'upgradeSchema' function. Stats stored in database from previous version are updated in order to find if some reset (clearInterfaceStats) happened. Previous version keep totalBytes counters growing when reset but now should be reset, set to 0. To do so, two things can happen, for a given network: - If there are less samples than max allowed it means a possible reset, which is confirmed if rx/txBytes are different than rx/txTotalBytes in the first sample. Then rx/txTotalBytes for the first sample are set to the same value of rx/txBytes and values of rxTxTotalBytes for the following samples are recalculated. - If there are more samples than max there is no way to know if reset happened. So nothing can be done, assuming that some reset happened the result will be that alarms will be fired before reaching the desired threshold.

Changes according to new patch.

Comment on attachment 8372998 [details] [diff] [review] Patch Review of attachment 8372998 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me! Thanks! ::: dom/network/src/NetworkStatsDB.jsm @@ +281,5 @@ > + if (cursor.value.rxTotalBytes == cursor.value.rxBytes && > + cursor.value.txTotalBytes == cursor.value.txBytes) { > + return; > + } > + nit: extra space @@ +282,5 @@ > + cursor.value.txTotalBytes == cursor.value.txBytes) { > + return; > + } > + > + cursor.value.rxTotalBytes = cursor.value.rxBytes; Ditto. @@ +290,5 @@ > + cursor.continue(); > + return; > + } > + > + cursor.value.rxTotalBytes = last.rxTotalBytes + cursor.value.rxBytes; Ditto.

Comment on attachment 8372998 [details] [diff] [review] Patch Review of attachment 8372998 [details] [diff] [review]: ----------------------------------------------------------------- Nice! Thank you! ::: dom/network/src/NetworkStatsDB.jsm @@ +213,5 @@ > if (DEBUG) { > debug("Added new key 'serviceType' for version 6"); > } > + } else if (currVersion == 6) { > + // Change threshold attribute of alarm index by relativeThreshold in alarms DB. s/Change/Replace/ @@ +227,5 @@ > + // Create new "alarm" index. > + alarmsStore.createIndex("alarm", ['networkId','relativeThreshold'], { unique: false }); > + > + // Populate new "alarm" index attributes. > + alarmsStore.openCursor().onsuccess = function(event) { So, the process of alarmsStore and statsStore will update at the same time. Right? Hmm... should be working. @@ +241,5 @@ > + cursor.update(cursor.value); > + cursor.continue(); > + } > + > + // Previous versions save accumulative totalBytes, increasing althought system s/althought system/although the system/ @@ +242,5 @@ > + cursor.continue(); > + } > + > + // Previous versions save accumulative totalBytes, increasing althought system > + // reboot or reset stats. But is necessary to reset total counters when reset s/reboot/reboots s/reset/resets s/total/the total @@ +269,5 @@ > + return; > + } > + > + let last = null; > + // Reset detected if first sample totalCounters are different than bytes s/first/the first @@ +270,5 @@ > + } > + > + let last = null; > + // Reset detected if first sample totalCounters are different than bytes > + // counters. If so, total counters should be recalculated. s/total/the total @@ +290,5 @@ > + cursor.continue(); > + return; > + } > + > + cursor.value.rxTotalBytes = last.rxTotalBytes + cursor.value.rxBytes; Add a simple comment above this line to specify these codes handle the last record.

Minor changes from comments 32 & 33

https://hg.mozilla.org/integration/b2g-inbound/rev/14f822c6147c https://hg.mozilla.org/integration/b2g-inbound/rev/7b911962c1ce

https://hg.mozilla.org/mozilla-central/rev/14f822c6147c https://hg.mozilla.org/mozilla-central/rev/7b911962c1ce

This needs rebasing for b2g28 or other dependent bugs nominated for uplift. Note that DB versions can cause migration problems if not handled carefully.

1.3 does not have the patch of 'serviceType', which added modifications in the database, so here last db version is lower. I modified the patch to update db in lower version, does it mean that I should open a new bug to modify 1.4 by reordering db upgrade versions?

Comment on attachment 8374337 [details] [diff] [review] Patch for 1.3 Review of attachment 8374337 [details] [diff] [review]: ----------------------------------------------------------------- I'm afraid we have to uplift the serviceType first... It would be a disaster if the DB version among branches is not sync'ed. Imaging if someone wants to update its V1.3 to V1.4, it would miss the update for serviceType. Please see bug 960741, comment #12 for the similar experience.

Comment on attachment 8374337 [details] [diff] [review] Patch for 1.3 Review of attachment 8374337 [details] [diff] [review]: ----------------------------------------------------------------- Due to Gene's concern, remove the review flag.

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/80f2342abeb6 https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/0d2d33f55571

I backed this out of 1.3 because it depended on bug 922926, which I backed out because it wasn't supposed to land on 1.3: https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/8fcbe4200236

Renoming also - we need to discuss this again in triage.

If bug 922926 is not allowed to uplift in any way, we have to rethink a work-around without touching the DB update, so that this bug can also be uplifted as well. However, this one have been checked in to V1.4 and potentially made users update DB already, so I'd suggest uplifting bug 922926 as well. I just discussed with John, uplifting bug 922926 wouldn't cause incompatibility with Gaia. Otherwise, we had better back out this one (and its child) from v1.4 as soon as possible.

Just came out with an alternative, we can just uplift the DB update part for bug 922926 without the real function part, so that we don't really need to uplift bug 922926 but keep the DB version sequence right. John and Albert, would you please analyze the possibility?

Since the DB modification in bug 922926 includes key path change, we can not avoid to have some corresponding implementation (e.g., add "serviceType" condition in find() function). The possible solution is to use a default serviceType as a workaround, what do you think?

If we uplift the DB update part, the main problem is the addition of the 'serviceType' in the keypath, thus we need to add the field also to all the IDBKeyRanges. Making this changes I think we won't have problems. What do you think John?

(In reply to Albert [:albert] from comment #47) > If we uplift the DB update part, the main problem is the addition of the > 'serviceType' in the keypath, thus we need to add the field also to all the > IDBKeyRanges. Making this changes I think we won't have problems. What do > you think John? yes, it also what I think. let's have const SERVICE_NO_TYPE = ""; and add it in all placed if needed.

Patch is needed for Tarako version

traige: 1.3T+ to get this into tarako for usage app memory saving

Hi Ying Xu, heard that you will be doing uplifts to 1.3T branch. After you completed the uplift, can you please set status-b2g-v1.3T to fixed? please let us know if you have problems with it. thanks

remote: https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/4862d7b8aa52 remote: https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/877fd11a1875