[Cronet] Pass metrics information from C++ BidirectionalStream to Java

[Cronet] Pass metrics information from C++ BidirectionalStream to Java

This CL adds plumbing to pass metrics information from C++
BidirectionalStream to its Java counterpart.

BUG=648346
Committed: https://crrev.com/abea839761daacc8c926ed6230c14c2a14d4e1c0
Cr-Commit-Position: refs/heads/master@{#423603}

[xunjieli, 2016-09-22 20:08:49]

Description was changed from

==========
[Cronet] Pass metrics information from C++ BidirectionalStream to Java

This CL adds plumbing to pass metrics information from C++ BidirectionalStream
to Java counterpart.

BUG=648346
==========

to

==========
[Cronet] Pass metrics information from C++ BidirectionalStream to Java

This CL adds plumbing to pass metrics information from C++ 
BidirectionalStream to its Java counterpart.

BUG=648346
==========

[xunjieli, 2016-09-23 18:53:32]

Patchset #1 (id:1) has been deleted

[xunjieli, 2016-09-23 18:53:42]

Patchset #1 (id:20001) has been deleted

[xunjieli, 2016-09-23 18:56:01]

xunjieli@chromium.org changed reviewers:
+ mgersh@chromium.org

[xunjieli, 2016-09-23 18:56:02]

Miriam, this is not ready for review yet. I am depending on you to get the main
framework in so I can copy what you did :) FYI in the meanwhile.

[xunjieli, 2016-10-05 18:34:44]

Patchset #2 (id:60001) has been deleted

[xunjieli, 2016-10-05 19:00:28]

PTAL. There's some duplication of code unfortunately. Let me know how it can be
improved!

https://codereview.chromium.org/2360813003/diff/120001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamQuicTest.java
(right):

https://codereview.chromium.org/2360813003/diff/120001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamQuicTest.java:13:
import org.chromium.net.MetricsTestUtil.TestRequestFinishedListener;
Any idea on why presubmit script gives the following warning?

" 
components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamQuicTest.java:14:
Wrong order for org.json.JSONObject import. Use :JavaImportOrganize (ECLIM) or
Ctrl+Shift+O (Eclipse) to sort imports. An importorder file for configuring
Eclipse can be found at //tools/android/eclipse/android.importorder."

I thought the order is correct...

[mgersh, 2016-10-05 19:36:48]

Things are looking good in the implementation, it mostly just needs more tests.
Besides adding some more checks in the existing tests, at least one and maybe
more than one for failure/error/canceled conditions would be good. I'm not sure
if you'll be able to use the methods from MetricsTestUtil or if bidi stream will
have different checks needed.

https://codereview.chromium.org/2360813003/diff/120001/components/cronet/andr...
File components/cronet/android/metrics_util.h (right):

