Add API for new Cronet metrics

components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 package org.chromium.net;
 
+import android.support.annotation.IntDef;
 import android.support.annotation.Nullable;
 
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
 import java.util.Collection;
+import java.util.Date;
 import java.util.concurrent.Executor;
 
 /**
  * Information about a finished request. Passed to {@link RequestFinishedInfo.Listener}.
  *
  * {@hide} as it's a prototype.
  */
 public final class RequestFinishedInfo {
     /**
      * Listens for finished requests for the purpose of collecting metrics.
@@ skipping 20 matching lines @@
         /**
          * Returns this listener's executor. Can be called on any thread.
          * @return this listener's {@link java.util.concurrent.Executor}
          */
         public Executor getExecutor() {
             return mExecutor;
         }
     }
 
     /**
      * Metrics collected for a single request.

[pauljensen, 2016/09/06 17:22:23]

Can we elaborate a bit more here?  Like by adding a statement of what these
metrics generally comprise?  We could also provide a segway into the event
ordering list.  Something like "The metrics provide timing details for different
events that are required to process a {@link UrlRequest}. These metrics can be
used to investigate and improve performance."

[mgersh, 2016/09/07 22:09:58]

Done.

      *
+     * Most timing metrics are taken from
+     * {@link https://cs.chromium.org/chromium/src/net/base/load_timing_info.h LoadTimingInfo},
+     * which holds the information for {@link http://w3c.github.io/navigation-timing/} and
+     * {@link https://www.w3.org/TR/resource-timing/}.

[pauljensen, 2016/09/06 17:22:23]

This is kind of an implementation detail.  Perhaps could we move this down to
the end of the description as sort of a foot-note?

[mgersh, 2016/09/07 22:09:57]

Done.

+     *
+     * Events happen in this order:
+     * request start

[pauljensen, 2016/09/06 17:22:23]

Use <ol> and <li>'s

[pauljensen, 2016/09/06 17:22:23]

I feel like we should include links from these list items to the getters below. 
Perhaps like:
Request start (see {@link Metrics#getRequestStart})

[mgersh, 2016/09/07 22:09:58]

Done.

[mgersh, 2016/09/07 22:09:58]

Done.

+     * dns start

[pauljensen, 2016/09/06 17:22:23]

dns->DNS
here and below

[mgersh, 2016/09/07 22:09:57]

Done.

+     * dns end
+     * connect start
+     * ssl start

[pauljensen, 2016/09/06 17:22:23]

ssl->SSL
here and below

[mgersh, 2016/09/07 22:09:58]

Done.

+     * ssl end
+     * connect end
+     * send start
+     * send end
+     * response start
+     * response end
+     *
+     * Start times are reported as the time when a request started blocking on event, not when the
+     * event actually occurred, with the exception of push start and end. If a metric is not
+     * meaningful or not available, including cases when a request finished before reaching that
+     * stage, start and end times will be null. If no time was spent blocking on an event, start

[pauljensen, 2016/09/06 17:22:23]

null->{@code null}

[mgersh, 2016/09/07 22:09:58]

Done.

+     * and end will be the same time.
+     *
+     * Timestamps are recorded using a clock that is guaranteed not to run backwards. All timestamps
+     * are correct relative to the system clock at the time of request start, and taking the
+     * difference between two timestamps will give the correct difference between the events. In
+     * order to preserve this property, timestamps for events other than request start are not
+     * guaranteed to match the system clock at the times they represent.

[pauljensen, 2016/09/06 17:22:23]

Could we simplify this paragraph or preface it with something like "In the
unlikely event that the system clock is adjusted during the request, these
{@link #Date} values may not match the system clock adjustment."
This doesn't seem like an issue that would affect many users, so I'd hate to
force many users to grok all this.

[mgersh, 2016/09/07 22:09:58]

Done.

+     *
      * {@hide} as it's a prototype.
      */
