Removing and deprecating PLT DataReductionProxy UMA

Removing and deprecating PLT DataReductionProxy UMA

DataReductionProxy has moved to reporting PLT via PageLoad.* and no
longer needs the renderer based metrics. While the browser based metrics
do not cover all of the use cases, the new metrics are sufficient for
the analysis DRP needs moving forward.

This also removes the DRP IPC completely.

BUG=643307
Committed: https://crrev.com/a43953b70da976a0c565150cd938fa1b5d42a0c2
Cr-Commit-Position: refs/heads/master@{#417337}

[RyanSturm, 2016-09-06 22:11:45]

The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-06 22:12:23]

Dry run: 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, 2016-09-06 23:25:54]

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

[commit-bot: I haz the power, 2016-09-06 23:25:54]

Dry run: This issue passed the CQ dry run.

[RyanSturm, 2016-09-07 16:29:02]

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

[RyanSturm, 2016-09-07 16:29:02]

tbansal: PTAL at *

[tbansal1, 2016-09-07 16:50:55]

https://codereview.chromium.org/2314163003/diff/1/chrome/renderer/page_load_h...
File chrome/renderer/page_load_histograms.cc (right):

https://codereview.chromium.org/2314163003/diff/1/chrome/renderer/page_load_h...
chrome/renderer/page_load_histograms.cc:193: bool lofi_active_for_page,  // LoFi
was used, unless part of
Remove this too?

[RyanSturm, 2016-09-07 16:52:56]

The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run

[tbansal1, 2016-09-07 16:53:06]

Description was changed from

==========
Removing and deprecating PLT DataReductionProxy UMA

DataReductionProxy has moved to reporting PLT via PageLoad.* and no
longer needs the renderer based metrics. While the browser based metrics
do not cover all of the use cases, the new metrics are sufficient for
the analysis DRP needs moving forward.

This also removes the DRP IPC completely.

BUG=643307
==========

to

==========
Removing and deprecating PLT DataReductionProxy UMA

DataReductionProxy has moved to reporting PLT via PageLoad.* and no
longer needs the renderer based metrics. While the browser based metrics
do not cover all of the use cases, the new metrics are sufficient for
the analysis DRP needs moving forward.

This also removes the DRP IPC completely.

BUG=643307
==========

[commit-bot: I haz the power, 2016-09-07 16:53:30]

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

[RyanSturm, 2016-09-07 16:54:08]

Patchset #2 (id:20001) has been deleted

[tbansal1, 2016-09-07 16:54:21]

lgtm % lofi variable is removed.

[RyanSturm, 2016-09-07 16:55:43]

Patchset #2 (id:40001) has been deleted

[RyanSturm, 2016-09-07 16:58:38]

The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-07 16:59:08]

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

[RyanSturm, 2016-09-07 18:04:48]

The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-07 18:05:21]

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

[RyanSturm, 2016-09-07 18:14:42]

ryansturm@chromium.org changed reviewers:
+ asvitkine@chromium.org, bmcquade@chromium.org, dcheng@chromium.org,
thestig@chromium.org

[RyanSturm, 2016-09-07 18:14:44]

asvitkine: PTAL @ histograms.xml

bmcquade: PTAL @ page_load_histograms.cc

dcheng: PTAL at deleted IPC and message filter @
components/data_reduction_proxy/content/common/.*

thestig: PTAL @ chrome/ BUILD.GN, DEPS, chrome/common/chrome_content_client.*

https://codereview.chromium.org/2314163003/diff/1/chrome/renderer/page_load_h...
File chrome/renderer/page_load_histograms.cc (right):

https://codereview.chromium.org/2314163003/diff/1/chrome/renderer/page_load_h...
chrome/renderer/page_load_histograms.cc:193: bool lofi_active_for_page,  // LoFi
was used, unless part of
On 2016/09/07 16:50:55, tbansal1 wrote:
> Remove this too?

Done.

[Alexei Svitkine (slow), 2016-09-07 18:15:20]

