Closed Bug 855951 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
1.2 FC (16sep)

People

(Reporter: johnshih.bugs, Assigned: ethan)

References

Details

Attachments

(1 file, 6 obsolete files)

No description provided.
Assignee: nobody → ettseng
No longer blocks: 784575
Depends on: 784575, 746073
This patch demonstrates the preliminary idea to collector the traffic of TCP socket.
Attachment #786216 - Flags: review?(gene.lian)
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.
Attachment #786216 - Flags: review?(gene.lian)
Blocks: 746073
No longer depends on: 746073
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
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.
Attachment #786216 - Attachment is obsolete: true
Attachment #792696 - Flags: review?(clian)
Just remove the trailing space in the last patch.
Attachment #792696 - Attachment is obsolete: true
Attachment #792696 - Flags: review?(clian)
Attachment #792705 - Flags: review?(clian)
Attachment #792705 - Flags: review?(clian) → review?(gene.lian)
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);
}
Attachment #792705 - Flags: review?(gene.lian) → review+
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?
Attachment #792705 - Attachment is obsolete: true
Attachment #795916 - Flags: review?(mcmanus)
Attached patch bug-855951-fix-v5.patch (obsolete) — Splinter Review
Modify moz.build to eliminate a compiler warning.
Attachment #795916 - Attachment is obsolete: true
Attachment #795916 - Flags: review?(mcmanus)
Attachment #797082 - Flags: review?(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.
Attachment #797082 - Flags: review?(mcmanus) → review+
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.
Flags: needinfo?(mcmanus)
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)
Flags: needinfo?(mcmanus)
oh, I see
I'll revise the patch bug 746073

thanks for clarifying that!
No longer blocks: 746073
Status: NEW → ASSIGNED
Attached patch bug-855951-fix-v6.patch (obsolete) — Splinter Review
Fixed failure happened in try server automation tests. (nsINetworkManager should be Gonk-specific)
Attachment #797082 - Attachment is obsolete: true
Attachment #805830 - Flags: review?(gene.lian)
Blocks: 917667
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.
Attachment #805830 - Attachment is obsolete: true
Attachment #805830 - Flags: review?(gene.lian)
Attachment #806459 - Flags: review?(gene.lian)
Attachment #806459 - Attachment description: Collect and save TX/RX traffic amounts of TCP connections per-App → bug-855951-fix-v7.patch
Attachment #806459 - Flags: review?(gene.lian) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3c9b91e9f8b6
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.2 FC (16sep)
Blocks: 925670
You need to log in before you can comment on or make changes to this bug.