This patch demonstrates the preliminary idea to collector the traffic of TCP socket.

This first patch demonstrates how we collect traffic statistics for TCP socket. Note this bug depends on bug 746073 and bug 784575, which provide "per-App Network statistics service". The API "saveAppStats()" provided by NetworkStatsService requires three parameters as the keys to distinguish between different Apps and network types: * appId * isInBrowser * connectionType We added some codes within the initialization procedure of TCP Socket, in order to retrive appId and isInBrowser from TabContext (inherited by TabParent), which encapsulates information about an iframe that may be a mozbrowser or mozapp. For now, we invoke saveAppStats() for every TCP send/recv call. This might cause too much overhead. I am going to perform some tests to observe the performance impact. We would keep improve the patch according to our further work.

Comment on attachment 786216 [details] [diff] [review] Patch: Call NetworkStatsService API to collect TCP socket traffic Review of attachment 786216 [details] [diff] [review]: ----------------------------------------------------------------- Overall, it looks good to me but we need Necko peers' review as well. Maybe :mcmanus. Also, you need to make it Gonk (B2G) specific since we only do this for B2G at this moment. Please refer to Bug 746073 to see if we could have chance to do this. And you're right we need to refine the logic so that we won't accumulate the traffic for each read/write operation. We need to set a threshold for that and save the traffic numbers when exceeding that threshold. To do this, some issues need to be considered like how to update the cached traffic when a connection suddenly halts or the system shuts down. We may discuss this in person tomorrow. ::: dom/network/src/TCPSocket.js @@ +129,5 @@ > // Internal > _hasPrivileges: null, > > + // App Information > + _appId: null, Like _connectionType, I remember there is a default value and and a constant definition for appId. Please try to use that. @@ +131,5 @@ > > + // App Information > + _appId: null, > + _isInBrowser: null, > + _connectionType: -1, //FIXME: Replace -1 as the constant defined in nsNetworkManager.idl You can directky use Ci.interface to access that constant. @@ +400,5 @@ > > + > + /* Parent side */ > + // Query and set connection type > + let nm = Cc["@mozilla.org/network/manager;1"].getService(Ci.nsINetworkManager); Please use networkManager or networkMan. nm is too short to understand. @@ +479,5 @@ > } > > + /* Parent side */ > + let networkStats = Cc["@mozilla.org/networkstatsService;1"].getService(Ci.nsINetworkStatsService); > + let timeInMicroSec = Date.now() * 1000; Are you sure we're using micro seconds? Usually, we use seconds or nanoseconds to measure timestamps. Maybe my recall is wrong. Just double check.

1. Make the traffic collection Gonk-specific. 2. Add a simple caching mechanism for storing TX/RX amounts to reduce overhead. 3. Initializing internal variables with proper constants.

Just remove the trailing space in the last patch.