-    public static class Metrics {
-        @Nullable
-        private final Long mTtfbMs;
-        @Nullable
-        private final Long mTotalTimeMs;
-        @Nullable
-        private final Long mSentBytesCount;
-        @Nullable
-        private final Long mReceivedBytesCount;
-
-        public Metrics(@Nullable Long ttfbMs, @Nullable Long totalTimeMs,
-                @Nullable Long sentBytesCount, @Nullable Long receivedBytesCount) {
-            mTtfbMs = ttfbMs;
-            mTotalTimeMs = totalTimeMs;
-            mSentBytesCount = sentBytesCount;
-            mReceivedBytesCount = receivedBytesCount;
-        }
+    public abstract static class Metrics {
+        /**
+         * @return {@link java.util.Date} representing when the request started from the perspective

[pauljensen, 2016/09/06 17:22:23]

All of these methods need a method description in addition to the @return tag.

Some guidance: "The @return tag is required for every method that returns
something other than void, even if it is redundant with the method description.
(Whenever possible, find something non-redundant (ideally, more specific) to use
for the tag comment.)"
from:
http://www.oracle.com/technetwork/java/javase/documentation/index-137868.html
which is linked from
https://source.android.com/source/code-style.html#use-javadoc-standard-comments
which is linked from http://dev.chromium.org/developers/coding-style/java

[mgersh, 2016/09/07 22:09:58]

Done.

+         * of the native URLRequest layer. This timestamp will match the system clock at the time it

[xunjieli, 2016/09/05 23:24:17]

nit: Avoid implementation details in the Javadoc.  Consumers don't really know
what "URLRequest" is, and they don't need to know. Could simply say "the native
request" instead of "the native URLRequest layer." Or you could say that this
represents the time when the request actually started. That'll give us room to
adjust the implementation if needed. 

[mgersh, 2016/09/07 22:09:57]

Done.

+         * represents.
+         */
+        @Nullable
+        public abstract Date getRequestStart();
+
+        /**
+         * @return {@link java.util.Date} representing when DNS lookup started. Null if a proxy is

[pauljensen, 2016/09/06 17:22:23]

What do you mean by "a proxy" here?

[pauljensen, 2016/09/06 17:22:23]

Null->{@code null}
here and below

[mgersh, 2016/09/07 22:09:57]

I think I mean the thing that Helen said doesn't apply to us, but forgot to
delete it here.

[mgersh, 2016/09/07 22:09:58]

Done.

+         * used to determine the DNS address.

[pauljensen, 2016/09/06 17:22:23]

doesn't this and some of the other methods return null when the socket is
reused?

[mgersh, 2016/09/07 22:09:58]

Done.

+         */
+        @Nullable
+        public abstract Date getDnsStart();
+
+        /**
+         * @return {@link java.util.Date} representing when DNS lookup finished. Null if a proxy is
+         * used to determine the DNS address.
+         */
+        @Nullable
+        public abstract Date getDnsEnd();
+
+        /**
+         * @return {@link java.util.Date} representing when connection establishment started,
+         * typically when DNS resolution finishes if no proxy is used.
+         */
+        @Nullable
+        public abstract Date getConnectStart();
+
+        /**
+         * @return {@link java.util.Date} representing when connection establishment finished,
+         * after TCP connection is established and, if using HTTPS, SSL handshake is completed.
+         */
+        @Nullable
+        public abstract Date getConnectEnd();
+
+        /**
+         * @return {@link java.util.Date} representing when SSL handshake started. Null if SSL is
+         * not used.
+         */
+        @Nullable
+        public abstract Date getSslStart();
+
+        /**
+         * @return {@link java.util.Date} representing when SSL handshake finished. Null if SSL is
+         * not used.
+         */
+        @Nullable
+        public abstract Date getSslEnd();
+
+        /**
+         * @return {@link java.util.Date} representing when sending HTTP request headers started.
+         */
+        @Nullable
+        public abstract Date getSendingStart();
+
+        /**
+         * @return {@link java.util.Date} representing when sending HTTP request body finished.

[pauljensen, 2016/09/06 17:22:23]

It's a little odd that getSendingStart() is documented to be the start of
something other than what getSendingEnd() is documented to be the end of.  Can
we elaborate to mention that the request body follows the request headers?

[mgersh, 2016/09/07 22:09:58]

Done.

+         */
+        @Nullable
+        public abstract Date getSendingEnd();
+
+        /**
+         * @return {@link java.util.Date} representing when the first byte of an HTTP2 server push

[pauljensen, 2016/09/06 17:22:23]

HTTP2->HTTP/2
here and below
so as to match things like CronetEngine.Builder.enableHTTP2 documentation

[mgersh, 2016/09/07 22:09:58]

Done.

+         * was received. Null if server push is not used.
+         */
+        @Nullable
+        public abstract Date getPushStart();
+
+        /**
+         * @return {@link java.util.Date} representing when the last byte of an HTTP2 server push
+         * was received. Null if server push is not used.
+         */
+        @Nullable
+        public abstract Date getPushEnd();
+
+        /**
+         * @return {@link java.util.Date} representing when the first byte of the response was
+         * received.
+         */
+        @Nullable
+        public abstract Date getResponseStart();
+
+        /**
+         * @return {@link java.util.Date} representing when the last byte of the response was
+         * received.
+         */
+        @Nullable
+        public abstract Date getResponseEnd();
+
+        /**
+         * @return whether this request reused a socket from a previous request. When true, DNS,

[pauljensen, 2016/09/06 17:22:23]

true->{@code true}

[mgersh, 2016/09/07 22:09:58]

Done.

+         * connection, and SSL times will be null.
+         */
+        @Nullable
+        public abstract boolean getSocketReused();
 
         /**
          * Returns milliseconds between request initiation and first byte of response headers,
          * or null if not collected.
-         */
-        @Nullable
-        public Long getTtfbMs() {
-            return mTtfbMs;
-        }
+         * TODO(mgersh): Remove once new API works http://crbug.com/629194
+         */
+        @Nullable
+        public abstract Long getTtfbMs();
 
         /**
          * Returns milliseconds between request initiation and finish,
          * including a failure or cancellation, or null if not collected.
-         */
-        @Nullable
-        public Long getTotalTimeMs() {
-            return mTotalTimeMs;
-        }
+         * TODO(mgersh): Remove once new API works http://crbug.com/629194
+         */
+        @Nullable
+        public abstract Long getTotalTimeMs();
 
         /**
          * Returns total bytes sent over the network transport layer, or null if not collected.
          */
         @Nullable
-        public Long getSentBytesCount() {
-            return mSentBytesCount;
-        }
+        public abstract Long getSentBytesCount();
 
         /**
          * Returns total bytes received over the network transport layer, or null if not collected.
          */
         @Nullable
-        public Long getReceivedBytesCount() {
-            return mReceivedBytesCount;
-        }
+        public abstract Long getReceivedBytesCount();
     }
 
     private final String mUrl;
     private final Collection<Object> mAnnotations;
     private final Metrics mMetrics;
+
+    /** @hide */
+    @IntDef({SUCCEEDED, FAILED, CANCELED})
+    @Retention(RetentionPolicy.SOURCE)
+    public @interface FinishedReason {}
+
+    /**
+     * The request succeeded. Returned from {@link #getFinishedReason}.

[pauljensen, 2016/09/06 17:22:23]

do you need to qualify the class, so Metrics#getFinishedReason?

[pauljensen, 2016/09/06 17:22:23]

should we prefix this description with "Reason value indicating "

[mgersh, 2016/09/07 22:09:58]

Done.

[mgersh, 2016/09/07 22:09:58]

No, both this and getFinishedReason() are in RequestFinishedInfo, not a nested
class.

+     */
+    public static final int SUCCEEDED = 0;
+    /**
+     * The request failed or errored. Returned from {@link #getFinishedReason}.
+     */
+    public static final int FAILED = 1;
+    /**
+     * The request was canceled. Returned from {@link #getFinishedReason}.
+     */
+    public static final int CANCELED = 2;
+
+    @FinishedReason
+    private final int mFinishedReason;
+
     @Nullable
     private final UrlResponseInfo mResponseInfo;
+    @Nullable
+    private final UrlRequestException mException;
 
     /**
      * @hide only used by internal implementation.
      */
     public RequestFinishedInfo(String url, Collection<Object> annotations, Metrics metrics,
-            @Nullable UrlResponseInfo responseInfo) {
+            @FinishedReason int finishedReason, @Nullable UrlResponseInfo responseInfo,
+            @Nullable UrlRequestException exception) {
         mUrl = url;
         mAnnotations = annotations;
         mMetrics = metrics;
+        mFinishedReason = finishedReason;
         mResponseInfo = responseInfo;
+        mException = exception;
     }
 
     /** Returns the request's original URL. */
     public String getUrl() {
         return mUrl;
     }
 
     /** Returns the objects that the caller has supplied when initiating the request. */
     public Collection<Object> getAnnotations() {
         return mAnnotations;
     }
 
     // TODO(klm): Collect and return a chain of Metrics objects for redirect responses.
+    // TODO(mgersh): Update this javadoc when new metrics are fully implemented
     /**
      * Returns metrics collected for this request.
      *
      * <p>The reported times and bytes account for all redirects, i.e.
      * the TTFB is from the start of the original request to the ultimate response headers,
      * the TTLB is from the start of the original request to the end of the ultimate response,
      * the received byte count is for all redirects and the ultimate response combined.
      * These cumulative metric definitions are debatable, but are chosen to make sense
      * for user-facing latency analysis.
      *
      * @return metrics collected for this request.
      */
     public Metrics getMetrics() {
         return mMetrics;
     }
 
     /**
+     * Returns the reason why the request finished.
+     * @return one of {@link #SUCCEEDED}, {@link #FAILED}, or {@link #CANCELED}
+     */
+    @FinishedReason
+    public int getFinishedReason() {
+        return mFinishedReason;
+    }
+
+    /**
      * Returns a {@link UrlResponseInfo} for the request, if its response had started.
      * @return {@link UrlResponseInfo} for the request, if its response had started.
      */
     @Nullable
     public UrlResponseInfo getResponseInfo() {
         return mResponseInfo;
     }
+
+    /**
+     * If the request failed, returns the same {@link UrlRequestException} provided to
+     * {@link UrlRequest.Callback#onFailed}.
+     *
+     * @return the request's {@link UrlRequestException}, if the request failed
+     */
+    @Nullable
+    public UrlRequestException getException() {
+        return mException;
+    }
 }