Record data use of user traffic by different core page transition types

Record data use of user traffic by different core page transition types

BUG=679026
Review-Url: https://codereview.chromium.org/2614203002
Cr-Commit-Position: refs/heads/master@{#458347}
Committed: https://chromium.googlesource.com/chromium/src/+/002f07fbe7776175f47bf49a284f673e26c409b3

[Raj, 2017-01-06 21:05:13]

rajendrant@chromium.org changed reviewers:
+ ryansturm@chromium.org

[Raj, 2017-01-06 21:05:14]

ptal.

Not sure if there is any easy way to add unittests for this.

[RyanSturm, 2017-01-06 21:56:39]

Hey, I won't have time to fully review this today, unfortunately. I will
probably have a few nits on Monday, but the design as a whole seems good.

My one question is if core page transitions are really the only thing you want
to record. I'm sure you don't care about some of the core transitions (e.g.,
subframes) and some of the non-core transitions are interesting; forward back
navigations are very interesting as they have different caching behavior and
getting information on those is pretty interesting.

If forward/back doesn't matter, than this approach works for me.

https://codereview.chromium.org/2614203002/diff/1/components/data_use_measure...
File components/data_use_measurement/core/data_use_measurement.cc (right):

https://codereview.chromium.org/2614203002/diff/1/components/data_use_measure...
components/data_use_measurement/core/data_use_measurement.cc:374: LOG(WARNING)
<< "DATAUSE page transition " << recorder << " " << &request
remove the log before landing.

[Raj, 2017-01-06 22:12:31]

On 2017/01/06 21:56:39, Ryan Sturm wrote:
> Hey, I won't have time to fully review this today, unfortunately. I will
> probably have a few nits on Monday, but the design as a whole seems good.
> 
> My one question is if core page transitions are really the only thing you want
> to record. I'm sure you don't care about some of the core transitions (e.g.,
> subframes) and some of the non-core transitions are interesting; forward back
> navigations are very interesting as they have different caching behavior and
> getting information on those is pretty interesting.
> 
> If forward/back doesn't matter, than this approach works for me.
> 
>
https://codereview.chromium.org/2614203002/diff/1/components/data_use_measure...
> File components/data_use_measurement/core/data_use_measurement.cc (right):
> 
>
https://codereview.chromium.org/2614203002/diff/1/components/data_use_measure...
> components/data_use_measurement/core/data_use_measurement.cc:374: LOG(WARNING)
> << "DATAUSE page transition " << recorder << " " << &request
> remove the log before landing.

Thanks. pls take your time to review.
Recording forward/back also matters. 
Maybe I should separate out enum in histograms.xml to add forward, back,
homepage navigations.

[RyanSturm, 2017-01-10 18:32:24]

Here's a few more more nits. The design looks good aside from needing to track
back/forward behavior.

https://codereview.chromium.org/2614203002/diff/1/chrome/browser/data_use_mea...
File chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.h
(right):

https://codereview.chromium.org/2614203002/diff/1/chrome/browser/data_use_mea...
chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.h:66: void
DidFinishNavigation(content::NavigationHandle* navigation_handle);
Comment, feel free to make it simple like: Forwarded from
DataUseWebContentsObserver

https://codereview.chromium.org/2614203002/diff/1/components/data_use_measure...
File components/data_use_measurement/content/content_url_request_classifier.cc
(right):

https://codereview.chromium.org/2614203002/diff/1/components/data_use_measure...
components/data_use_measurement/content/content_url_request_classifier.cc:35:
ui::PageTransition core_page_transition = ui::PageTransitionStripQualifier(
Like I mentioned before, this shouldn't be necessary.

https://codereview.chromium.org/2614203002/diff/1/components/data_use_measure...
components/data_use_measurement/content/content_url_request_classifier.cc:45:
"DataUse.PageTransition.UserTraffic",
Do you need to address the issue mpearson raised on a bug about these
histograms? You can differ if you are cleaning them all up at once.

https://codereview.chromium.org/2614203002/diff/1/components/data_use_measure...
File components/data_use_measurement/content/content_url_request_classifier.h
(right):

https://codereview.chromium.org/2614203002/diff/1/components/data_use_measure...
components/data_use_measurement/content/content_url_request_classifier.h:21:
bool IsUserRequest(const net::URLRequest& request) const override;
Two things: Can you add "// UrlRequestClassifier:" comment and can you make
these methods private so they are only accessible through the base class

https://codereview.chromium.org/2614203002/diff/1/components/data_use_measure...
File components/data_use_measurement/core/data_use_recorder.h (right):

https://codereview.chromium.org/2614203002/diff/1/components/data_use_measure...
components/data_use_measurement/core/data_use_recorder.h:107: uint64_t
page_transition_;
Can you add a comment since this is just a uint64_t and not an enum.

[Raj, 2017-01-10 22:21:58]

https://codereview.chromium.org/2614203002/diff/1/chrome/browser/data_use_mea...
File chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.h
(right):

https://codereview.chromium.org/2614203002/diff/1/chrome/browser/data_use_mea...
chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.h:66: void
DidFinishNavigation(content::NavigationHandle* navigation_handle);
On 2017/01/10 18:32:23, Ryan Sturm wrote:
> Comment, feel free to make it simple like: Forwarded from
> DataUseWebContentsObserver

Done.

https://codereview.chromium.org/2614203002/diff/1/components/data_use_measure...
File components/data_use_measurement/content/content_url_request_classifier.cc
(right):

https://codereview.chromium.org/2614203002/diff/1/components/data_use_measure...
components/data_use_measurement/content/content_url_request_classifier.cc:35:
ui::PageTransition core_page_transition = ui::PageTransitionStripQualifier(
On 2017/01/10 18:32:23, Ryan Sturm wrote:
> Like I mentioned before, this shouldn't be necessary.

Done.

https://codereview.chromium.org/2614203002/diff/1/components/data_use_measure...
components/data_use_measurement/content/content_url_request_classifier.cc:45:
"DataUse.PageTransition.UserTraffic",
On 2017/01/10 18:32:23, Ryan Sturm wrote:
> Do you need to address the issue mpearson raised on a bug about these
> histograms? You can differ if you are cleaning them all up at once.

Its not clear whether the overflow bug should be fixed, and how it should be
fixed. In general, UMA is uploaded once every 30 mins, and the overflow occurs
at 4GB. So, this is a less-likely corner case for android users.

One approach is to log sparse histograms (individual UMA for each of the types,
instead of using enum). However, that would increase the amount of histograms
and may be overkill. tbansal@ mentioned it is performance inefficient too. 

Another approach is to aggregate and log the UMA in terms of 1K bytes, which
would lose some precision, and miss some bytes from users with low usage.

https://g3doc.corp.google.com/analysis/uma/g3doc/basic-introduction.md?cl=head

https://codereview.chromium.org/2614203002/diff/1/components/data_use_measure...
File components/data_use_measurement/content/content_url_request_classifier.h
(right):

https://codereview.chromium.org/2614203002/diff/1/components/data_use_measure...
components/data_use_measurement/content/content_url_request_classifier.h:21:
bool IsUserRequest(const net::URLRequest& request) const override;
On 2017/01/10 18:32:23, Ryan Sturm wrote:
> Two things: Can you add "// UrlRequestClassifier:" comment and can you make
> these methods private so they are only accessible through the base class

Done.

https://codereview.chromium.org/2614203002/diff/1/components/data_use_measure...
File components/data_use_measurement/core/data_use_measurement.cc (right):

https://codereview.chromium.org/2614203002/diff/1/components/data_use_measure...
components/data_use_measurement/core/data_use_measurement.cc:374: LOG(WARNING)
<< "DATAUSE page transition " << recorder << " " << &request
On 2017/01/06 21:56:38, Ryan Sturm wrote:
> remove the log before landing.

Done.

https://codereview.chromium.org/2614203002/diff/1/components/data_use_measure...
File components/data_use_measurement/core/data_use_recorder.h (right):

https://codereview.chromium.org/2614203002/diff/1/components/data_use_measure...
components/data_use_measurement/core/data_use_recorder.h:107: uint64_t
page_transition_;
On 2017/01/10 18:32:24, Ryan Sturm wrote:
> Can you add a comment since this is just a uint64_t and not an enum.

Done.

[RyanSturm, 2017-01-10 22:56:19]

lgtm % nit

https://codereview.chromium.org/2614203002/diff/20001/chrome/browser/data_use...
File chrome/browser/data_use_measurement/chrome_data_use_ascriber.h (right):

https://codereview.chromium.org/2614203002/diff/20001/chrome/browser/data_use...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.h:107: void
DidFinishNavigation(int render_process_id,
method Comment.

[Raj, 2017-01-12 03:26:58]

rajendrant@chromium.org changed reviewers:
+ rkaplow@chromium.org

[Raj, 2017-01-12 03:26:58]

rkaplow: ptal histograms.xml

[rkaplow, 2017-01-12 16:28:06]

https://codereview.chromium.org/2614203002/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2614203002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:9531: +<histogram
name="DataUse.PageTransition.UserTraffic"
I don't think this design is correct, since this way you won't know the
distribution of bytes, everything will get grouped together.

I think you want a separate histogram for eac page transition. You can use the
histogram_suffix notation in the xml file to help generate the correct config.
Each one will be a numeric histogram measuring it in bytes.

[Raj, 2017-02-14 22:52:51]

rkaplow: ptal

https://codereview.chromium.org/2614203002/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2614203002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:9531: +<histogram
name="DataUse.PageTransition.UserTraffic"
On 2017/01/12 16:28:06, rkaplow wrote:
> I don't think this design is correct, since this way you won't know the
> distribution of bytes, everything will get grouped together.
> 
> I think you want a separate histogram for eac page transition. You can use the
> histogram_suffix notation in the xml file to help generate the correct config.
> Each one will be a numeric histogram measuring it in bytes.

I thought about it. It will be 10+ histograms to log that. We are not planning
to look at distribution of bytes by page transition. We have
DataUse.TrafficSize.* histograms to get the general distribution of bytes.

WDYT.

[rkaplow, 2017-02-15 15:28:06]

lgtm

[Raj, 2017-02-22 19:44:38]

rajendrant@chromium.org changed reviewers:
+ ccameron@chromium.org

[Raj, 2017-02-22 19:44:39]

ccameron: ptal components/data_use_measurement/content/DEPS

[ccameron, 2017-03-20 18:05:56]

On 2017/02/22 19:44:39, Raj wrote:
> ccameron: ptal components/data_use_measurement/content/DEPS

lgtm

[rajendrant, 2017-03-20 19:49:56]

The CQ bit was checked by rajendrant@google.com

[rajendrant, 2017-03-20 19:49:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
ryansturm@chromium.org, rkaplow@chromium.org
Link to the patchset: https://codereview.chromium.org/2614203002/#ps40001
(title: "rebased")

[commit-bot: I haz the power, 2017-03-20 19:50:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-20 19:59:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-20 19:59:11]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[rajendrant, 2017-03-21 06:40:32]

The CQ bit was checked by rajendrant@google.com

[rajendrant, 2017-03-21 06:40:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
ryansturm@chromium.org, rkaplow@chromium.org, ccameron@chromium.org
Link to the patchset: https://codereview.chromium.org/2614203002/#ps60001
(title: "rebased")

[commit-bot: I haz the power, 2017-03-21 06:40:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-21 07:42:07]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1490078432248890,
"parent_rev": "5edd9265accbe0191b1eddfdce65ffa3e7d631d8", "commit_rev":
"002f07fbe7776175f47bf49a284f673e26c409b3"}

[commit-bot: I haz the power, 2017-03-21 07:42:48]

Description was changed from

==========
Record data use of user traffic by different core page transition types

BUG=679026
==========

to

==========
Record data use of user traffic by different core page transition types

BUG=679026

Review-Url: https://codereview.chromium.org/2614203002
Cr-Commit-Position: refs/heads/master@{#458347}
Committed:
https://chromium.googlesource.com/chromium/src/+/002f07fbe7776175f47bf49a284f...
==========

[commit-bot: I haz the power, 2017-03-21 07:42:49]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/002f07fbe7776175f47bf49a284f...