https://codereview.chromium.org/2360813003/diff/120001/components/cronet/andr...
components/cronet/android/metrics_util.h:14: class MetricsUtil {
This should be a namespace instead of a class:
https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_a...

https://codereview.chromium.org/2360813003/diff/120001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamQuicTest.java
(right):

https://codereview.chromium.org/2360813003/diff/120001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamQuicTest.java:13:
import org.chromium.net.MetricsTestUtil.TestRequestFinishedListener;
On 2016/10/05 19:00:28, xunjieli wrote:
> Any idea on why presubmit script gives the following warning?
> 
> " 
>
components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamQuicTest.java:14:
> Wrong order for org.json.JSONObject import. Use :JavaImportOrganize (ECLIM) or
> Ctrl+Shift+O (Eclipse) to sort imports. An importorder file for configuring
> Eclipse can be found at //tools/android/eclipse/android.importorder."
> 
> I thought the order is correct...

I think this must have been a recent change to the presubmit, because I ran into
the same thing. I tried a bunch of places and the one that eventually made the
warning go away was putting it after android imports but before org.chromium
imports.

https://codereview.chromium.org/2360813003/diff/120001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamQuicTest.java:110:
requestFinishedListener.blockUntilDone();
Can we also do some checks on the timing metrics here?

https://codereview.chromium.org/2360813003/diff/120001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java
(right):

https://codereview.chromium.org/2360813003/diff/120001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:229:
TestRequestFinishedListener requestFinishedListener = new
TestRequestFinishedListener(1);
I think this is the only place that does new TestRequestFinishedListener(int),
and it does the same thing that it would do without the int argument. You can
change this one, and then delete the extra constructor from
TestRequestFinishedListener.

https://codereview.chromium.org/2360813003/diff/120001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:251:
// TODO(xunjieli): Check other fields once metrics support is in
crbug.com/648346.
Now would be a good time to add tests for the timing metrics.

[xunjieli, 2016-10-06 03:51:47]

PTAL? thanks!

https://codereview.chromium.org/2360813003/diff/120001/components/cronet/andr...
File components/cronet/android/metrics_util.h (right):

https://codereview.chromium.org/2360813003/diff/120001/components/cronet/andr...
components/cronet/android/metrics_util.h:14: class MetricsUtil {
On 2016/10/05 19:36:47, mgersh wrote:
> This should be a namespace instead of a class:
>
https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_a...

Done. Good to know!

https://codereview.chromium.org/2360813003/diff/120001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamQuicTest.java
(right):

https://codereview.chromium.org/2360813003/diff/120001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamQuicTest.java:13:
import org.chromium.net.MetricsTestUtil.TestRequestFinishedListener;
On 2016/10/05 19:36:47, mgersh wrote:
> On 2016/10/05 19:00:28, xunjieli wrote:
> > Any idea on why presubmit script gives the following warning?
> > 
> > " 
> >
>
components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamQuicTest.java:14:
> > Wrong order for org.json.JSONObject import. Use :JavaImportOrganize (ECLIM)
or
> > Ctrl+Shift+O (Eclipse) to sort imports. An importorder file for configuring
> > Eclipse can be found at //tools/android/eclipse/android.importorder."
> > 
> > I thought the order is correct...
> 
> I think this must have been a recent change to the presubmit, because I ran
into
> the same thing. I tried a bunch of places and the one that eventually made the
> warning go away was putting it after android imports but before org.chromium
> imports.

Done. That's good to know. Thanks!

https://codereview.chromium.org/2360813003/diff/120001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamQuicTest.java:110:
requestFinishedListener.blockUntilDone();
On 2016/10/05 19:36:47, mgersh wrote:
> Can we also do some checks on the timing metrics here?

Done.

https://codereview.chromium.org/2360813003/diff/120001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java
(right):

https://codereview.chromium.org/2360813003/diff/120001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:229:
TestRequestFinishedListener requestFinishedListener = new
TestRequestFinishedListener(1);
On 2016/10/05 19:36:47, mgersh wrote:
> I think this is the only place that does new TestRequestFinishedListener(int),
> and it does the same thing that it would do without the int argument. You can
> change this one, and then delete the extra constructor from
> TestRequestFinishedListener.

Done.

https://codereview.chromium.org/2360813003/diff/120001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:251:
// TODO(xunjieli): Check other fields once metrics support is in
crbug.com/648346.
On 2016/10/05 19:36:47, mgersh wrote:
> Now would be a good time to add tests for the timing metrics.

Done.

[xunjieli, 2016-10-06 11:08:16]

On 2016/10/06 03:51:47, xunjieli wrote:
> PTAL? thanks!
There is one test that fails on a NPE. I think I know what is wrong. Will fix it
when I get to the office.

[mgersh, 2016-10-06 14:44:10]

lgtm once the test is fixed. Thanks for finishing this up so fast!

https://codereview.chromium.org/2360813003/diff/160001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/MetricsTestUtil.java
(right):

https://codereview.chromium.org/2360813003/diff/160001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/MetricsTestUtil.java:87:
// Some implementation of java.util.Date broke asymmetric property, so
Huh, weird. I'm curious where?

nit: "asymmetric property" -> "the symmetric property"

https://codereview.chromium.org/2360813003/diff/160001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/MetricsTestUtil.java:89:
public static void assertAfter(Date date1, Date date2) {
Thanks for adding this. There are a few places in RequestFinishedInfoTest that
should also be switched to it. You can do that now if you want, otherwise I'll
get to it later.

[xunjieli, 2016-10-06 16:14:21]

Thanks! Now I will wait for the tests to pass :)

https://codereview.chromium.org/2360813003/diff/160001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/MetricsTestUtil.java
(right):

https://codereview.chromium.org/2360813003/diff/160001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/MetricsTestUtil.java:87:
// Some implementation of java.util.Date broke asymmetric property, so
On 2016/10/06 14:44:09, mgersh wrote:
> Huh, weird. I'm curious where?
> 
> nit: "asymmetric property" -> "the symmetric property"

Done. ah, you are right!

http://stackoverflow.com/questions/27661640/java-util-date-equals-doesnt-seem...

I think I am being paranoia. But I think it might be good to assert both
directions.

https://codereview.chromium.org/2360813003/diff/160001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/MetricsTestUtil.java:89:
public static void assertAfter(Date date1, Date date2) {
On 2016/10/06 14:44:09, mgersh wrote:
> Thanks for adding this. There are a few places in RequestFinishedInfoTest that
> should also be switched to it. You can do that now if you want, otherwise I'll
> get to it later.

Acknowledged. I will leave it to you then. My brain is still a bit fuzzy after
the redeye flight :)

[xunjieli, 2016-10-06 16:14:46]

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

[commit-bot: I haz the power, 2016-10-06 16:15:16]

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

[xunjieli, 2016-10-06 16:16:18]

https://codereview.chromium.org/2360813003/diff/160001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/MetricsTestUtil.java
(right):

https://codereview.chromium.org/2360813003/diff/160001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/MetricsTestUtil.java:87:
// Some implementation of java.util.Date broke asymmetric property, so
On 2016/10/06 16:14:21, xunjieli wrote:
> On 2016/10/06 14:44:09, mgersh wrote:
> > Huh, weird. I'm curious where?
> > 
> > nit: "asymmetric property" -> "the symmetric property"
> 
> Done. ah, you are right!
> 
>
http://stackoverflow.com/questions/27661640/java-util-date-equals-doesnt-seem...
> 
> I think I am being paranoia. But I think it might be good to assert both
> directions.

s/paranoia/paranoid

[xunjieli, 2016-10-06 17:02:47]

The CQ bit was unchecked by xunjieli@chromium.org

[xunjieli, 2016-10-06 17:02:51]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2016-10-06 17:02:52]

The patchset sent to the CQ was uploaded after l-g-t-m from mgersh@chromium.org
Link to the patchset: https://codereview.chromium.org/2360813003/#ps180001
(title: "Address comments")

[commit-bot: I haz the power, 2016-10-06 17:03:22]

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-10-06 17:33:27]

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

