Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2721913002:
Add AuthPolicy histograms (Closed)
|
Created:
3 years, 9 months ago by ljusten (tachyonic) Modified:
3 years, 9 months ago Reviewers:
Mark P CC:
chromium-reviews, asvitkine+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd AuthPolicy histograms
Adds histograms used by the AuthPolicy daemon in Chrome OS, see
authpolicy_metrics.cc in https://chromium-review.googlesource.com/c/440084/.
BUG=chromium:676299
TEST=Ran pretty_print.py and validate_format.py
Review-Url: https://codereview.chromium.org/2721913002
Cr-Commit-Position: refs/heads/master@{#454551}
Committed: https://chromium.googlesource.com/chromium/src/+/4830d657333322f85bb11a5ff431aaab85d8cb3c
Patch Set 1 #
Total comments: 8
Patch Set 2 : Incorporated reviewer comments. #
Total comments: 4
Patch Set 3 : Rebase #Patch Set 4 : Record failed tries instead of tries. #Messages
Total messages: 12 (4 generated)
ljusten@chromium.org changed reviewers: + mpearson@chromium.org
Hey Mark, please take a look at our stats.
https://codereview.chromium.org/2721913002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2721913002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:3038: + Active Directory domain. Please state when this is recorded. https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo... https://codereview.chromium.org/2721913002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:3042: +<histogram name="AuthPolicy.TimeToAuthenticateUser"> units="ms" ditto on almost every histogram below https://codereview.chromium.org/2721913002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:3045: + Time in milliseconds to authenticate a user to an Active Directory domain. Recorded always or only on success? Please make the description clear. ditto on almost every histogram below. https://codereview.chromium.org/2721913002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:3123: + Number of times 'kinit' was tried until it succeeded or gave up. What's the give-up criteria? A fixed number of failures? A certain amount of time? etc. Depending on this, it might be really hard to interpret this histogram without breaking it up by successes versus failures. ditto below
Mark, PTAL. https://codereview.chromium.org/2721913002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2721913002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:3038: + Active Directory domain. On 2017/03/01 04:52:28, Mark P wrote: > Please state when this is recorded. > https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo... Done. https://codereview.chromium.org/2721913002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:3042: +<histogram name="AuthPolicy.TimeToAuthenticateUser"> On 2017/03/01 04:52:28, Mark P wrote: > units="ms" > ditto on almost every histogram below Done. https://codereview.chromium.org/2721913002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:3045: + Time in milliseconds to authenticate a user to an Active Directory domain. On 2017/03/01 04:52:28, Mark P wrote: > Recorded always or only on success? Please make the description clear. > > ditto on almost every histogram below. Done. https://codereview.chromium.org/2721913002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:3123: + Number of times 'kinit' was tried until it succeeded or gave up. On 2017/03/01 04:52:28, Mark P wrote: > What's the give-up criteria? A fixed number of failures? A certain amount of > time? etc. Depending on this, it might be really hard to interpret this > histogram without breaking it up by successes versus failures. > > ditto below Max number of tries. I have a question about this. Say the max number of tries is 10. It would be nice to distinguish 9 failures + 1 success from 10 failures, but both are technically 10 tries. I suggested to record 10 failures as 11 tries and catch 11 in the overflow bucket, so that it's very clear from the histograms that it actually gave up. However, my reviewers didn't like that since it's not 11 tries. How do others handle this? Maybe I should record failures instead of tries?
lgtm, but please come back to me if you decide to make the suggested extra histograms revisions. --mark https://codereview.chromium.org/2721913002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2721913002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3038: + Active Directory domain. This value is recorded while user or device policy nit: while -> when https://codereview.chromium.org/2721913002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3146: + a maximum number of tries was exceeded. 'smbclient' is run for Active On 2017/03/01 10:16:22, ljusten wrote: > On 2017/03/01 04:52:28, Mark P wrote: > > What's the give-up criteria? A fixed number of failures? A certain amount of > > time? etc. Depending on this, it might be really hard to interpret this > > histogram without breaking it up by successes versus failures. > > > > ditto below > > Max number of tries. I have a question about this. Say the max number of tries > is 10. It would be nice to distinguish 9 failures + 1 success from 10 failures, > but both are technically 10 tries. I suggested to record 10 failures as 11 tries > and catch 11 in the overflow bucket, so that it's very clear from the histograms > that it actually gave up. However, my reviewers didn't like that since it's not > 11 tries. Yeah, I don't like that either. > How do others handle this? Maybe I should record failures instead of tries? I'd suggest two histograms, something like AuthPolicy.SmbClient.Result, which is a boolean histogram recording whether the final result is a success or failure, and AuthPolicy.TriesOfSmbClient.Success, which is a histogram like this that records the number of tries but only emits on success. The total number of emits to the latter histogram should be the number of successes in the former. ditto for kinit tries histogram
PTAL. Recording the number of failed attempts. https://codereview.chromium.org/2721913002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2721913002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3146: + a maximum number of tries was exceeded. 'smbclient' is run for Active On 2017/03/01 20:02:02, Mark P wrote: > I'd suggest two histograms, something like AuthPolicy.SmbClient.Result, which is > a boolean histogram recording whether the final result is a success or failure, > and AuthPolicy.TriesOfSmbClient.Success, which is a histogram like this that > records the number of tries but only emits on success. > > The total number of emits to the latter histogram should be the number of > successes in the former. > > ditto for kinit tries histogram There's a complication that we only retry in case of certain errors, but not for all. Thus, you could fail even after 1 try and just recording failures would not show us how many attempts it took. We're primarily interested whether we're retrying often enough, so I believe the number of failed tries is the best stat to record.
histograms.xml still lgtm; see one comment below --mark https://codereview.chromium.org/2721913002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2721913002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3146: + a maximum number of tries was exceeded. 'smbclient' is run for Active On 2017/03/02 09:22:42, ljusten wrote: > On 2017/03/01 20:02:02, Mark P wrote: > > I'd suggest two histograms, something like AuthPolicy.SmbClient.Result, which > is > > a boolean histogram recording whether the final result is a success or > failure, > > and AuthPolicy.TriesOfSmbClient.Success, which is a histogram like this that > > records the number of tries but only emits on success. > > > > The total number of emits to the latter histogram should be the number of > > successes in the former. > > > > ditto for kinit tries histogram > > There's a complication that we only retry in case of certain errors, but not for > all. Thus, you could fail even after 1 try and just recording failures would not > show us how many attempts it took. We're primarily interested whether we're > retrying often enough, so I believe the number of failed tries is the best stat > to record. Your new solution seems reasonable to me. Make sure your logging code has the new correct logic and new histogram name!
The CQ bit was checked by ljusten@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": 60001, "attempt_start_ts": 1488529229901390,
"parent_rev": "856526d83e9120f70f27823f6399f596379a2600", "commit_rev":
"4830d657333322f85bb11a5ff431aaab85d8cb3c"}
Message was sent while issue was closed.
Description was changed from ========== Add AuthPolicy histograms Adds histograms used by the AuthPolicy daemon in Chrome OS, see authpolicy_metrics.cc in https://chromium-review.googlesource.com/c/440084/. BUG=chromium:676299 TEST=Ran pretty_print.py and validate_format.py ========== to ========== Add AuthPolicy histograms Adds histograms used by the AuthPolicy daemon in Chrome OS, see authpolicy_metrics.cc in https://chromium-review.googlesource.com/c/440084/. BUG=chromium:676299 TEST=Ran pretty_print.py and validate_format.py Review-Url: https://codereview.chromium.org/2721913002 Cr-Commit-Position: refs/heads/master@{#454551} Committed: https://chromium.googlesource.com/chromium/src/+/4830d657333322f85bb11a5ff431... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/4830d657333322f85bb11a5ff431... |
