1042149 - Expose Application Manifest URL in Network Statistics

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::Cost Control, defect)

Description

[jhobin]

Modify NetworkStats API, adding a field to returned data to make it easier to track data by app with a minimal amount of round-trips to Gecko. Also make querying for all available network data possible.

Comment 1

[jhobin]

Attachment: Patch to expose manifestURL in data results

This also improves the process for querying all available data, considering the `null` appManifestURL to be a wildcard.

Comment 2

[Marshall Culpepper [:marshall_law]]

Comment on attachment 8462913 [details] [diff] [review]
Patch to expose manifestURL in data results

Tagging NetworkStatsDB folks for review (not sure which or how many we should need)

Comment 3

[John Shih]

Comment on attachment 8462913 [details] [diff] [review]
Patch to expose manifestURL in data results

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

::: dom/network/src/NetworkStatsDB.jsm
@@ +760,5 @@
> +        upperAppId = aAppId;
> +      }
> +
> +      let lowerFilter = [lowerAppId, aServiceType, network, start];
> +      let upperFilter = [upperAppId, aServiceType, network, end];

You don't have to do this. AppId and ServiceType were designed to be mutually exclusive. For per-app stats, serviceType is forced to be an empty string and vice versa (appId is forced to be 0).

Per-app stats: network traffic of an app.
Per-service: network traffic of a system service, which is a part of main process such as OTA, Tethering, WiFi Scanning.. etc.