lgtm

[Bryan McQuade, 2016-09-07 18:24:32]

Thank you very much for this cleanup! Just a couple small comments. While we are
planning on killing this file, and it's really helpful to have you remove the
DRP histograms as part of that cleanup, I do want to retain the non-DRP
histograms a bit longer in case people need them. Please see comments below for
details.

Thanks again for this!

https://codereview.chromium.org/2314163003/diff/80001/chrome/renderer/page_lo...
File chrome/renderer/page_load_histograms.cc (left):

https://codereview.chromium.org/2314163003/diff/80001/chrome/renderer/page_lo...
chrome/renderer/page_load_histograms.cc:285:
PLT_HISTOGRAM_DRP("PLT.NT_DomainLookup",
IIUC PLT_HISTOGRAM_DRP logged both a DRP histogram and the regular non-DRP
histogram. If that's the case, can we retain these and convert them to use
PLT_HISTOGRAM instead of PLT_HISTOGRAM_DRP? That way we'll continue to log the
non-DRP variants until we're ready to fully kill this class.

https://codereview.chromium.org/2314163003/diff/80001/chrome/renderer/page_lo...
chrome/renderer/page_load_histograms.cc:468:
PLT_HISTOGRAM_DRP("PLT.NT_LoadEvent",
same here

https://codereview.chromium.org/2314163003/diff/80001/chrome/renderer/page_lo...
chrome/renderer/page_load_histograms.cc:840: Send(new
DataReductionProxyViewHostMsg_IsDataReductionProxy(
i just want to confirm that this IPC message is no longer needed by other DRP
code?

[Lei Zhang, 2016-09-07 18:25:31]

chrome/ lgtm

[RyanSturm, 2016-09-07 18:36:16]

bmcquade: PTAL

https://codereview.chromium.org/2314163003/diff/80001/chrome/renderer/page_lo...
File chrome/renderer/page_load_histograms.cc (left):

https://codereview.chromium.org/2314163003/diff/80001/chrome/renderer/page_lo...
chrome/renderer/page_load_histograms.cc:285:
PLT_HISTOGRAM_DRP("PLT.NT_DomainLookup",
On 2016/09/07 18:24:32, Bryan McQuade wrote:
> IIUC PLT_HISTOGRAM_DRP logged both a DRP histogram and the regular non-DRP
> histogram. If that's the case, can we retain these and convert them to use
> PLT_HISTOGRAM instead of PLT_HISTOGRAM_DRP? That way we'll continue to log the
> non-DRP variants until we're ready to fully kill this class.

I removed these because I thought DRP were the owners of the baseline histograms
as well, and we no longer need them (I deprecated them in the histograms.xml
part of this change as well). I will keep them if you prefer. Can I switch the
ownership in histograms.xml to you, or should I leave the ownership on bolian or
me?

https://codereview.chromium.org/2314163003/diff/80001/chrome/renderer/page_lo...
chrome/renderer/page_load_histograms.cc:468:
PLT_HISTOGRAM_DRP("PLT.NT_LoadEvent",
On 2016/09/07 18:24:32, Bryan McQuade wrote:
> same here

See other reply.

https://codereview.chromium.org/2314163003/diff/80001/chrome/renderer/page_lo...
chrome/renderer/page_load_histograms.cc:840: Send(new
DataReductionProxyViewHostMsg_IsDataReductionProxy(
On 2016/09/07 18:24:32, Bryan McQuade wrote:
> i just want to confirm that this IPC message is no longer needed by other DRP
> code?

Yup. It's only used here, and I've deleted it in the DRP code.

[Bryan McQuade, 2016-09-07 19:08:15]

LGTM, thank you again!

https://codereview.chromium.org/2314163003/diff/80001/chrome/renderer/page_lo...
File chrome/renderer/page_load_histograms.cc (left):

https://codereview.chromium.org/2314163003/diff/80001/chrome/renderer/page_lo...
chrome/renderer/page_load_histograms.cc:285:
PLT_HISTOGRAM_DRP("PLT.NT_DomainLookup",
On 2016/09/07 at 18:36:16, Ryan Sturm wrote:
> On 2016/09/07 18:24:32, Bryan McQuade wrote:
> > IIUC PLT_HISTOGRAM_DRP logged both a DRP histogram and the regular non-DRP
> > histogram. If that's the case, can we retain these and convert them to use
> > PLT_HISTOGRAM instead of PLT_HISTOGRAM_DRP? That way we'll continue to log
the
> > non-DRP variants until we're ready to fully kill this class.
> 
> I removed these because I thought DRP were the owners of the baseline
histograms as well, and we no longer need them (I deprecated them in the
histograms.xml part of this change as well). I will keep them if you prefer. Can
I switch the ownership in histograms.xml to you, or should I leave the ownership
on bolian or me?

Ah, in that case, let's go ahead and remove them as you've done. Thanks!

[commit-bot: I haz the power, 2016-09-07 20:22:35]

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

[commit-bot: I haz the power, 2016-09-07 20:22:36]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-09-07 22:50:06]

ipc lgtm

[RyanSturm, 2016-09-07 23:28:50]

The CQ bit was checked by ryansturm@chromium.org

[RyanSturm, 2016-09-07 23:28:51]

The patchset sent to the CQ was uploaded after l-g-t-m from tbansal@chromium.org
Link to the patchset: https://codereview.chromium.org/2314163003/#ps80001
(title: "DEPS change")

[commit-bot: I haz the power, 2016-09-07 23:29:15]

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, 2016-09-07 23:36:46]

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

[commit-bot: I haz the power, 2016-09-07 23:36:48]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[RyanSturm, 2016-09-08 17:51:57]

The CQ bit was checked by ryansturm@chromium.org

[commit-bot: I haz the power, 2016-09-08 17:52:25]

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, 2016-09-08 17:58:17]

Description was changed from

==========
Removing and deprecating PLT DataReductionProxy UMA

DataReductionProxy has moved to reporting PLT via PageLoad.* and no
longer needs the renderer based metrics. While the browser based metrics
do not cover all of the use cases, the new metrics are sufficient for
the analysis DRP needs moving forward.

This also removes the DRP IPC completely.

BUG=643307
==========

to

==========
Removing and deprecating PLT DataReductionProxy UMA

DataReductionProxy has moved to reporting PLT via PageLoad.* and no
longer needs the renderer based metrics. While the browser based metrics
do not cover all of the use cases, the new metrics are sufficient for
the analysis DRP needs moving forward.

This also removes the DRP IPC completely.

BUG=643307
==========

[commit-bot: I haz the power, 2016-09-08 17:58:19]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2016-09-08 17:59:46]

Description was changed from

==========
Removing and deprecating PLT DataReductionProxy UMA

DataReductionProxy has moved to reporting PLT via PageLoad.* and no
longer needs the renderer based metrics. While the browser based metrics
do not cover all of the use cases, the new metrics are sufficient for
the analysis DRP needs moving forward.

This also removes the DRP IPC completely.

BUG=643307
==========

to

==========
Removing and deprecating PLT DataReductionProxy UMA

DataReductionProxy has moved to reporting PLT via PageLoad.* and no
longer needs the renderer based metrics. While the browser based metrics
do not cover all of the use cases, the new metrics are sufficient for
the analysis DRP needs moving forward.

This also removes the DRP IPC completely.

BUG=643307

Committed: https://crrev.com/a43953b70da976a0c565150cd938fa1b5d42a0c2
Cr-Commit-Position: refs/heads/master@{#417337}
==========

[commit-bot: I haz the power, 2016-09-08 17:59:47]

Patchset 3 (id:??) landed as
https://crrev.com/a43953b70da976a0c565150cd938fa1b5d42a0c2
Cr-Commit-Position: refs/heads/master@{#417337}