Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
17333003:
Pretty-print QUIC CONNECTION_CLOSE and RST_STREAM error codes. (Closed)
|
Created:
7 years, 6 months ago by Ryan Hamilton Modified:
7 years, 6 months ago Reviewers:
eroman CC:
chromium-reviews, cbentzel+watch_chromium.org, eroman, arv+watch_chromium.org, mmenke Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionPretty-print QUIC CONNECTION_CLOSE and RST_STREAM error codes.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=207773
Patch Set 1 : #Patch Set 2 : Cleanup #
Total comments: 13
Patch Set 3 : Wrap #
Total comments: 16
Patch Set 4 : Fix comments #Patch Set 5 : Rebase #Patch Set 6 : Put NO_ERROR back into the list. #Patch Set 7 : Fix offline comments #Patch Set 8 : Fix enum/int conversion problems. #
Messages
Total messages: 15 (0 generated)
How's this?
Add unittest to chrome/test/data/webui/net_internals/log_view_painter.js https://codereview.chromium.org/17333003/diff/5001/chrome/browser/resources/n... File chrome/browser/resources/net_internals/main.js (right): https://codereview.chromium.org/17333003/diff/5001/chrome/browser/resources/n... chrome/browser/resources/net_internals/main.js:351: * Example: quicErrorToString(25) would return not: "would" may be too strong if the C++ code ever changes. https://codereview.chromium.org/17333003/diff/5001/chrome/browser/resources/n... chrome/browser/resources/net_internals/main.js:363: * Example: quicRstStreamErrorToString(3) would return Same comment as above. https://codereview.chromium.org/17333003/diff/5001/net/quic/quic_error_list.h File net/quic/quic_error_list.h (right): https://codereview.chromium.org/17333003/diff/5001/net/quic/quic_error_list.h... net/quic/quic_error_list.h:14: // There were data frames after the a fin or reset. "the a fin": remove "a" https://codereview.chromium.org/17333003/diff/5001/net/quic/quic_error_list.h... net/quic/quic_error_list.h:42: // The peer is going away.May be a client or server. space after period https://codereview.chromium.org/17333003/diff/5001/net/quic/quic_error_list.h... net/quic/quic_error_list.h:70: // Hanshake failed. Hanshake: (noun) when you shake hands with Han Solo. https://codereview.chromium.org/17333003/diff/5001/net/quic/quic_error_list.h... net/quic/quic_error_list.h:112: QUIC_ERROR(LAST_ERROR) note: this will be exposed in the log file and javascript dictionary. not a big deal, but something to be aware of. https://codereview.chromium.org/17333003/diff/5001/net/quic/quic_protocol.h File net/quic/quic_protocol.h (right): https://codereview.chromium.org/17333003/diff/5001/net/quic/quic_protocol.h#n... net/quic/quic_protocol.h:175: enum QuicRstStreamErrorCode { I dont know this code, but would it make sense for these to be QuicErrorCode (in which case wouldn't need the separate log names). Seeing how the prefix is the same for both sets of errors (QUIC_)
https://codereview.chromium.org/17333003/diff/5001/chrome/browser/resources/n... File chrome/browser/resources/net_internals/main.js (right): https://codereview.chromium.org/17333003/diff/5001/chrome/browser/resources/n... chrome/browser/resources/net_internals/main.js:351: * Example: quicErrorToString(25) would return On 2013/06/17 23:18:19, eroman wrote: > not: "would" may be too strong if the C++ code ever changes. Heh, I simply copied the language from netErrorToString. I changed "would" to "should". Is that better, or did you have something better in mind? ("could") https://codereview.chromium.org/17333003/diff/5001/net/quic/quic_error_list.h File net/quic/quic_error_list.h (right): https://codereview.chromium.org/17333003/diff/5001/net/quic/quic_error_list.h... net/quic/quic_error_list.h:14: // There were data frames after the a fin or reset. On 2013/06/17 23:18:19, eroman wrote: > "the a fin": remove "a" Done. https://codereview.chromium.org/17333003/diff/5001/net/quic/quic_error_list.h... net/quic/quic_error_list.h:42: // The peer is going away.May be a client or server. On 2013/06/17 23:18:19, eroman wrote: > space after period Done. https://codereview.chromium.org/17333003/diff/5001/net/quic/quic_error_list.h... net/quic/quic_error_list.h:70: // Hanshake failed. On 2013/06/17 23:18:19, eroman wrote: > Hanshake: (noun) when you shake hands with Han Solo. BWAHAHAHA! Well played, sir! Done. https://codereview.chromium.org/17333003/diff/5001/net/quic/quic_error_list.h... net/quic/quic_error_list.h:112: QUIC_ERROR(LAST_ERROR) On 2013/06/17 23:18:19, eroman wrote: > note: this will be exposed in the log file and javascript dictionary. not a big > deal, but something to be aware of. Good point. Moved to the enum declaration. https://codereview.chromium.org/17333003/diff/5001/net/quic/quic_protocol.h File net/quic/quic_protocol.h (right): https://codereview.chromium.org/17333003/diff/5001/net/quic/quic_protocol.h#n... net/quic/quic_protocol.h:175: enum QuicRstStreamErrorCode { On 2013/06/17 23:18:19, eroman wrote: > I dont know this code, but would it make sense for these to be QuicErrorCode (in > which case wouldn't need the separate log names). The code started out like this, but it became clear that we needed to distinguish the stream-specific errors from the connection level errors. > Seeing how the prefix is the same for both sets of errors (QUIC_) *nod* We could use a different prefix, I suppose. However, it "works" as is, so I'm inclined to leave it alone. Cool?
lgtm https://codereview.chromium.org/17333003/diff/15001/chrome/test/data/webui/ne... File chrome/test/data/webui/net_internals/log_view_painter.js (right): https://codereview.chromium.org/17333003/diff/15001/chrome/test/data/webui/ne... chrome/test/data/webui/net_internals/log_view_painter.js:1086: * Tests the custom formatting of net_errors across several different event Update this comment to reflect it is a quic error https://codereview.chromium.org/17333003/diff/15001/chrome/test/data/webui/ne... chrome/test/data/webui/net_internals/log_view_painter.js:1109: 'quic_rst_stream_error': 3, [optional] Note that in the future if you change the numeric values of error codes this test will break. If you want to make this resistant to error code changes, use symbolic names such as "QuicRstStreamError.BAD_APPLICATION_PAYLOAD" rather than "3". https://codereview.chromium.org/17333003/diff/15001/chrome/test/data/webui/ne... chrome/test/data/webui/net_internals/log_view_painter.js:1122: 'quic_error': 25 Same comment as above https://codereview.chromium.org/17333003/diff/15001/chrome/test/data/webui/ne... chrome/test/data/webui/net_internals/log_view_painter.js:1151: ' --> quic_rst_stream_error = 3 (' + Same comment as above https://codereview.chromium.org/17333003/diff/15001/chrome/test/data/webui/ne... chrome/test/data/webui/net_internals/log_view_painter.js:1157: ' --> quic_error = 25 (CONNECTION_TIMED_OUT)'; Same comment as above. https://codereview.chromium.org/17333003/diff/15001/net/quic/quic_error_list.h File net/quic/quic_error_list.h (right): https://codereview.chromium.org/17333003/diff/15001/net/quic/quic_error_list.... net/quic/quic_error_list.h:10: QUIC_ERROR(NO_ERROR) [optional] put this in the enum block too. That way can explicitly initialize to =0, and doesn't need to be part of log data https://codereview.chromium.org/17333003/diff/15001/net/quic/quic_rst_stream_... File net/quic/quic_rst_stream_error_list.h (right): https://codereview.chromium.org/17333003/diff/15001/net/quic/quic_rst_stream_... net/quic/quic_rst_stream_error_list.h:10: QUIC_RST_STREAM_ERROR(STREAM_NO_ERROR) [optional] put this in the enum block https://codereview.chromium.org/17333003/diff/15001/net/quic/quic_rst_stream_... net/quic/quic_rst_stream_error_list.h:25: QUIC_RST_STREAM_ERROR(STREAM_LAST_ERROR) put this in the enum block.
https://codereview.chromium.org/17333003/diff/15001/chrome/test/data/webui/ne... File chrome/test/data/webui/net_internals/log_view_painter.js (right): https://codereview.chromium.org/17333003/diff/15001/chrome/test/data/webui/ne... chrome/test/data/webui/net_internals/log_view_painter.js:1086: * Tests the custom formatting of net_errors across several different event On 2013/06/18 19:39:17, eroman wrote: > Update this comment to reflect it is a quic error Done. https://codereview.chromium.org/17333003/diff/15001/chrome/test/data/webui/ne... chrome/test/data/webui/net_internals/log_view_painter.js:1109: 'quic_rst_stream_error': 3, On 2013/06/18 19:39:17, eroman wrote: > [optional] Note that in the future if you change the numeric values of error > codes this test will break. > > If you want to make this resistant to error code changes, use symbolic names > such as "QuicRstStreamError.BAD_APPLICATION_PAYLOAD" rather than "3". Great idea. Done! https://codereview.chromium.org/17333003/diff/15001/chrome/test/data/webui/ne... chrome/test/data/webui/net_internals/log_view_painter.js:1122: 'quic_error': 25 On 2013/06/18 19:39:17, eroman wrote: > Same comment as above Done. https://codereview.chromium.org/17333003/diff/15001/chrome/test/data/webui/ne... chrome/test/data/webui/net_internals/log_view_painter.js:1134: 'quic_error': 25 And done here too. https://codereview.chromium.org/17333003/diff/15001/chrome/test/data/webui/ne... chrome/test/data/webui/net_internals/log_view_painter.js:1151: ' --> quic_rst_stream_error = 3 (' + On 2013/06/18 19:39:17, eroman wrote: > Same comment as above Oh, clever! That didn't occur to me even after the earlier comments. Thanks! Done. https://codereview.chromium.org/17333003/diff/15001/chrome/test/data/webui/ne... chrome/test/data/webui/net_internals/log_view_painter.js:1157: ' --> quic_error = 25 (CONNECTION_TIMED_OUT)'; On 2013/06/18 19:39:17, eroman wrote: > Same comment as above. Done. https://codereview.chromium.org/17333003/diff/15001/net/quic/quic_rst_stream_... File net/quic/quic_rst_stream_error_list.h (right): https://codereview.chromium.org/17333003/diff/15001/net/quic/quic_rst_stream_... net/quic/quic_rst_stream_error_list.h:10: QUIC_RST_STREAM_ERROR(STREAM_NO_ERROR) On 2013/06/18 19:39:17, eroman wrote: > [optional] put this in the enum block Hm. I could definitely go either way. I'm inclined to leave it in the list so that in net-internals we see quic_error = 0 (NO_ERROR) instead of quic_error = 0. But perhaps that's not actually important? Ok, done. (But I am curious what you think) https://codereview.chromium.org/17333003/diff/15001/net/quic/quic_rst_stream_... net/quic/quic_rst_stream_error_list.h:25: QUIC_RST_STREAM_ERROR(STREAM_LAST_ERROR) On 2013/06/18 19:39:17, eroman wrote: > put this in the enum block. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rch@chromium.org/17333003/21001
Failed to apply patch for net/quic/quic_connection_logger.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file net/quic/quic_connection_logger.cc
Hunk #1 succeeded at 114 with fuzz 2 (offset 1 line).
Hunk #2 FAILED at 122.
1 out of 2 hunks FAILED -- saving rejects to file
net/quic/quic_connection_logger.cc.rej
Patch: net/quic/quic_connection_logger.cc
Index: net/quic/quic_connection_logger.cc
diff --git a/net/quic/quic_connection_logger.cc
b/net/quic/quic_connection_logger.cc
index
5b335443a86c92642cee6508379c7a36464e98bd..9a0350921923ff74af51ad2bfb44b0804a8caf6d
100644
--- a/net/quic/quic_connection_logger.cc
+++ b/net/quic/quic_connection_logger.cc
@@ -113,7 +113,7 @@ Value* NetLogQuicRstStreamFrameCallback(const
QuicRstStreamFrame* frame,
NetLog::LogLevel /* log_level */) {
DictionaryValue* dict = new DictionaryValue();
dict->SetInteger("stream_id", frame->stream_id);
- dict->SetInteger("error_code", frame->error_code);
+ dict->SetInteger("quic_rst_stream_error", frame->error_code);
dict->SetString("details", frame->error_details);
return dict;
}
@@ -122,7 +122,7 @@ Value* NetLogQuicConnectionCloseFrameCallback(
const QuicConnectionCloseFrame* frame,
NetLog::LogLevel /* log_level */) {
DictionaryValue* dict = new DictionaryValue();
- dict->SetInteger("error_code", frame->error_code);
+ dict->SetInteger("quic_error", frame->error_code);
dict->SetString("details", frame->error_details);
return dict;
}
regarding NO_ERROR: I was assuming you would only output netlog events in the case where err != NO_ERROR (similar to what we do with net_error). If that is not the case, then I agree you should leave NO_ERROR in the list On Wed, Jun 19, 2013 at 9:59 AM, <rch@chromium.org> wrote: > > https://codereview.chromium.**org/17333003/diff/15001/** > chrome/test/data/webui/net_**internals/log_view_painter.js<https://codereview.chromium.org/17333003/diff/15001/chrome/test/data/webui/net_internals/log_view_painter.js> > File chrome/test/data/webui/net_**internals/log_view_painter.js (right): > > https://codereview.chromium.**org/17333003/diff/15001/** > chrome/test/data/webui/net_**internals/log_view_painter.js#**newcode1086<https://codereview.chromium.org/17333003/diff/15001/chrome/test/data/webui/net_internals/log_view_painter.js#newcode1086> > chrome/test/data/webui/net_**internals/log_view_painter.js:**1086: * Tests > the custom formatting of net_errors across several different event > On 2013/06/18 19:39:17, eroman wrote: > >> Update this comment to reflect it is a quic error >> > > Done. > > > https://codereview.chromium.**org/17333003/diff/15001/** > chrome/test/data/webui/net_**internals/log_view_painter.js#**newcode1109<https://codereview.chromium.org/17333003/diff/15001/chrome/test/data/webui/net_internals/log_view_painter.js#newcode1109> > chrome/test/data/webui/net_**internals/log_view_painter.js:**1109: > 'quic_rst_stream_error': 3, > On 2013/06/18 19:39:17, eroman wrote: > >> [optional] Note that in the future if you change the numeric values of >> > error > >> codes this test will break. >> > > If you want to make this resistant to error code changes, use symbolic >> > names > >> such as "QuicRstStreamError.BAD_**APPLICATION_PAYLOAD" rather than "3". >> > > Great idea. Done! > > > https://codereview.chromium.**org/17333003/diff/15001/** > chrome/test/data/webui/net_**internals/log_view_painter.js#**newcode1122<https://codereview.chromium.org/17333003/diff/15001/chrome/test/data/webui/net_internals/log_view_painter.js#newcode1122> > chrome/test/data/webui/net_**internals/log_view_painter.js:**1122: > 'quic_error': 25 > On 2013/06/18 19:39:17, eroman wrote: > >> Same comment as above >> > > Done. > > https://codereview.chromium.**org/17333003/diff/15001/** > chrome/test/data/webui/net_**internals/log_view_painter.js#**newcode1134<https://codereview.chromium.org/17333003/diff/15001/chrome/test/data/webui/net_internals/log_view_painter.js#newcode1134> > chrome/test/data/webui/net_**internals/log_view_painter.js:**1134: > 'quic_error': 25 > And done here too. > > > https://codereview.chromium.**org/17333003/diff/15001/** > chrome/test/data/webui/net_**internals/log_view_painter.js#**newcode1151<https://codereview.chromium.org/17333003/diff/15001/chrome/test/data/webui/net_internals/log_view_painter.js#newcode1151> > chrome/test/data/webui/net_**internals/log_view_painter.js:**1151: ' > --> quic_rst_stream_error = 3 (' + > On 2013/06/18 19:39:17, eroman wrote: > >> Same comment as above >> > > Oh, clever! That didn't occur to me even after the earlier comments. > Thanks! Done. > > > https://codereview.chromium.**org/17333003/diff/15001/** > chrome/test/data/webui/net_**internals/log_view_painter.js#**newcode1157<https://codereview.chromium.org/17333003/diff/15001/chrome/test/data/webui/net_internals/log_view_painter.js#newcode1157> > chrome/test/data/webui/net_**internals/log_view_painter.js:**1157: ' > --> quic_error = 25 (CONNECTION_TIMED_OUT)'; > On 2013/06/18 19:39:17, eroman wrote: > >> Same comment as above. >> > > Done. > > > https://codereview.chromium.**org/17333003/diff/15001/net/** > quic/quic_rst_stream_error_**list.h<https://codereview.chromium.org/17333003/diff/15001/net/quic/quic_rst_stream_error_list.h> > File net/quic/quic_rst_stream_**error_list.h (right): > > https://codereview.chromium.**org/17333003/diff/15001/net/** > quic/quic_rst_stream_error_**list.h#newcode10<https://codereview.chromium.org/17333003/diff/15001/net/quic/quic_rst_stream_error_list.h#newcode10> > net/quic/quic_rst_stream_**error_list.h:10: > QUIC_RST_STREAM_ERROR(STREAM_**NO_ERROR) > On 2013/06/18 19:39:17, eroman wrote: > >> [optional] put this in the enum block >> > > Hm. I could definitely go either way. I'm inclined to leave it in the > list so that in net-internals we see quic_error = 0 (NO_ERROR) instead > of quic_error = 0. But perhaps that's not actually important? Ok, > done. (But I am curious what you think) > > > https://codereview.chromium.**org/17333003/diff/15001/net/** > quic/quic_rst_stream_error_**list.h#newcode25<https://codereview.chromium.org/17333003/diff/15001/net/quic/quic_rst_stream_error_list.h#newcode25> > net/quic/quic_rst_stream_**error_list.h:25: > QUIC_RST_STREAM_ERROR(STREAM_**LAST_ERROR) > On 2013/06/18 19:39:17, eroman wrote: > >> put this in the enum block. >> > > Done. > > https://codereview.chromium.**org/17333003/<https://codereview.chromium.org/1... >
On 2013/06/19 17:28:44, eroman wrote: > regarding NO_ERROR: I was assuming you would only output netlog events in > the case where err != NO_ERROR (similar to what we do with net_error). > > If that is not the case, then I agree you should leave NO_ERROR in the list Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rch@chromium.org/17333003/34001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rch@chromium.org/17333003/48001
Sorry for I got bad news for ya. Compile failed with a clobber build on android_dbg. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rch@chromium.org/17333003/57001
Message was sent while issue was closed.
Change committed as 207773 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
