Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2220023002:
Add API for new Cronet metrics (Closed)
DescriptionAdd API for new Cronet metrics
This API is not yet connected to anything in the net stack. Most of
these new metrics will be plumbed through from net::LoadTimingInfo, and
a few from other places.
Includes a unit test for the big mess of longs/Dates in
RequestFinishedInfo.Metrics, but no other tests yet because nothing
works yet.
Leaves the old APIs in place for now, since those actually work.
BUG=629194
Committed: https://crrev.com/ee557cce67e828cc6f31fcef06480ab570ee6e7c
Cr-Commit-Position: refs/heads/master@{#419185}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Separate Metrics API and impl, and improve documentation #
Total comments: 12
Patch Set 3 : Address comments and add request start (how did I miss that?) #
Total comments: 44
Patch Set 4 : address comments, mostly on javadoc #
Total comments: 16
Patch Set 5 : more comments #Patch Set 6 : rebase #Patch Set 7 : delete bad assert (this isn't true for QUIC 0-RTT) #
Total comments: 12
Patch Set 8 : words #
Dependent Patchsets: Messages
Total messages: 28 (5 generated)
mgersh@chromium.org changed reviewers: + mef@chromium.org, xunjieli@chromium.org
PTAL. This is still somewhat work in progress, because I don't think I'm happy with the state of the javadoc yet, but I thought I'd send it out so you can get started looking at the code and I'll finish writing better documentation on Monday. Notable change from previous design: timestamps are stored as longs and turned into Dates by the getters, because Dates are mutable for some reason.
https://codereview.chromium.org/2220023002/diff/1/components/cronet/android/a... File components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java (right): https://codereview.chromium.org/2220023002/diff/1/components/cronet/android/a... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:79: @Nullable Is simple long nullable or do you need to use Long for that? https://codereview.chromium.org/2220023002/diff/1/components/cronet/android/a... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:141: mProxyEndMs = proxyEndMs; shouldall these constructors and member variables go into implementation and just keep abstract public methods here? Consider what would happen if app is build with Cronet v.55 and later on in Cronet.v65 we would want to add another metrics value. In current scenario Cronet.v65 in GMS Core will be unable to do that as metrics are implemented in api. If we just keep definition here, then we could update Metrics implementation and new apps aware of v.65 will be able to use that, while v.55 will keep working as before. https://codereview.chromium.org/2220023002/diff/1/components/cronet/android/a... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:264: * was received. For all @Nullable results we need to explain the situation when they could be null.
Here's a version which is ready for review. https://codereview.chromium.org/2220023002/diff/1/components/cronet/android/a... File components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java (right): https://codereview.chromium.org/2220023002/diff/1/components/cronet/android/a... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:79: @Nullable On 2016/08/10 03:03:37, mef wrote: > Is simple long nullable or do you need to use Long for that? I didn't actually want to make these nullable, they'll just be 0 and then get turned into null when converted to Date. Deleted all the @Nullable annotations. https://codereview.chromium.org/2220023002/diff/1/components/cronet/android/a... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:141: mProxyEndMs = proxyEndMs; On 2016/08/10 03:03:37, mef wrote: > shouldall these constructors and member variables go into implementation and > just keep abstract public methods here? > > Consider what would happen if app is build with Cronet v.55 and later on in > Cronet.v65 we would want to add another metrics value. In current scenario > Cronet.v65 in GMS Core will be unable to do that as metrics are implemented in > api. If we just keep definition here, then we could update Metrics > implementation and new apps aware of v.65 will be able to use that, while v.55 > will keep working as before. Done.
I'd like to understand why we decided to use java.util.Date instead of a type that simply says how many milliseconds have elapsed (something like Long). The latter will allow us to match the NavigationTiming specs (https://www.w3.org/TR/navigation-timing-2/), which says that start time is strictly 0. My concern with using java.util.Date is that it adds to the complexity without bringing much benefit -- Embedders need to take care of comparing these with the timestamps that they keep. They also need to worry about the possibility of clocks running backwards. Although that our native stack ensures that these values will be strictly increasing, the fact that these values are java.util.Date introduces these questions and consumers might wonder whether special handling is needed. How about we use Long or a custom TimeStamp class that allow us to enforce stricter restrictions (e.g. start time is 0) and make the API contract clearer?
On 2016/08/30 21:37:04, xunjieli wrote: > I'd like to understand why we decided to use java.util.Date instead of a type > that simply says how many milliseconds have elapsed (something like Long). The > latter will allow us to match the NavigationTiming specs > (https://www.w3.org/TR/navigation-timing-2/), which says that start time is > strictly 0. My concern with using java.util.Date is that it adds to the > complexity without bringing much benefit -- Embedders need to take care of > comparing these with the timestamps that they keep. They also need to worry > about the possibility of clocks running backwards. Although that our native > stack ensures that these values will be strictly increasing, the fact that these > values are java.util.Date introduces these questions and consumers might wonder > whether special handling is needed. > > How about we use Long or a custom TimeStamp class that allow us to enforce > stricter restrictions (e.g. start time is 0) and make the API contract clearer? I used java.util.Date because Paul suggested it on the design doc; I'm not sure what his reasons were. I don't have a strong opinion on whether Date or long is better, but that's a good point about it being confusing to represent a monotonically increasing clock as a Date. I don't want to just set request start to 0, though, because that doesn't provide a way for embedders to anchor our timestamps to their timeline. I think the use case we're targeting is different from the navigation/resource timing use case, even though we use the same numbers, and we'd be better off providing specific points in time rather than only relative times. Another option is providing a single Date representing request start time and having the rest as offsets from that time, but that seems like it would be even more confusing than having everything represented as ms since unix epoch.
On 2016/08/31 18:05:32, mgersh wrote: > On 2016/08/30 21:37:04, xunjieli wrote: > > I'd like to understand why we decided to use java.util.Date instead of a type > > that simply says how many milliseconds have elapsed (something like Long). The > > latter will allow us to match the NavigationTiming specs > > (https://www.w3.org/TR/navigation-timing-2/), which says that start time is > > strictly 0. My concern with using java.util.Date is that it adds to the > > complexity without bringing much benefit -- Embedders need to take care of > > comparing these with the timestamps that they keep. They also need to worry > > about the possibility of clocks running backwards. Although that our native > > stack ensures that these values will be strictly increasing, the fact that > these > > values are java.util.Date introduces these questions and consumers might > wonder > > whether special handling is needed. > > > > How about we use Long or a custom TimeStamp class that allow us to enforce > > stricter restrictions (e.g. start time is 0) and make the API contract > clearer? > > I used java.util.Date because Paul suggested it on the design doc; I'm not sure > what his reasons were. I don't have a strong opinion on whether Date or long is > better, but that's a good point about it being confusing to represent a > monotonically increasing clock as a Date. I don't want to just set request start > to 0, though, because that doesn't provide a way for embedders to anchor our > timestamps to their timeline. I think the use case we're targeting is different > from the navigation/resource timing use case, even though we use the same > numbers, and we'd be better off providing specific points in time rather than > only relative times. Another option is providing a single Date representing > request start time and having the rest as offsets from that time, but that seems > like it would be even more confusing than having everything represented as ms > since unix epoch. I think there are pros and cons to each approach. I don't oppose using Date, but I wanted to understand the reason behind the choice. I will get back to this review by tomorrow. Sorry for the delay. I was catching up on my emails and sheriff duties.
Looks great. A couple of comments below. https://codereview.chromium.org/2220023002/diff/20001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java (right): https://codereview.chromium.org/2220023002/diff/20001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:91: * is no PAC. As far as I know, Cronet doesn't do PAC resolution. Chrome's PAC resolution uses V8. Cronet doesn't package V8. If there is a PAC URL, Android platform hides that and gives us the address of a local HTTP server that's created by the platform for this purpose instead of a PAC url. "proxy start" and "proxy end" will always be null. I don't think we should introduce these two since they aren't meaningful at least right now. https://codereview.chromium.org/2220023002/diff/20001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:190: public abstract Long getTtfbMs(); I am inclining to suggest to spell out Tfbs since it doesn't seem to be a common acronym? https://codereview.chromium.org/2220023002/diff/20001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:214: public static final int CANCELED = 2; Maybe use intDef and add a documentation on what these mean? https://codereview.chromium.org/2220023002/diff/20001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/impl/CronetMetrics.java (right): https://codereview.chromium.org/2220023002/diff/20001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetMetrics.java:50: * Old-style constructor Is this going away? Should we add a TODO here so it's clearer? https://codereview.chromium.org/2220023002/diff/20001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetMetrics.java:84: long receivedBytesCount) { Might worth doing some assertions in the constructor to make sure we are passing the values correctly? The argument list is pretty long. The arguments are all longs. no pun intended :) If we switch things around accidentally, we wouldn't know. How about: eg. assert dnsEnd > dnsStart assert connectStartMs > dnsStart , etc.? https://codereview.chromium.org/2220023002/diff/20001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetMetrics.java:103: // Don't care about these anymore Why do we not care about these anymore? Could you add TODO to get rid of them in the parent class?
Thanks, these were very helpful comments. I also added in request start because I'd somehow missed it before, and added a paragraph about how the timestamps work. https://codereview.chromium.org/2220023002/diff/20001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java (right): https://codereview.chromium.org/2220023002/diff/20001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:91: * is no PAC. On 2016/09/01 22:17:42, xunjieli wrote: > As far as I know, Cronet doesn't do PAC resolution. Chrome's PAC resolution uses > V8. Cronet doesn't package V8. If there is a PAC URL, Android platform hides > that and gives us the address of a local HTTP server that's created by the > platform for this purpose instead of a PAC url. > > "proxy start" and "proxy end" will always be null. I don't think we should > introduce these two since they aren't meaningful at least right now. Ah, didn't know that, thanks. Done. https://codereview.chromium.org/2220023002/diff/20001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:190: public abstract Long getTtfbMs(); On 2016/09/01 22:17:42, xunjieli wrote: > I am inclining to suggest to spell out Tfbs since it doesn't seem to be a common > acronym? This is part of the old API which will go away soon, so no point renaming it now. (We provide request start and response start, embedders can calculate ttfb on their own if they care.) Added a TODO to make it more clear. https://codereview.chromium.org/2220023002/diff/20001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:214: public static final int CANCELED = 2; On 2016/09/01 22:17:42, xunjieli wrote: > Maybe use intDef and add a documentation on what these mean? Huh, IntDef is cool. Done. https://codereview.chromium.org/2220023002/diff/20001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/impl/CronetMetrics.java (right): https://codereview.chromium.org/2220023002/diff/20001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetMetrics.java:50: * Old-style constructor On 2016/09/01 22:17:42, xunjieli wrote: > Is this going away? > Should we add a TODO here so it's clearer? Done. https://codereview.chromium.org/2220023002/diff/20001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetMetrics.java:84: long receivedBytesCount) { On 2016/09/01 22:17:42, xunjieli wrote: > Might worth doing some assertions in the constructor to make sure we are passing > the values correctly? > > The argument list is pretty long. The arguments are all longs. no pun intended > :) If we switch things around accidentally, we wouldn't know. > > How about: > eg. > assert dnsEnd > dnsStart > assert connectStartMs > dnsStart > , etc.? Yeah, I was worried about that too. Done. https://codereview.chromium.org/2220023002/diff/20001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetMetrics.java:103: // Don't care about these anymore On 2016/09/01 22:17:42, xunjieli wrote: > Why do we not care about these anymore? Could you add TODO to get rid of them in > the parent class? Done.
Looks good. Ready to sign off after the following comments are addressed. https://codereview.chromium.org/2220023002/diff/40001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java (right): https://codereview.chromium.org/2220023002/diff/40001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:91: * of the native URLRequest layer. This timestamp will match the system clock at the time it 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. https://codereview.chromium.org/2220023002/diff/40001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/impl/CronetMetrics.java (right): https://codereview.chromium.org/2220023002/diff/40001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetMetrics.java:15: * Implementation of RequestFinishedInfo.Metrics nit: Add a {@link} and a period at the end of the half sentence? https://codereview.chromium.org/2220023002/diff/40001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetMetrics.java:64: // Everything else is 0 for now Should the null case be -1 instead of 0? See my other comment for why I am voting for the -1 case. https://codereview.chromium.org/2220023002/diff/40001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/RequestFinishedInfoTest.java (right): https://codereview.chromium.org/2220023002/diff/40001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/RequestFinishedInfoTest.java:270: long dnsEnd = 3; Shouldn't this test case trigger an assertion? Passing a dnsStart that is > requestStart seems to be a bad argument. I know that you meant to have dnsStart as a null timestamp, but the JavaDoc doesn't indicate that it's valid to have a null Start but a non-null End. To make things clearer, can we pass the null timestamps from C++ to Java as -1 longs? That way it will be easier to add assertions and will allow us to differentiate between timestamps been 0 and null. WDYT?
pauljensen@chromium.org changed reviewers: + pauljensen@chromium.org
I think this is going in the right direction; my comments are mostly just about the javadocs. https://codereview.chromium.org/2220023002/diff/40001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java (right): https://codereview.chromium.org/2220023002/diff/40001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:54: * Metrics collected for a single request. 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." https://codereview.chromium.org/2220023002/diff/40001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:59: * {@link https://www.w3.org/TR/resource-timing/}. 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? https://codereview.chromium.org/2220023002/diff/40001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:62: * request start Use <ol> and <li>'s https://codereview.chromium.org/2220023002/diff/40001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:62: * request start I feel like we should include links from these list items to the getters below. Perhaps like: Request start (see {@link Metrics#getRequestStart}) https://codereview.chromium.org/2220023002/diff/40001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:63: * dns start dns->DNS here and below https://codereview.chromium.org/2220023002/diff/40001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:66: * ssl start ssl->SSL here and below https://codereview.chromium.org/2220023002/diff/40001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:77: * stage, start and end times will be null. If no time was spent blocking on an event, start null->{@code null} https://codereview.chromium.org/2220023002/diff/40001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:84: * guaranteed to match the system clock at the times they represent. 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. https://codereview.chromium.org/2220023002/diff/40001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:90: * @return {@link java.util.Date} representing when the request started from the perspective 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 https://codereview.chromium.org/2220023002/diff/40001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:98: * @return {@link java.util.Date} representing when DNS lookup started. Null if a proxy is Null->{@code null} here and below https://codereview.chromium.org/2220023002/diff/40001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:98: * @return {@link java.util.Date} representing when DNS lookup started. Null if a proxy is What do you mean by "a proxy" here? https://codereview.chromium.org/2220023002/diff/40001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:99: * used to determine the DNS address. doesn't this and some of the other methods return null when the socket is reused? https://codereview.chromium.org/2220023002/diff/40001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:146: * @return {@link java.util.Date} representing when sending HTTP request body finished. 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? https://codereview.chromium.org/2220023002/diff/40001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:152: * @return {@link java.util.Date} representing when the first byte of an HTTP2 server push HTTP2->HTTP/2 here and below so as to match things like CronetEngine.Builder.enableHTTP2 documentation https://codereview.chromium.org/2220023002/diff/40001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:180: * @return whether this request reused a socket from a previous request. When true, DNS, true->{@code true} https://codereview.chromium.org/2220023002/diff/40001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:225: * The request succeeded. Returned from {@link #getFinishedReason}. do you need to qualify the class, so Metrics#getFinishedReason? https://codereview.chromium.org/2220023002/diff/40001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:225: * The request succeeded. Returned from {@link #getFinishedReason}. should we prefix this description with "Reason value indicating "
https://codereview.chromium.org/2220023002/diff/40001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/impl/CronetMetrics.java (right): https://codereview.chromium.org/2220023002/diff/40001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetMetrics.java:34: // TODO(mgersh): Delete once we switch to the new API http://crbug.com/629194 nit: s/once we/after/g
https://codereview.chromium.org/2220023002/diff/40001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java (right): https://codereview.chromium.org/2220023002/diff/40001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:54: * Metrics collected for a single request. On 2016/09/06 17:22:23, pauljensen wrote: > 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." Done. https://codereview.chromium.org/2220023002/diff/40001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:59: * {@link https://www.w3.org/TR/resource-timing/}. On 2016/09/06 17:22:23, pauljensen wrote: > 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? Done. https://codereview.chromium.org/2220023002/diff/40001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:62: * request start On 2016/09/06 17:22:23, pauljensen wrote: > Use <ol> and <li>'s Done. https://codereview.chromium.org/2220023002/diff/40001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:62: * request start On 2016/09/06 17:22:23, pauljensen wrote: > I feel like we should include links from these list items to the getters below. > Perhaps like: > Request start (see {@link Metrics#getRequestStart}) Done. https://codereview.chromium.org/2220023002/diff/40001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:63: * dns start On 2016/09/06 17:22:23, pauljensen wrote: > dns->DNS > here and below Done. https://codereview.chromium.org/2220023002/diff/40001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:66: * ssl start On 2016/09/06 17:22:23, pauljensen wrote: > ssl->SSL > here and below Done. https://codereview.chromium.org/2220023002/diff/40001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:77: * stage, start and end times will be null. If no time was spent blocking on an event, start On 2016/09/06 17:22:23, pauljensen wrote: > null->{@code null} Done. https://codereview.chromium.org/2220023002/diff/40001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:84: * guaranteed to match the system clock at the times they represent. On 2016/09/06 17:22:23, pauljensen wrote: > 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. Done. https://codereview.chromium.org/2220023002/diff/40001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:90: * @return {@link java.util.Date} representing when the request started from the perspective On 2016/09/06 17:22:23, pauljensen wrote: > 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 Done. https://codereview.chromium.org/2220023002/diff/40001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:91: * of the native URLRequest layer. This timestamp will match the system clock at the time it On 2016/09/05 23:24:17, xunjieli wrote: > 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. Done. https://codereview.chromium.org/2220023002/diff/40001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:98: * @return {@link java.util.Date} representing when DNS lookup started. Null if a proxy is On 2016/09/06 17:22:23, pauljensen wrote: > Null->{@code null} > here and below Done. https://codereview.chromium.org/2220023002/diff/40001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:98: * @return {@link java.util.Date} representing when DNS lookup started. Null if a proxy is On 2016/09/06 17:22:23, pauljensen wrote: > What do you mean by "a proxy" here? I think I mean the thing that Helen said doesn't apply to us, but forgot to delete it here. https://codereview.chromium.org/2220023002/diff/40001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:99: * used to determine the DNS address. On 2016/09/06 17:22:23, pauljensen wrote: > doesn't this and some of the other methods return null when the socket is > reused? Done. https://codereview.chromium.org/2220023002/diff/40001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:146: * @return {@link java.util.Date} representing when sending HTTP request body finished. On 2016/09/06 17:22:23, pauljensen wrote: > 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? Done. https://codereview.chromium.org/2220023002/diff/40001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:152: * @return {@link java.util.Date} representing when the first byte of an HTTP2 server push On 2016/09/06 17:22:23, pauljensen wrote: > HTTP2->HTTP/2 > here and below > so as to match things like CronetEngine.Builder.enableHTTP2 documentation Done. https://codereview.chromium.org/2220023002/diff/40001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:180: * @return whether this request reused a socket from a previous request. When true, DNS, On 2016/09/06 17:22:23, pauljensen wrote: > true->{@code true} Done. https://codereview.chromium.org/2220023002/diff/40001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:225: * The request succeeded. Returned from {@link #getFinishedReason}. On 2016/09/06 17:22:23, pauljensen wrote: > should we prefix this description with "Reason value indicating " Done. https://codereview.chromium.org/2220023002/diff/40001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:225: * The request succeeded. Returned from {@link #getFinishedReason}. On 2016/09/06 17:22:23, pauljensen wrote: > do you need to qualify the class, so Metrics#getFinishedReason? No, both this and getFinishedReason() are in RequestFinishedInfo, not a nested class. https://codereview.chromium.org/2220023002/diff/40001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/impl/CronetMetrics.java (right): https://codereview.chromium.org/2220023002/diff/40001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetMetrics.java:15: * Implementation of RequestFinishedInfo.Metrics On 2016/09/05 23:24:17, xunjieli wrote: > nit: Add a {@link} and a period at the end of the half sentence? Done. https://codereview.chromium.org/2220023002/diff/40001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetMetrics.java:34: // TODO(mgersh): Delete once we switch to the new API http://crbug.com/629194 On 2016/09/07 21:10:50, mef wrote: > nit: s/once we/after/g Done, but could you explain why? Is first person discouraged in TODOs or something? https://codereview.chromium.org/2220023002/diff/40001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetMetrics.java:64: // Everything else is 0 for now On 2016/09/05 23:24:17, xunjieli wrote: > Should the null case be -1 instead of 0? See my other comment for why I am > voting for the -1 case. Done. https://codereview.chromium.org/2220023002/diff/40001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/RequestFinishedInfoTest.java (right): https://codereview.chromium.org/2220023002/diff/40001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/RequestFinishedInfoTest.java:270: long dnsEnd = 3; On 2016/09/05 23:24:17, xunjieli wrote: > Shouldn't this test case trigger an assertion? > Passing a dnsStart that is > requestStart seems to be a bad argument. I know > that you meant to have dnsStart as a null timestamp, but the JavaDoc doesn't > indicate that it's valid to have a null Start but a non-null End. > > To make things clearer, can we pass the null timestamps from C++ to Java as -1 > longs? That way it will be easier to add assertions and will allow us to > differentiate between timestamps been 0 and null. > WDYT? Good point, that wouldn't make any sense if it were real data. And sure, I can change it to -1.
lgtm https://codereview.chromium.org/2220023002/diff/60001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/impl/CronetMetrics.java (right): https://codereview.chromium.org/2220023002/diff/60001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetMetrics.java:96: // or exist when start time doesn't nit: might want to complete the sentence with a period at the end.
https://codereview.chromium.org/2220023002/diff/60001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java (right): https://codereview.chromium.org/2220023002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:92: * {hide} as it's a prototype. hide->@hide https://codereview.chromium.org/2220023002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:112: * Returns time when DNS lookup finished. We should address what happens if Cronet internally caches DNS results (which it does for 2 minutes IIRC). Will the DNS lookup timestamps be null (I'd guess not) or is the time spend doing the DNS lookup very short (my guess)? If it's just very short we should mention in these comments that it may not actually be a DNS request over the network to a DNS server but instead my be a cache lookup. https://codereview.chromium.org/2220023002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:120: * Returns time when connection establishmentn started. extra n on the end of establishment https://codereview.chromium.org/2220023002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:146: * Returns time when SSL handshake finished. What are the SSL timestamps for QUIC 0-RTT? https://codereview.chromium.org/2220023002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:185: * Returns time when first byte of response was received. Does this include the headers and/or the body? https://codereview.chromium.org/2220023002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:203: * DNS, connection, and SSL times will be {@code null}. What happens for multiplexed streams (i.e. HTTP/2 or QUIC) when the socket is reused? Is there any stream-establishment time? if so, where is it accounted for in these metrics? https://codereview.chromium.org/2220023002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:211: * TODO(mgersh): Remove once new API works http://crbug.com/629194 add a @hide here and on line 220; that way if someone unhides this class we don't include TODO's in javadocs.
https://codereview.chromium.org/2220023002/diff/60001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java (right): https://codereview.chromium.org/2220023002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:92: * {hide} as it's a prototype. On 2016/09/12 11:50:09, pauljensen wrote: > hide->@hide Oops. I was testing and forgot to change it back. Done. https://codereview.chromium.org/2220023002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:112: * Returns time when DNS lookup finished. On 2016/09/12 11:50:09, pauljensen wrote: > We should address what happens if Cronet internally caches DNS results (which it > does for 2 minutes IIRC). Will the DNS lookup timestamps be null (I'd guess > not) or is the time spend doing the DNS lookup very short (my guess)? If it's > just very short we should mention in these comments that it may not actually be > a DNS request over the network to a DNS server but instead my be a cache lookup. Done. https://codereview.chromium.org/2220023002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:120: * Returns time when connection establishmentn started. On 2016/09/12 11:50:09, pauljensen wrote: > extra n on the end of establishment Done. https://codereview.chromium.org/2220023002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:146: * Returns time when SSL handshake finished. On 2016/09/12 11:50:09, pauljensen wrote: > What are the SSL timestamps for QUIC 0-RTT? Done. https://codereview.chromium.org/2220023002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:185: * Returns time when first byte of response was received. On 2016/09/12 11:50:09, pauljensen wrote: > Does this include the headers and/or the body? Took another look at what this is and rewrote it. https://codereview.chromium.org/2220023002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:203: * DNS, connection, and SSL times will be {@code null}. On 2016/09/12 11:50:09, pauljensen wrote: > What happens for multiplexed streams (i.e. HTTP/2 or QUIC) when the socket is > reused? Is there any stream-establishment time? if so, where is it accounted > for in these metrics? Done. https://codereview.chromium.org/2220023002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:211: * TODO(mgersh): Remove once new API works http://crbug.com/629194 On 2016/09/12 11:50:09, pauljensen wrote: > add a @hide here and on line 220; that way if someone unhides this class we > don't include TODO's in javadocs. Done. https://codereview.chromium.org/2220023002/diff/60001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/impl/CronetMetrics.java (right): https://codereview.chromium.org/2220023002/diff/60001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetMetrics.java:96: // or exist when start time doesn't On 2016/09/08 18:45:38, xunjieli wrote: > nit: might want to complete the sentence with a period at the end. Done.
lgtm if you include the proposed changes https://codereview.chromium.org/2220023002/diff/120001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java (right): https://codereview.chromium.org/2220023002/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:191: * Returns time when the end of the response headers were received. were->was https://codereview.chromium.org/2220023002/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:192: * @return {@link java.util.Date} representing when the first byte of the response was response->response body https://codereview.chromium.org/2220023002/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:263: * Reason value indicating that the request failed or errored. Returned from errored isn't a word, perhaps say "returned an error" https://codereview.chromium.org/2220023002/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:268: * Reason value indicating that the request was canceled. Returned from There are two L's in cancelled https://codereview.chromium.org/2220023002/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:271: public static final int CANCELED = 2; There are two L's in cancelled
lgtm https://codereview.chromium.org/2220023002/diff/120001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java (right): https://codereview.chromium.org/2220023002/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:271: public static final int CANCELED = 2; On 2016/09/16 13:43:31, pauljensen wrote: > There are two L's in cancelled Not in Amerika: http://grammarist.com/spelling/cancel/ :)
https://codereview.chromium.org/2220023002/diff/120001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java (right): https://codereview.chromium.org/2220023002/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:271: public static final int CANCELED = 2; On 2016/09/16 14:06:00, mef wrote: > On 2016/09/16 13:43:31, pauljensen wrote: > > There are two L's in cancelled > > Not in Amerika: http://grammarist.com/spelling/cancel/ :) According to Google it's cancelled: http://google.com/#q=cancelled But reading Misha's link, I'm fine with either spelling.
https://codereview.chromium.org/2220023002/diff/120001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java (right): https://codereview.chromium.org/2220023002/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:271: public static final int CANCELED = 2; On 2016/09/16 14:09:46, pauljensen wrote: > On 2016/09/16 14:06:00, mef wrote: > > On 2016/09/16 13:43:31, pauljensen wrote: > > > There are two L's in cancelled > > > > Not in Amerika: http://grammarist.com/spelling/cancel/ :) > > According to Google it's cancelled: http://google.com/#q=cancelled > But reading Misha's link, I'm fine with either spelling. Ha, if you search for the other spelling, it tells you that you're right: http://google.com/#q=canceled
Thanks everyone for all your comments! https://codereview.chromium.org/2220023002/diff/120001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java (right): https://codereview.chromium.org/2220023002/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:191: * Returns time when the end of the response headers were received. On 2016/09/16 13:43:31, pauljensen wrote: > were->was Done. https://codereview.chromium.org/2220023002/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:192: * @return {@link java.util.Date} representing when the first byte of the response was On 2016/09/16 13:43:31, pauljensen wrote: > response->response body Oops, I forgot to update this part when I updated the first sentence. https://codereview.chromium.org/2220023002/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:263: * Reason value indicating that the request failed or errored. Returned from On 2016/09/16 13:43:31, pauljensen wrote: > errored isn't a word, perhaps say "returned an error" Done. https://codereview.chromium.org/2220023002/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/RequestFinishedInfo.java:271: public static final int CANCELED = 2; On 2016/09/16 14:11:10, pauljensen wrote: > On 2016/09/16 14:09:46, pauljensen wrote: > > On 2016/09/16 14:06:00, mef wrote: > > > On 2016/09/16 13:43:31, pauljensen wrote: > > > > There are two L's in cancelled > > > > > > Not in Amerika: http://grammarist.com/spelling/cancel/ :) > > > > According to Google it's cancelled: http://google.com/#q=cancelled > > But reading Misha's link, I'm fine with either spelling. > > Ha, if you search for the other spelling, it tells you that you're right: > http://google.com/#q=canceled I asked git grep about this. Results: - In components/cronet/ we almost always use "canceled" - In net/ we usually use "cancelled" but sometimes also "canceled" - In all of Chromium we use the two about equally often ¯\_(ツ)_/¯
The CQ bit was checked by mgersh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xunjieli@chromium.org, pauljensen@chromium.org, mef@chromium.org Link to the patchset: https://codereview.chromium.org/2220023002/#ps140001 (title: "words")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Add API for new Cronet metrics This API is not yet connected to anything in the net stack. Most of these new metrics will be plumbed through from net::LoadTimingInfo, and a few from other places. Includes a unit test for the big mess of longs/Dates in RequestFinishedInfo.Metrics, but no other tests yet because nothing works yet. Leaves the old APIs in place for now, since those actually work. BUG=629194 ========== to ========== Add API for new Cronet metrics This API is not yet connected to anything in the net stack. Most of these new metrics will be plumbed through from net::LoadTimingInfo, and a few from other places. Includes a unit test for the big mess of longs/Dates in RequestFinishedInfo.Metrics, but no other tests yet because nothing works yet. Leaves the old APIs in place for now, since those actually work. BUG=629194 Committed: https://crrev.com/ee557cce67e828cc6f31fcef06480ab570ee6e7c Cr-Commit-Position: refs/heads/master@{#419185} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/ee557cce67e828cc6f31fcef06480ab570ee6e7c Cr-Commit-Position: refs/heads/master@{#419185} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
