855951 - [Per App Network Traffic Metering] Collect per app traffic in TCP Socket API

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Comment 1

[Ethan Tseng [:ethan]]

Attachment: Patch: Call NetworkStatsService API to collect TCP socket traffic

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

Comment 2

[Ethan Tseng [:ethan]]

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 3

[Gene Lian (I already quit Mozilla)]

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.

Comment 4

[Ethan Tseng [:ethan]]

Attachment: A patch to collect TCP socket traffic statistics per-App

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.

Comment 5

[Ethan Tseng [:ethan]]

Attachment: A patch to collect TCP socket traffic statistics per-App

Just remove the trailing space in the last patch.

Comment 6

[Gene Lian (I already quit Mozilla)]

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);
}

Comment 7

[Ethan Tseng [:ethan]]

Attachment: A patch to collect TCP socket traffic statistics per-App

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?

Comment 8

[Ethan Tseng [:ethan]]

Attachment: bug-855951-fix-v5.patch

Modify moz.build to eliminate a compiler warning.

Comment 9

[Patrick McManus [:mcmanus]]

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.

Comment 10

[John Shih]

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.

Comment 11

[Ethan Tseng [:ethan]]

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.

Comment 12

[Patrick McManus [:mcmanus]]

(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)

Comment 13

[John Shih]

oh, I see
I'll revise the patch bug 746073

thanks for clarifying that!

Comment 14

[Ethan Tseng [:ethan]]

Attachment: bug-855951-fix-v6.patch

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

Comment 15

[Ethan Tseng [:ethan]]

Attachment: bug-855951-fix-v7.patch

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.

Comment 16

[Gene Lian (I already quit Mozilla)]

https://hg.mozilla.org/integration/b2g-inbound/rev/3c9b91e9f8b6

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/3c9b91e9f8b6