Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2491363002:
UMA metrics for LogDnsClient (Closed)
|
Created:
4 years, 1 month ago by Rob Percival Modified:
3 years, 9 months ago CC:
chromium-reviews, rsleevi+watch_chromium.org, certificate-transparency-chrome_googlegroups.com, martijn+crwatch_martijnc.be, Eran Messeri, asvitkine+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUMA metrics for LogDnsClient
Records the time spent and status of retrieving an inclusion proof for a signed certificate timestamp (SCT) over DNS. This will show the effectiveness of retrieving proofs using Chromium's built-in DNS client.
BUG=624894
Review-Url: https://codereview.chromium.org/2491363002
Cr-Commit-Position: refs/heads/master@{#453898}
Committed: https://chromium.googlesource.com/chromium/src/+/70544e886eb9d0751fc233b54c724d792bf53da1
Patch Set 1 #
Total comments: 4
Patch Set 2 : Addresses Eran's comments #Patch Set 3 : Rebase #
Messages
Total messages: 24 (13 generated)
The CQ bit was checked by robpercival@chromium.org 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: This issue passed the CQ dry run.
robpercival@chromium.org changed reviewers: + eranm@chromium.org
Can you suggest another other metrics and/or QueryStatus enum values that might be helpful?
overall lgtm, left a few minor comments. I'm wondering if we shouldn't have different histograms for measuring time it took to get the leaf index and time it took to get the inclusion proof, since one carries more data than the other. https://codereview.chromium.org/2491363002/diff/1/components/certificate_tran... File components/certificate_transparency/log_dns_client.cc (right): https://codereview.chromium.org/2491363002/diff/1/components/certificate_tran... components/certificate_transparency/log_dns_client.cc:39: QUERY_STATUS_FAILED_MISC = 1, Nit: Rename FAILED_MISC to FAILED_UNKNOWN / FAILED_OTHER_DNS_ERROR ? MISC is a bit too general a name, but I don't have a better suggestion. https://codereview.chromium.org/2491363002/diff/1/components/certificate_tran... components/certificate_transparency/log_dns_client.cc:303: LogQueryStatus(QUERY_STATUS_FAILED_MISC); Nit: Don't log here, since it's unreachable code (and may be conflated with the other case FAILED_MISC is logged anyway).
rsleevi@chromium.org changed reviewers: + rsleevi@chromium.org
Please update the description before landing. (Note: In general, comments about what the CL does and why should be in the description, and comments about nuance/constraint/dependencies - for your reviewers - should be in the message when you publish for comments/review)
Description was changed from ========== UMA metrics for LogDnsClient WIP: Still thinking about exactly what metrics would be useful. Awaiting Eran Messeri's code that actually uses a LogDnsClient, so that manual testing of the histograms can be performed. Alternatively, or additionally, the unit tests could be updated to use HistogramTester. BUG=624894 ========== to ========== UMA metrics for LogDnsClient Records the time spent and status of retrieving an inclusion proof for a signed certificate timestamp (SCT) over DNS. This will show the effectiveness of retrieving proofs using Chromium's built-in DNS client. BUG=624894 ==========
I could certainly add a couple of extra histograms to measure the time spent getting the leaf index and proof individually. We'd still need the histogram for overall time spent though (see https://chromium.googlesource.com/chromium/src/+/master/tools/metrics/histogr...). https://codereview.chromium.org/2491363002/diff/1/components/certificate_tran... File components/certificate_transparency/log_dns_client.cc (right): https://codereview.chromium.org/2491363002/diff/1/components/certificate_tran... components/certificate_transparency/log_dns_client.cc:39: QUERY_STATUS_FAILED_MISC = 1, On 2016/12/20 11:41:54, Eran Messeri wrote: > Nit: Rename FAILED_MISC to FAILED_UNKNOWN / FAILED_OTHER_DNS_ERROR ? > MISC is a bit too general a name, but I don't have a better suggestion. Done. https://codereview.chromium.org/2491363002/diff/1/components/certificate_tran... components/certificate_transparency/log_dns_client.cc:303: LogQueryStatus(QUERY_STATUS_FAILED_MISC); On 2016/12/20 11:41:54, Eran Messeri wrote: > Nit: Don't log here, since it's unreachable code (and may be conflated with the > other case FAILED_MISC is logged anyway). Done.
Just trying to clear out my backlog - was Eran's LG sufficient for this to land? My understanding is he's an OWNER and that should be sufficient, mod getting feedback from metrics folks.
robpercival@chromium.org changed reviewers: + ericwilligers@chromium.org
ericwilligers@chromium.org: Please review changes in histograms.xml
ericwilligers@chromium.org changed reviewers: + mpearson@chromium.org
+mpearson for tools/metrics OWNERS (I only review UseCounter changes.)
histograms.xml lgtm
The CQ bit was checked by robpercival@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eranm@chromium.org Link to the patchset: https://codereview.chromium.org/2491363002/#ps40001 (title: "Rebase")
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": 40001, "attempt_start_ts": 1488360997675020,
"parent_rev": "8f4fd7677a68e465ba2db9c6c1eb969d0d9c8c73", "commit_rev":
"70544e886eb9d0751fc233b54c724d792bf53da1"}
Message was sent while issue was closed.
Description was changed from ========== UMA metrics for LogDnsClient Records the time spent and status of retrieving an inclusion proof for a signed certificate timestamp (SCT) over DNS. This will show the effectiveness of retrieving proofs using Chromium's built-in DNS client. BUG=624894 ========== to ========== UMA metrics for LogDnsClient Records the time spent and status of retrieving an inclusion proof for a signed certificate timestamp (SCT) over DNS. This will show the effectiveness of retrieving proofs using Chromium's built-in DNS client. BUG=624894 Review-Url: https://codereview.chromium.org/2491363002 Cr-Commit-Position: refs/heads/master@{#453898} Committed: https://chromium.googlesource.com/chromium/src/+/70544e886eb9d0751fc233b54c72... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/70544e886eb9d0751fc233b54c72... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
