Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2192053002:
Metric & meta-metric for CECPQ1 handshake latency. (Closed)
DescriptionMetric & meta-metric for CECPQ1 handshake latency.
To fairly measure the latency impact of CECPQ1, we need to measure only
hosts that will negotiate it. Define a small group of Google hosts that
we believe ought to be negotiating CECPQ1, and a new histogram that
measures latency when connecting to these hosts.
In addition, add a meta-metric that (in the experiment group only)
allows us to verify that CECPQ1 is being negotiated in cases where we
expect it to be. This will serve as a check on the quality of the first
metric.
BUG=626363
Committed: https://crrev.com/e0d8c5878789398cd393b6184b54b6cbfcb5948b
Cr-Commit-Position: refs/heads/master@{#411518}
Patch Set 1 #
Total comments: 5
Patch Set 2 : review #
Total comments: 9
Patch Set 3 : review #Patch Set 4 : comment #Patch Set 5 : move comment #Patch Set 6 : rebase #Patch Set 7 : Fix an unreachable code error (Windows compiler) #
Messages
Total messages: 35 (18 generated)
Description was changed from ========== Metric & meta-metric for CECPQ1 handshake latency. To fairly measure the latency impact of CECPQ1, we need to measure only hosts that will negotiate it. Define a small group of Google hosts that we believe ought to be negotiating CECPQ1, and a new histogram that measures latency when connecting to these hosts. In addition, add a meta-metric that (in the experiment group only) allows us to verify that CECPQ1 is being negotiated in cases where we expect it to be. This will serve as a check on the quality of the first metric. BUG= ========== to ========== Metric & meta-metric for CECPQ1 handshake latency. To fairly measure the latency impact of CECPQ1, we need to measure only hosts that will negotiate it. Define a small group of Google hosts that we believe ought to be negotiating CECPQ1, and a new histogram that measures latency when connecting to these hosts. In addition, add a meta-metric that (in the experiment group only) allows us to verify that CECPQ1 is being negotiated in cases where we expect it to be. This will serve as a check on the quality of the first metric. BUG=626363 ==========
mab@google.com changed reviewers: + agl@chromium.org
agl@chromium.org changed reviewers: + davidben@chromium.org
I think this is good. See whether davidben can see a problem. https://codereview.chromium.org/2192053002/diff/1/net/socket/ssl_client_socke... File net/socket/ssl_client_socket_pool.cc (right): https://codereview.chromium.org/2192053002/diff/1/net/socket/ssl_client_socke... net/socket/ssl_client_socket_pool.cc:439: #if !defined(OS_NACL) (nit: I would have a blank line above this one.)
https://codereview.chromium.org/2192053002/diff/1/net/socket/ssl_client_socke... File net/socket/ssl_client_socket_pool.cc (right): https://codereview.chromium.org/2192053002/diff/1/net/socket/ssl_client_socke... net/socket/ssl_client_socket_pool.cc:35: #if !defined(OS_NACL) #include "build/build_config.h" (assuming the comment below doesn't just lose the OS_NACL bits) https://codereview.chromium.org/2192053002/diff/1/net/socket/ssl_client_socke... net/socket/ssl_client_socket_pool.cc:37: base::FEATURE_DISABLED_BY_DEFAULT}; Having two base::Features for the same thing seems a little wonky. Given it's const and we don't allow static initializers, it clearly works, but I'd feel a little better if we hid this behind, I dunno, a static SSLClientSocket::IsPostQuantumExperimentEnabled(). Also means we probably don't need the OS_NACL ifdef everywhere. (I don't believe NaCl can ever hit those hosts. Also it probably doesn't even send UMA to the same bucket, if at all. It's for Chromoting.) Tangentially, we should fix the part where OS_NACL doesn't initialize a base::FeatureList...
mab@google.com changed reviewers: + jwd@chromium.org
+jwd for histograms. https://codereview.chromium.org/2192053002/diff/1/net/socket/ssl_client_socke... File net/socket/ssl_client_socket_pool.cc (right): https://codereview.chromium.org/2192053002/diff/1/net/socket/ssl_client_socke... net/socket/ssl_client_socket_pool.cc:35: #if !defined(OS_NACL) done https://codereview.chromium.org/2192053002/diff/1/net/socket/ssl_client_socke... net/socket/ssl_client_socket_pool.cc:37: base::FEATURE_DISABLED_BY_DEFAULT}; You're right, and it's even expressly prohibited to have multiple instances of the same base::Feature. Done.
Did you forget to upload the new patch set?
I sure did, sorry.
lgtm https://codereview.chromium.org/2192053002/diff/20001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.cc (right): https://codereview.chromium.org/2192053002/diff/20001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:446: // should always be true if we get here. (You should probably expect a small fraction of false here due to antivirus and other SSL MITMs.) https://codereview.chromium.org/2192053002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2192053002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:31491: +<histogram name="Net.SSL_Connection_PostQuantum_Negotiated" enum="Boolean"> BooleanSupported? I think we usually like to pick one of the named booleans just so there's less confusion about true vs false.
mab@google.com changed required reviewers: + davidben@chromium.org, jwd@chromium.org
https://codereview.chromium.org/2192053002/diff/20001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.cc (right): https://codereview.chromium.org/2192053002/diff/20001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:446: // should always be true if we get here. Added comment. https://codereview.chromium.org/2192053002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2192053002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:31491: +<histogram name="Net.SSL_Connection_PostQuantum_Negotiated" enum="Boolean"> done
On 2016/07/30 03:12:48, mab wrote: > https://codereview.chromium.org/2192053002/diff/20001/net/socket/ssl_client_s... > File net/socket/ssl_client_socket_pool.cc (right): > > https://codereview.chromium.org/2192053002/diff/20001/net/socket/ssl_client_s... > net/socket/ssl_client_socket_pool.cc:446: // should always be true if we get > here. > Added comment. > > https://codereview.chromium.org/2192053002/diff/20001/tools/metrics/histogram... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/2192053002/diff/20001/tools/metrics/histogram... > tools/metrics/histograms/histograms.xml:31491: +<histogram > name="Net.SSL_Connection_PostQuantum_Negotiated" enum="Boolean"> > done Just so I understand your plan. Net.SSL_Connection_Latency_PostQuantumSupported_Full_Handshake will be collected for clients with and without the feature enabled, and you will compare this histogram for clients with the feature enabled to those without. Always assuming thot your is supported check is correct. And you will use the Net.SSL_Connection_PostQuantum_Negotiated to validate that the assumption is correct, right? I just want to make sure you're not hoping to correlate the two histogram values.
https://codereview.chromium.org/2192053002/diff/20001/net/socket/ssl_client_s... File net/socket/ssl_client_socket.cc (right): https://codereview.chromium.org/2192053002/diff/20001/net/socket/ssl_client_s... net/socket/ssl_client_socket.cc:22: const base::Feature kPostQuantumExperiment{"SSLPostQuantumExperiment", We tend not to like features including "experiment" in the name. Mostly an FYI, since you already have this deployed, and it would be a hassle to change. https://codereview.chromium.org/2192053002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2192053002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:31491: +<histogram name="Net.SSL_Connection_PostQuantum_Negotiated" enum="Boolean"> On 2016/07/30 03:12:48, mab wrote: > done Was there a patch set that wasn't uploaded?
Correct. The only purpose of Net.SSL_Connection_PostQuantum_Negotiated is to ensure that the servers we expect to offer post-quantum ciphersuites actually do so.
https://codereview.chromium.org/2192053002/diff/20001/net/socket/ssl_client_s... File net/socket/ssl_client_socket.cc (right): https://codereview.chromium.org/2192053002/diff/20001/net/socket/ssl_client_s... net/socket/ssl_client_socket.cc:22: const base::Feature kPostQuantumExperiment{"SSLPostQuantumExperiment", OK, leaving alone. Would you have called it "SSLPostQuantum"? "Experiment" aims to convey that this will never be enabled for all users, & that we plan to turn it off in a couple years regardless of outcome. https://codereview.chromium.org/2192053002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2192053002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:31491: +<histogram name="Net.SSL_Connection_PostQuantum_Negotiated" enum="Boolean"> Whoops. Done.
LGTM with request for added comment. https://codereview.chromium.org/2192053002/diff/20001/net/socket/ssl_client_s... File net/socket/ssl_client_socket.cc (right): https://codereview.chromium.org/2192053002/diff/20001/net/socket/ssl_client_s... net/socket/ssl_client_socket.cc:22: const base::Feature kPostQuantumExperiment{"SSLPostQuantumExperiment", On 2016/08/09 21:04:41, mab wrote: > OK, leaving alone. Would you have called it "SSLPostQuantum"? > > "Experiment" aims to convey that this will never be enabled for all users, & > that we plan to turn it off in a couple years regardless of outcome. Probably SSLPostQuantum, but I might have gone with SSLPostQuantumExperiment too, for the reason you describe. Though I think SSLPostQuantum, and a comment describing the temporaryness of it would be better. So actually, can you add a comment that this isn't a permanent thing, and should be removed in the future? ;)
done
The CQ bit was checked by mab@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by mab@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by mab@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mab@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from davidben@chromium.org, jwd@chromium.org Link to the patchset: https://codereview.chromium.org/2192053002/#ps120001 (title: "Fix an unreachable code error (Windows compiler)")
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 #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Metric & meta-metric for CECPQ1 handshake latency. To fairly measure the latency impact of CECPQ1, we need to measure only hosts that will negotiate it. Define a small group of Google hosts that we believe ought to be negotiating CECPQ1, and a new histogram that measures latency when connecting to these hosts. In addition, add a meta-metric that (in the experiment group only) allows us to verify that CECPQ1 is being negotiated in cases where we expect it to be. This will serve as a check on the quality of the first metric. BUG=626363 ========== to ========== Metric & meta-metric for CECPQ1 handshake latency. To fairly measure the latency impact of CECPQ1, we need to measure only hosts that will negotiate it. Define a small group of Google hosts that we believe ought to be negotiating CECPQ1, and a new histogram that measures latency when connecting to these hosts. In addition, add a meta-metric that (in the experiment group only) allows us to verify that CECPQ1 is being negotiated in cases where we expect it to be. This will serve as a check on the quality of the first metric. BUG=626363 Committed: https://crrev.com/e0d8c5878789398cd393b6184b54b6cbfcb5948b Cr-Commit-Position: refs/heads/master@{#411518} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/e0d8c5878789398cd393b6184b54b6cbfcb5948b Cr-Commit-Position: refs/heads/master@{#411518} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
