Record time to first data reduction proxy request

Record time to first data reduction proxy request

Record the time duration between data saver initialization and the first
request whose resolved porxy is a data saver proxy. Also, record the
time duration between IP address change event, and the next subsequent
request whose resolved proxy is a data saver proxy.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

BUG=666134
Committed: https://crrev.com/e7e2d51baadbf0febbe84bfb8e50fe2840d3fbf3
Cr-Commit-Position: refs/heads/master@{#434898}

[tbansal1, 2016-11-17 01:21:09]

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

[tbansal1, 2016-11-17 01:21:09]

sclittle: ptal. Thanks.

[tbansal1, 2016-11-21 22:19:43]

sclittle: ping. Thanks.

[sclittle, 2016-11-21 22:47:21]

https://codereview.chromium.org/2504963004/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc
(right):

https://codereview.chromium.org/2504963004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc:117:
DCHECK(thread_checker_.CalledOnValidThread());
nit: remove this line, you already have a DCHECK on the thread_checker_ above.

https://codereview.chromium.org/2504963004/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate_unittest.cc
(right):

https://codereview.chromium.org/2504963004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate_unittest.cc:947:
"Content-Length: 1000\r\n\r\n";
nit: I'd recommend simplifying these fake responses. Remove the Date header, the
empty via header, just use the simple "Via: 1.1 Chrome-Compression-Proxy"
header. Also, I'd reduce the Content-Length down to like 10 maybe.

https://codereview.chromium.org/2504963004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate_unittest.cc:949:
std::unique_ptr<net::URLRequest> request_1 = FetchURLRequest(
nit: If you're not using the |request_1| at all, then I think you can just call
FetchURLRequest without assigning it to anything. Same with the other requests
here.

https://codereview.chromium.org/2504963004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate_unittest.cc:952:
"DataReductionProxy.TimeToFirstDataSaverRequest", 1);
Could you test that the recorded values are correct?

[tbansal1, 2016-11-22 02:22:22]

Description was changed from

==========
Record time to first data reduction proxy request

Record the time duration between data saver initialization and the first
request whose resolved porxy is a data saver proxy. Also, record the
time duration between IP address change event, and the next subsequent
request whose resolved proxy is a data saver proxy.

BUG=666134
==========

to

==========
Record time to first data reduction proxy request

Record the time duration between data saver initialization and the first
request whose resolved porxy is a data saver proxy. Also, record the
time duration between IP address change event, and the next subsequent
request whose resolved proxy is a data saver proxy.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

BUG=666134
==========

[tbansal1, 2016-11-22 02:23:43]

Patchset #2 (id:20001) has been deleted

[tbansal1, 2016-11-22 02:24:28]

sclittle: ptal. I have also renamed an existing function. It was not really
related to this CL, so let me know if you want me to move this to a different
CL.

https://codereview.chromium.org/2504963004/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc
(right):

https://codereview.chromium.org/2504963004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc:117:
DCHECK(thread_checker_.CalledOnValidThread());
On 2016/11/21 22:47:21, sclittle wrote:
> nit: remove this line, you already have a DCHECK on the thread_checker_ above.

Done.

https://codereview.chromium.org/2504963004/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate_unittest.cc
(right):

https://codereview.chromium.org/2504963004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate_unittest.cc:947:
"Content-Length: 1000\r\n\r\n";
On 2016/11/21 22:47:21, sclittle wrote:
> nit: I'd recommend simplifying these fake responses. Remove the Date header,
the
> empty via header, just use the simple "Via: 1.1 Chrome-Compression-Proxy"
> header. Also, I'd reduce the Content-Length down to like 10 maybe.

Done.

https://codereview.chromium.org/2504963004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate_unittest.cc:949:
std::unique_ptr<net::URLRequest> request_1 = FetchURLRequest(
On 2016/11/21 22:47:21, sclittle wrote:
> nit: If you're not using the |request_1| at all, then I think you can just
call
> FetchURLRequest without assigning it to anything. Same with the other requests
> here.

Done.

https://codereview.chromium.org/2504963004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate_unittest.cc:952:
"DataReductionProxy.TimeToFirstDataSaverRequest", 1);
On 2016/11/21 22:47:21, sclittle wrote:
> Could you test that the recorded values are correct?

Done.

[tbansal1, 2016-11-22 02:24:36]

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

[commit-bot: I haz the power, 2016-11-22 02:25:09]

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-11-22 04:01:19]

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

