922926 - [NetworkStats API] Allow to track system-only network traffic

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[John Shih]

This bug is aiming at allowing to track system-only network traffic.

In the current work, we are able to track
* total system traffic (bug 746069)
* network per-app traffic (bug 784575)

however, there may some network traffic consumed by system itself (such as downloading, system updating,.. etc.)
In these cases, we are not able to get the valid app id (nonzero value) and therefore the traffic won't be saved into database based on original design.

Comment 2

[John Shih]

"System" network traffic can be in term of services, such as tethering, networking related, OTA, etc.
It would be a complex work to define all the services.
To make more flexible, I proposed to create a separated table from existing
per-app table to maintain the "system" network traffic.
Also, makes the implementation of each service as follow-up bugs.

Comment 3

[John Shih]

After discuss with Gene, we decide to extend original table instead of creating a new one.
The implementation is to reserve a unique Id for "system" traffic, and then add a new column/index (says "service")
to save traffic per service.

Comment 4

[John Shih]

Attachment: (wip) Bug 922926 - IDL Changes.

Comment 5

[John Shih]

Attachment: (wip) Bug 922926 - Part 1: IDL modifications.

Comment 6

[John Shih]

Attachment: Bug 922926 - Part 2: Implementation. r=gene

Comment 7

[John Shih]

Attachment: Bug 922926 - Part 3: Rebase code in necko.

Comment 8

[John Shih]

Attachment: Bug 922926 - Part 3: Rebase code in necko.

rebase code in necko.

Comment 9

[John Shih]

Attachment: Bug 922926 - Part 2: Implementation. r=gene

Comment 10

[John Shih]

There need some discussion on IDL changes,
to support obtaining data by system type, we need to provide new option in getSamples function
the direct way is to add an optional parameter (serviceType).
However, there is already one optional parameter (manifestURL), which makes adding a new optional
parameter difficult.

Do you have any idea on this?

Maybe we need to re-think the input structure on getSamples function and both require modifications
in gecko and gaia.

Need your inputs, thanks

Comment 11

[Albert [:albert]]

From my point of view I see two options:

1) Make serviceType mandatory and assign one value to the current behavior of getSamples, for example serviceType can be 'standard' or whatever.

2) As manifestURL is meaningless when requesting system stats, add a new function like getSystemStamples.

Comment 12

[John Shih]

(In reply to Albert [:albert] from comment #11)
> From my point of view I see two options:
> 
> 1) Make serviceType mandatory and assign one value to the current behavior
> of getSamples, for example serviceType can be 'standard' or whatever.

Or can simply assign null to serviceType.
however, it will raise another concern - why isn't manifestURL mandatory as well?
maybe we can just force developers to assign value (at least null, in case they don't need them) on both manifestURL and serviceType.

> 
> 2) As manifestURL is meaningless when requesting system stats, add a new
> function like getSystemStamples.

It is surely one of solutions. Although it's ok to me, I have more concern about which API
design is better. That is, to have multiple interfaces and split up the functions (getInterfaceSamples/getAppSamples/getSystemSamples) or to have a single interface which can provide multiple functions.

Comment 13

[Patrick McManus [:mcmanus]]

Comment on attachment 8336699 [details] [diff] [review]
Bug 922926 - Part 3: Rebase code in necko.

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

I would prefer having two methods: SaveAppStats() and SaveServiceStats()

Comment 14

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

I suggest to add just one optional parameter named `detailOptions` which is an object with the following fields:

- manifestURL, if provided, refers to the application for which we want the statistics. If it is the keyword `system` it is interpreted as the system application. Other values are ignored or throw an explanatory error (as you want).

- serviceType, if provided, refers to a fine-grained type of traffic. In current implementation, it is ignored for any value of manifestURL except `system`. It must be one of the items returned by `getAvailableServiceTypes()`

I like the optional `detailOptions` object because it allow to add other kind of constrains (i.e. traffic protocol: ftp, https, http, ssh...) only by adding an extra field. Furthermore, I keep `serviceType` unbound from the system application. It could be useful in a future.

And I think is a very friendly API. ;)

And just to clarify:
In the same way I see a method to get all the available service types, what about manfestURL, how can I retrieve a list of all installed applications or all the manifests?

Comment 15

[John Shih]

(In reply to Salvador de la Puente González [:salva] from comment #14)
> I suggest to add just one optional parameter named `detailOptions` which is
> an object with the following fields:
> 
> - manifestURL, if provided, refers to the application for which we want the
> statistics. If it is the keyword `system` it is interpreted as the system
> application. Other values are ignored or throw an explanatory error (as you
> want).
> 
> - serviceType, if provided, refers to a fine-grained type of traffic. In
> current implementation, it is ignored for any value of manifestURL except
> `system`. It must be one of the items returned by
> `getAvailableServiceTypes()`
> 
> I like the optional `detailOptions` object because it allow to add other
> kind of constrains (i.e. traffic protocol: ftp, https, http, ssh...) only by
> adding an extra field. Furthermore, I keep `serviceType` unbound from the
> system application. It could be useful in a future.
> 
> And I think is a very friendly API. ;)

Thanks your reply :)
Sounds good to me, it's more flexible if there is needed to have some additional options.
 
> And just to clarify:
> In the same way I see a method to get all the available service types, what
> about manfestURL, how can I retrieve a list of all installed applications or
> all the manifests?

Currently all the available manifestURLs can be got by mozApps.mgmt.getAll() in webapps API [1][2]
and it needs the "webapps-manage" permission.
It's due to the security concern so we don't to expose these information through NetworkStats API.
Actually, I'm also thinking about whether it is appropriate to expose available systemTypes here.
It may need further discussion or be a following bug :)

[1] https://developer.mozilla.org/en-US/docs/Web/API/Apps.mgmt.getAll
[2] https://developer.mozilla.org/en-US/Apps/Developing/JavaScript_API?redirectlocale=en-US&redirectslug=Web%2FApps%2FJavaScript_API

Comment 16

[Albert [:albert]]

(In reply to John Shih from comment #15)
> (In reply to Salvador de la Puente González [:salva] from comment #14)
> > I suggest to add just one optional parameter named `detailOptions` which is
> > an object with the following fields:
> > 
> > - manifestURL, if provided, refers to the application for which we want the
> > statistics. If it is the keyword `system` it is interpreted as the system
> > application. Other values are ignored or throw an explanatory error (as you
> > want).
> > 
> > - serviceType, if provided, refers to a fine-grained type of traffic. In
> > current implementation, it is ignored for any value of manifestURL except
> > `system`. It must be one of the items returned by
> > `getAvailableServiceTypes()`
> > 
> > I like the optional `detailOptions` object because it allow to add other
> > kind of constrains (i.e. traffic protocol: ftp, https, http, ssh...) only by
> > adding an extra field. Furthermore, I keep `serviceType` unbound from the
> > system application. It could be useful in a future.
> > 
> > And I think is a very friendly API. ;)
> 
> Thanks your reply :)
> Sounds good to me, it's more flexible if there is needed to have some
> additional options.

I don't like the approach of having an open options parameters in this function because it makes complex to understand how the function works. We are mixing parameters that are not related, while the manifestURL is used to return the stats per application, the serviceType is used to return the system stats. I think we should try to make it simple and clear, 'getSamples' is growing and with your suggestions depending on the parameters it will return:

- Stats per app.
- System stats.
- Global stats.