[commit-bot: I haz the power, 2016-10-06 17:33:32]

Failed to apply patch for
components/cronet/android/cronet_url_request_adapter.cc:
While running git apply --index -3 -p1;
  error: patch failed:
components/cronet/android/cronet_url_request_adapter.cc:33
  Falling back to three-way merge...
  Applied patch to 'components/cronet/android/cronet_url_request_adapter.cc'
with conflicts.
  U components/cronet/android/cronet_url_request_adapter.cc

Patch:       components/cronet/android/cronet_url_request_adapter.cc
Index: components/cronet/android/cronet_url_request_adapter.cc
diff --git a/components/cronet/android/cronet_url_request_adapter.cc
b/components/cronet/android/cronet_url_request_adapter.cc
index
e0820aa46b6db0584abd0cd4147aaa33b0d02f9b..46faa6ac07aa7a69b7758a2461bfbba037979ad2
100644
--- a/components/cronet/android/cronet_url_request_adapter.cc
+++ b/components/cronet/android/cronet_url_request_adapter.cc
@@ -13,6 +13,7 @@
 #include "base/logging.h"
 #include "components/cronet/android/cronet_url_request_context_adapter.h"
 #include "components/cronet/android/io_buffer_with_byte_buffer.h"
+#include "components/cronet/android/metrics_util.h"
 #include "components/cronet/android/url_request_error.h"
 #include "jni/CronetUrlRequest_jni.h"
 #include "net/base/load_flags.h"
