746073 - Meter network usage per "web app" on HTTP layer

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Following on from bug 746069, we want to charge network usage *per installed app*.  The use case is to show UI that looks something like

 Facebook  Sent 5MB  | Received 100MB
 Twitter   Sent 10MB | Received 20MB

so that users know which apps are eating their data plans.

This is actually considerably trickier than bug 746069, in general.  (I think.)  We care about this most for b2g initially, and b2g will be multi-process.  That's nice because in that setup, all network traffic will go through a choke point at the PNecko interfaces.  And further, each PNecko can find an associated PBrowser pretty easily, which is bound to a domain / app-manifest for the installed web-app case.  From there, we just have to meter the usage and store it in a DB somewhere.  We also need the PNecko things to known which network interface they're talking through.  We might be able to re-use some existing web-app DB, adding some new columns.  Not sure.

AFAIK this would be considerably harder for the same-process case.  So we might want to punt the general case, initially.  (Necko folks might have better suggestions.)

After that, we have to expose API to read the usage data.  I don't have a strong opinion on how to do that.

Comment 1

[Honza Bambas (:mayhemer)]

We may collect data per-domain.  It's doable by inspecting http connection manager.  But probably not what you want.

We can then collect all requests by inspecting load groups (=doc shell =?web app) requests.  That will give you usage per-window, even in a single process case.

Comment 2

[Vincent Chang[:vchang][changyihsin]]

In ICS, it uses netfilter function xt_qtaguid to associate socket fd, tag name and user id. When application creates a socket or accepts a remote connection, the socket fd and user id will be associated with user defined tag. Then, we can find traffic statistic for this socket in below proc entries. 
 
/proc/net/xt_qtaguid/iface_stat/interface name/rx_bytes    
/proc/net/xt_qtaguid/iface_stat/interface name/rx_packets  
/proc/net/xt_qtaguid/iface_stat/interface name/tx_bytes
/proc/net/xt_qtaguid/iface_stat/interface name/tx_packets  

Maybe we can use the same mechanism if we can support multi-process per-apps ?

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

It's OK to use Gonk-specific mechanisms if we need to, but this is something that's useful on all platforms, so if we can already get high-quality enough data from Gecko internally it would be better.

Comment 4

[Gene Lian (I already quit Mozilla)]

I'm jumping into this bug. After some discussions with Shian-yow and Vincent, in the initiative step I'll try to work out a Gonk-specific solution. Hopefully, I can upload a WIP patch in a couple of weeks.

Comment 5

[Gene Lian (I already quit Mozilla)]