Having fixed parameters makes easier the validation of those and keep the logic of the function more simple.

I like the suggestion of the 'detailOptions' if options help to filter the returning value, but here are used to change the semantic of the returned value. I prefer the other approach but if everybody is ok with 'detailsOptions'... Anyway, lets see what Gene says.

Comment 17

[John Shih]

(In reply to Albert [:albert] from comment #16)
> (In reply to John Shih from comment #15)
> > (In reply to Salvador de la Puente González [:salva] from comment #14)
> > > I suggest to add just one optional parameter named `detailOptions` which is
> > > an object with the following fields:
> > > 
> > > - manifestURL, if provided, refers to the application for which we want the
> > > statistics. If it is the keyword `system` it is interpreted as the system
> > > application. Other values are ignored or throw an explanatory error (as you
> > > want).
> > > 
> > > - serviceType, if provided, refers to a fine-grained type of traffic. In
> > > current implementation, it is ignored for any value of manifestURL except
> > > `system`. It must be one of the items returned by
> > > `getAvailableServiceTypes()`
> > > 
> > > I like the optional `detailOptions` object because it allow to add other
> > > kind of constrains (i.e. traffic protocol: ftp, https, http, ssh...) only by
> > > adding an extra field. Furthermore, I keep `serviceType` unbound from the
> > > system application. It could be useful in a future.
> > > 
> > > And I think is a very friendly API. ;)
> > 
> > Thanks your reply :)
> > Sounds good to me, it's more flexible if there is needed to have some
> > additional options.
> 
> I don't like the approach of having an open options parameters in this
> function because it makes complex to understand how the function works. We
> are mixing parameters that are not related, while the manifestURL is used to
> return the stats per application, the serviceType is used to return the
> system stats. I think we should try to make it simple and clear,
> 'getSamples' is growing and with your suggestions depending on the
> parameters it will return:
> 
> - Stats per app.
> - System stats.
> - Global stats.
> 
> Having fixed parameters makes easier the validation of those and keep the
> logic of the function more simple.
> 
> I like the suggestion of the 'detailOptions' if options help to filter the
> returning value, but here are used to change the semantic of the returned
> value. I prefer the other approach but if everybody is ok with
> 'detailsOptions'... Anyway, lets see what Gene says.

I agree with your point. Though 'detailsOptions' seems like a good way, we need to
see which approach can really make the API more friendly as well as clear.

Definitely need more inputs here :)

Comment 18

[Gene Lian (I already quit Mozilla)]

I vote for Salva's comment #14.

Comment 19

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