@@ -33,22 +34,6 @@ using base::android::JavaParamRef;
 
 namespace cronet {
 
-namespace {
-
-// Converts timing metrics stored as TimeTicks into the format expected by the
-// Java layer: ms since Unix epoch, or -1 for null
-int64_t ConvertTime(const base::TimeTicks& ticks,
-                    const base::TimeTicks& start_ticks,
-                    const base::Time& start_time) {
-  if (ticks.is_null() || start_ticks.is_null()) {
-    return -1;
-  }
-  DCHECK(!start_time.is_null());
-  return (start_time + (ticks - start_ticks)).ToJavaTime();
-}
-
-}  // namespace
-
 // Explicitly register static JNI functions.
 bool CronetUrlRequestAdapterRegisterJni(JNIEnv* env) {
   return RegisterNativesImpl(env);
@@ -416,20 +401,28 @@ void CronetURLRequestAdapter::MaybeReportMetrics(JNIEnv*
env) const {
   base::Time start_time = metrics.request_start_time;
   base::TimeTicks start_ticks = metrics.request_start;
   Java_CronetUrlRequest_onMetricsCollected(
-      env, owner_.obj(), ConvertTime(start_ticks, start_ticks, start_time),
-      ConvertTime(metrics.connect_timing.dns_start, start_ticks, start_time),
-      ConvertTime(metrics.connect_timing.dns_end, start_ticks, start_time),
-      ConvertTime(metrics.connect_timing.connect_start, start_ticks,
-                  start_time),
-      ConvertTime(metrics.connect_timing.connect_end, start_ticks, start_time),
-      ConvertTime(metrics.connect_timing.ssl_start, start_ticks, start_time),
-      ConvertTime(metrics.connect_timing.ssl_end, start_ticks, start_time),
-      ConvertTime(metrics.send_start, start_ticks, start_time),
-      ConvertTime(metrics.send_end, start_ticks, start_time),
-      ConvertTime(metrics.push_start, start_ticks, start_time),
-      ConvertTime(metrics.push_end, start_ticks, start_time),
-      ConvertTime(metrics.receive_headers_end, start_ticks, start_time),
-      ConvertTime(base::TimeTicks::Now(), start_ticks, start_time),
+      env, owner_.obj(),
+      metrics_util::ConvertTime(start_ticks, start_ticks, start_time),
+      metrics_util::ConvertTime(metrics.connect_timing.dns_start, start_ticks,
+                                start_time),
+      metrics_util::ConvertTime(metrics.connect_timing.dns_end, start_ticks,
+                                start_time),
+      metrics_util::ConvertTime(metrics.connect_timing.connect_start,
+                                start_ticks, start_time),
+      metrics_util::ConvertTime(metrics.connect_timing.connect_end,
start_ticks,
+                                start_time),
+      metrics_util::ConvertTime(metrics.connect_timing.ssl_start, start_ticks,
+                                start_time),
+      metrics_util::ConvertTime(metrics.connect_timing.ssl_end, start_ticks,
+                                start_time),
+      metrics_util::ConvertTime(metrics.send_start, start_ticks, start_time),
+      metrics_util::ConvertTime(metrics.send_end, start_ticks, start_time),
+      metrics_util::ConvertTime(metrics.push_start, start_ticks, start_time),
+      metrics_util::ConvertTime(metrics.push_end, start_ticks, start_time),
+      metrics_util::ConvertTime(metrics.receive_headers_end, start_ticks,
+                                start_time),
+      metrics_util::ConvertTime(base::TimeTicks::Now(), start_ticks,
+                                start_time),
       metrics.socket_reused, url_request_->GetTotalSentBytes(),
       url_request_->GetTotalReceivedBytes());
 }

[xunjieli, 2016-10-06 17:39:54]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2016-10-06 17:39:54]

The patchset sent to the CQ was uploaded after l-g-t-m from mgersh@chromium.org
Link to the patchset: https://codereview.chromium.org/2360813003/#ps200001
(title: "Rebased")

[commit-bot: I haz the power, 2016-10-06 17:40:35]

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-10-06 18:43:46]

Description was changed from

==========
[Cronet] Pass metrics information from C++ BidirectionalStream to Java

This CL adds plumbing to pass metrics information from C++ 
BidirectionalStream to its Java counterpart.

BUG=648346
==========

to

==========
[Cronet] Pass metrics information from C++ BidirectionalStream to Java

This CL adds plumbing to pass metrics information from C++ 
BidirectionalStream to its Java counterpart.

BUG=648346
==========

[commit-bot: I haz the power, 2016-10-06 18:43:47]

Committed patchset #8 (id:200001)

[commit-bot: I haz the power, 2016-10-06 18:47:08]

Description was changed from

==========
[Cronet] Pass metrics information from C++ BidirectionalStream to Java

This CL adds plumbing to pass metrics information from C++ 
BidirectionalStream to its Java counterpart.

BUG=648346
==========

to

==========
[Cronet] Pass metrics information from C++ BidirectionalStream to Java

This CL adds plumbing to pass metrics information from C++
BidirectionalStream to its Java counterpart.

BUG=648346

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

[commit-bot: I haz the power, 2016-10-06 18:47:09]

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