@@ +773,4 @@
>        let request = aStore.openCursor(range).onsuccess = function(event) {
>          var cursor = event.target.result;
>          if (cursor){
> +          let app = this.appsService.getAppByLocalId(cursor.value.appId) || {manifestURL: ""};

You can check if appId is 0 or not before doing this.

::: dom/network/src/NetworkStatsService.jsm
@@ +405,4 @@
>      let network = msg.network;
>      let netId = this.getNetworkId(network.id, network.type);
>  
> +    let appId = null;

Per design, please leave appId to be 0 by default.

Comment 4

[John Shih]

Note, NetworkStats API is under refactoring recently. I'm afraid it will cause some dependencies for this bug (or in the opposite way). Please check Bug 1043830 for more detail! Thanks.

Comment 5

[Marshall Culpepper [:marshall_law]]

(In reply to John Shih from comment #4)
> Note, NetworkStats API is under refactoring recently. I'm afraid it will
> cause some dependencies for this bug (or in the opposite way). Please check
> Bug 1043830 for more detail! Thanks.

Thanks John.. is this refactor work scheduled to land for 2.1?

Comment 6

[jhobin]

(In reply to John Shih from comment #3)
> Comment on attachment 8462913 [details] [diff] [review]
> Patch to expose manifestURL in data results
> 
> Review of attachment 8462913 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/network/src/NetworkStatsDB.jsm
> @@ +760,5 @@
> > +        upperAppId = aAppId;
> > +      }
> > +
> > +      let lowerFilter = [lowerAppId, aServiceType, network, start];
> > +      let upperFilter = [upperAppId, aServiceType, network, end];
> 
> You don't have to do this. AppId and ServiceType were designed to be
> mutually exclusive. For per-app stats, serviceType is forced to be an empty
> string and vice versa (appId is forced to be 0).
> 
> Per-app stats: network traffic of an app.
> Per-service: network traffic of a system service, which is a part of main
> process such as OTA, Tethering, WiFi Scanning.. etc.
> 

The issue I have with this is that it doesn't solve the problem of needing to know an app's localId in order to access its stats.
The process in the current layout is effectively:
  1. Get a list of all installed apps
  2. For all of these apps, query some gecko service to get the localIds
  3. For all of these local ids, query NetworkStatsDB to get the per-app statistics

If null (as I have it) or 0 maps to returning every value and returning those values with their manifestURL, this is reduced to one query with no juggling of localIds.

> @@ +773,4 @@
> >        let request = aStore.openCursor(range).onsuccess = function(event) {
> >          var cursor = event.target.result;
> >          if (cursor){
> > +          let app = this.appsService.getAppByLocalId(cursor.value.appId) || {manifestURL: ""};
> 
> You can check if appId is 0 or not before doing this.
> 

This check also prevents junk data from being returned if an app gets installed by the user, uses some data, then is uninstalled by the user.

> ::: dom/network/src/NetworkStatsService.jsm
> @@ +405,4 @@
> >      let network = msg.network;
> >      let netId = this.getNetworkId(network.id, network.type);
> >  
> > +    let appId = null;
> 
> Per design, please leave appId to be 0 by default.

The problem with the current API is that using appId 0 to query for all app usage data is that it discards each individual data point's appId.

Comment 7

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

Comment on attachment 8462913 [details] [diff] [review]
Patch to expose manifestURL in data results

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

I don't understand:

1) Why do we need a duplicated "manifestURL" field in MozNetworkstatsData? We have already one in NetworkStats. Besides, reflected in ResourceStats API, interface ResourceStats has a field "manifestURL", but neither does NetworkStatsData nor PowerStatsData has that field.  A FooData interface is an interface representing those raw data. Why do you need manifestURL for? In what case do you need such field or things just can't be done?

2) Even we do want a duplicated field in MozNetworkstatsData, can its value differ from that in NetworkStats? Is a query with a specific app manifest URL supposed to return stats data with a different manifest URL at all?

Comment 8

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

Comment on attachment 8462913 [details] [diff] [review]
Patch to expose manifestURL in data results

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

::: dom/network/src/NetworkStatsDB.jsm
@@ +760,5 @@
> +        upperAppId = aAppId;
> +      }
> +
> +      let lowerFilter = [lowerAppId, aServiceType, network, start];
> +      let upperFilter = [upperAppId, aServiceType, network, end];

Here is the problem. NetworkStatsService::getSamples call NetworkStatsDB::find with aAppId and aAppManifestURL passed.  That aAppId is always nsIScriptSecurityManager::NO_APP_ID when aAppManifestURL is an empty string.  The original openCursor only returns records with following conditions met:

  1. cursor.value.appId === aAppId,
  2. cursor.value.serviceType === aServiceType
  3. cursor.value.network === network
  4. cursor.timestamp falls between "start" and "end"

So it means, the function call NetworkStatsService::getSamples, when invoked with an empty appManifestURL string, it returns stats data belongs to no apps -- either belongs to some system service or accumulative total sum.

However, this patch is to alter this behaviour but that cause another disaster. The modified cursor now returns:

  1. When no manifest url was passed: almost all the stats data in database no matter what you have in other fields.
  2. Otherwise, stats data belong to a certain app and trans-received via a certain network during a given time.

Note that IDBKeyRange is a "range". It's not some kind of "filter". The variable name here is quite misleading.  With a IDBKeyRange [0, "a", n1, 100]..[MAX, "a", n1, 10000], following records match:

  [0, "b", n3, 1]
  [1, null, null, 0]
  [2, "what", dontcare, -1]
  ...

That's always the thing you have to take special care with a multi-entry key.

Comment 9

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

(In reply to jhobin from comment #6)
> The problem with the current API is that using appId 0 to query for all app
> usage data is that it discards each individual data point's appId.

Yes. That's the problem with current API design.

  (appManifestURL, null) => per app stats
  (null, serviceType)    => per service stats
  (null, null)           => accumulative total stats

The same thing happens to service types. What if you want to plot a graph with the stats from all services? Even all services and all apps as individuals? I'd like to propose a "any" manifest URL a "any" service type instead of reinterpreting empty string names. Actually I don't like that empty string names, either. They should have been "all" apps/services.

Comment 10

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

Initiated a discussion on dev-webapi: https://groups.google.com/d/msg/mozilla.dev.webapi/tWkgbD1v_Gg/5pMH3KweITYJ

Comment 11

[Gene Lian (I already quit Mozilla)]

Sorry I already quited Mozilla and have to fully focus on my new job. Please ask Albert or Vicamo for reviews. Thanks!

Comment 12

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

Per https://groups.google.com/d/msg/mozilla.dev.webapi/tWkgbD1v_Gg/DucipIbK4uIJ , marked as WONTFIX. Please NI :ehsan if you still have other concern.