Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1775233002:
Add histogram to log whether a QuicGoAwayFrame is received for connection migration. (Closed)
|
Created:
4 years, 9 months ago by Zhongyi Shi Modified:
4 years, 9 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, sburnett, pavlos Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd histogram to log whether a QuicGoAwayFrame is received for connection migration with port change only.
BUG=593163
Committed: https://crrev.com/856aea72a1e0f5039193805d455078ba6e7cbfdb
Cr-Commit-Position: refs/heads/master@{#380579}
Patch Set 1 #
Total comments: 7
Patch Set 2 : work on after server sends out QUIC_ERROR_MIGRATING_PORT #Patch Set 3 : rebase #
Messages
Total messages: 29 (14 generated)
Description was changed from ========== Add histograms to log whether a QuicGoAwayFrame is received for connection migration. BUG=593163 ========== to ========== Add histogram to log whether a QuicGoAwayFrame is received for connection migration. BUG=593163 ==========
zhongyi@chromium.org changed reviewers: + rch@chromium.org
zhongyi@chromium.org changed reviewers: + asvitkine@chromium.org
https://codereview.chromium.org/1775233002/diff/1/net/quic/quic_connection_lo... File net/quic/quic_connection_logger.cc (right): https://codereview.chromium.org/1775233002/diff/1/net/quic/quic_connection_lo... net/quic/quic_connection_logger.cc:617: frame.reason_phrase == "peer connection migration"); Can "peer connection migration" be a constant defined somewhere and used from both the code that emits it and here? This way, if the string changes, this doesn't break. https://codereview.chromium.org/1775233002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1775233002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:26985: + to client's port change. Mention when it's logged. https://codereview.chromium.org/1775233002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:60210: +<enum name="BooleanGoAwayForConnectionMigrationReceived" type="int"> Nit: If the values are just the ones below, you can use BooleanReceived instead of defining the new enum.
lgtm, modulo asvitkine's suggestions https://codereview.chromium.org/1775233002/diff/1/net/quic/quic_connection_lo... File net/quic/quic_connection_logger.cc (right): https://codereview.chromium.org/1775233002/diff/1/net/quic/quic_connection_lo... net/quic/quic_connection_logger.cc:617: frame.reason_phrase == "peer connection migration"); On 2016/03/09 18:58:20, Alexei Svitkine (very slow) wrote: > Can "peer connection migration" be a constant defined somewhere and used from > both the code that emits it and here? This way, if the string changes, this > doesn't break. Good idea.
Thanks guys, I have just submitted a CL internally so that server will send a specific error code: QUIC_ERROR_MIGRATING_PORT when the go away is introduced by connection migration with port change only. That cl is now being merged into chromium. And we are using the error code to identify the case and log it to histogram. Thanks! https://codereview.chromium.org/1775233002/diff/1/net/quic/quic_connection_lo... File net/quic/quic_connection_logger.cc (right): https://codereview.chromium.org/1775233002/diff/1/net/quic/quic_connection_lo... net/quic/quic_connection_logger.cc:617: frame.reason_phrase == "peer connection migration"); On 2016/03/09 19:06:14, Ryan Hamilton wrote: > On 2016/03/09 18:58:20, Alexei Svitkine (very slow) wrote: > > Can "peer connection migration" be a constant defined somewhere and used from > > both the code that emits it and here? This way, if the string changes, this > > doesn't break. > > Good idea. Done. https://codereview.chromium.org/1775233002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1775233002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:26985: + to client's port change. On 2016/03/09 18:58:20, Alexei Svitkine (very slow) wrote: > Mention when it's logged. Done. https://codereview.chromium.org/1775233002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:60210: +<enum name="BooleanGoAwayForConnectionMigrationReceived" type="int"> On 2016/03/09 18:58:20, Alexei Svitkine (very slow) wrote: > Nit: If the values are just the ones below, you can use BooleanReceived instead > of defining the new enum. Done.
Description was changed from ========== Add histogram to log whether a QuicGoAwayFrame is received for connection migration. BUG=593163 ========== to ========== Add histogram to log whether a QuicGoAwayFrame is received for connection migration with port change only. BUG=593163 ==========
lgtm
The CQ bit was checked by zhongyi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rch@chromium.org Link to the patchset: https://codereview.chromium.org/1775233002/#ps40001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1775233002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1775233002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by zhongyi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1775233002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1775233002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by zhongyi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1775233002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1775233002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by zhongyi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1775233002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1775233002/40001
Message was sent while issue was closed.
Description was changed from ========== Add histogram to log whether a QuicGoAwayFrame is received for connection migration with port change only. BUG=593163 ========== to ========== Add histogram to log whether a QuicGoAwayFrame is received for connection migration with port change only. BUG=593163 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Add histogram to log whether a QuicGoAwayFrame is received for connection migration with port change only. BUG=593163 ========== to ========== Add histogram to log whether a QuicGoAwayFrame is received for connection migration with port change only. BUG=593163 Committed: https://crrev.com/856aea72a1e0f5039193805d455078ba6e7cbfdb Cr-Commit-Position: refs/heads/master@{#380579} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/856aea72a1e0f5039193805d455078ba6e7cbfdb Cr-Commit-Position: refs/heads/master@{#380579} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
