Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2732103003:
Adding new SSL Connection Error/Latency metrics (Closed)
DescriptionAdding new SSL Connection Error/Latency metrics
This adds TLS 1.3 specific metrics for SSL connection errors and
latency and a Google specific metric for SSL connection errors.
BUG=699226
Review-Url: https://codereview.chromium.org/2732103003
Cr-Commit-Position: refs/heads/master@{#455775}
Committed: https://chromium.googlesource.com/chromium/src/+/a52e21fed09cb51aabfa18b632f754b6c9c68a21
Patch Set 1 #
Total comments: 11
Patch Set 2 : Adding new SSL Connection Error/Latency metrics #
Messages
Total messages: 22 (12 generated)
The CQ bit was checked by svaldez@chromium.org to run a CQ dry run
svaldez@chromium.org changed reviewers: + davidben@chromium.org, rkaplow@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Add it to one of the TLS 1.3 bugs? lgtm https://codereview.chromium.org/2732103003/diff/1/net/socket/ssl_client_socke... File net/socket/ssl_client_socket_pool.cc (right): https://codereview.chromium.org/2732103003/diff/1/net/socket/ssl_client_socke... net/socket/ssl_client_socket_pool.cc:354: // These are hosts that we expect to always offer TLS 1.3 Connections to Period after TLS 1.3. https://codereview.chromium.org/2732103003/diff/1/net/socket/ssl_client_socke... net/socket/ssl_client_socket_pool.cc:356: // basis of our comparisons. Not sure if this comment is true yet. Perhaps: These are hosts that we intend to use in the initial TLS 1.3 deployment. TLS connections to them, whether or not this browser is in the experiment group, form the basis of our comparisons. https://codereview.chromium.org/2732103003/diff/1/net/socket/ssl_client_socke... net/socket/ssl_client_socket_pool.cc:434: UMA_HISTOGRAM_CUSTOM_TIMES("Net.SSL_Connection_Latency_TLS13", TLS13 is going to get confusing in the future since it reads like we're measuring all TLS 1.3 servers. TLS13Experiment? https://codereview.chromium.org/2732103003/diff/1/net/socket/ssl_client_socke... net/socket/ssl_client_socket_pool.cc:449: UMA_HISTOGRAM_SPARSE_SLOWLY("Net.SSL_Connection_Error_TLS13", Ditto. https://codereview.chromium.org/2732103003/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2732103003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:36829: + an endpoint we expect to always offer TLS 1.3. for an endpoint we are using in the initial TLS 1.3 deployment. https://codereview.chromium.org/2732103003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:36952: + that we expect to always offer TLS 1.3. Ditto.
Description was changed from ========== Adding new SSL Connection Error/Latency metrics This adds TLS 1.3 specific metrics for SSL connection errors and latency and a Google specific metric for SSL connection errors. BUG= ========== to ========== Adding new SSL Connection Error/Latency metrics This adds TLS 1.3 specific metrics for SSL connection errors and latency and a Google specific metric for SSL connection errors. BUG=699226 ==========
https://codereview.chromium.org/2732103003/diff/1/net/socket/ssl_client_socke... File net/socket/ssl_client_socket_pool.cc (right): https://codereview.chromium.org/2732103003/diff/1/net/socket/ssl_client_socke... net/socket/ssl_client_socket_pool.cc:354: // These are hosts that we expect to always offer TLS 1.3 Connections to On 2017/03/07 19:51:13, davidben wrote: > Period after TLS 1.3. Done. https://codereview.chromium.org/2732103003/diff/1/net/socket/ssl_client_socke... net/socket/ssl_client_socket_pool.cc:356: // basis of our comparisons. On 2017/03/07 19:51:13, davidben wrote: > Not sure if this comment is true yet. Perhaps: > > These are hosts that we intend to use in the initial TLS 1.3 deployment. TLS > connections to them, whether or not this browser is in the experiment group, > form the basis of our comparisons. Done. https://codereview.chromium.org/2732103003/diff/1/net/socket/ssl_client_socke... net/socket/ssl_client_socket_pool.cc:434: UMA_HISTOGRAM_CUSTOM_TIMES("Net.SSL_Connection_Latency_TLS13", On 2017/03/07 19:51:13, davidben wrote: > TLS13 is going to get confusing in the future since it reads like we're > measuring all TLS 1.3 servers. TLS13Experiment? Done. https://codereview.chromium.org/2732103003/diff/1/net/socket/ssl_client_socke... net/socket/ssl_client_socket_pool.cc:449: UMA_HISTOGRAM_SPARSE_SLOWLY("Net.SSL_Connection_Error_TLS13", On 2017/03/07 19:51:13, davidben wrote: > Ditto. Done. https://codereview.chromium.org/2732103003/diff/1/net/socket/ssl_client_socke... net/socket/ssl_client_socket_pool.cc:449: UMA_HISTOGRAM_SPARSE_SLOWLY("Net.SSL_Connection_Error_TLS13", On 2017/03/07 19:51:13, davidben wrote: > Ditto. Done.
lgtm
The CQ bit was checked by svaldez@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from davidben@chromium.org Link to the patchset: https://codereview.chromium.org/2732103003/#ps20001 (title: "Adding new SSL Connection Error/Latency metrics")
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
Try jobs failed on following builders: linux_chromium_chromeos_ozone_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 svaldez@chromium.org
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
Try jobs failed on following builders: linux_chromium_chromeos_ozone_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 svaldez@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1489074632446490,
"parent_rev": "4358c39b82d2be186367a3280df1f115db642139", "commit_rev":
"a52e21fed09cb51aabfa18b632f754b6c9c68a21"}
Message was sent while issue was closed.
Description was changed from ========== Adding new SSL Connection Error/Latency metrics This adds TLS 1.3 specific metrics for SSL connection errors and latency and a Google specific metric for SSL connection errors. BUG=699226 ========== to ========== Adding new SSL Connection Error/Latency metrics This adds TLS 1.3 specific metrics for SSL connection errors and latency and a Google specific metric for SSL connection errors. BUG=699226 Review-Url: https://codereview.chromium.org/2732103003 Cr-Commit-Position: refs/heads/master@{#455775} Committed: https://chromium.googlesource.com/chromium/src/+/a52e21fed09cb51aabfa18b632f7... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/a52e21fed09cb51aabfa18b632f7... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