(In reply to Vincent Chang from comment #2)
> In ICS, it uses netfilter function xt_qtaguid to associate socket fd, tag
> name and user id. When application creates a socket or accepts a remote
> connection, the socket fd and user id will be associated with user defined
> tag. Then, we can find traffic statistic for this socket in below proc
> entries. 
>  
> /proc/net/xt_qtaguid/iface_stat/interface name/rx_bytes    
> /proc/net/xt_qtaguid/iface_stat/interface name/rx_packets  
> /proc/net/xt_qtaguid/iface_stat/interface name/tx_bytes
> /proc/net/xt_qtaguid/iface_stat/interface name/tx_packets  
> 
> Maybe we can use the same mechanism if we can support multi-process per-apps
> ?

After more investigations, I guess it's more correct to retrieve the per-app traffic statistics from /proc/net/xt_qtaguid/stats, which is supported in ICS (linux kernel 3.0). Some introductions regarding the kernel/net/netfilter/xt_qtaguid can be found at link [1]. In addition, it seems the netd socket doesn't support related commands for querying the traffic statistics (see link [2]), so we probably need to parse it ourself.

[1] http://source.android.com/tech/datausage/kernel-overview.html
[2] http://androidxref.com/4.0.4/xref/system/netd/CommandListener.cpp

Gene

Comment 6

[Gene Lian (I already quit Mozilla)]

(In reply to Honza Bambas (:mayhemer) from comment #1)
> We may collect data per-domain.  It's doable by inspecting http connection
> manager.  But probably not what you want.
> 
> We can then collect all requests by inspecting load groups (=doc shell =?web
> app) requests.  That will give you usage per-window, even in a single
> process case.

Hi Honza,

I'd like to ask some questions following up your comments. Could you please give us some quick hints or suggestions? Any words will be appreciated! :)

1. The |nsHttpConnectionMgr| you mentioned is used as a type of global instance (i.e. only a single manager would be created for each platform)? Or should be one instance for each connection? I mean... what is the exact moment that |nsHttpConnectionMgr| is being used?

2. Following up the previous question, how about |nsHttpProtocolHandler| and |nsHttpHandler|?

3. I saw there are some interesting variables like |mTotalBytesRead| and |mTotalBytesWritten| (please see |nsHttpConnection::PrintDiagnostics()|). Do these information have direct relations to what we're going to do?

4. My very first plan is to add new attributes as below, which can be exposed to the Network Stats APIs. In this way, the |nsHttpHandler::GetNumSentBytes()| and |nsHttpHandler::GetNumReceivedBytes()| can get the numbers from its |mConnMgr|. How do you feel?

  interface nsIHttpProtocolHandler : nsIProxiedProtocolHandler
  {
    ...
    readonly attribute unsigned long numSentBytes;
    readonly attribute unsigned long numReceivedBytes;
    ...
  };

Sorry for asking stupid questions if any (I'm still very new to Mozilla somehow) :)

Thanks,
Gene

Comment 7

[Honza Bambas (:mayhemer)]

(In reply to Gene Lian [:gene] from comment #6)
> (In reply to Honza Bambas (:mayhemer) from comment #1)
> > We may collect data per-domain.  It's doable by inspecting http connection
> > manager.  But probably not what you want.
> > 
> > We can then collect all requests by inspecting load groups (=doc shell =?web
> > app) requests.  That will give you usage per-window, even in a single
> > process case.
> 
> Hi Honza,
> 
> I'd like to ask some questions following up your comments. Could you please
> give us some quick hints or suggestions? Any words will be appreciated! :)
> 
> 1. The |nsHttpConnectionMgr| you mentioned is used as a type of global
> instance (i.e. only a single manager would be created for each platform)? Or
> should be one instance for each connection? I mean... what is the exact
> moment that |nsHttpConnectionMgr| is being used?

It is a global instance.  You cannot access it out of necko module right now, since it has no XPCOM interface and the header is not published.

But we can expose data on nsIHttpProtocolHandler interface that is on standard handler implementation for "http" and "https" scheme.

Side note: I would like to make that API async - i.e. get result via callback, since I really want to avoid adding any new locking to STS.

> 
> 2. Following up the previous question, how about |nsHttpProtocolHandler| and
> |nsHttpHandler|?

There is nsHttpHandler with nsIHttpProtocolHandler iface and accessible as "@mozilla.org/network/protocol;1?name=http"

We can modify nsIHttpProtocolHandler (add what you need) or have a new open interface to expose stat data on it (I vote for new iface).

> 
> 3. I saw there are some interesting variables like |mTotalBytesRead| and
> |mTotalBytesWritten| (please see |nsHttpConnection::PrintDiagnostics()|). Do
> these information have direct relations to what we're going to do?

Counters on nsHttpConnection are not precise.

I think you are more interested in nsSocketInputStream::mByteCount and nsSocketOutputStream::mByteCount.  Those are directly working with net sockets and cover all data I/O (excluding SSL overhead, though).  

Socket transports are referenced by socket transport service (although, through an abstract class) that can collect the stats.  Socket transport service is nsSocketTransportService with nsPISocketTransportService and accessible as "@mozilla.org/network/socket-transport-service;1".

A single socket transport instance represents a single TCP (+SSL) connection.  It has info about host name + port + if SSL is used.  We can group/query results by this info.

> 
> 4. My very first plan is to add new attributes as below, which can be
> exposed to the Network Stats APIs. In this way, the
> |nsHttpHandler::GetNumSentBytes()| and
> |nsHttpHandler::GetNumReceivedBytes()| can get the numbers from its
> |mConnMgr|. How do you feel?
> 
>   interface nsIHttpProtocolHandler : nsIProxiedProtocolHandler
>   {
>     ...
>     readonly attribute unsigned long numSentBytes;
>     readonly attribute unsigned long numReceivedBytes;
>     ...
>   };

Depends on what are the future plans, but for a global counter I would rather move this to the socket transport service.  

According title of this bug - have data "per web app" - I have a question, what is the "web app's" context?  WebRT?  Domain?


> 
> Sorry for asking stupid questions if any (I'm still very new to Mozilla
> somehow) :)
> 
> Thanks,
> Gene

No problem.  I like people asking intelligent questions more then those being quiet ;)

Comment 8

[Shian-Yow Wu [:swu]]

Can someone help to answer this question from Honza Bambas?
what is the "web app's" context?  WebRT?  Domain?

Thanks. :)

Comment 9

[Gene Lian (I already quit Mozilla)]

(In reply to Honza Bambas (:mayhemer) from comment #7)

Hi Honza,

Thanks for your detailed suggestions. They are very helpful. Really appreciate. :)

> According title of this bug - have data "per web app" - I have a question,
> what is the "web app's" context?  WebRT?  Domain?
> 

As my best understanding, the domain address can represent the unique identity of an web application in B2G. For example, the domain address of an Alarm-Clock is http://clock.gaiamobile.org/ in Gaia, which could be a useful grouping indicator to specify this app. Another possibility is something like http://clock.gaiamobile.org/manifest.webapp, which is the so-called manifestURL to specity the apps that has been installed in B2G. Note that it also has the same domain name.

I think if the domain name information can be caught and saved whenever a socket is opened by an app, it seems it's worth doing to meet our purpose (to specify per app context). Not familiar with the WebRT part. Could anyone elaborate more on that?

Thanks,
Gene

Comment 10

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Gene Lian [:gene] from comment #9)
> (In reply to Honza Bambas (:mayhemer) from comment #7)
> 
> Hi Honza,
> 
> Thanks for your detailed suggestions. They are very helpful. Really
> appreciate. :)
> 
> > According title of this bug - have data "per web app" - I have a question,
> > what is the "web app's" context?  WebRT?  Domain?
> > 
> 
> As my best understanding, the domain address can represent the unique
> identity of an web application in B2G. For example, the domain address of an
> Alarm-Clock is http://clock.gaiamobile.org/ in Gaia, which could be a useful
> grouping indicator to specify this app. Another possibility is something
> like http://clock.gaiamobile.org/manifest.webapp, which is the so-called
> manifestURL to specity the apps that has been installed in B2G. Note that it
> also has the same domain name.
> 
> I think if the domain name information can be caught and saved whenever a
> socket is opened by an app, it seems it's worth doing to meet our purpose
> (to specify per app context). Not familiar with the WebRT part. Could anyone
> elaborate more on that?
> 
> Thanks,
> Gene
Found another thread that is talking about identifying an app. It seems that Gaia is going to identify apps by manifest URL.
https://github.com/mozilla-b2g/gaia/issues/1915

Comment 11

[Honza Bambas (:mayhemer)]

I'm a bit worried that identifying apps just by the host name is good enough.

Comment 12

[[:fabrice] Fabrice Desré]

We definitely identify apps by their manifest url, not just the host name/origin. Using only the origin would prevent us to move to a model with several apps per origin, which is what we plan to do.

Comment 13

[Jason Duell]

:sicking or :mounir can probably answer questions about this better than me, but here's a start:

So I just landed bug 775860, which provides an integer "AppId" for necko channels (it's in the nsILoadContext of any necko channel that's loading content, and is accessible via the NS_GetAppInfo() in nsNetUtil.h).  There's at least a good chance that this is the App ID that you're looking for.  I'm not sure of how we map domains, app manifests, etc to the integer appID but I know it's supposed to be unique per app.

There's also a 'IsInBrowserElement' field that says whether the load is happening within a 'mozbrowser' frame of the app.  You may need to track based on that too, but only if you're trying to split out the difference between B2G Firefox as an app (i.e. the HTML, CSS, JS etc needed to implement a B2G browser) and the the web content that that browser loads.  I'm guessing you don't care about this, so it's just AppId.

Comment 14

[Gene Lian (I already quit Mozilla)]

(In reply to Jason Duell (:jduell) from comment #13)
> :sicking or :mounir can probably answer questions about this better than me,
> but here's a start:
> 
> So I just landed bug 775860, which provides an integer "AppId" for necko
> channels (it's in the nsILoadContext of any necko channel that's loading
> content, and is accessible via the NS_GetAppInfo() in nsNetUtil.h).  There's
> at least a good chance that this is the App ID that you're looking for.  I'm
> not sure of how we map domains, app manifests, etc to the integer appID but
> I know it's supposed to be unique per app.

Thanks Jason for the good point! I'd like to have some more questions about this:

1. Could you please briefly explain about the difference/relations between a "channel" and a "socket"?

2. I cannot find the function NS_GetAppInfo() in netwerk/base/public/nsNetUtil.h or anywhere. Could you please point it out?

----------
So far, I'm having a very rough plan to achieve this goal based on the previous comments:

1. Whenever a socket is opened, save the amount of reading/writing data corresponding to a given App ID in the database (supposing we can pass or refer to the App ID info in the socket connection).

2. Clients can query the data usage amount of an installed App through a given manifest URL. That is, manifestURL -> AppID -> auery #bytes in database (supposing we can construct a manifestURL-to-AppID mapping table).

Do the above 2 steps make sense? Please let me know if they're not feasible.

Comment 15

[Jason Duell]

NS_GetAppInfo is here:

  http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsNetUtil.h#1338

Channel vs Socket:   A channel represents a single HTTP/FTP etc transaction (for an HTTP redirect, for instance we open a new channel for the new URI we're redirected to).  They are protocol-specific (HTTP/FTP/Websockets/etc).

A socket is a connection to a TCP socket.  The socket transport is used by all the various protocols, and a socket may be reused by multiple channels (HTTP keep-alive), or in the case of websockets, start out as HTTP and Upgrade to Websockets.

It's not clear to me whether you're better off tracking bytes read/written at the socket or the protocol level--I'd guess protocol.   mcmanus and mayhemer are the ones to help design that.

Comment 16

[Gene Lian (I already quit Mozilla)]

(In reply to Jason Duell (:jduell) from comment #15)
> NS_GetAppInfo is here:
> 
>  
> http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsNetUtil.
> h#1338

Ah! I got it. It seems it's just been checked and is available now. Thank you!

> 
> Channel vs Socket:   A channel represents a single HTTP/FTP etc transaction
> (for an HTTP redirect, for instance we open a new channel for the new URI
> we're redirected to).  They are protocol-specific (HTTP/FTP/Websockets/etc).
> 
> A socket is a connection to a TCP socket.  The socket transport is used by
> all the various protocols, and a socket may be reused by multiple channels
> (HTTP keep-alive), or in the case of websockets, start out as HTTP and
> Upgrade to Websockets.
> 
> It's not clear to me whether you're better off tracking bytes read/written
> at the socket or the protocol level--I'd guess protocol.   mcmanus and
> mayhemer are the ones to help design that.

Thanks Jason for the clarification! Understand better :) It seems more reasonable to count the IO in the protocol level (i.e. nsHttpTransaction) if the sockets can be reused and shared.

Hi mcmanus and mayhemer - I'd appreciate if I could have your comments or suggestions to help the design. :) Currently, I'll try to pass the AppId into the nsHttpTransaction, aggregate the IO counts, and save them in the database. Please let me know if it doesn't sound reasonable to you.

Comment 17

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Hi Gene,

Could you record the interface that traffic flow through? For example,

  app     | traffic | interface | direction
  --------+---------+-----------+----------
  browser | 300 k   | wifi      | in

the interface information may be also used in 746074 for executing the network policy.

Thanks

Comment 18

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Attachment: Proposed IDL

As discussed in IRC, I added a new parameter to the IDL of 746069 in order to include the applicationDomain as a new parameter of NetworkStatsOptions to be used by this one.

The basic idea:

* If applicationDomain is empty, aggregated results by interface will be returned (like 746069)

* If applicationDomain is provided, application consume is returned

Comment 19

[Patrick McManus [:mcmanus]]

(In reply to Gene Lian [:gene] from comment #16)
> (In reply to Jason Duell (:jduell) from comment #15)

> Hi mcmanus and mayhemer - I'd appreciate if I could have your comments or
> suggestions to help the design. :) Currently, I'll try to pass the AppId
> into the nsHttpTransaction, aggregate the IO counts, and save them in the
> database. Please let me know if it doesn't sound reasonable to you.

if your appid is per transaction you'll need to record this information in nsHttpTransaction.. the connection and sockets are shared and under spdy even nultiplexed across the lifetime of a transaction.

read/write segments is probably the right place to count it

this means you'll count bodies and http overhead which isn't exactly the precise network byte count but it will be fairly close. (things that make it wrong: tcp/ip framing, ssl framing, and a potential reduction in header bytes through the spdy translation). You could get the accounting for that stuff right but it would be a pain (you'd need the nsAHttpConnection to correlate things and report it back to the transaction).

Comment 20

[Gene Lian (I already quit Mozilla)]

(In reply to Patrick McManus [:mcmanus] from comment #19)
> if your appid is per transaction you'll need to record this information in
> nsHttpTransaction.. the connection and sockets are shared and under spdy
> even nultiplexed across the lifetime of a transaction.
> 
> read/write segments is probably the right place to count it
> 

Thanks Patrick for the suggestions :). I'd like to share some log segments for this approach: counting the read/write segments per nsHttpTransaction by passing the appID and isInBrowserElement into it. Note that the appID = 0 is the result of Firefox and the other two (21 and 23) are web games.

   appID | isInBrowser | count | countRead | countWritten
  -------+-------------+-------+-----------+--------------
       0 |        true | 16384 |       514 |
       0 |        true | 16384 |         0 |
       0 |        true | 16384 |           |        1154
       0 |        true |   307 |       307 |
       0 |        true | 16384 |           |        2645
     ... |         ... |   ... |       ... |         ...
      21 |       false |   470 |       470 |
      21 |       false | 16384 |           |        1024
      21 |       false |   470 |       470 |
      21 |       false |   494 |       494 |
      21 |       false | 16384 |           |         962
     ... |         ... |   ... |       ... |
      23 |       false |   452 |       452 |
      23 |       false | 16384 |           |         185
      23 |       false |   456 |       456 |
      23 |       false | 16384 |           |        3676
      23 |       false |   461 |       461 |
     ... |         ... |   ... |       ... |

> this means you'll count bodies and http overhead which isn't exactly the
> precise network byte count but it will be fairly close. (things that make it
> wrong: tcp/ip framing, ssl framing, and a potential reduction in header
> bytes through the spdy translation). You could get the accounting for that
> stuff right but it would be a pain (you'd need the nsAHttpConnection to
> correlate things and report it back to the transaction).

Ah... thanks for sharing these. It sounds it's not that direct to count all of the numbers :O Not sure how precise we're going to pursue or in other words which standard we're going to adopt to meter the network traffic.

Another question: can we get the interface types (i.e. 3G or Wifi) in the nsHttpTransaction level or say, within the Necko? It seems we also need to count the bytes separately per interface for bug 746074.

Comment 21

[Patrick McManus [:mcmanus]]

(In reply to Gene Lian [:gene] from comment #20)
> (In reply to Patrick McManus [:mcmanus] from comment #19)
> > 
> Another question: can we get the interface types (i.e. 3G or Wifi) in the
> nsHttpTransaction level or say, within the Necko? It seems we also need to
> count the bytes separately per interface for bug 746074.

I don't know off the top of my head how to do it, but it has to be correlated to the socket being used and you can get to that from nshttptransaction->mConnection->Transport()

Comment 22

[Gene Lian (I already quit Mozilla)]

(In reply to Patrick McManus [:mcmanus] from comment #21)
> (In reply to Gene Lian [:gene] from comment #20)
> > (In reply to Patrick McManus [:mcmanus] from comment #19)
> > > 
> > Another question: can we get the interface types (i.e. 3G or Wifi) in the
> > nsHttpTransaction level or say, within the Necko? It seems we also need to
> > count the bytes separately per interface for bug 746074.
> 
> I don't know off the top of my head how to do it, but it has to be
> correlated to the socket being used and you can get to that from
> nshttptransaction->mConnection->Transport()

Thanks Patrick! After some discussions with Patrick Wang, I think we can directly get the interface types (i.e. Wifi or 3G) via /dom/system/gonk/nsINetworkManager.idl which should be installed and available in the B2G platform.

Comment 23

[Honza Bambas (:mayhemer)]

Have we ever think of separating connection pools with its limits for apps from those used by firefox?  (btw, then you could measure easily on the socket transports level)  I remember there were some thought about having a pool for each tab (smaller).

Comment 24

[Gene Lian (I already quit Mozilla)]

Attachment: Network Metering Architecture

Uploaded an diagram to illustrate the network metering architecture.

Comment 25

[Gene Lian (I already quit Mozilla)]

Attachment: Part1, Count and Save Per-App Traffic in Necko (WIP)

Hi Honza,

May I invite you to give us some feedback regarding this part? :) Please feel free to let me know if you think there is somebody else suitable for this. The following summarizes this patch:

1. You can find comment #24 for the architecture design in the attachment. Note that for the per-app metering, we'll count the traffic for each read/write IO within a transaction.

2. Most of the IDL, multi-process logic and database storage designs have pretty much done by Albert at bug 746069 (please see the "Network Stats" patch there). We'll follow his work to save the traffic count into the database. For this patch, just focusing on the traffic retrieval.

3. To get the connection type (i.e. Wifi or 3G... etc), we need to use the networkManager service to retrieve it. Note that only the traffic counts with specified connection type will be saved into the database.

Comment 26

[Gene Lian (I already quit Mozilla)]

Attachment: Network Metering Architecture V2

After some discussions with Albert, it seems much better to save per-app traffic through the central NetworkStatsService (instead of the direct access to NetworkStatsDB). To so so, the Necko end will send an async message to the NetworkStatsService.

Comment 27

[Albert [:albert]]

Attachment: Part 2, Connector to Network Stats Service to send async message and store data in indexedDB (WIP)

Based on patch V5 of bug 746069

Comment 28

[Honza Bambas (:mayhemer)]

(In reply to Gene Lian [:gene] from comment #25)
> May I invite you to give us some feedback regarding this part? :) 

Sure, I already have some draft comments in splinter, so please don't obsolete the patch.

Comment 29

[Honza Bambas (:mayhemer)]

Comment on attachment 649975 [details] [diff] [review]
Part1, Count and Save Per-App Traffic in Necko (WIP)

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

Sad we have to instrument the code like this, but I don't see another option right now.

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +28,5 @@
>  #include "nsServiceManagerUtils.h"   // do_GetService
>  #include "nsIHttpActivityObserver.h"
>  
> +#include "nsINetworkManager.h"
> +#include "nsIScriptSecurityManager.h"

Please don't include this header in necko.  I believe you are using it just for NO_APP_ID on this interface.  Just do and use what I do in this (pending) patch hunk: https://bugzilla.mozilla.org/attachment.cgi?id=647475&action=diff#a/netwerk/base/public/nsNetUtil.h_sec1

@@ +655,5 @@
>              rv = NS_ERROR_UNEXPECTED;
>          }
>      }
>  
> +    CountTraffic(false, *countWritten);

countWritten is more precise (for what you may want) right after trans->mWriter->OnWriteSegment call.  That is the place we write to the socket and it can later in the method altered.

Check for rv prior to collecting the data, it has to be NS_SUCCEEDED(rv)

Same for read.

@@ +660,5 @@
>      return rv;
>  }
>  
>  void
> +nsHttpTransaction::CountTraffic(bool isRead, PRUint32 count)

Have an enum instead of bool for indicating read/write or a set of CountReadTraffic()/CountWriteTraffic()/CountTrafficInternal(type) methods.

@@ +662,5 @@
>  
>  void
> +nsHttpTransaction::CountTraffic(bool isRead, PRUint32 count)
> +{
> +    LOG(("nsHttpTransaction::CountTraffic\n"));

Maybe the log should be after the mHasAppInfo check?

@@ +674,5 @@
> +        mAppID, mIsInBrowserElement, isRead, count, mConnectionType));
> +
> +    // TODO save the metering info into database by async codes,
> +    // which will follow the database design in bug 746069.
> +    // ...

Hmm.. so here you update the sum on every read/write.  Might be a perf issue to do for every operation.  Maybe update only after some larger increment and in dtor?

Comment 30

[Gene Lian (I already quit Mozilla)]

Thanks Honza very much for the quick review! I'll come back with the fixes. Btw, it seems you won't read bugmail since Aug 11th (curious about why's that? :)). If so, may I ask who should I ask for the following reviews?

Comment 31

[Jason Duell]

Honza is taking a proper vacation :)

:mcmanus should look over code that is actually gathering measurements.   I can look over anything that related to AppId, etc.   That part seems fine so far.

Comment 32

[Albert [:albert]]

Attachment: Part 0, IDL

Comment 33

[Gene Lian (I already quit Mozilla)]

(In reply to Albert from comment #27)
> Created attachment 650210 [details] [diff] [review]
> Part 2, Connector to Network Stats Service to send async message and store
> data in indexedDB (WIP)
> 
> Based on patch V5 of bug 746069

Hi Albert,

Thanks for your great support on this patch! :) After looking to the Necko, I'd like to have some concerns about the original plan of sending messages (carrying metering info) between Necko socket processes and the NetworkStatsService process, because:

1. I cannot find any existing examples of using such a mechanism in Necko to talk to other processes out of Necko.

2. Using multi-process communications between Necko processes and non-Necko processes makes the Necko module seem kind of non-independent.

3. As far as I know, the nsIFrameMessageManager mechanism is only applied on the child-parent (webapp-service) process communication so far. Not sure if the same way is also suitable for the Necko module.

So I'd like to propose another solution which seems more straight forward ;) I can find a proper place in the Necko XPCOM services to expose a registering function like: .setAppNerworkMeteringCb() which can be pre-called in the NetworkStatsService to register its .updateAppStats() during initialization. Whenever an HTTP transaction IO occurs, the Necko part can then execute this callback to save the metering data. On the other hand, if there were no callbacks registered for it then nothing happen, so that we don't need to waste resources to send messages to somewhere else even if there is no process catching it. I personally think this makes the Necko module more independent.

What do you think about this approach? Please feel free to let me know if it's comfortable to you or not. :)

Comment 34

[Jason Duell]

mcmanus,

Do you know if nsITransportEventSink.onProgress notifications report only the payload bytes for aProgress, or do they include the length of HTTP headers, etc?   

I'm wondering if we can use that mechanism (or perhaps a nsITransportEventSink2 that we create with the right API if onProgress isn't right) and have the Network Stats Service subscribe to it to get the network traffic amounts.  I agree with :gene that this is a more natural way to do things from the necko side.  It also means it would be easier for other observers to get this info--such as apps themselves, if we provide a JS API at some point--w/o having to get it from the Network stats manager, which I assume won't exist in a non-B2G environment.

Comment 35

[Patrick McManus [:mcmanus]]

(In reply to Jason Duell (:jduell) from comment #34)
> mcmanus,
> 
> Do you know if nsITransportEventSink.onProgress notifications report only
> the payload bytes for aProgress, or do they include the length of HTTP
> headers, etc?   
> 

i think its kinda messy.. for plain http it looks like they include headers and the absolute aProgress counter is just the bytes read from the socket - so it could include prior transactions. That could probably be fixed up (I've never looked at it before - so its probably always been that way). spdy is a bit smarter - it sends just the response body sizes for the relevant channel. (it has to basically ignore the notifications from the socket layer because it can't figure out the accounting for them, and it regenerates the notifications when the demux has been done).

Comment 36

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 651269 [details] [diff] [review]
Part 0, IDL

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

Would like to see a new patch due to the "array of longs" issue.

::: dom/network/interfaces/nsIDOMNetworkStats.idl
@@ +15,5 @@
> +  /**
> +   * Number of received / transmitted bytes
> +   */
> +  readonly attribute jsval        rxBytes;   // Array of longs.
> +  readonly attribute jsval        txBytes;   // Array of longs.

I thought the idea was to only expose a total for the requested period? I think that's what we discussed on the list.

If we're exposing per-day data for the selected period, which I think would make sense, then I think we should instead return an array of objects like:

{
  rxBytes: <number of bytes>,
  txBytes: <number of bytes>,
  date: <date>
}

That way we can also get rid of the startDate/endDate properties.

@@ +25,5 @@
> +  readonly attribute jsval        endDate;   // Date.
> +};
> +
> +[scriptable, builtinclass, uuid(9b0e87e0-714a-40b0-a0e3-241920193a6e)]
> +interface nsIDOMMozNetworkAppStats : nsISupports

Hmm.. no need to add two interfaces. Just have one interface where you set appID to null when a total for all apps is requested.

@@ +30,5 @@
> +{
> +  /**
> +   * Application Name
> +   */
> +  readonly attribute DOMString    appID;

Rename this to just "app" since appID is something we use internally for something else. I assume that this is intended to be the manifest/package URL, right?

@@ +41,5 @@
> +  /**
> +   * Number of received / transmitted bytes
> +   */
> +  readonly attribute double       rxBytes;   // Array of longs.
> +  readonly attribute double       txBytes;   // Array of longs.

if the type is 'double' then this can't be an array. But see above about not having these properties at all.

::: dom/network/interfaces/nsIDOMNetworkStatsManager.idl
@@ +16,5 @@
> +   * Connection type used to filter which network stats will be returned:
> +   * mobile, wifi or null (if null -> stats for both mobile and wifi)
> +   */
> +  DOMString     connectionType;
> +  DOMString     app;

Add a similar comment for 'app'. It can be set to 'null' to get stats for all apps, right?

You probably need to change both app and connectionType to be of type "DOMString?". Otherwise the dictionary parser will convert null to the string "null".

@@ +54,5 @@
> +   * If dataType is true, clear stats for ifaces.
> +   *
> +   * If dataType is false, clear stat for apps.
> +   *
> +   * If dataType is not provided, clear all database.

Do we really need this? I'd rather keep it simple and have a clearAllData() function which always clears all data.

Comment 37

[Albert [:albert]]

(In reply to Gene Lian [:gene] from comment #33)
> (In reply to Albert from comment #27)
> > Created attachment 650210 [details] [diff] [review]
> > Part 2, Connector to Network Stats Service to send async message and store
> > data in indexedDB (WIP)
> > 
> > Based on patch V5 of bug 746069
> 
> Hi Albert,
> 
> Thanks for your great support on this patch! :) After looking to the Necko,
> I'd like to have some concerns about the original plan of sending messages
> (carrying metering info) between Necko socket processes and the
> NetworkStatsService process, because:
> 
> 1. I cannot find any existing examples of using such a mechanism in Necko to
> talk to other processes out of Necko.
> 
> 2. Using multi-process communications between Necko processes and non-Necko
> processes makes the Necko module seem kind of non-independent.
> 
> 3. As far as I know, the nsIFrameMessageManager mechanism is only applied on
> the child-parent (webapp-service) process communication so far. Not sure if
> the same way is also suitable for the Necko module.
> 
> So I'd like to propose another solution which seems more straight forward ;)
> I can find a proper place in the Necko XPCOM services to expose a
> registering function like: .setAppNerworkMeteringCb() which can be
> pre-called in the NetworkStatsService to register its .updateAppStats()
> during initialization. Whenever an HTTP transaction IO occurs, the Necko
> part can then execute this callback to save the metering data. On the other
> hand, if there were no callbacks registered for it then nothing happen, so
> that we don't need to waste resources to send messages to somewhere else
> even if there is no process catching it. I personally think this makes the
> Necko module more independent.
> 
> What do you think about this approach? Please feel free to let me know if
> it's comfortable to you or not. :)

I'm agree, as you say in this way necko will be independent. I will add an observer.

Comment 38

[Albert [:albert]]

(In reply to Jonas Sicking (:sicking) from comment #36)
> Comment on attachment 651269 [details] [diff] [review]
> Part 0, IDL
> 
> Review of attachment 651269 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Would like to see a new patch due to the "array of longs" issue.
> 
> ::: dom/network/interfaces/nsIDOMNetworkStats.idl
> @@ +15,5 @@
> > +  /**
> > +   * Number of received / transmitted bytes
> > +   */
> > +  readonly attribute jsval        rxBytes;   // Array of longs.
> > +  readonly attribute jsval        txBytes;   // Array of longs.
> 
> I thought the idea was to only expose a total for the requested period? I
> think that's what we discussed on the list.
> 
> If we're exposing per-day data for the selected period, which I think would
> make sense, then I think we should instead return an array of objects like:
> 
> {
>   rxBytes: <number of bytes>,
>   txBytes: <number of bytes>,
>   date: <date>
> }
> 
> That way we can also get rid of the startDate/endDate properties.
>

You are right, the idea is to expose the total for a period. The arrays of longs are here because they are used to expose data usage per interface, not per app. As you can see they are in NetworkIfaceStats interface
 
> @@ +25,5 @@
> > +  readonly attribute jsval        endDate;   // Date.
> > +};
> > +
> > +[scriptable, builtinclass, uuid(9b0e87e0-714a-40b0-a0e3-241920193a6e)]
> > +interface nsIDOMMozNetworkAppStats : nsISupports
> 
> Hmm.. no need to add two interfaces. Just have one interface where you set
> appID to null when a total for all apps is requested.
> 

The reason of having two interfaces is that one is for iface stats and the other for app stats, because while stats for iface needs to expose each values of the period, for apps just is exposed the total.

> @@ +30,5 @@
> > +{
> > +  /**
> > +   * Application Name
> > +   */
> > +  readonly attribute DOMString    appID;
> 
> Rename this to just "app" since appID is something we use internally for
> something else. I assume that this is intended to be the manifest/package
> URL, right?
> 

Ok.

> @@ +41,5 @@
> > +  /**
> > +   * Number of received / transmitted bytes
> > +   */
> > +  readonly attribute double       rxBytes;   // Array of longs.
> > +  readonly attribute double       txBytes;   // Array of longs.
> 
> if the type is 'double' then this can't be an array. But see above about not
> having these properties at all.
>

Wrong comment from copy&paste.  

> ::: dom/network/interfaces/nsIDOMNetworkStatsManager.idl
> @@ +16,5 @@
> > +   * Connection type used to filter which network stats will be returned:
> > +   * mobile, wifi or null (if null -> stats for both mobile and wifi)
> > +   */
> > +  DOMString     connectionType;
> > +  DOMString     app;
> 
> Add a similar comment for 'app'. It can be set to 'null' to get stats for
> all apps, right?
>

Ok.
 
> You probably need to change both app and connectionType to be of type
> "DOMString?". Otherwise the dictionary parser will convert null to the
> string "null".

Ok.

> 
> @@ +54,5 @@
> > +   * If dataType is true, clear stats for ifaces.
> > +   *
> > +   * If dataType is false, clear stat for apps.
> > +   *
> > +   * If dataType is not provided, clear all database.
> 
> Do we really need this? I'd rather keep it simple and have a clearAllData()
> function which always clears all data.

I though it can be usefull, but it's ok to keep ip simple. If we need different 'clears' we can implement it in the future.

Comment 39

[Albert [:albert]]

(In reply to Jonas Sicking (:sicking) from comment #36)
> Comment on attachment 651269 [details] [diff] [review]
> Part 0, IDL
> 
> Review of attachment 651269 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: dom/network/interfaces/nsIDOMNetworkStatsManager.idl
> @@ +16,5 @@
> > +   * Connection type used to filter which network stats will be returned:
> > +   * mobile, wifi or null (if null -> stats for both mobile and wifi)
> > +   */
> > +  DOMString     connectionType;
> > +  DOMString     app;
> 
> Add a similar comment for 'app'. It can be set to 'null' to get stats for
> all apps, right?
> 

app can be null, but it doesn't mean that stats for all apps will be processed. It means that client wants stats per iface.

Comment 40

[Gene Lian (I already quit Mozilla)]

Hi Albert,

I've already integrated all your patches and constructed the callback tunnel between NetworkStatsService and Necko. So far, everything looks fine :) I think I can upload a new patch tomorrow sooner or later. Just some quick questions:

The following shows the per-app-traffic metering in the DB log you provided:

I/Gecko   ( 3759): -*- NetworkStatsDB: {"appID":5,"isInBrowserElement":false,"timestamp":1344945593460000,"rxBytes":614,"txBytes":0,"rxTotalBytes":614,"txTotalBytes":0}

The following shows all-traffic metering log:

I/Gecko   ( 3759): -*- NetworkStatsDB: _saveStats: {"appID":"wifi","connectionType":"wifi","isInBrowserElement":false,"timestamp":1344945660000,"rxBytes":0,"txBytes":328140,"rxTotalBytes":575464,"txTotalBytes":22712492}

It seems a little bit different for the |appID| and |connectionType| fields. Regarding the |appID| field, did you intentionally do so to distinguish the per-app-traffic metering (number type) and all-traffic metering records (string type)? Regarding the |connectionType|, do you prefer using strings instead of numbers (i.e. nsINetworkInterface::NETWORK_TYPE_WIFI)?

Another question is: sometimes I'd encounter the following logs which save the stats with 2 records at one time. May I confirm under what circumstances will this happen?

I/Gecko   ( 3759): -*- NetworkStatsDB: _saveStats: [{"appID":0,"isInBrowserElement":true,"timestamp":1344945592440000,"rxBytes":0,"txBytes":0,"rxTotalBytes":1818,"txTotalBytes":44569},{"appID":0,"isInBrowserElement":true,"timestamp":1344945592500000,"rxBytes":0,"txBytes":15268,"rxTotalBytes":1818,"txTotalBytes":59837}]

Thanks for your replies in advance!

Comment 41

[Albert [:albert]]

Attachment: Part 0, IDL V2

Comment 42

[Albert [:albert]]

Attachment: Part 2, IDL implementation and indexedDB support (WIP)

Removed sendAsyncMessage mecanism to update application stats.
Added IDL implementation.

Comment 43

[Albert [:albert]]

(In reply to Gene Lian [:gene] from comment #40)
> Hi Albert,
> 
> I've already integrated all your patches and constructed the callback tunnel
> between NetworkStatsService and Necko. So far, everything looks fine :) I
> think I can upload a new patch tomorrow sooner or later. Just some quick
> questions:
> 
> The following shows the per-app-traffic metering in the DB log you provided:
> 
> I/Gecko   ( 3759): -*- NetworkStatsDB:
> {"appID":5,"isInBrowserElement":false,"timestamp":1344945593460000,"rxBytes":
> 614,"txBytes":0,"rxTotalBytes":614,"txTotalBytes":0}
> 
> The following shows all-traffic metering log:
> 
> I/Gecko   ( 3759): -*- NetworkStatsDB: _saveStats:
> {"appID":"wifi","connectionType":"wifi","isInBrowserElement":false,
> "timestamp":1344945660000,"rxBytes":0,"txBytes":328140,"rxTotalBytes":575464,
> "txTotalBytes":22712492}
> 
> It seems a little bit different for the |appID| and |connectionType| fields.
> Regarding the |appID| field, did you intentionally do so to distinguish the
> per-app-traffic metering (number type) and all-traffic metering records
> (string type)? Regarding the |connectionType|, do you prefer using strings
> instead of numbers (i.e. nsINetworkInterface::NETWORK_TYPE_WIFI)?

The connectionType for app stats was missing due a bug, in last patch it is corrected.

I defined appID as a string  because I don't know if it is an integer or the application domain. By you comment I see it is an integer, right?

For connectionType, you have to give me the constant number, I convert it to the string just to expose it to clients.

> 
> Another question is: sometimes I'd encounter the following logs which save
> the stats with 2 records at one time. May I confirm under what circumstances
> will this happen?
> 
> I/Gecko   ( 3759): -*- NetworkStatsDB: _saveStats:
> [{"appID":0,"isInBrowserElement":true,"timestamp":1344945592440000,"rxBytes":
> 0,"txBytes":0,"rxTotalBytes":1818,"txTotalBytes":44569},{"appID":0,
> "isInBrowserElement":true,"timestamp":1344945592500000,"rxBytes":0,"txBytes":
> 15268,"rxTotalBytes":1818,"txTotalBytes":59837}]
> 
> Thanks for your replies in advance!

Yes, as stats are requested aggregated in a period they are stored using a kind of sample rate. For testing purposes the sample rate is configured to be 1 minute, this means that if you send two or more stats during 1 minute, it will be stored aggregated in one sample.

Comment 44

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> > Hmm.. no need to add two interfaces. Just have one interface where you set
> > appID to null when a total for all apps is requested.
> 
> The reason of having two interfaces is that one is for iface stats and the
> other for app stats, because while stats for iface needs to expose each
> values of the period, for apps just is exposed the total.

But the behavior is exactly the same as for connectionType. I.e. we can either expose data per connection type, or as total for all connectionTypes. But you don't have separate interfaces for per connectionType vs. total for all types.

I think we should use a single interface with both a "connectionType" attribute and a "app" attribute. We can then set either of those attributes to <null> when exposing a total for all apps/connectionTypes, or set to a non-null value when exposing data for a specific app/connectionType.

Comment 45

[Albert [:albert]]

(In reply to Jonas Sicking (:sicking) from comment #44)
> > > Hmm.. no need to add two interfaces. Just have one interface where you set
> > > appID to null when a total for all apps is requested.
> > 
> > The reason of having two interfaces is that one is for iface stats and the
> > other for app stats, because while stats for iface needs to expose each
> > values of the period, for apps just is exposed the total.
> 
> But the behavior is exactly the same as for connectionType. I.e. we can
> either expose data per connection type, or as total for all connectionTypes.
> But you don't have separate interfaces for per connectionType vs. total for
> all types.
> 
> I think we should use a single interface with both a "connectionType"
> attribute and a "app" attribute. We can then set either of those attributes
> to <null> when exposing a total for all apps/connectionTypes, or set to a
> non-null value when exposing data for a specific app/connectionType.

I also prefer the approach of having just one interface instead of two. My concern is that interface stats return an two arrays of values (one array for rx and other for tx) and application stats will return two values (one for rx and the other for tx). If you don't think that returning different data types depending on the kind of stats requested, I will go with just one interface.

Comment 46

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Albert from comment #45)
> I also prefer the approach of having just one interface instead of two. My
> concern is that interface stats return an two arrays of values (one array
> for rx and other for tx) and application stats will return two values (one
> for rx and the other for tx). If you don't think that returning different
> data types depending on the kind of stats requested, I will go with just one
> interface.

Wait, why do you want to do that? APIs aside, why do you need different types of data when returning per-app rather than per-interface data?

To keep things simple, I think we should always return per-day data. If the application wants to coalesce data to show per-week graphs or show totals for a selected time period, it can do that itself.

So I would say that we should always have just a single property called "data" which is an array of objects like:

[{
  txBytes: 100,
  rxBytes: 10000,
  date:    <date object>,
 },
 {
  txBytes: 90,
  rxBytes: 50000,
  date:    <date object for next day>,
 },
 ...
]

Comment 47

[Gene Lian (I already quit Mozilla)]

(In reply to Albert from comment #43)
> (In reply to Gene Lian [:gene] from comment #40)
> 
> The connectionType for app stats was missing due a bug, in last patch it is
> corrected.

Ah! I got it (s/connectionTpe/connectionType). I can have this fixed myself.

> 
> I defined appID as a string  because I don't know if it is an integer or the
> application domain. By you comment I see it is an integer, right?

Yes, it's going to be an integer for per-app metering.

Actually, what I was hoping to ask is why we would have something like "appID":"wifi" in the log? Is that a keyword for per-interface purpose?

> 
> For connectionType, you have to give me the constant number, I convert it to
> the string just to expose it to clients.

OK. Got it! I'll feed the numbers. :)

Comment 48

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, Count Per-App Traffic in Necko and Pass It to Observers

Hi Patrick,

May I have your support to review this patch when you're available? Thanks for your help in advance :) The following summarizes what I've done in this patch:

1. First of all, I included all the suggestions addressed by Honza at comment #29.

2. nsHttpTransaction::CountTraffic() is used to aggregate the traffic counts for each transaction IO.

3. nsHttpTransaction::GetConnectionType() is used to get the current connection type (i.e. 3G or Wifi), which will be called whenever a transcription is initialized.

4. When a transcription is destructed, I call mActivityDistributor->ObserveActivity() to notify the subscribers how many valid byte counts are read/written. Note that I use the existing parameter |aExtraStringData| to pass the App info in a JSON format which can be prepared by calling nsHttpTransaction::GetAppInfoJSONData().

5. One new type |ACTIVITY_TYPE_HTTP_TRANSACTION_COUNT| and two new subtypes |ACTIVITY_SUBTYPE_TRANSACTION_COUNT_READ| / |ACTIVITY_SUBTYPE_TRANSACTION_COUNT_WRITTEN| are added to specify the the above needs so that we won't break the compatibilities of existing observers.

6. The remaining things are minor code clean-ups.

Comment 49

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, IDL Implementation and IndexedDB Support

Hi Albert,

I follow your original patch to have some slight changes to support the callback registering, which will play the role of tunnel between Necko and NetworkStatsService (please see NetworkStatsService.observeActivity()). Please take a look on it and let me know if you don't agree with these changes. :) We'll formally ask a reviewer to review this after the observer mechanism in Necko part is confirmed. Thank you!

Comment 50

[Gene Lian (I already quit Mozilla)]

Attachment: Network Metering Architecture V3

Comment 51

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 651743 [details] [diff] [review]
Part 0, IDL V2

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

r- per comment 46

::: dom/network/interfaces/nsIDOMNetworkStats.idl
@@ +41,5 @@
> +  /**
> +   * Number of received / transmitted bytes
> +   */
> +  readonly attribute double       rxBytes;
> +  readonly attribute double       txBytes;

Hmm.. so I'm a bit confused. So the intent is not to expose per-day data, but rather just the total for the period?

Comment 52

[Albert [:albert]]

(In reply to Jonas Sicking (:sicking) from comment #51)
> Comment on attachment 651743 [details] [diff] [review]
> Part 0, IDL V2
> 
> Review of attachment 651743 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- per comment 46
> 
> ::: dom/network/interfaces/nsIDOMNetworkStats.idl
> @@ +41,5 @@
> > +  /**
> > +   * Number of received / transmitted bytes
> > +   */
> > +  readonly attribute double       rxBytes;
> > +  readonly attribute double       txBytes;
> 
> Hmm.. so I'm a bit confused. So the intent is not to expose per-day data,
> but rather just the total for the period?

Yes, the idea is to expose the total for the period.

Comment 53

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

See comment 46. Why do you want to sometimes expose per-day data, and sometimes total for period?

I think we should always expose per-day data and let the app sum it up if it wants to.

Comment 54

[Albert [:albert]]

(In reply to Jonas Sicking (:sicking) from comment #53)
> See comment 46. Why do you want to sometimes expose per-day data, and
> sometimes total for period?
> 
> I think we should always expose per-day data and let the app sum it up if it
> wants to.

Ok, then I'll modify long arrays to use the data structure proposed in your comment 46 and just define one interface for stats.

Comment 55

[Patrick McManus [:mcmanus]]

Comment on attachment 652065 [details] [diff] [review]
Part 1, Count Per-App Traffic in Necko and Pass It to Observers

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

a few important issues, but if you agree with them all we can push this forward quickly. good stuff!

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +124,5 @@
>  nsHttpTransaction::~nsHttpTransaction()
>  {
>      LOG(("Destroying nsHttpTransaction @%x\n", this));
>  
> +    // To notify subscribers how many valid byte counts are read/written.

you should probably do this in close() rather than waiting for the reference counts to all sort out

@@ +561,5 @@
>      if (NS_FAILED(rv)) return rv;
>  
> +    // Count the traffic of transaction read IO.
> +    if (NS_SUCCEEDED(rv))
> +        trans->CountTraffic(HTTP_TRANSACTION_IO_TYPE_READ, *countRead);

I think you have this backwards - ReadSegments deals with data being written towards network. I assume that's TYPE_WRITE for your code. (I don't care for the perspsective of the naming either - but ReadSegments is referring to data being read from the input source (the post, whatever) and not the network.

you can see that the sentData flag is set just a couple lines further down.

@@ +641,5 @@
>      NS_ASSERTION(*countWritten > 0, "bad writer");
> +
> +    // Count the traffic of transaction write IO.
> +    if (NS_SUCCEEDED(rv))
> +        trans->CountTraffic(HTTP_TRANSACTION_IO_TYPE_WRITE, *countWritten);

obviously backwards if the previous one was too.

@@ +642,5 @@
> +
> +    // Count the traffic of transaction write IO.
> +    if (NS_SUCCEEDED(rv))
> +        trans->CountTraffic(HTTP_TRANSACTION_IO_TYPE_WRITE, *countWritten);
> +

also, its possible to "over-read" in a pipelined scenario. Look for the line that says mConnection->Pushback() - that is returning some bytes that were consumed by this OnWriteSegment() back to the network buffer because they didn't belong to this transaction. You should make a negative adjustment to your bytes read count when that happens.

::: netwerk/protocol/http/nsHttpTransaction.h
@@ +34,5 @@
> +// Enums to specify the types of transaction IO
> +typedef enum _HTTP_TRANSACTION_IO_TYPE {
> +    HTTP_TRANSACTION_IO_TYPE_READ,
> +    HTTP_TRANSACTION_IO_TYPE_WRITE
> +} HTTP_TRANSACTION_IO_TYPE;

I don't normally comment much on style, leaving that as an obligation of the author, but it would be more consistent to just define this as part of the nsHttpTransaaction class as 

enum nsHttpTransaction::IODirection {READ, WRITE};

an example -  nsAHttpTransaction::Classifier

@@ +221,5 @@
>  
> +    // network metering information
> +    bool                            mHasAppInfo;
> +    PRUint32                        mAppID;
> +    bool                            mIsInBrowserElement;

declare the bools adjacent to each other and save an exciting 4 bytes

@@ +224,5 @@
> +    PRUint32                        mAppID;
> +    bool                            mIsInBrowserElement;
> +    PRInt32                         mConnectionType;
> +    PRUint32                        mCountRead;
> +    PRUint32                        mCountWritten;

The counts need to be 64 bit even if you can't report more than 0xffffffff through the observer interface (I'm not sure off the top of my head). Such things definitely happen (though hopefully only on wifi for mobile!)

also, can you rename countread/countwritten to mHttpBytes{Read,Written).. this is to avoid confusion with things like ContentRead which are without-framing counts and have different semantics when retrying, etc..

Comment 56

[Albert [:albert]]

Attachment: Part 0, IDL V2

Changes based on comments 46, 51 and 53.
Patch over IDL patch of bug 746069

Comment 57

[Jason Duell]

Comment on attachment 652065 [details] [diff] [review]
Part 1, Count Per-App Traffic in Necko and Pass It to Observers

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

gene:  This patch will become part of bug 784575.   You can start designing your other patches to use the IDLs in that bug to get all necko traffic.

Comment 58

[Gene Lian (I already quit Mozilla)]

(In reply to Jason Duell (:jduell) from comment #57)
> Comment on attachment 652065 [details] [diff] [review]
> Part 1, Count Per-App Traffic in Necko and Pass It to Observers
> 
> Review of attachment 652065 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> gene:  This patch will become part of bug 784575.   You can start designing
> your other patches to use the IDLs in that bug to get all necko traffic.

Surprised to hear that because we've had pretty much done by nsIHttpActivityDistributor, but if it's a better way to aggregate traffic there then I'm glad to switch to that.

We need to know what the current IDL design is for the new service as well as its behaviors. That would change lots of mechanism we've already had in the NetworkStatsService.jsm.

The original design is using nsIHttpActivityDistributor to notify the NetworkStatsService whenever a HTTP transaction occurs, and the NetworkStatsService will aggregate and assign the counts to the corresponding time sample. If the new service will aggregate traffic itself, it seems the NetworkStatsService needs to querying the new service from time to time.

Comment 59

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Note that I don't think it's acceptable to just measure HTTP traffic. We need to measure traffic done over any protocol since user charges are protocol agnostic.

Comment 60

[Daniel Buchner [:dbuc]]

(In reply to Jonas Sicking (:sicking) from comment #59)
> Note that I don't think it's acceptable to just measure HTTP traffic. We
> need to measure traffic done over any protocol since user charges are
> protocol agnostic.

I read through all the comments here, and I believe this case is covered (please excuse if redundant), but I wanted to ask and make sure:

If an app on domain X requests content from domain Y - either separate/sub domain or different protocol, as Jonas mentioned - it will show up as data usage for the app registered and installed under domain X under this scheme, correct?

Comment 61

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yes.

Comment 62

[Daniel Buchner [:dbuc]]

What is the Product-blocking element of this bug that needs to be addressed?

Comment 63

[Dietrich Ayala (:dietrich)]

The product-blocking element was a decision about whether we'd hold the release for per-app metering.

Moz & Telefonica met today and decided that for V1 we could ship without per-app metering. We still need system-wide metering and control, but that work is in other bugs. Marking this bug blocking-.

Comment 65

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Needed for competitive parity and to fix bad UX problem, apps draining avail bandwidth without user knowing.

Comment 66

[Gene Lian (I already quit Mozilla)]

Hi John, since you're working on it, I reassign this bug to you.

Comment 67

[Aus Lacroix [:aus]]

Is this bug actively being worked on? If not, I would like to take it over.

Comment 68

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Per my understanding John Shih is working on it, cc him for clarification.

Comment 69

[John Shih]

yes, I'm working on this bug
thanks Hsin-Yi for clarifying!

Comment 70

[John Shih]

Attachment: Bug 746073 - Network Per-App Metering on HTTP layer

Base on bug 784575, now we are going to collect per-app traffic data on different protocol layers (e.g., HTTP, FTP, .. etc)

On each protocol, we accumulate tx/rx amount per connection (a connection is used by an app). Then we send the collected data and app information of the connection (including appId, isInBrowser) to NetworkStatsService (please see Bug 784575)

This patch is for HTTP layer.

Comment 71

[Patrick McManus [:mcmanus]]

Comment on attachment 768831 [details] [diff] [review]
Bug 746073 - Network Per-App Metering on HTTP layer

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

John - I know you didn't ask for a review but I saw the patch this morning and took a quick look and gave some WIP comments. I hope you don't mind and hope it helps.

how are you going to handle the stopping issues?

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +135,5 @@
>      LOG(("Destroying nsHttpTransaction @%x\n", this));
>  
> +    // send out the network traffic information to networkStatsService
> +    if (mHasAppInfo && mConnectionType.get() != "unknown" && mAppId != 0) {
> +        mNetworkStatsService->SendAppStats(mAppId, mIsInBrowser, mConnectionType.get(), PR_Now(), mCountRead, mCountWrite);

I don't think this dtor is consistently called from the same thread.. and mNetworkStatsService is definitely main thread only. So you'll want an event to deal with that. Init() is always on the main thread.

I think this interface can be improved.. in a couple of ways:

1] don't use a string for the connection type.. nsINetworkInterface is already sensibly using an enum, why would we add the cost of string processing into the mix?

2] We're trying to get rid of nspr time (PR_Now).. does this interface really need a timestamp or should it just be assuming "now" when it is called.

@@ +208,5 @@
>      MOZ_EVENT_TRACER_WAIT(static_cast<nsAHttpTransaction*>(this),
>                            "net::http::transaction");
>      nsresult rv;
> +    mNetworkStatsService = do_GetService("@mozilla.org/networkstatsService;1", &rv);
> +    if (NS_FAILED(rv)) return rv;

don't fail the http transaction just because the stats service isn't available.

also, move this code to after the LOG()

@@ +713,5 @@
>  
> +nsCString
> +nsHttpTransaction::GetConnectionType()
> +{
> +    nsresult rv;

rv means return value for the function which isn't what you are doing here.. so it needs another name.

@@ +716,5 @@
> +{
> +    nsresult rv;
> +    nsCOMPtr<nsINetworkManager> networkManager = do_GetService("@mozilla.org/network/manager;1", &rv);
> +
> +    nsCString connectionType("unknown");

nsAutoCString connectionType(NS_LITERAL_CSTRING("unknown"))

@@ +723,5 @@
> +    }
> +
> +    nsCOMPtr<nsINetworkInterface> networkInterface;
> +    rv = networkManager->GetActive(getter_AddRefs(networkInterface));
> +    

whitespace

@@ +730,5 @@
> +        rv = networkInterface->GetType(&type);
> +    }
> +
> +    switch (type) {
> +    case 0:  // NETWORK_TYPE_WIFI

aren't these enums defined as nsINetworkInterface::Network_TYPE_WIFI ?

::: netwerk/protocol/http/nsHttpTransaction.h
@@ +21,5 @@
>  #include "nsISocketTransportService.h"
>  #include "nsITransport.h"
>  #include "nsIEventTarget.h"
>  #include "TimingStruct.h"
> +#include "nsINetworkStatsService.h"

I think you can forward declare this without the #include

@@ +120,5 @@
>      nsILoadGroupConnectionInfo *LoadGroupConnectionInfo() { return mLoadGroupCI.get(); }
>      void DispatchedAsBlocking();
>      void RemoveDispatchedAsBlocking();
>  
> +    enum IODirection {READ, WRITE};

this is clunky and I would prefer that private code just updates countread and countwrite directly

@@ +340,5 @@
>      bool mPassedRatePacing;
>      bool mSynchronousRatePaceRequest;
>      nsCOMPtr<nsICancelable> mTokenBucketCancel;
> +
> +    int64_t                            mCountRead;

please make a comment about what these members and methods are used for..

the counters make more sense as uint64_t

@@ +344,5 @@
> +    int64_t                            mCountRead;
> +    int64_t                            mCountWrite;
> +    uint32_t                           mAppId;
> +    bool                               mIsInBrowser;
> +    bool                               mHasAppInfo;

at least based on what you've got so far I don't think you need mHasAppInfo.. it is redundant to connectiontype != unknown

Comment 72

[John Shih]

(In reply to Patrick McManus [:mcmanus] from comment #71)
> Comment on attachment 768831 [details] [diff] [review]
> Bug 746073 - Network Per-App Metering on HTTP layer
> 
> Review of attachment 768831 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> John - I know you didn't ask for a review but I saw the patch this morning
> and took a quick look and gave some WIP comments. I hope you don't mind and
> hope it helps.
> 
> how are you going to handle the stopping issues?

I'm not sure what the behavior the transaction will have..
will it hangs or close in some other way?

> 
> ::: netwerk/protocol/http/nsHttpTransaction.cpp
> @@ +135,5 @@
> >      LOG(("Destroying nsHttpTransaction @%x\n", this));
> >  
> > +    // send out the network traffic information to networkStatsService
> > +    if (mHasAppInfo && mConnectionType.get() != "unknown" && mAppId != 0) {
> > +        mNetworkStatsService->SendAppStats(mAppId, mIsInBrowser, mConnectionType.get(), PR_Now(), mCountRead, mCountWrite);
> 
> I don't think this dtor is consistently called from the same thread.. and
> mNetworkStatsService is definitely main thread only. So you'll want an event
> to deal with that. Init() is always on the main thread.

I was trying to move this part to Close() but encounter the situation you mentioned (Close() isn't called in main thread). I'll try to improve this.

> 
> I think this interface can be improved.. in a couple of ways:
> 
> 1] don't use a string for the connection type.. nsINetworkInterface is
> already sensibly using an enum, why would we add the cost of string
> processing into the mix?
> 
> 2] We're trying to get rid of nspr time (PR_Now).. does this interface
> really need a timestamp or should it just be assuming "now" when it is
> called.
> 

thanks for the advice, I'll improve the interface.

> @@ +208,5 @@
> >      MOZ_EVENT_TRACER_WAIT(static_cast<nsAHttpTransaction*>(this),
> >                            "net::http::transaction");
> >      nsresult rv;
> > +    mNetworkStatsService = do_GetService("@mozilla.org/networkstatsService;1", &rv);
> > +    if (NS_FAILED(rv)) return rv;
> 
> don't fail the http transaction just because the stats service isn't
> available.
> 
> also, move this code to after the LOG()
> 
> @@ +713,5 @@
> >  
> > +nsCString
> > +nsHttpTransaction::GetConnectionType()
> > +{
> > +    nsresult rv;
> 
> rv means return value for the function which isn't what you are doing here..
> so it needs another name.
> 
> @@ +716,5 @@
> > +{
> > +    nsresult rv;
> > +    nsCOMPtr<nsINetworkManager> networkManager = do_GetService("@mozilla.org/network/manager;1", &rv);
> > +
> > +    nsCString connectionType("unknown");
> 
> nsAutoCString connectionType(NS_LITERAL_CSTRING("unknown"))
> 
> @@ +723,5 @@
> > +    }
> > +
> > +    nsCOMPtr<nsINetworkInterface> networkInterface;
> > +    rv = networkManager->GetActive(getter_AddRefs(networkInterface));
> > +    
> 
> whitespace
> 
> @@ +730,5 @@
> > +        rv = networkInterface->GetType(&type);
> > +    }
> > +
> > +    switch (type) {
> > +    case 0:  // NETWORK_TYPE_WIFI
> 
> aren't these enums defined as nsINetworkInterface::Network_TYPE_WIFI ?
> 
> ::: netwerk/protocol/http/nsHttpTransaction.h
> @@ +21,5 @@
> >  #include "nsISocketTransportService.h"
> >  #include "nsITransport.h"
> >  #include "nsIEventTarget.h"
> >  #include "TimingStruct.h"
> > +#include "nsINetworkStatsService.h"
> 
> I think you can forward declare this without the #include
> 
> @@ +120,5 @@
> >      nsILoadGroupConnectionInfo *LoadGroupConnectionInfo() { return mLoadGroupCI.get(); }
> >      void DispatchedAsBlocking();
> >      void RemoveDispatchedAsBlocking();
> >  
> > +    enum IODirection {READ, WRITE};
> 
> this is clunky and I would prefer that private code just updates countread
> and countwrite directly
> 
> @@ +340,5 @@
> >      bool mPassedRatePacing;
> >      bool mSynchronousRatePaceRequest;
> >      nsCOMPtr<nsICancelable> mTokenBucketCancel;
> > +
> > +    int64_t                            mCountRead;
> 
> please make a comment about what these members and methods are used for..
> 
> the counters make more sense as uint64_t
> 
> @@ +344,5 @@
> > +    int64_t                            mCountRead;
> > +    int64_t                            mCountWrite;
> > +    uint32_t                           mAppId;
> > +    bool                               mIsInBrowser;
> > +    bool                               mHasAppInfo;
> 
> at least based on what you've got so far I don't think you need
> mHasAppInfo.. it is redundant to connectiontype != unknown 


Very appreciate for the quick review, I'll improve the patch as soon as possible.

Comment 73

[John Shih]

Attachment: Bug 746073 - Network Per-App Metering on HTTP layer

patch update.

Comment 74

[John Shih]

Attachment: Bug 746073 - Network Per-App Metering on HTTP layer

Comment 75

[Patrick McManus [:mcmanus]]

Comment on attachment 773137 [details] [diff] [review]
Bug 746073 - Network Per-App Metering on HTTP layer

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

Better. Thanks!

In addition to the notes below:

1] Jonas has made it pretty clear he's interested in stopping big downloads that cross a quota while they are in progress.. this doesn't seem to support that use case. How are we going to do that?

2] I'm worried about cross platform issues.. so I'd like to see a try run with all platforms and all tests in it

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +32,5 @@
>  #include "nsServiceManagerUtils.h"   // do_GetService
>  #include "nsIHttpActivityObserver.h"
>  #include "nsSocketTransportService2.h"
> +#include "nsINetworkManager.h"
> +#include "nsINetworkStatsService.h"

are these includes available on all builds, or does this need some kind of b2g ifdef? I'm pretty sure they aren't cross platform.

@@ +124,5 @@
>      , mPassedRatePacing(false)
>      , mSynchronousRatePaceRequest(false)
> +    , mCountRecv(0)
> +    , mCountSent(0)
> +    , mAppId(0)

nsIScriptSecurityManager::NO_APP_ID

@@ +125,5 @@
>      , mSynchronousRatePaceRequest(false)
> +    , mCountRecv(0)
> +    , mCountSent(0)
> +    , mAppId(0)
> +    , mConnectionType(-1)

it would be nice if nsinetwork* defined a NO_TYPE of -1 that can be used here.. if that can't be done, at least make it a const in the h file and comment it

@@ +237,5 @@
> +        NS_GetAppInfo(channel, &mAppId, &mIsInBrowser);
> +    }
> +
> +    // obtain active connection type
> +    if (mAppId != 0) {

nsIScriptSecurityManager::NO_APP_ID

@@ +708,5 @@
>  }
>  
> +int32_t
> +nsHttpTransaction::GetConnectionType()
> +{

it would be nice to add MOZ_ASSERT(NS_IsMainThread()) here

@@ +710,5 @@
> +int32_t
> +nsHttpTransaction::GetConnectionType()
> +{
> +    nsresult result;
> +    nsCOMPtr<nsINetworkManager> networkManager = do_GetService("@mozilla.org/network/manager;1", &result);

I think we need some ifdef magic in here too corresponding to the includes

@@ +747,5 @@
> +    {}
> +
> +    NS_IMETHOD Run()
> +    {
> +        nsresult rv;

MOZ_ASSERT(NS_IsMainThread())

@@ +748,5 @@
> +
> +    NS_IMETHOD Run()
> +    {
> +        nsresult rv;
> +        nsCOMPtr<nsINetworkStatsService> mNetworkStatsService =

ifdef b2g again, right?

@@ +755,5 @@
> +            return rv;
> +        }
> +
> +        // send out the network traffic information to networkStatsService
> +        if (connectionType != -1 && appId != 0) {

move these checks to ::Close so that we don't create the event and send it to the main thread just to find out that it isn't actually going to do the sendappstats() call. You should then MOZ_ASSERT them in the event's ctor

@@ +756,5 @@
> +        }
> +
> +        // send out the network traffic information to networkStatsService
> +        if (connectionType != -1 && appId != 0) {
> +        mNetworkStatsService->SendAppStats(appId, isInBrowser, connectionType,

indentation

@@ +763,5 @@
> +
> +        return NS_OK;
> +    }
> +private:
> +    uint32_t appId;

naming on all these.. mAppID, mIsInBrowser, etc..

::: netwerk/protocol/http/nsHttpTransaction.h
@@ +337,5 @@
>      bool mSubmittedRatePacing;
>      bool mPassedRatePacing;
>      bool mSynchronousRatePaceRequest;
>      nsCOMPtr<nsICancelable> mTokenBucketCancel;
> +

add a comment indicating that these new members are used for per app resource monitoring.. please make it clear that they are only successfully populated on gonk (not android, and not desktop) at this time.. (or clear it up for me if that's not true)

Comment 76

[John Shih]

(In reply to Patrick McManus [:mcmanus] from comment #75)
> Comment on attachment 773137 [details] [diff] [review]
> Bug 746073 - Network Per-App Metering on HTTP layer
> 
> Review of attachment 773137 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Better. Thanks!
> 
> In addition to the notes below:
> 
> 1] Jonas has made it pretty clear he's interested in stopping big downloads
> that cross a quota while they are in progress.. this doesn't seem to support
> that use case. How are we going to do that?

> 
> 2] I'm worried about cross platform issues.. so I'd like to see a try run
> with all platforms and all tests in it
> 
> ::: netwerk/protocol/http/nsHttpTransaction.cpp
> @@ +32,5 @@
> >  #include "nsServiceManagerUtils.h"   // do_GetService
> >  #include "nsIHttpActivityObserver.h"
> >  #include "nsSocketTransportService2.h"
> > +#include "nsINetworkManager.h"
> > +#include "nsINetworkStatsService.h"
> 
> are these includes available on all builds, or does this need some kind of
> b2g ifdef? I'm pretty sure they aren't cross platform.
> 
> @@ +124,5 @@
> >      , mPassedRatePacing(false)
> >      , mSynchronousRatePaceRequest(false)
> > +    , mCountRecv(0)
> > +    , mCountSent(0)
> > +    , mAppId(0)
> 
> nsIScriptSecurityManager::NO_APP_ID
> 
> @@ +125,5 @@
> >      , mSynchronousRatePaceRequest(false)
> > +    , mCountRecv(0)
> > +    , mCountSent(0)
> > +    , mAppId(0)
> > +    , mConnectionType(-1)
> 
> it would be nice if nsinetwork* defined a NO_TYPE of -1 that can be used
> here.. if that can't be done, at least make it a const in the h file and
> comment it
> 

I encounter a problem here..
I found that there is already a definition of NETWORK_TYPE_UNKOWN = -1 in
nsINetworkInterface
but once the nsINetworkManager.h is not included (cross-platform situation)
, nsINetworkInterface::NETWORK_TYPE_UNKOWN will be unavailable
such case can cause errors since it is used in ctor as and some dtor for checking the connectionType

I wonder how to handle this case..
should I
1. Simply define the NETWORK_NO_TYPE = -1 in the nsHttpTransaction.h
or
2. use ifdef to avoid it

what do you think?

Comment 77

[John Shih]

(In reply to John Shih from comment #76)
> (In reply to Patrick McManus [:mcmanus] from comment #75)
> > Comment on attachment 773137 [details] [diff] [review]
> > Bug 746073 - Network Per-App Metering on HTTP layer
> > 
> > Review of attachment 773137 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Better. Thanks!
> > 
> > In addition to the notes below:
> > 
> > 1] Jonas has made it pretty clear he's interested in stopping big downloads
> > that cross a quota while they are in progress.. this doesn't seem to support
> > that use case. How are we going to do that?
> 
> > 
> > 2] I'm worried about cross platform issues.. so I'd like to see a try run
> > with all platforms and all tests in it
> > 
> > ::: netwerk/protocol/http/nsHttpTransaction.cpp
> > @@ +32,5 @@
> > >  #include "nsServiceManagerUtils.h"   // do_GetService
> > >  #include "nsIHttpActivityObserver.h"
> > >  #include "nsSocketTransportService2.h"
> > > +#include "nsINetworkManager.h"
> > > +#include "nsINetworkStatsService.h"
> > 
> > are these includes available on all builds, or does this need some kind of
> > b2g ifdef? I'm pretty sure they aren't cross platform.
> > 
> > @@ +124,5 @@
> > >      , mPassedRatePacing(false)
> > >      , mSynchronousRatePaceRequest(false)
> > > +    , mCountRecv(0)
> > > +    , mCountSent(0)
> > > +    , mAppId(0)
> > 
> > nsIScriptSecurityManager::NO_APP_ID
> > 
> > @@ +125,5 @@
> > >      , mSynchronousRatePaceRequest(false)
> > > +    , mCountRecv(0)
> > > +    , mCountSent(0)
> > > +    , mAppId(0)
> > > +    , mConnectionType(-1)
> > 
> > it would be nice if nsinetwork* defined a NO_TYPE of -1 that can be used
> > here.. if that can't be done, at least make it a const in the h file and
> > comment it
> > 
> 
> I encounter a problem here..
> I found that there is already a definition of NETWORK_TYPE_UNKOWN = -1 in
> nsINetworkInterface

Sorry, I mean I define the NETWORK_TYPE_UNKOWN = -1 in nsINetworkInterface 
(since it is a suitable place)

but once the nsINetworkManager.h is not included (cross-platform situation)
, nsINetworkInterface::NETWORK_TYPE_UNKOWN will be unavailable
such case can cause errors since it is used in ctor as and some dtor for
checking the connectionType

I wonder how to handle this case..
should I
1. Simply define the NETWORK_NO_TYPE = -1 in the nsHttpTransaction.h
or
2. use ifdef to avoid it
 
what do you think?

Comment 78

[John Shih]

Attachment: Bug 746073: Part 0: nsINetwormManager.idl Modification.

define the NETWORK_TYPE_UNKNOWN = -1 in nsINetworkInterface.

Comment 79

[John Shih]

Attachment: Bug 746073: Part 1: Collect Per-App Traffic in HTTP Transaction.

patch update.

Comment 80

[John Shih]

Attachment: Bug 746073: Part 1: Collect Per-App Traffic in HTTP Transaction.

Comment 81

[John Shih]

Attachment: Bug 746073: Part 1: Collect Per-App Traffic in HTTP Transaction.

Comment 82

[John Shih]

Attachment: Bug 746073: Part 0: nsINetworkManager.idl Modification.

Comment 83

[John Shih]

Attachment: Bug 746073: Per-App Traffic Metering in HTTP Transaction.

patch update

Comment 84

[John Shih]

Attachment: Bug 746073: Per-App Traffic Metering in HTTP Transaction.

Comment 85

[John Shih]

Attachment: Bug 746073: Per-App Traffic Metering in HTTP Transaction.

Comment 86

[John Shih]

Attachment: bug_746073.patch

Comment 87

[John Shih]

Attachment: Bug 746073: Per-App Traffic Metering in HTTP Transaction.

Comment 88

[John Shih]

Build Result:

https://tbpl.mozilla.org/?tree=Try&rev=0e108bffd094

Comment 89

[John Shih]

Attachment: Bug 746073: Per-App Traffic Metering in HTTP Transaction.

Comment 90

[John Shih]

Comment on attachment 786835 [details] [diff] [review]
Bug 746073: Per-App Traffic Metering in HTTP Transaction.

Hi Patrick,
here are the responses of the two questions you mentioned,
please see below

1) stop issue
After several tests, we found that the stopping issue will not cause traffic loss
that is, when we stop big downloads that cross a quota while they are in progress, the quota which already sent/received will still be saved after the app/process is killed.

2) cross-platform issue
I've added flag to handle the cross-platform issue,
the push result is in: https://tbpl.mozilla.org/?tree=Try&rev=0e108bffd094

thanks

Comment 91

[John Shih]

Attachment: Bug 746073: Per-App Traffic Metering in HTTP Transaction.

Comment 92

[Patrick McManus [:mcmanus]]

Comment on attachment 790000 [details] [diff] [review]
Bug 746073: Per-App Traffic Metering in HTTP Transaction.

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

it looks like a let a review bitrot - sorry about that, somehow I didn't notice the request.

::: netwerk/protocol/http/nsHttpTransaction.h
@@ +339,5 @@
>      nsCOMPtr<nsICancelable> mTokenBucketCancel;
> +
> +    // These members are used for network per-app metering (bug 746073)
> +    // Currently, they are only available on gonk.
> +#define NETWORK_NO_TYPE -1

const static int32_t NETWORK_NO_TYPE = -1

(so we don't pollute the preprocessor)

Comment 93

[John Shih]

Attachment: Bug 746073: Per-App Traffic Metering in HTTP Transaction.

Patch Revise

Comment 95

[John Shih]

Attachment: Bug 746073: Per-App Traffic Metering in HTTP Transaction.

Improve the patch with add finer granularity in recording traffic.

Comment 96

[Patrick McManus [:mcmanus]]

Comment on attachment 806457 [details] [diff] [review]
Bug 746073: Per-App Traffic Metering in HTTP Transaction.

thanks goodness interdiff worked :)

Comment 97

[John Shih]

Thanks for your quick review! ;)

Comment 98

[Honza Bambas (:mayhemer)]

Comment on attachment 806457 [details] [diff] [review]
Bug 746073: Per-App Traffic Metering in HTTP Transaction.

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

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +718,5 @@
> +    nsCOMPtr<nsINetworkManager> networkManager = do_GetService("@mozilla.org/network/manager;1", &result);
> +
> +    if (NS_FAILED(result) || !networkManager) {
> +        *type = NETWORK_NO_TYPE;
> +    }

Are you missing return NS_OK; here?

@@ +731,5 @@
> +    return NS_OK;
> +#else
> +    return NS_ERROR_NOT_IMPLEMENTED;
> +#endif
> +}

This method is not using anything from the transaction, actually is independent on transaction.  So why is it part of nsHttpTransaction and not just a helper function?

Comment 99

[Honza Bambas (:mayhemer)]

Comment on attachment 806457 [details] [diff] [review]
Bug 746073: Per-App Traffic Metering in HTTP Transaction.

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

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +731,5 @@
> +    return NS_OK;
> +#else
> +    return NS_ERROR_NOT_IMPLEMENTED;
> +#endif
> +}

Maybe leave this as a method and call it DetermineConnectionType and make it work with the mConnectionType member directly?

@@ +743,5 @@
> +    SaveNetworkStatsEvent(uint32_t aAppId,
> +                          int32_t aConnectionType,
> +                          uint64_t aCountRecv,
> +                          uint64_t aCountSent)
> +#ifdef MOZ_WIDGET_GONK

Can you #ifdef the whole class rather?  It's a dead code on !MOZ_WIDGET_GONK anyway.

Or do you need this class somewhere else in the code even when !MOZ_WIDGET_GONK?

If not, it should also be in namespace { } to prevent export of this class from the module.

::: netwerk/protocol/http/nsHttpTransaction.h
@@ +341,5 @@
> +// These members are used for network per-app metering (bug 746073)
> +// Currently, they are only available on gonk.
> +public:
> +    const static int32_t NETWORK_NO_TYPE = -1; // default conntection type
> +    const static uint64_t NETWORK_STATS_THRESHOLD = 65536;

I wouldn't be against making this threshold even larger.  For downloads on faster connection we could flood the main thread.

Comment 100

[Patrick McManus [:mcmanus]]

type
> > +    const static uint64_t NETWORK_STATS_THRESHOLD = 65536;
> 
> I wouldn't be against making this threshold even larger.  For downloads on
> faster connection we could flood the main thread.

I don't think there is a lot of evidence for simple events being a problem. complicated events (parsing, decoding, etc..) sure have that problem. but ontransport() and the like are not an issue and they would be 40x more often.

Part of the issue here is that the manager might want to cancel the transaction based on quota management of some sort.. so some kind of responsive granularity is required.

Comment 101

[John Shih]

(In reply to Patrick McManus [:mcmanus] from comment #100)
> type
> > > +    const static uint64_t NETWORK_STATS_THRESHOLD = 65536;
> > 
> > I wouldn't be against making this threshold even larger.  For downloads on
> > faster connection we could flood the main thread.
> 
> I don't think there is a lot of evidence for simple events being a problem.
> complicated events (parsing, decoding, etc..) sure have that problem. but
> ontransport() and the like are not an issue and they would be 40x more often.
> 
> Part of the issue here is that the manager might want to cancel the
> transaction based on quota management of some sort.. so some kind of
> responsive granularity is required.

According to what Patric said, I think we can keep the original threshold (64 kb)
for now. right? 
larger one

Comment 102

[Jason Duell]

>  keep the original threshold (64 kb)

Yes, let's keep that for now.  Go ahead and fix the things Honza found in his review and we should be ready to land with one more quick round of review if we're lucky.

Comment 103

[John Shih]

Attachment: Bug 746073: Per-App Traffic Metering in HTTP Transaction.

patch update

Comment 104

[John Shih]

Attachment: Bug 746073: Per-App Traffic Metering in HTTP Transaction.

Comment 105

[Honza Bambas (:mayhemer)]

Comment on attachment 810556 [details] [diff] [review]
Bug 746073: Per-App Traffic Metering in HTTP Transaction.

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

Thanks!

r=honzab

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +714,5 @@
> +{
> +#ifdef MOZ_WIDGET_GONK
> +    MOZ_ASSERT(NS_IsMainThread());
> +
> +    nsresult result;

nit: we use 'rv' as name for nsresult local vars

Comment 106

[John Shih]

Attachment: Bug 746073: Per-App Traffic Metering in HTTP Transaction.

rebase the patch according to the changes in bug 887699.

Comment 107

[Honza Bambas (:mayhemer)]

Comment on attachment 817732 [details] [diff] [review]
Bug 746073: Per-App Traffic Metering in HTTP Transaction.

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

r=honzab with all the comments fixed before landing.

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +231,5 @@
>      }
>  
> +    // obtain app info
> +    mChannel = do_QueryInterface(eventsink);
> +    nsCOMPtr<nsIChannel> channel = do_QueryInterface(mChannel);

first, you are doing QI twice, second I don't think it's a good idea to keep channel referenced when not needed.  Just revert the mChannel = do_... line back at its place.

@@ +713,5 @@
> +#ifdef MOZ_WIDGET_GONK
> +    MOZ_ASSERT(NS_IsMainThread());
> +
> +    nsresult rv;
> +    nsCOMPtr<nsINetworkManager> networkManager = do_GetService("@mozilla.org/network/manager;1", &rv);

80 cols

@@ +720,5 @@
> +        mActiveNetwork = nullptr;
> +        return;
> +    }
> +
> +    rv = networkManager->GetActive(getter_AddRefs(mActiveNetwork));

the lone rv assignment is strange.

@@ +764,5 @@
> +        return NS_OK;
> +    }
> +private:
> +    uint32_t mAppId;
> +    nsINetworkInterface *mActiveNetwork;

definitely needs a com ptr

::: netwerk/protocol/http/nsHttpTransaction.h
@@ +345,5 @@
> +private:
> +    uint64_t                           mCountRecv;
> +    uint64_t                           mCountSent;
> +    uint32_t                           mAppId;
> +    int32_t                            mConnectionType;

still needed?

@@ +346,5 @@
> +    uint64_t                           mCountRecv;
> +    uint64_t                           mCountSent;
> +    uint32_t                           mAppId;
> +    int32_t                            mConnectionType;
> +    bool                               mIsInBrowser;

mIsInBrowser is not used

Comment 108

[John Shih]

> 
> @@ +764,5 @@
> > +        return NS_OK;
> > +    }
> > +private:
> > +    uint32_t mAppId;
> > +    nsINetworkInterface *mActiveNetwork;
> 
> definitely needs a com ptr
> 

I've tried to used nsCOMPtr<nsINetworkInterface> here but failed at the line

mActiveNetwork(aActiveNetwork)

because such initialization must be done in main thread and we can only make sure
the code will run in the main thread in Run()

So this might be a work around to solve the problem.

do you have any concern or other thoughts on it? Thanks.

Comment 109

[Honza Bambas (:mayhemer)]

(In reply to John Shih from comment #108)
> do you have any concern or other thoughts on it? Thanks.

Hmm.. the mainthread ptr holder/handler may help (but not sure, I've never used it) [1].  Other option is to href the channel that hrefs the network.  Thus you will be sure the object won't die because of gc.  Then just add a comment why you do it.

[1] http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsProxyRelease.h#67

Comment 110

[John Shih]

Attachment: Bug 746073: Per-App Traffic Metering in HTTP Transaction.

I tried to use mainthread ptr holder/handler and it seems work!
can you have the check on it again? thanks

Comment 111

[Honza Bambas (:mayhemer)]

Comment on attachment 821457 [details] [diff] [review]
Bug 746073: Per-App Traffic Metering in HTTP Transaction.

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

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +763,5 @@
> +
> +        // save the network stats through NetworkStatsServiceProxy
> +        mNetworkStatsServiceProxy->SaveAppStats(mAppId, mActiveNetwork,
> +                                                PR_Now() / 1000, mCountRecv,
> +                                                mCountSent, nullptr);

nit: leave mCountRecv, mCountSent on the same line or put each on a separate line.

::: netwerk/protocol/http/nsHttpTransaction.h
@@ +18,5 @@
> +#include "nsProxyRelease.h"
> +
> +#ifdef MOZ_WIDGET_GONK
> +#include "nsINetworkManager.h"
> +#endif

Do you really need to include this in the header?

Seems like predeclaring nsINetworkInterface should work.

Comment 112

[John Shih]

Attachment: Bug 746073: Per-App Traffic Metering in HTTP Transaction.

r=honzab

Comment 113

[John Shih]

Thanks for your review!

(In reply to Honza Bambas (:mayhemer) from comment #111)
> Comment on attachment 821457 [details] [diff] [review]
> Bug 746073: Per-App Traffic Metering in HTTP Transaction.
> 
> Review of attachment 821457 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/protocol/http/nsHttpTransaction.cpp
> @@ +763,5 @@
> > +
> > +        // save the network stats through NetworkStatsServiceProxy
> > +        mNetworkStatsServiceProxy->SaveAppStats(mAppId, mActiveNetwork,
> > +                                                PR_Now() / 1000, mCountRecv,
> > +                                                mCountSent, nullptr);
> 
> nit: leave mCountRecv, mCountSent on the same line or put each on a separate
> line.
> 
> ::: netwerk/protocol/http/nsHttpTransaction.h
> @@ +18,5 @@
> > +#include "nsProxyRelease.h"
> > +
> > +#ifdef MOZ_WIDGET_GONK
> > +#include "nsINetworkManager.h"
> > +#endif
> 
> Do you really need to include this in the header?
> 
> Seems like predeclaring nsINetworkInterface should work.

I think we still need it
because nsINetworkInterface is defined in the nsINetworkManager
and we also need to obtain the nsINetworkInterface through NetworkManager.

Comment 114

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f1b840b5717d

Comment 115

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f1b840b5717d

Comment 116

[John Shih]

In addition to internal testing, it should wait for implementation in Bug 927793 so that this bug can be verified.

Comment 117

[Ioana (away)]

removing keyword for now, per c116