Comment on attachment 792705 [details] [diff] [review] A patch to collect TCP socket traffic statistics per-App Review of attachment 792705 [details] [diff] [review]: ----------------------------------------------------------------- r=gene when the nits fixed. We still need mcmanus' second review. ::: dom/network/interfaces/nsIDOMTCPSocket.idl @@ +251,5 @@ > nsIDOMTCPSocket createAcceptedChild(in nsITCPSocketChild socketChild, > in DOMString binaryType, > in nsIDOMWindow window); > + > + // Set App ID Add a period at the end of a complete comment. @@ +252,5 @@ > in DOMString binaryType, > in nsIDOMWindow window); > + > + // Set App ID > + void setAppId(in uint32_t appId); s/unit32_t/unsigned long/ ::: dom/network/src/TCPSocket.js @@ +298,5 @@ > BUFFER_SIZE, /* close source*/ false, /* close sink */ false); > }, > > + /* Helper method for collecting network statistics */ > + // Note this method is Gonk-specific nit: add a period. @@ +302,5 @@ > + // Note this method is Gonk-specific > + _saveNetworkStats: function ts_saveNetworkStats(enforce) { > +#ifdef MOZ_WIDGET_GONK > + let totalBytes = this._txBytes + this._rxBytes; > + let timestamp = Date.now(); // unit: millisecond It's too early to get the timestamp. Please get it when calling: nssProxy.saveAppsStats(this._appId, this._connectionType, Date.now(), this._rxBytes, this._txBytes); and drop this line. @@ +304,5 @@ > +#ifdef MOZ_WIDGET_GONK > + let totalBytes = this._txBytes + this._rxBytes; > + let timestamp = Date.now(); // unit: millisecond > + > + if (totalBytes <= 0) { // no traffic at all, no need to save statistics Can you just use: // "N"o traffic at all, no need to save statistics"." if (this._txBytes <= 0 && this._rxBytes <= 0) { return; } So you don't need an extra summation. @@ +308,5 @@ > + if (totalBytes <= 0) { // no traffic at all, no need to save statistics > + return; > + } > + > + // If "enforce" is false, the traffic amount is saved to NetworkStatsServiceProxy nit: period. @@ +312,5 @@ > + // If "enforce" is false, the traffic amount is saved to NetworkStatsServiceProxy > + // only when the total amount exceeds the predefined threshold value. > + // The purpose is to avoid too much overhead for collecting traffic statistics. > + if (!enforce) { > + if (totalBytes < NETWORK_STATS_THRESHOLD) { if (!enforce && this._txBytes < NETWORK_STATS_THRESHOLD && this._rxBytes < NETWORK_STATS_THRESHOLD) { return; } @@ +318,5 @@ > + } > + } > + > + let nssProxy = Cc["@mozilla.org/networkstatsServiceProxy;1"] > + .getService(Ci.nsINetworkStatsServiceProxy); if (!nssProxy) { debug("Error: nsINetworkStatsServiceProxy service is not available.") return; } @@ +323,5 @@ > + > + nssProxy.saveAppsStats(this._appId, this._connectionType, timestamp, > + this._rxBytes, this._txBytes); > + > + // reset counters once the statistics is saved to NetworkStatsServiceProxy nit: "R"... NetworkStatsServiceProxy"." @@ +327,5 @@ > + // reset counters once the statistics is saved to NetworkStatsServiceProxy > + this._txBytes = this._rxBytes = 0; > +#endif > + }, > + /* end of Helper method for network statistics */ nit: s/H/h @@ +499,5 @@ > transport.setEventSink(that, Services.tm.currentThread); > that._initStream(that._binaryType); > + > + let networkManager = Cc["@mozilla.org/network/manager;1"].getService(Ci.nsINetworkManager); > + if (networkManager.active) { if (networkManager && networkManager.active) @@ +500,5 @@ > that._initStream(that._binaryType); > + > + let networkManager = Cc["@mozilla.org/network/manager;1"].getService(Ci.nsINetworkManager); > + if (networkManager.active) { > + that._connectionType = networkManager.active.type; nit: 2-space indention. @@ +584,5 @@ > } > > this._ensureCopying(); > + > + // Collect transmitted amount for network statistics nit: period. @@ +813,5 @@ > } else { > this.callListener("data", this._inputStreamScriptable.read(count)); > } > + > + // Collect transmitted amount for network statistics nit: period. ::: dom/network/src/TCPSocketParent.cpp @@ +96,5 @@ > return true; > } > > + nsRefPtr<mozilla::dom::TabParent> mTabParent; > + mTabParent = static_cast<mozilla::dom::TabParent*>(aBrowser); 1. Please merge this two lines into one. 2. You don't need mozilla::dom because you are already in this namespace. nsRefPtr<TabParent> tabParent = static_cast<TabParent*>(aBrowser); @@ +99,5 @@ > + nsRefPtr<mozilla::dom::TabParent> mTabParent; > + mTabParent = static_cast<mozilla::dom::TabParent*>(aBrowser); > + > + rv = mIntermediary->Open(this, aHost, aPort, aUseSSL, aBinaryType, mTabParent->OwnAppId(), > + getter_AddRefs(mSocket)); Fold this line to be: rv = mIntermediary->Open(this, aHost, aPort, aUseSSL, aBinaryType, tabParent->OwnAppId(), getter_AddRefs(mSocket)); ::: dom/network/src/TCPSocketParentIntermediary.js @@ +36,5 @@ > if (!socket) > return null; > > + let socketInternal = socket.QueryInterface(Ci.nsITCPSocketInternal); > + socketInternal.setAppId(aAppId); if (socketInternal) { socketInternal.setAppId(aAppId); }

Although Gene already review+ the last patch, we have to redo the patch due to code conflicts from "Bug 890570 - PTCPSocket constructor doesn't need PBrowser". Hi, Patrick, Gene recommended you as my reviewer. Would you like to review my patch?

Modify moz.build to eliminate a compiler warning.

Comment on attachment 797082 [details] [diff] [review] bug-855951-fix-v5.patch Review of attachment 797082 [details] [diff] [review]: ----------------------------------------------------------------- I'm fine with this. The only thing I would mention is that this implementation tracks bytes as the stream progresses (in 64KB chunks), but the HTTP version in 746073 only reports at the end of the http transaction. I wonder if the HTTP version needs finer granularity too.

Hi Patrick, absolutely it would be better to get more accurate data in HTTP layer, do you have any idea? I remember that it seems hard to get more accurate traffic in http transaction. however, it also the more convenient place to identify the per-app traffic.

Hi, Patrick, Thanks for your review. This bug depends on bug 784575. I will push this patch to the try server once the changesets of bug 784575 are landed.

(In reply to John Shih from comment #10) > Hi Patrick, > absolutely it would be better to get more accurate data in HTTP layer, > do you have any idea? > > I remember that it seems hard to get more accurate traffic in http > transaction. > however, it also the more convenient place to identify the per-app traffic. Is there a reason you can't do the same thing in http as you do here? (i.e. check a threshold at the time you update the counters and report the data if the threshold is exceeded)

oh, I see I'll revise the patch bug 746073 thanks for clarifying that!

Fixed failure happened in try server automation tests. (nsINetworkManager should be Gonk-specific)

This patch deals with the build problem across different platforms. TCPSocket API itself is a cross-platform component, however, NetworkStatsServiceProxy and NetworkManager are both Gonk-specific components. In order to avoid unnecesary code bloat and make the code more clear, we use #ifdef to wrap all the code snippets related to saving network statistics.