Log urls and headers in BIDIRECTIONAL_STREAM events

Log urls and headers in BIDIRECTIONAL_STREAM events

BIDIRECTIONAL_STREAM events do not have urls and headers in NetLog. This makes
debugging hard. Headers are currently logged in HTTP2_SESSION and QUIC_SESSION,
but the session events can have a bunch of requests going on, and it is hard to
associate headers with each bidirectional stream.

This CL logs urls and the headers sent and received in BIDIRECTIONAL_STREAM
events.

BUG=607613
Committed: https://crrev.com/d4c21ab215ec191359bd8bb3f85f1249e219845c
Cr-Commit-Position: refs/heads/master@{#390518}

[xunjieli, 2016-04-28 20:10:52]

Patchset #1 (id:1) has been deleted

[xunjieli, 2016-04-28 20:11:22]

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

[xunjieli, 2016-04-28 20:11:22]

Misha: PTAL. Thanks!

[mef, 2016-04-28 20:43:18]

looks good, mostly nits.

https://codereview.chromium.org/1932853002/diff/20001/net/http/bidirectional_...
File net/http/bidirectional_stream.cc (right):

https://codereview.chromium.org/1932853002/diff/20001/net/http/bidirectional_...
net/http/bidirectional_stream.cc:35: const SpdyHeaderBlock* headers,
#include "net/spdy/spdy_header_block.h"

https://codereview.chromium.org/1932853002/diff/20001/net/http/bidirectional_...
net/http/bidirectional_stream.cc:36: NetLogCaptureMode capture_mode) {
#include "net/log/net_log_capture_mode.h"

https://codereview.chromium.org/1932853002/diff/20001/net/http/bidirectional_...
net/http/bidirectional_stream.cc:127:
net_log_.EndEvent(NetLog::TYPE_BIDIRECTIONAL_STREAM_ALIVE);
Should this also be guarded by net_log_.IsCapturing()?

https://codereview.chromium.org/1932853002/diff/20001/net/http/bidirectional_...
File net/http/bidirectional_stream_unittest.cc (right):

https://codereview.chromium.org/1932853002/diff/20001/net/http/bidirectional_...
net/http/bidirectional_stream_unittest.cc:502: SpdyHeaderBlock late_headers;
maybe rename late_headers => trailers and trailers => response_trailers?

https://codereview.chromium.org/1932853002/diff/20001/net/http/bidirectional_...
net/http/bidirectional_stream_unittest.cc:608: index =
ExpectLogContainsSomewhere(entries, index,
anything interesting to check at this index?

https://codereview.chromium.org/1932853002/diff/20001/net/http/http_log_util.h
File net/http/http_log_util.h (right):

https://codereview.chromium.org/1932853002/diff/20001/net/http/http_log_util....
net/http/http_log_util.h:35: std::unique_ptr<base::ListValue>
SpdyHeaderBlockToListValue(
Maybe ElideSpdyHeaderBlockForNetLog? I see that you didn't change/choose the
name, so feel free to ignore.

https://codereview.chromium.org/1932853002/diff/20001/net/log/net_log_event_t...
File net/log/net_log_event_type_list.h (right):

https://codereview.chromium.org/1932853002/diff/20001/net/log/net_log_event_t...
net/log/net_log_event_type_list.h:1174: // The event parameters are:
The following parameters are attached:

https://codereview.chromium.org/1932853002/diff/20001/net/log/net_log_event_t...
net/log/net_log_event_type_list.h:1176: //      "url": <The URL actually being
used>,
I'm not sure that "actually" is applicable here as there is no implicit redirect
handling.

https://codereview.chromium.org/1932853002/diff/20001/net/log/net_log_event_t...
net/log/net_log_event_type_list.h:1178: //      "headers": <The HTTP headers
going to be sent with this request>,
<The list of header:value pairs>,

[xunjieli, 2016-04-28 21:06:16]

Thanks for the review!

https://codereview.chromium.org/1932853002/diff/20001/net/http/bidirectional_...
File net/http/bidirectional_stream.cc (right):

https://codereview.chromium.org/1932853002/diff/20001/net/http/bidirectional_...
net/http/bidirectional_stream.cc:35: const SpdyHeaderBlock* headers,
On 2016/04/28 20:43:18, mef wrote:
> #include "net/spdy/spdy_header_block.h"

Done.

https://codereview.chromium.org/1932853002/diff/20001/net/http/bidirectional_...
net/http/bidirectional_stream.cc:36: NetLogCaptureMode capture_mode) {
On 2016/04/28 20:43:18, mef wrote:
> #include "net/log/net_log_capture_mode.h"

Done.

https://codereview.chromium.org/1932853002/diff/20001/net/http/bidirectional_...
net/http/bidirectional_stream.cc:127:
net_log_.EndEvent(NetLog::TYPE_BIDIRECTIONAL_STREAM_ALIVE);
On 2016/04/28 20:43:18, mef wrote:
> Should this also be guarded by net_log_.IsCapturing()?

Done. good catch! thanks

https://codereview.chromium.org/1932853002/diff/20001/net/http/bidirectional_...
File net/http/bidirectional_stream_unittest.cc (right):

https://codereview.chromium.org/1932853002/diff/20001/net/http/bidirectional_...
net/http/bidirectional_stream_unittest.cc:502: SpdyHeaderBlock late_headers;
On 2016/04/28 20:43:18, mef wrote:
> maybe rename late_headers => trailers and trailers => response_trailers?

Done.

https://codereview.chromium.org/1932853002/diff/20001/net/http/bidirectional_...
net/http/bidirectional_stream_unittest.cc:608: index =
ExpectLogContainsSomewhere(entries, index,
On 2016/04/28 20:43:18, mef wrote:
> anything interesting to check at this index?

Done. Nothing in particular, so i have got rid of "index =". Thanks.

https://codereview.chromium.org/1932853002/diff/20001/net/http/http_log_util.h
File net/http/http_log_util.h (right):

https://codereview.chromium.org/1932853002/diff/20001/net/http/http_log_util....
net/http/http_log_util.h:35: std::unique_ptr<base::ListValue>
SpdyHeaderBlockToListValue(
On 2016/04/28 20:43:18, mef wrote:
> Maybe ElideSpdyHeaderBlockForNetLog? I see that you didn't change/choose the
> name, so feel free to ignore.

Done.

https://codereview.chromium.org/1932853002/diff/20001/net/log/net_log_event_t...
File net/log/net_log_event_type_list.h (right):

https://codereview.chromium.org/1932853002/diff/20001/net/log/net_log_event_t...
net/log/net_log_event_type_list.h:1174: // The event parameters are:
On 2016/04/28 20:43:18, mef wrote:
> The following parameters are attached:

Done.

https://codereview.chromium.org/1932853002/diff/20001/net/log/net_log_event_t...
net/log/net_log_event_type_list.h:1176: //      "url": <The URL actually being
used>,
On 2016/04/28 20:43:18, mef wrote:
> I'm not sure that "actually" is applicable here as there is no implicit
redirect
> handling.

Done. Artifact of copy and paste :) Good catch.

https://codereview.chromium.org/1932853002/diff/20001/net/log/net_log_event_t...
net/log/net_log_event_type_list.h:1178: //      "headers": <The HTTP headers
going to be sent with this request>,
On 2016/04/28 20:43:18, mef wrote:
> <The list of header:value pairs>,

Done.

[mef, 2016-04-28 21:30:12]

lgtm

https://codereview.chromium.org/1932853002/diff/40001/net/http/bidirectional_...
File net/http/bidirectional_stream_unittest.cc (right):

https://codereview.chromium.org/1932853002/diff/40001/net/http/bidirectional_...
net/http/bidirectional_stream_unittest.cc:574: // Destroy the delegate will
destroy the stream, so we can get a
nit: I think the 'a' at the end is not needed.

[xunjieli, 2016-04-28 21:33:36]

Thanks!

https://codereview.chromium.org/1932853002/diff/40001/net/http/bidirectional_...
File net/http/bidirectional_stream_unittest.cc (right):

https://codereview.chromium.org/1932853002/diff/40001/net/http/bidirectional_...
net/http/bidirectional_stream_unittest.cc:574: // Destroy the delegate will
destroy the stream, so we can get a
On 2016/04/28 21:30:11, mef wrote:
> nit: I think the 'a' at the end is not needed.

Done.

[xunjieli, 2016-04-28 21:33:49]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2016-04-28 21:33:51]

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

[commit-bot: I haz the power, 2016-04-28 21:34:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1932853002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1932853002/60001

[commit-bot: I haz the power, 2016-04-28 22:44:56]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-04-30 17:22:19]

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

[commit-bot: I haz the power, 2016-04-30 17:58:08]

Description was changed from

==========
Log urls and headers in BIDIRECTIONAL_STREAM events

BIDIRECTIONAL_STREAM events do not have urls and headers in NetLog. This makes
debugging hard. Headers are currently logged in HTTP2_SESSION and QUIC_SESSION,
but the session events can have a bunch of requests going on, and it is hard to
associate headers with each bidirectional stream.

This CL logs urls and the headers sent and received in BIDIRECTIONAL_STREAM
events.

BUG=607613
==========

to

==========
Log urls and headers in BIDIRECTIONAL_STREAM events

BIDIRECTIONAL_STREAM events do not have urls and headers in NetLog. This makes
debugging hard. Headers are currently logged in HTTP2_SESSION and QUIC_SESSION,
but the session events can have a bunch of requests going on, and it is hard to
associate headers with each bidirectional stream.

This CL logs urls and the headers sent and received in BIDIRECTIONAL_STREAM
events.

BUG=607613

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