[commit-bot: I haz the power, 2016-11-22 04:01:20]

Dry run: This issue passed the CQ dry run.

[sclittle, 2016-11-22 21:41:44]

lgtm

[tbansal1, 2016-11-22 21:53:05]

The CQ bit was checked by tbansal@chromium.org

[commit-bot: I haz the power, 2016-11-22 21:53:36]

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-11-22 22:03:11]

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

[commit-bot: I haz the power, 2016-11-22 22:03:13]

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

[tbansal1, 2016-11-22 22:09:04]

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

[tbansal1, 2016-11-22 22:09:04]

holte: ptal at histograms.xml. Thanks.

[tbansal1, 2016-11-28 20:55:25]

holte: ping. Thanks.

[Steven Holte, 2016-11-29 00:14:33]

lgtm

[tbansal1, 2016-11-29 00:15:37]

The CQ bit was checked by tbansal@chromium.org

[commit-bot: I haz the power, 2016-11-29 00:17:11]

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-11-29 02:25:08]

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

[commit-bot: I haz the power, 2016-11-29 02:25:09]

Failed to apply patch for
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.h:
While running git apply --index -p1;
  error: patch failed:
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.h:32
  error:
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.h:
patch does not apply

Patch:      
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.h
Index:
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.h
diff --git
a/components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.h
b/components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.h
index
e65a9a5caac21ea35e64a38db29b57c69b6892db..721398c27e3b19f014665f5cb130e6c1dc88f821
100644
---
a/components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.h
+++
b/components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.h
@@ -8,11 +8,17 @@
 #include <string>
 
 #include "base/macros.h"
+#include "base/threading/thread_checker.h"
+#include "net/base/network_change_notifier.h"
 #include "net/base/proxy_delegate.h"
 #include "net/proxy/proxy_retry_info.h"
 #include "net/proxy/proxy_server.h"
 #include "url/gurl.h"
 