(In reply to Albert [:albert] from comment #16)
> 
> Having fixed parameters makes easier the validation of those and keep the
> logic of the function more simple.
> 
> I like the suggestion of the 'detailOptions' if options help to filter the
> returning value, but here are used to change the semantic of the returned
> value.

I agree with you. The difference is that I did not think we are changing the semantics of the returned value. It is always data usage, but I want to focus on 'system' and then in 'download manager'. Indeed, INMHO, the service type may  eventually disappear because the `download manager` should be a separated application with its own manifestURL but, currently, we have a lot of behaviour coupled with the system app.

> I prefer the other approach but if everybody is ok with
> 'detailsOptions'... Anyway, lets see what Gene says.

If voting for this solution, I propose to have only two with the same signature of current getSample():

 - getSamples(network, start, end [, manifestURL]) to do the same work we already do
 - getSytemSamples(network, start, end [, serviceType]) to do the same but with system

One last comment based on comment 3. The serviceType sounds great to add some semantics to the download. It is something like 'tagged data' but with the horizontal expansion of the table, don't you think your are coupling a lot Gecko with system app? And another question? How the system application "tags" the data consumed? I mean, how do you know the actor of a download y the download manager and not an OTA?

Comment 20

[John Shih]

(In reply to Salvador de la Puente González [:salva] from comment #19)
> (In reply to Albert [:albert] from comment #16)
> > 
> > Having fixed parameters makes easier the validation of those and keep the
> > logic of the function more simple.
> > 
> > I like the suggestion of the 'detailOptions' if options help to filter the
> > returning value, but here are used to change the semantic of the returned
> > value.
> 
> I agree with you. The difference is that I did not think we are changing the
> semantics of the returned value. It is always data usage, but I want to
> focus on 'system' and then in 'download manager'. Indeed, INMHO, the service
> type may  eventually disappear because the `download manager` should be a
> separated application with its own manifestURL but, currently, we have a lot
> of behaviour coupled with the system app.

Thanks for your information! It's very happy we are going to have it.
Actually I don't know about any progress of 'download manager' before you
mentioned here. ;)

Back to the discussion, I think we'll still need to add the category of seviceType to save some
types of traffic, which can not be identified by 'app'.
DM app can just help to remove the potential type of 'downloadings'. There should be some other types, such as 'tethering', the current case we have. I'll try to make the proposal more clear and also identify/define more types as follow-up bugs.
There is a similar work in iOS, please refers to http://d3gz38qkvyffk.cloudfront.net/2013/09/ios-as.jpg.

> > I prefer the other approach but if everybody is ok with
> > 'detailsOptions'... Anyway, lets see what Gene says.
> 
> If voting for this solution, I propose to have only two with the same
> signature of current getSample():
> 
>  - getSamples(network, start, end [, manifestURL]) to do the same work we
> already do
>  - getSytemSamples(network, start, end [, serviceType]) to do the same but
> with system
> 
> One last comment based on comment 3. The serviceType sounds great to add
> some semantics to the download. It is something like 'tagged data' but with
> the horizontal expansion of the table, don't you think your are coupling a
> lot Gecko with system app?

I assumed what you mean here is something like make traffic consumed by downloading can be classified by 'app'.
For example,
Downloaded  Traffic
Facebook     30mb
Youtube      20mb
Web Game     50mb
right?

Actually, what I intent to do is something like:
App     SystemType   Traffic
System  Tethering      3.5gb
System  OTA            40mb
System  ...

Browser ""            350mb
YouTube ""            200mb
App     ...

does it make sense to you?

> And another question? How the system application
> "tags" the data consumed? I mean, how do you know the actor of a download y
> the download manager and not an OTA?

If there is going to have DM app, I will surely leave the downloading traffic counted into it.
For how to identify the traffic, I'll do some further study on how download(app/OTA) works in Gecko/Necko layer.
If it followed the same rule as the current app's behavior (that is, it can be identified by manifestURL), I think it can be solved more easily.

Comment 21

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

Oh, I think there was a misunderstood here, I did not say there will be a Download Manager application but, IMO, it should be one trying to slim the system. But I think this is not the plan right now. Honestly, I don't know, I only think it should.

In the other hand, I understood you want to do:

App     SystemType   Traffic
System  Tethering      3.5gb
System  OTA            40mb
System  ...

That SystemType column is what you speak about in comment 3. What I was asking is how do you know what traffic belongs to each specified service? If it is somehow hardcoded, I think it is a bad idea because it introduces a dependency between Gaia and Gecko (Gecko knows about "services" in Gaia) but I'm not being critic nor destructive because I understand it is necessary and it can be the best option right now.

Thanks.

Comment 22

[John Shih]

(In reply to Salvador de la Puente González [:salva] from comment #21)
> Oh, I think there was a misunderstood here, I did not say there will be a
> Download Manager application but, IMO, it should be one trying to slim the
> system. But I think this is not the plan right now. Honestly, I don't know,
> I only think it should.
> 

I though that what you said was bug 926955 (I just found it by searching with 'Download Manager')
Never mind, at least I can keep tracking it.

> In the other hand, I understood you want to do:
> 
> App     SystemType   Traffic
> System  Tethering      3.5gb
> System  OTA            40mb
> System  ...
> 
> That SystemType column is what you speak about in comment 3. What I was
> asking is how do you know what traffic belongs to each specified service? If
> it is somehow hardcoded, I think it is a bad idea because it introduces a
> dependency between Gaia and Gecko (Gecko knows about "services" in Gaia) but
> I'm not being critic nor destructive because I understand it is necessary
> and it can be the best option right now.
> 
> Thanks.

Hmm, I think you are right. What you point out is actually the biggest concern
in this work (if I don't get it wrong). The design of FFOS would force us to add several hardcode to track traffic with
different 'service' (much similar to what we did in track per-app traffic). Moreover, there is currently no flexible way to let Gaia define 'services'. The implementation must be done in Gecko, so do the definition of serviceType. (is this what you mean "introduces dependency
"?)

The flow will be:
Gecko                                     Gaia
Track pre-defined 'Tethering' traffic     
                                          Obtain 'Tethering' from getAvailableServiceType
                               <--        getSamples by assigning 'Tethering'
Return 'Tethering' traffic      -->       Receive 'Tethering' traffic


Indeed, this is really not a good design, but I have no better idea for it. :(

Comment 23

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

(In reply to John Shih from comment #22)
> (is this what you mean "introduces dependency
> "?)
> 

Yes, that is what I mean. Now we are totally aligned. Thanks for the clarifications. ;)

IMO, the only "bad thing" is that pre-defined traffic but it can be solve or discussed in another bug to not add noise here.

Comment 24

[John Shih]

Let me conclude the previous discussion.
There are two solutions:
1. add the detailsOptions, which would contains 'manifestURL', 'serviceType', and any possibly further options.
IDL definition
 - getSamples(network, start, end, [detailsOptions])

Pros: flexible
Cons: make the interface unclear

2. add another interface, says getSystemSamples (according to Comment 19).
IDL definition
 - getSamples(network, start, end [, manifestURL])
 - getSytemSamples(network, start, end [, serviceType])

little problem here, what will the result return if there is no optional parameter provided?
(That is, getSampes(network, start, end) and getSystemSamples(network, start, end))
So maybe there is needed to have three interfaces:
 - getIfaceSamples(network, start, end)
 - getAppSamples(network, start, end, manifestURL)
 - getSytemSamples(network, start, end, serviceType)

Again, a little bit trivial here..
So I'd like take solution 1 as current design.

Salva, is it ok with you?

Comment 25

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

Solution 1 is ok for me. Thank you John!

Comment 26

[John Shih]

Attachment: Bug 922926 - Part 1: IDL modifications.

IDL changes based on previous discussion.

Comment 27

[John Shih]

Attachment: Bug 922926 - Part 2: Implementation. r=gene

Comment 28

[John Shih]

Attachment: Bug 922926 - Part 3: Rebase code in necko.

Comment 29

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

Comment on attachment 8339087 [details] [diff] [review]
Bug 922926 - Part 1: IDL modifications.

Oh, now I see your concerns about mixing the `serviceType` with `manifestURL`. Note in my original proposal I said we can pass the denormalized `manifestURL='system'` (not `null`, the string is more meaningful) to ask for the `system` data usage.

This way, note how `serviceType` is naturally a refinement upon `manifestURL`. So, if `serviceType` is distinct to undefined, then `manifestURL` is equal to the string 'system' (mandatory).

In other words, you can provide both `manifestURL` and `serviceType` in the detailOptions. Rules are simple:

* manifestURL is not provided:
 - All statistics by interface are returned
 - serviceType is ignored

* manifestURL is provided and it is NOT the special word 'system':
 - All statistics for that APP by interface are returned
 - serviceType is ignored

* manifestURL is provided and it IS the special word 'system':
 - If not `serviceType`, the aggregation of all system statistics by interface are returned
 - If `serviceType`, only that service is returned

I insist, this way we are normalizing the cases, we are not returning mixed data. In a future even serviceType could become a refinement for some manifests distinct that `system`.

If I did not explain myself clearly, do not doubt to ping me. :)

Comment 30

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

Comment on attachment 8339087 [details] [diff] [review]
Bug 922926 - Part 1: IDL modifications.

Sorry, I don't know why I set the r flag to -. Restoring.

Comment 31

[John Shih]

(In reply to Salvador de la Puente González [:salva] from comment #29)
> Comment on attachment 8339087 [details] [diff] [review]
> Bug 922926 - Part 1: IDL modifications.
> 
> Oh, now I see your concerns about mixing the `serviceType` with
> `manifestURL`. Note in my original proposal I said we can pass the
> denormalized `manifestURL='system'` (not `null`, the string is more
> meaningful) to ask for the `system` data usage.
> 
> This way, note how `serviceType` is naturally a refinement upon
> `manifestURL`. So, if `serviceType` is distinct to undefined, then
> `manifestURL` is equal to the string 'system' (mandatory).
> 
> In other words, you can provide both `manifestURL` and `serviceType` in the
> detailOptions. Rules are simple:
> 
> * manifestURL is not provided:
>  - All statistics by interface are returned
>  - serviceType is ignored
> 
> * manifestURL is provided and it is NOT the special word 'system':
>  - All statistics for that APP by interface are returned
>  - serviceType is ignored
> 
> * manifestURL is provided and it IS the special word 'system':
>  - If not `serviceType`, the aggregation of all system statistics by
> interface are returned
>  - If `serviceType`, only that service is returned
> 
> I insist, this way we are normalizing the cases, we are not returning mixed
> data. In a future even serviceType could become a refinement for some
> manifests distinct that `system`.
> 
> If I did not explain myself clearly, do not doubt to ping me. :)

I'm fine with this design. So basically the structure remains the same with just different rules, right?
I will modified the comments. Thanks for your feedback!

Comment 32

[John Shih]

(In reply to John Shih from comment #31)
> (In reply to Salvador de la Puente González [:salva] from comment #29)
> > Comment on attachment 8339087 [details] [diff] [review]
> > Bug 922926 - Part 1: IDL modifications.
> > 
> > Oh, now I see your concerns about mixing the `serviceType` with
> > `manifestURL`. Note in my original proposal I said we can pass the
> > denormalized `manifestURL='system'` (not `null`, the string is more
> > meaningful) to ask for the `system` data usage.
> > 
> > This way, note how `serviceType` is naturally a refinement upon
> > `manifestURL`. So, if `serviceType` is distinct to undefined, then
> > `manifestURL` is equal to the string 'system' (mandatory).
> > 
> > In other words, you can provide both `manifestURL` and `serviceType` in the
> > detailOptions. Rules are simple:
> > 
> > * manifestURL is not provided:
> >  - All statistics by interface are returned
> >  - serviceType is ignored
> > 
> > * manifestURL is provided and it is NOT the special word 'system':
> >  - All statistics for that APP by interface are returned
> >  - serviceType is ignored
> > 
> > * manifestURL is provided and it IS the special word 'system':
> >  - If not `serviceType`, the aggregation of all system statistics by
> > interface are returned
> >  - If `serviceType`, only that service is returned
> > 
> > I insist, this way we are normalizing the cases, we are not returning mixed
> > data. In a future even serviceType could become a refinement for some
> > manifests distinct that `system`.
> > 
> > If I did not explain myself clearly, do not doubt to ping me. :)
> 
> I'm fine with this design. So basically the structure remains the same with
> just different rules, right?
> I will modified the comments. Thanks for your feedback!

By the way, the system app actually has its own manifestURL.
Maybe it can help the design more clear:
* If system app's manifestURL is provided, the serviceType can be further assigned to obtain
  specified network stats.

But the concern would be the difference between 'system app' in gaia (part of b2g process) and actual 'system' (whole b2g process).
If we can simply take these two as the same, then I think my proposal can work.

Comment 33

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

One question: where the functionality related to tethering, download manager, OTA... is placed? If it is in the Gaia System App, then let us provide the system manifest as you are saying and then we have a totally consistent model.

Indeed, I was thinking you were referring the Gaia's System App when you talked about the system cause AFAIK, there are no different applications for each serviceType so all this usage must come from the Gaia's System app. I was suggesting the use of 'system' keyword because I thought it was not possible to obtain the manifest of the Gaia's System APP.

In few words, If we can warranty all the serviceType-tracked usage comes from the Gaia's System APP, let us provide the system's manifest instead of 'system' keyword. Just as you says.

Comment 34

[John Shih]

(In reply to Salvador de la Puente González [:salva] from comment #33)
> One question: where the functionality related to tethering, download
> manager, OTA... is placed? If it is in the Gaia System App, then let us
> provide the system manifest as you are saying and then we have a totally
> consistent model.

> 
> Indeed, I was thinking you were referring the Gaia's System App when you
> talked about the system cause AFAIK, there are no different applications for
> each serviceType so all this usage must come from the Gaia's System app. I
> was suggesting the use of 'system' keyword because I thought it was not
> possible to obtain the manifest of the Gaia's System APP.
> 
> In few words, If we can warranty all the serviceType-tracked usage comes
> from the Gaia's System APP, let us provide the system's manifest instead of
> 'system' keyword. Just as you says.

Actually it's hard to define if the place of these functionality is in Gaia System App or not.
I would say Gaia System App may be involved with part of the process, but there is no direct way to connect/map the functionality to System App in gecko, where is the implementation placed. (For example, we can not obtain System App's manifestURL or appId when track the traffic of the functionality, although we're sure it is the system traffic)

However, there is no conflict if we take Gaia System App as 'whole' system, just as what you thought, even if it may not that clear from the view of Gecko side.

Another thing you pointed out was important - use 'system' keyword can make the API much more simple can clear, because developers don't need to get the manifestURL first. So here I proposed another design (I've also posted in the mail):

Add one more parameter, says statsType, which is used to specify 'app' or 'system'.
   We can also further integrate manifestURL and serviceType to one parameter (says statsTarget), make rules become:
   * if statsType is 'app', then statsTarget is taken as manifestURL.
   * if statsType is 'system', then statsTarget is taken as serviceType.

what do you think?

Comment 35

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

I think it adds unnecessary complexity. Let's keep the things simple:

- If manifestURL is provided and it is not 'system' keyword, ignore serviceType.
- If manifestURL is the keyword 'system', attend serviceType if provided.

The only improvement I suggest here is a rename of `manifestURL` into `application`.

This way it reads:

`getSamples` for `network` from `start` to `end` focusing on `application app://browser.gaiamobile.org/manifest.webapp`

or

`getSamples` for `network` from `start` to `end` focusing on `application system and serviceType tethering`

Quite expressive. Isn't it?

Comment 36

[John Shih]

(In reply to Salvador de la Puente González [:salva] from comment #35)
> I think it adds unnecessary complexity. Let's keep the things simple:
> 
> - If manifestURL is provided and it is not 'system' keyword, ignore
> serviceType.
> - If manifestURL is the keyword 'system', attend serviceType if provided.
> 
> The only improvement I suggest here is a rename of `manifestURL` into
> `application`.
> 
> This way it reads:
> 
> `getSamples` for `network` from `start` to `end` focusing on `application
> app://browser.gaiamobile.org/manifest.webapp`
> 
> or
> 
> `getSamples` for `network` from `start` to `end` focusing on `application
> system and serviceType tethering`
> 
> Quite expressive. Isn't it?

Sounds good to me. If Gene and Albert can both agree with this, I'll take it as the final design.
Thanks for your feedback :)

Comment 37

[Albert [:albert]]

(In reply to John Shih from comment #36)
> (In reply to Salvador de la Puente González [:salva] from comment #35)
> > I think it adds unnecessary complexity. Let's keep the things simple:
> > 
> > - If manifestURL is provided and it is not 'system' keyword, ignore
> > serviceType.
> > - If manifestURL is the keyword 'system', attend serviceType if provided.
> > 
> > The only improvement I suggest here is a rename of `manifestURL` into
> > `application`.
> > 
> > This way it reads:
> > 
> > `getSamples` for `network` from `start` to `end` focusing on `application
> > app://browser.gaiamobile.org/manifest.webapp`
> > 
> > or
> > 
> > `getSamples` for `network` from `start` to `end` focusing on `application
> > system and serviceType tethering`
> > 
> > Quite expressive. Isn't it?
> 
> Sounds good to me. If Gene and Albert can both agree with this, I'll take it
> as the final design.
> Thanks for your feedback :)

What about Gene's propossal?

Dictionary NetworkStatsDetailOptions
{
  DOMString type;   // 'app' or 'system'
  DOMString source; // manifuestURL or serviceType
};

Seems easier to understand.

Comment 38

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

I did not see Gene's answer in the list. I provided my own answer there. Summarizing, given the service type is another way for aggregating data, I agree with all of you and specifically with Gene's opinion and I suggest an alternative syntax (just renaming things).

You can see the complete answer in the mail thread.

Comment 39

[John Shih]

Hi Gene, Albert,
let's have a final conclusion for IDL change.

There are two options:
1.
DOMString type; // 'app' or 'system'. default is set to 'app', if |type| isn't provided
DOMString soruce; // manifestURL or serviceType

2.
DOMString by; // 'app' or 'system'. default is set to 'app', if |by| isn't provided
DOMString matching; // manifestURL or serviceType

You can check more detail discussion in mail ;)

Thanks!

Comment 40

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

Yep, just to copy & paste my portion of the discussion:

Dictionary NetworkStatsAggregationOptions
{
  DOMString by;   // 'application' or 'serviceType' (or simply 'service')
  DOMString matching;   // manifuestURL or serviceType
};

I know this scapes from a traditional approach but they read quite clear.
It is just a suggestion:

  getSamples(network, start, end, { by: 'application', matching: 'app://browser.gaiamobile.org/manifest.webapp' })
  getSamples(network, start, end, { by: 'service', matching: 'OTA' })

For usability, I did not force 'by' to be mandatory and make it defaults to 'application' This way, you don't force the developer to provide two parameters each time he is asking for a specific application usage and it continue reading quite good:

  getSamples(network, start, end, { matching: 'app://browser.gaiamobile.org/manifest.webapp' })

Comment 41

[Albert [:albert]]

I like Salva's approach regarding by/matching for this particular case because it makes gaia code more easy to read, but as Gene said it is not generic and could need to be changed if we add new fields to the dictionary. So I vote for:

Dictionary NetworkStatsGetOptions
{
  DOMString type;   // 'app' or 'system'. default is set to 'app', if |type| isn't provided
  DOMString source; // manifestURL or serviceType
}

Comment 42

[Albert [:albert]]

Comment on attachment 8339087 [details] [diff] [review]
Bug 922926 - Part 1: IDL modifications.

I remove the flag until IDL discussion is done.

Comment 43

[Albert [:albert]]

Comment on attachment 8339089 [details] [diff] [review]
Bug 922926 - Part 2: Implementation. r=gene

I remove the flag until IDL discussion is completed.

Comment 44

[John Shih]

Attachment: Bug 922926 - Part 2: Implementation. r=gene, albert

patch update.

Comment 45

[John Shih]

Comment on attachment 8339087 [details] [diff] [review]
Bug 922926 - Part 1: IDL modifications.

Since we made a final decision to back to original design (as this patch did), please re-check the patch. Thanks.

Comment 46

[John Shih]

Attachment: Bug 922926 - Part 1: IDL modifications.

Comment 47

[John Shih]

Attachment: Bug 922926 - Part 1: IDL modifications.

Comment 48

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8342179 [details] [diff] [review]
Bug 922926 - Part 1: IDL modifications.

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

::: dom/network/interfaces/nsIDOMNetworkStats.idl
@@ +24,5 @@
>    readonly attribute DOMString    manifestURL;
>  
>    /**
> +   * Service Type is used to retrieve the corresponding "system-only" stats.
> +   * e.g., "Tethering", "OTA", etc.

s/Type/type
s/e.g.,/E.g.,/

::: dom/network/interfaces/nsIDOMNetworkStatsManager.idl
@@ +6,5 @@
>  
>  interface nsIDOMDOMRequest;
>  
>  /**
> + * Provide the detail options for specifying different kinds of data in

s/detail/detailed/
s/data/data filtering/

@@ +7,5 @@
>  interface nsIDOMDOMRequest;
>  
>  /**
> + * Provide the detail options for specifying different kinds of data in
> + * getSampes function.

s/getSampes/getSamples

@@ +12,5 @@
> + */
> +dictionary NetworkStatsGetOptions
> +{
> +  /**
> +   * Manifest url is used to filter network stats by app, while service type

s/url/URL

@@ +14,5 @@
> +{
> +  /**
> +   * Manifest url is used to filter network stats by app, while service type
> +   * is used to filter stats by system service.
> +   * Note that, these two options can not be provided at the same time.

* Note that these two options cannot be specified at the same time for now;
* otherwise, an NS_ERROR_NOT_IMPLEMENTED exception will be thrown.

@@ +47,5 @@
>    /**
>     * Find samples between two dates start and end, both included.
>     *
> +   * If options is provided, per-app or per-system service usage is retrieved,
> +   * otherwise the target will be overall system usage.

* If options is provided, per-app or per-system service usage will be retrieved;
* otherwise the target will just be overall system usage.

::: dom/network/interfaces/nsINetworkStatsServiceProxy.idl
@@ +33,5 @@
>                      in nsINetworkInterface aNetwork,
>                      in unsigned long long aTimeStamp,
>                      in unsigned long long aRxBytes,
>                      in unsigned long long aTxBytes,
> +                    in boolean aIsAccumulative,

Just a reminder this change will also need to fix the callers.

Comment 49

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

Comment on attachment 8342179 [details] [diff] [review]
Bug 922926 - Part 1: IDL modifications.

Ok, just a recommendation when describing the API for Gaia developers: please let's clarify system is not the System App but the Gecko platform. I think this prevent the developer for thinking that, when omitting manifestURL he is querying the System App.

And just a nit:
 - I really liked `application` instead of `manifestURL` because is a higher lever concept but not blocking on this.

Thank you Josh for your work, effort and patience.

Comment 50

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8342128 [details] [diff] [review]
Bug 922926 - Part 2: Implementation. r=gene, albert

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

Looks very very nice! I can almost give you review+ but I hope to take another look after the minor nits are fixed. :)

::: dom/network/src/NetworkStatsDB.jsm
@@ +16,5 @@
>  Cu.importGlobalProperties(["indexedDB"]);
>  
>  const DB_NAME = "net_stats";
> +const DB_VERSION = 5;
> +const STORE_NAME = "net_stats"; // Deprecated. Use "net_stats_v5" instead.

s/"net_stats_v5"/STATS_STORE_NAME

@@ +17,5 @@
>  
>  const DB_NAME = "net_stats";
> +const DB_VERSION = 5;
> +const STORE_NAME = "net_stats"; // Deprecated. Use "net_stats_v5" instead.
> +const STORE_NAME_V5 = "net_stats_v5";

s/STORE_NAME_V5/STATS_STORE_NAME
s/"net_stats_v5"/"net_stats_store"

@@ +30,5 @@
>  this.NetworkStatsDB = function NetworkStatsDB() {
>    if (DEBUG) {
>      debug("Constructor");
>    }
> +  this.initDBHelper(DB_NAME, DB_VERSION, [STORE_NAME_V5]);

s/STORE_NAME_V5/STATS_STORE_NAME

@@ +43,5 @@
>      }
>      function errorCb(error) {
>        txnCb(error, null);
>      }
> +    return this.newTxn(txn_type, STORE_NAME_V5, callback, successCb, errorCb);

s/STORE_NAME_V5/STATS_STORE_NAME

@@ +124,5 @@
> +      } else if (currVersion == 4) {
> +        // In contrast to "per-app" traffic data, "system-only" traffic data
> +        // refers to data which can not be identified by any applications.
> +        // To further support "system-only" data storage, the data can be
> +        // saved by service type (e.g., Tethering, OTA). Thus there is needed

s/there is needed/it's needed/

@@ +127,5 @@
> +        // To further support "system-only" data storage, the data can be
> +        // saved by service type (e.g., Tethering, OTA). Thus there is needed
> +        // to have a new key ("serviceType") for the ojectStore.
> +        let newObjectStore;
> +        newObjectStore = db.createObjectStore(STORE_NAME_V5,

s/STORE_NAME_V5/STATS_STORE_NAME

@@ +274,5 @@
>        debug("New: " + aNewSample.timestamp + " - Last: " +
>              lastSample.timestamp + " - diff: " + diff);
>      }
>  
> +    // If incoming data has a accumulation feature, the new

// If the incoming data is accumulative, the new

@@ +280,2 @@
>      // |txTotalBytes|/|rxTotalBytes| and the last |txTotalBytes|/|rxTotalBytes|.
> +    // Else, if incoming data is non-accumulative, the |txBytes|/|rxBytes|

s/incoming/the incoming/

@@ +293,5 @@
>  
>      if (diff == 1) {
>        // New element.
>  
> +      // If the incoming data is non-accumulative, new

s/new/the new/

@@ +294,5 @@
>      if (diff == 1) {
>        // New element.
>  
> +      // If the incoming data is non-accumulative, new
> +      // |rxTotalBytes|/|txTotalBytes| needs to be obtained by adding new

s/obtained/updated/

@@ +295,5 @@
>        // New element.
>  
> +      // If the incoming data is non-accumulative, new
> +      // |rxTotalBytes|/|txTotalBytes| needs to be obtained by adding new
> +      // |rxBytes|/|txBytes| to last |rxTotalBytes|/|txTotalBytes|.

s/last/the last/

@@ +341,5 @@
>  
>        lastSample.rxBytes += aNewSample.rxBytes;
>        lastSample.txBytes += aNewSample.txBytes;
>  
> +      // If incoming data is accumulative, last |rxTotalBytes|/|txTotalBytes|

s/incoming/the incoming/
s/last/the last/

@@ +342,5 @@
>        lastSample.rxBytes += aNewSample.rxBytes;
>        lastSample.txBytes += aNewSample.txBytes;
>  
> +      // If incoming data is accumulative, last |rxTotalBytes|/|txTotalBytes|
> +      // needs to get updated by replacing new |rxTotalBytes|/|txTotalBytes|.

s/new/the new/

@@ +348,4 @@
>          lastSample.rxTotalBytes = aNewSample.rxTotalBytes;
>          lastSample.txTotalBytes = aNewSample.txTotalBytes;
>        } else {
> +        // Else, the incoming data is non-accumulative, old |rxTotalBytes|/

s/old/the old/

@@ +352,2 @@
>          // |txTotalBytes| needs to get updated by adding the new
>          // |rxBytes|/|txBytes| to last |rxTotalBytes|/|txTotalBytes|.

s/last/the last/

@@ +479,5 @@
>      let start = this.normalizeDate(aStart);
>      let end = this.normalizeDate(aEnd);
>  
>      if (DEBUG) {
> +      debug("Find samples for appId: " + aAppId + " serviceType " +

s/serviceType/serviceType:/

@@ +480,5 @@
>      let end = this.normalizeDate(aEnd);
>  
>      if (DEBUG) {
> +      debug("Find samples for appId: " + aAppId + " serviceType " +
> +            aServiceType + " network " + JSON.stringify(aNetwork) + " from " +

s/network/network:/

::: dom/network/src/NetworkStatsManager.js
@@ +160,5 @@
> +    let manifestURL = null;
> +    let serviceType = null;
> +    if (aOptions) {
> +      if (aOptions.manifestURL && aOptions.serviceType) {
> +        throw Components.results.NS_ERROR_INVALID_ARG;

I'd prefer:

s/NS_ERROR_INVALID_ARG/NS_ERROR_NOT_IMPLEMENTED

for now.

::: dom/network/src/NetworkStatsService.jsm
@@ +270,5 @@
>          return;
>        }
>      }
>  
> +    let serviceType = msg.serviceType;

let serviceType = msg.serviceType || "";

@@ +271,5 @@
>        }
>      }
>  
> +    let serviceType = msg.serviceType;
> +    if (!serviceType) {

and drop this if-block.

@@ +524,5 @@
>      });
>    },
>  
>    /*
> +   * Function responsible for receiving stats not from netd.

... which are not from netd.

@@ +542,3 @@
>            aTimeStamp + " " + aRxBytes + " " + aTxBytes);
>  
> +    // Check if and |aConnectionType|, |aAppId| and |aServiceType| are valid.

Drop the first "and".

@@ +545,5 @@
> +    // There are two invalid cases for the combination of |aAppId| and
> +    // |aServiceType|:
> +    // a. Both |aAppId| is non-zero and |aServiceType| is non-empty.
> +    // b. Both |aAppId| is zero and |aServiceType| is empty.
> +    if (!this._networks[netId] || aAppId && aServiceType ||

s/aAppId && aServiceType/(aAppId && aServiceType)
s/!aAppId && !aServiceType/(!aAppId && !aServiceType)

Comment 51

[Gene Lian (I already quit Mozilla)]

(In reply to Salvador de la Puente González [:salva] from comment #49)
> And just a nit:
>  - I really liked `application` instead of `manifestURL` because is a higher
> lever concept but not blocking on this.

Sounds great but "appManifestURL" is better. Please search "appManifestURL" and we can see lots of examples in the DOM APIs. However, we've already used "manifestURL" in [1]. Can we also change it as well without breaking the compatibility with Gaia?

[1] http://mxr.mozilla.org/mozilla-central/source/dom/network/interfaces/nsIDOMNetworkStats.idl#24

Comment 52

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

AFAIK, there is no applications using the `manifestURL` parameter (maybe some tests in Gecko but even Cost Control is not using it yet).

If `appManifestURL` is better, go on but it is more abstract to refer it simply as application. Tomorrow, instead of a string being the manifest we have an object representing the app? It does not loos the sense. But I start to feeling Josh is gonna kill me if I make more changes, xD

I agree with you.

Comment 53

[Gene Lian (I already quit Mozilla)]

(In reply to Salvador de la Puente González [:salva] from comment #52)
> AFAIK, there is no applications using the `manifestURL` parameter (maybe
> some tests in Gecko but even Cost Control is not using it yet).

Sounds great! Let's s/manifestURL/appManifestURL for nsIDOMNetworkStats.idl in this bug as well. ;)

> But I start to feeling Josh is gonna kill me if I make more changes, xD

No worries! John (btw, not Josh XD) is a nice guy! Thanks John for all the efforts and thanks Salva for all the inputs!

Comment 54

[John Shih]

(In reply to Salvador de la Puente González [:salva] from comment #52)
> AFAIK, there is no applications using the `manifestURL` parameter (maybe
> some tests in Gecko but even Cost Control is not using it yet).
>

I think it's mostly due to there is currently no API provides per-app information.
Back to naming, using 'application' could cause confusion because there is already App Object [1] in gaia.
According to [1], 'manifestURL' is clearly defined as one of attributes in App Object.
Unless we take App Object as input, I still prefer to use 'manifestURL'.
(There will be security concerns if we use App Object as input. It also make me think that we should
provide a interface for getting availableManfestURL for database, so that developers don't need to get
the high-sensitive permission 'webapps-manage')

 
> If `appManifestURL` is better, go on but it is more abstract to refer it
> simply as application. Tomorrow, instead of a string being the manifest we
> have an object representing the app? It does not loos the sense. But I start
> to feeling Josh is gonna kill me if I make more changes, xD

It's ok! Gaia's side input is definitely important on IDL definition ;)

> 
> I agree with you.

However, if 'appManifestURL' is clearer than 'manifestURL', then I think we take use it.
But should 'serviceType' also change to 'systemServiceType'?

Comment 55

[John Shih]

[1] https://developer.mozilla.org/en-US/docs/Web/API/App

Comment 56

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

(In reply to John Shih from comment #54)
> However, if 'appManifestURL' is clearer than 'manifestURL', then I think we
> take use it.

Go on then.

> But should 'serviceType' also change to 'systemServiceType'?

No, please, `serviceType` is ok. As I said before, there is already a System app and we don't want to cause confusion about what system we are referring.

Comment 57

[Albert [:albert]]

Comment on attachment 8342179 [details] [diff] [review]
Bug 922926 - Part 1: IDL modifications.

Looks good to me. Finally we can reach an agreement, nice!

Comment 58

[Albert [:albert]]

Comment on attachment 8342179 [details] [diff] [review]
Bug 922926 - Part 1: IDL modifications.

I forgot the +

Comment 59

[Albert [:albert]]

Comment on attachment 8342128 [details] [diff] [review]
Bug 922926 - Part 2: Implementation. r=gene, albert

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

Looks fine.

However I have a concern with the 'isAccumulative' in the database because we have two fields per record that already have the 'accumulative' meaning. For one sample, on one side we have rx/tx bytes storing the bytes consumed since last sample and, on the other side, we have rx/tx TotalBytes storing the accumulation. So we do we need 'isAccumulative'? Can you clarify me?

::: dom/network/src/NetworkStatsService.jsm
@@ +134,5 @@
>        case "NetworkStats:GetAvailableNetworks":
>          this.getAvailableNetworks(mm, msg);
>          break;
> +      case "NetworkStats:GetAvailableServiceTypes":
> +        this.GetAvailableServiceTypes(mm, msg);

s/GetAvailableServiceTypes/getAvailableServiceTypes:/

@@ +238,5 @@
>                            { id: msg.id, error: aError, result: aResult });
>      });
>    },
>  
> +  GetAvailableServiceTypes: function GetAvailableServiceTypes(mm, msg) {

s/GetAvailableServiceTypes/getAvailableServiceTypes:/

Comment 60

[John Shih]

(In reply to Albert [:albert] from comment #59)
> Comment on attachment 8342128 [details] [diff] [review]
> Bug 922926 - Part 2: Implementation. r=gene, albert
> 
> Review of attachment 8342128 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks fine.
> 
> However I have a concern with the 'isAccumulative' in the database because
> we have two fields per record that already have the 'accumulative' meaning.
> For one sample, on one side we have rx/tx bytes storing the bytes consumed
> since last sample and, on the other side, we have rx/tx TotalBytes storing
> the accumulation. So we do we need 'isAccumulative'? Can you clarify me?
> 

This flag is basically set for handling tethering traffic, which is also obtained from netd and thus similar to per-interface traffic. That is, the stats is accumulative once the connection is activated.
As a result, only use appId to distinguish how to handle accumulative/non-accumulative seems not reasonable.
After several discussion with Gene, we decide to have this flag, which can also improve the flexibility if there is more of accumulative data in the future.

Comment 61

[Albert [:albert]]

Thanks for clarify!

But currently it is not being used, all calls in the patch rebasing gecko are using false for 'isAccummulative' and 'saveServiceStats' is neither called.

If tethering data is obtained from netd, we could obtain it using the logic that we have already in the networkStats API because it is optimized to avoid a lot of calls to the database, just updating stats when clients make a request. Does it makes sense?

Comment 62

[John Shih]

(In reply to Albert [:albert] from comment #61)
> Thanks for clarify!
> 
> But currently it is not being used, all calls in the patch rebasing gecko
> are using false for 'isAccummulative' and 'saveServiceStats' is neither
> called.
> 
> If tethering data is obtained from netd, we could obtain it using the logic
> that we have already in the networkStats API because it is optimized to
> avoid a lot of calls to the database, just updating stats when clients make
> a request. Does it makes sense?

You are right, tethering data will be handled in a similar way as interface data (using networkstatsAvailable function). But in addition to that, other service data are more possible to be a non-accumulative data (I forgot to mention that ;) ). So it is also prepared for the follow-up works.

Comment 63

[John Shih]

> 
> ::: dom/network/interfaces/nsINetworkStatsServiceProxy.idl
> @@ +33,5 @@
> >                      in nsINetworkInterface aNetwork,
> >                      in unsigned long long aTimeStamp,
> >                      in unsigned long long aRxBytes,
> >                      in unsigned long long aTxBytes,
> > +                    in boolean aIsAccumulative,
> 
> Just a reminder this change will also need to fix the callers.

The callers will be fixed in part 3 & 4 (Rebase necko and test codes)

Comment 64

[John Shih]

Attachment: Bug 922926 - Part 1: IDL modifications.

patch update.

Comment 65

[John Shih]

Attachment: Bug 922926 - Part 2: Implementation. r=gene

patch update.

Comment 66

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8342928 [details] [diff] [review]
Bug 922926 - Part 2: Implementation. r=gene

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

Nice!

r=gene,albert

::: dom/network/src/NetworkStatsService.jsm
@@ +540,3 @@
>            aTimeStamp + " " + aRxBytes + " " + aTxBytes);
>  
> +    // Check if and |aConnectionType|, |aAppId|, |aServiceType| are valid.

No, I mean drop the "and" before |aConnectionType|:

// Check if |aConnectionType|, |aAppId| and |aServiceType| are valid.

Comment 67

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8342927 [details] [diff] [review]
Bug 922926 - Part 1: IDL modifications.

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

::: dom/network/interfaces/nsIDOMNetworkStatsManager.idl
@@ +47,5 @@
>  
>    /**
>     * Find samples between two dates start and end, both included.
>     *
> +   * If options is provided, per-app or per-system service usage wil be

s/wil/will/

@@ +49,5 @@
>     * Find samples between two dates start and end, both included.
>     *
> +   * If options is provided, per-app or per-system service usage wil be
> +   * retrieved;
> +   * otherwise the target will be overall system usage.

Please put this line directly after "... retrieved;".

Comment 68

[John Shih]

Attachment: Bug 922926 - Part 1: IDL modifications. r=gene

rebase for bug 858005.

Comment 69

[John Shih]

Attachment: Bug 922926 - Part 2: Implementation. r=gene, albert

Comment 70

[John Shih]

Attachment: Bug 922926 - Part 3: Rebase code in necko. r=mamanus

Comment 71

[John Shih]

Attachment: Bug 922926 - Part 4: Tests. r=gene, albert

Comment 72

[John Shih]

try server: https://tbpl.mozilla.org/?tree=Try&rev=c758ecc996ee

Comment 73

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8349210 [details] [diff] [review]
Bug 922926 - Part 2: Implementation. r=gene, albert

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

Nice!

::: dom/network/src/NetworkStatsDB.jsm
@@ +16,5 @@
>  Cu.importGlobalProperties(["indexedDB"]);
>  
>  const DB_NAME = "net_stats";
> +const DB_VERSION = 6;
> +const STORE_NAME = "net_stats"; // Deprecated. Use "net_stats_store" instead.

s/STORE_NAME/DEPRECATED_STATS_STORE_NAME

@@ +59,5 @@
>          /**
>           * Create the initial database schema.
>           */
>  
> +        objectStore = db.createObjectStore(STORE_NAME, { keyPath: ["connectionType", "timestamp"] });

Ditto.

@@ +78,5 @@
>          // [networkId, networkType] not just by their connectionType,
>          // to modify the keyPath is mandatory to delete the object store
>          // and create it again. Old data is going to be deleted because the
>          // networkId for each sample can not be set.
> +        db.deleteObjectStore(STORE_NAME);

Ditto.

@@ +83,2 @@
>  
> +        objectStore = db.createObjectStore(STORE_NAME, { keyPath: ["appId", "network", "timestamp"] });

Ditto.

@@ +95,5 @@
>            debug("Created object stores and indexes for version 3");
>          }
>        } else if (currVersion == 3) {
>          // Delete redundent indexes (leave "network" only).
> +        objectStore = aTransaction.objectStore(STORE_NAME);

Ditto.

@@ +124,5 @@
>          }
>        } else if (currVersion == 4) {
>          // In order to manage alarms, it is necessary to use a global counter
>          // (totalBytes) that will increase regardless of the system reboot.
> +        objectStore = aTransaction.objectStore(STORE_NAME);

Ditto.

@@ +195,5 @@
> +                         { keyPath: ["appId", "serviceType", "network", "timestamp"] });
> +        newObjectStore.createIndex("network", "network", { unique: false });
> +
> +        // Copy the data from the original objectStore to the new objectStore.
> +        objectStore = aTransaction.objectStore(STORE_NAME);

Ditto.

@@ +199,5 @@
> +        objectStore = aTransaction.objectStore(STORE_NAME);
> +        objectStore.openCursor().onsuccess = function(event) {
> +          let cursor = event.target.result;
> +          if (!cursor) {
> +            db.deleteObjectStore(STORE_NAME);

Ditto.

@@ +210,5 @@
> +          cursor.continue();
> +        };
> +
> +        if (DEBUG) {
> +          debug("Added new key for version 6");

"Added new key 'serviceType' for version 6"

@@ +349,3 @@
>      // |txTotalBytes|/|rxTotalBytes| and the last |txTotalBytes|/|rxTotalBytes|.
> +    // Else, if incoming data is non-accumulative, the |txBytes|/|rxBytes|
> +    // is directly the new |txBytes|/|rxBytes|.

Drop "directly".

Comment 74

[Albert [:albert]]

Comment on attachment 8349210 [details] [diff] [review]
Bug 922926 - Part 2: Implementation. r=gene, albert

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

Looks nice!

Just one comment, we need to fix indentation at NetworkStatsDB.js:247

let request = aStore.index("network").openCursor(stats.network, "prev");

Comment 75

[John Shih]

Attachment: Bug 922926 - Part 2: Implementation. r=gene, albert

r=gene, albert

Comment 76

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/d68cfbc4d762
https://hg.mozilla.org/integration/b2g-inbound/rev/f1b82c93b4fb
https://hg.mozilla.org/integration/b2g-inbound/rev/d4cb277a6e0e
https://hg.mozilla.org/integration/b2g-inbound/rev/3efcc5932d2c

Comment 77

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d68cfbc4d762
https://hg.mozilla.org/mozilla-central/rev/f1b82c93b4fb
https://hg.mozilla.org/mozilla-central/rev/d4cb277a6e0e
https://hg.mozilla.org/mozilla-central/rev/3efcc5932d2c

Comment 78

[Albert [:albert]]

nominating to v1.3? since it blocks bug 965319

Comment 79

[Albert [:albert]]

Attachment: Bug 922926 - Part 2: Implementation for 1.3.

Rebase for 1.3 in case that we get 1.3+.

Other patches are applied ok in 1.3

Comment 80

[John Shih]

Comment on attachment 8374669 [details] [diff] [review]
Bug 922926 - Part 2: Implementation for 1.3.

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

Thanks for your work!

Comment 81

[Joe Cheng [:jcheng] (please needinfo)]

triage; blocks 1.3+ blocker. 1.3+

Comment 82

[Albert [:albert]]

Patches to uplift:

Bug 922926 - Part 1: IDL modifications. r=gene
Bug 922926 - Part 2: Implementation for 1.3.
Bug 922926 - Part 3: Rebase code in necko. r=mamanus
Bug 922926 - Part 4: Tests. r=gene, albert

Comment 83

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/a201046ae8cb
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/71bc87ffccaa
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/19d78f35ff55
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/8045fe0f83d3

Comment 84

[[:fabrice] Fabrice Desré]

I'm really unhappy about that landing without asking sheriff (i.e me for 1.3) approval. Don't do that again for big risky patches as this one.

Comment 85

[Jason Smith [:jsmith]]

(In reply to Fabrice Desré [:fabrice] from comment #84)
> I'm really unhappy about that landing without asking sheriff (i.e me for
> 1.3) approval. Don't do that again for big risky patches as this one.

Should we backout? There's already huge churn on this API regressions-wise, so it might be better to get rid of this before we get regressions from this patch as well on 1.3.

Comment 86

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #85)
> (In reply to Fabrice Desré [:fabrice] from comment #84)
> > I'm really unhappy about that landing without asking sheriff (i.e me for
> > 1.3) approval. Don't do that again for big risky patches as this one.
> 
> Should we backout? There's already huge churn on this API regressions-wise,
> so it might be better to get rid of this before we get regressions from this
> patch as well on 1.3.

Discussed offline with Fabrice - this should have never landed on 1.3. blocking-. We need a backout here.

Comment 87

[Jason Smith [:jsmith]]

Discussed with Wes in IRC - he's going to back this out. Leaving needinfo on him for this.

Comment 88

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out of 1.3 in https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/832886eb6e7e

Comment 89

[Gene Lian (I already quit Mozilla)]

(In reply to Fabrice Desré [:fabrice] from comment #84)
> I'm really unhappy about that landing without asking sheriff (i.e me for
> 1.3) approval. Don't do that again for big risky patches as this one.

Yeap, I was also shocked why we uplift this without getting v1.3 first.

Anyway, the reason why we want to uplift this to v1.3 is because Bug 965319 is going to have a DB version change which also needs to be uplifted. Therefore, we have to uplift this first to make sure the DB version sequence is correct. Otherwise, it will cause disaster when migrating the DB version.

Just discussed with John, uplifting this doesn't harm since it's a new API feature and can be compatible with Gaia (Gaia hasn't apply the new API yet).

Let's ask for v1.3? again.

Comment 90

[Gene Lian (I already quit Mozilla)]

Just came up with an alternative, *maybe* we can just uplift the DB update part (by the v1.3 patch at Bug 965319) without including the real feature implementation for this bug. In this way, we don't really need to make this bug v1.3+.

Need Albert and John's help to analyze if this is feasible.

Comment 91

[Noemí Freire (:noemi)]

This bug blocks bug 965319 that is needed for tarako so asking for 1.3t?

Comment 92

[Joe Cheng [:jcheng] (please needinfo)]

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

Comment 93

[Joe Cheng [:jcheng] (please needinfo)]

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

Comment 94

[[:fabrice] Fabrice Desré]

That patch doesn't apply on 1.3t. Please rebase it (and also bugs 964270, 966244 and 965319 if needed).

Comment 95

[Albert [:albert]]

Attachment: Bug 922926 - Part 2: Implementation for 1.3t.

Rebase for 1.3t

Comment 96

[John Shih]

Thanks, Albert!

Comment 97

[[:fabrice] Fabrice Desré]

remote:   https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/de85ef1d4687
remote:   https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/7e9a267f2b46
remote:   https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/9d48fe66a024
remote:   https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/36e542d52a43

Comment 98

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8355133 [details] [diff] [review]
Bug 922926 - Part 2: Implementation. r=gene, albert

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

::: dom/network/src/NetworkStatsDB.jsm
@@ +681,5 @@
>      }, aResultCb);
>    },
>  
> +  getAvailableServiceTypes: function getAvailableServiceTypes(aResultCb) {
> +    this.dbNewTxn("readonly", function(aTxn, aStore) {

|this.dbNewTxn| has prototype |function dbNewTxn(store_name, txn_type, callback, txnCb)|.

Comment 99

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #98)
> Comment on attachment 8355133 [details] [diff] [review]
> Bug 922926 - Part 2: Implementation. r=gene, albert
> 
> Review of attachment 8355133 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/network/src/NetworkStatsDB.jsm
> @@ +681,5 @@
> >      }, aResultCb);
> >    },
> >  
> > +  getAvailableServiceTypes: function getAvailableServiceTypes(aResultCb) {
> > +    this.dbNewTxn("readonly", function(aTxn, aStore) {
> 
> |this.dbNewTxn| has prototype |function dbNewTxn(store_name, txn_type,
> callback, txnCb)|.

Filed bug 993834 for this.