+namespace base {
+class TickClock;
+}
+
 namespace net {
 class HostPortPair;
 class HttpRequestHeaders;
@@ -32,19 +38,23 @@ class DataReductionProxyConfigurator;
 class DataReductionProxyEventCreator;
 class DataReductionProxyRequestOptions;
 
-class DataReductionProxyDelegate : public net::ProxyDelegate {
+class DataReductionProxyDelegate
+    : public net::ProxyDelegate,
+      public net::NetworkChangeNotifier::IPAddressObserver {
  public:
   // ProxyDelegate instance is owned by io_thread. |auth_handler| and |config|
   // outlives this class instance.
-  explicit DataReductionProxyDelegate(
-      DataReductionProxyConfig* config,
-      const DataReductionProxyConfigurator* configurator,
-      DataReductionProxyEventCreator* event_creator,
-      DataReductionProxyBypassStats* bypass_stats,
-      net::NetLog* net_log);
+  DataReductionProxyDelegate(DataReductionProxyConfig* config,
+                             const DataReductionProxyConfigurator*
configurator,
+                             DataReductionProxyEventCreator* event_creator,
+                             DataReductionProxyBypassStats* bypass_stats,
+                             net::NetLog* net_log);
 
   ~DataReductionProxyDelegate() override;
 
+  // Performs initialization on the IO thread.
+  void InitializeOnIOThread();
+
   // net::ProxyDelegate implementation:
   void OnResolveProxy(const GURL& url,
                       const std::string& method,
@@ -62,6 +72,8 @@ class DataReductionProxyDelegate : public net::ProxyDelegate {
       const net::HostPortPair& proxy_server,
       const net::HttpResponseHeaders& response_headers) override;
 
+  void SetTickClockForTesting(std::unique_ptr<base::TickClock> tick_clock);
+
  protected:
   // Protected so that these methods are accessible for testing.
   // net::ProxyDelegate implementation:
@@ -104,6 +116,9 @@ class DataReductionProxyDelegate : public net::ProxyDelegate
{
   void RecordGetDefaultAlternativeProxy(
       DefaultAlternativeProxyStatus status) const;
 
+  // NetworkChangeNotifier::IPAddressObserver:
+  void OnIPAddressChanged() override;
+
   const DataReductionProxyConfig* config_;
   const DataReductionProxyConfigurator* configurator_;
   DataReductionProxyEventCreator* event_creator_;
@@ -112,8 +127,22 @@ class DataReductionProxyDelegate : public
net::ProxyDelegate {
   // True if the use of alternate proxies is disabled.
   bool alternative_proxies_broken_;
 
+  // Tick clock used for obtaining the current time.
+  std::unique_ptr<base::TickClock> tick_clock_;
+
+  // True if the metrics related to the first request whose resolved proxy was
a
+  // data saver proxy has been recorded. |first_data_saver_request_recorded_|
is
+  // reset to false on IP address change events.
+  bool first_data_saver_request_recorded_;
+
+  // Set to the time when last IP address change event was received, or the
time
+  // of initialization of |this|, whichever is later.
+  base::TimeTicks last_network_change_time_;
+
   net::NetLog* net_log_;
 
+  base::ThreadChecker thread_checker_;
+
   DISALLOW_COPY_AND_ASSIGN(DataReductionProxyDelegate);
 };

[tbansal1, 2016-11-29 02:55:23]

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

[commit-bot: I haz the power, 2016-11-29 02:56:09]

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-11-29 04:43:51]

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

[commit-bot: I haz the power, 2016-11-29 04:43:52]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2016-11-29 04:52:27]

The CQ bit was checked by tbansal@chromium.org

[tbansal1, 2016-11-29 04:52:27]

The patchset sent to the CQ was uploaded after l-g-t-m from holte@chromium.org,
sclittle@chromium.org
Link to the patchset: https://codereview.chromium.org/2504963004/#ps60001
(title: "Rebased")

[commit-bot: I haz the power, 2016-11-29 04:52:43]

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-11-29 04:56:24]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480395147399590,
"parent_rev": "c01f62cb762aaf97a7b7fa37fb18d62023d666b4", "commit_rev":
"fdda0eb1809c108faccdc36b0f86ea0a37a045a4"}

[commit-bot: I haz the power, 2016-11-29 04:56:55]

Description was changed from

==========
Record time to first data reduction proxy request

Record the time duration between data saver initialization and the first
request whose resolved porxy is a data saver proxy. Also, record the
time duration between IP address change event, and the next subsequent
request whose resolved proxy is a data saver proxy.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

BUG=666134
==========

to

==========
Record time to first data reduction proxy request

Record the time duration between data saver initialization and the first
request whose resolved porxy is a data saver proxy. Also, record the
time duration between IP address change event, and the next subsequent
request whose resolved proxy is a data saver proxy.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

BUG=666134
==========

[commit-bot: I haz the power, 2016-11-29 04:56:56]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-11-29 04:59:39]

Description was changed from

==========
Record time to first data reduction proxy request

Record the time duration between data saver initialization and the first
request whose resolved porxy is a data saver proxy. Also, record the
time duration between IP address change event, and the next subsequent
request whose resolved proxy is a data saver proxy.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

BUG=666134
==========

to

==========
Record time to first data reduction proxy request

Record the time duration between data saver initialization and the first
request whose resolved porxy is a data saver proxy. Also, record the
time duration between IP address change event, and the next subsequent
request whose resolved proxy is a data saver proxy.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

BUG=666134

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

[commit-bot: I haz the power, 2016-11-29 04:59:40]

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

[pauljensen, 2016-12-01 11:57:58]

Tarun, I think this broke Cronet:
https://build.chromium.org/p/chromium.android/builders/Android%20Cronet%20Dat...
BTW that bot is kinda stuck:
https://bugs.chromium.org/p/chromium/issues/detail?id=669037