Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14060013:
Add UMA stats to track whitelist download latency. (Closed)
|
Created:
7 years, 8 months ago by benquan Modified:
7 years, 8 months ago CC:
chromium-reviews, Raman Kakilate, jar (doing other things), benquan, dhollowa+watch_chromium.org, ahutter, browser-components-watch_chromium.org, dbeam+watch-autofill_chromium.org, MAD, Dane Wallinga, dyu1, estade+watch_chromium.org, Albert Bodenhamer, Ilya Sherman Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd UMA stats to track whitelist download latency.
BUG=232274
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=194777
Patch Set 1 #
Total comments: 43
Patch Set 2 : rename variables and update comments. #
Total comments: 2
Patch Set 3 : move UMA tracking code out of the if/else block. #
Messages
Total messages: 11 (0 generated)
https://codereview.chromium.org/14060013/diff/1/components/autofill/browser/a... File components/autofill/browser/autocheckout/whitelist_manager.h (right): https://codereview.chromium.org/14060013/diff/1/components/autofill/browser/a... components/autofill/browser/autocheckout/whitelist_manager.h:53: // Returns the |AutofillMetrics| instance that should be used for logging Take a look at autocheckout_manager.h. Might be good to be consistent with how that deals with the metrics logger. https://codereview.chromium.org/14060013/diff/1/components/autofill/browser/a... components/autofill/browser/autocheckout/whitelist_manager.h:93: // Metrics Logging stuff. "Logger for UMA metrics" or something like that. https://codereview.chromium.org/14060013/diff/1/components/autofill/browser/a... components/autofill/browser/autocheckout/whitelist_manager.h:94: AutofillMetrics metrics_logger_; new line after this. https://codereview.chromium.org/14060013/diff/1/components/autofill/browser/a... File components/autofill/browser/autofill_metrics.cc (right): https://codereview.chromium.org/14060013/diff/1/components/autofill/browser/a... components/autofill/browser/autofill_metrics.cc:464: LogUMAHistogramLongTimes("Autocheckout.WhitelistDownloadDuration", duration); I think you want to use LogUMAHistogramTimes since this isn't going to be a long running call. https://codereview.chromium.org/14060013/diff/1/components/autofill/browser/a... File components/autofill/browser/autofill_metrics.h (right): https://codereview.chromium.org/14060013/diff/1/components/autofill/browser/a... components/autofill/browser/autofill_metrics.h:354: enum WhitelistDownloadStatus { Probably want to throw Autocheckout on the front of this. https://codereview.chromium.org/14060013/diff/1/components/autofill/browser/a... components/autofill/browser/autofill_metrics.h:355: // The download was failed. The download failed. https://codereview.chromium.org/14060013/diff/1/components/autofill/browser/a... components/autofill/browser/autofill_metrics.h:356: WHITELIST_DOWNLOAD_FAILED, Ditto about AUTOCHECKOUT https://codereview.chromium.org/14060013/diff/1/components/autofill/browser/a... components/autofill/browser/autofill_metrics.h:357: // The download was succeeded. The download was successful. https://codereview.chromium.org/14060013/diff/1/components/autofill/browser/a... components/autofill/browser/autofill_metrics.h:358: WHITELIST_DOWNLOAD_SUCCEEDED, ditto about AUTOCHECKOUT. https://codereview.chromium.org/14060013/diff/1/components/autofill/browser/a... components/autofill/browser/autofill_metrics.h:446: virtual void LogAutocheckoutWhitelistDownloadDuration( Docs. https://codereview.chromium.org/14060013/diff/1/tools/metrics/histograms/hist... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/14060013/diff/1/tools/metrics/histograms/hist... tools/metrics/histograms/histograms.xml:162: <histogram name="Autocheckout.WhitelistDownloadDuration" units="ms"> +ramankk, does this look right? https://codereview.chromium.org/14060013/diff/1/tools/metrics/histograms/hist... tools/metrics/histograms/histograms.xml:164: Measures the duration for download Autocheckout whitelist file. Measures time taken to download the... To be consistent with Raman's summaries. https://codereview.chromium.org/14060013/diff/1/tools/metrics/histograms/hist... tools/metrics/histograms/histograms.xml:170: Measures the duration for download Autocheckout whitelist file in case the Ditto. https://codereview.chromium.org/14060013/diff/1/tools/metrics/histograms/hist... tools/metrics/histograms/histograms.xml:177: Measures the duration for download Autocheckout whitelist file in case the Ditto.
https://codereview.chromium.org/14060013/diff/1/tools/metrics/histograms/hist... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/14060013/diff/1/tools/metrics/histograms/hist... tools/metrics/histograms/histograms.xml:168: <histogram name="Autocheckout.WhitelistDownloadDuration.Failed" units="ms"> nit: better would be to define a <field_trial...> below with the two possible groups "Failed" and "Succeeded" as well as a separator="". This way you don't need to repeat the bulk of the prose 3 times.
FWIW, crbug.com seems to be back up right now https://codereview.chromium.org/14060013/diff/1/components/autofill/browser/a... File components/autofill/browser/autocheckout/whitelist_manager.cc (right): https://codereview.chromium.org/14060013/diff/1/components/autofill/browser/a... components/autofill/browser/autocheckout/whitelist_manager.cc:113: AutofillMetrics::WHITELIST_DOWNLOAD_FAILED); nit: Would be nice to move most of this code out of the if/else, and just have a variable that is set to either AutofillMetrics::WHITELIST_DOWNLOAD_SUCCEEDED or AutofillMetrics::WHITELIST_DOWNLOAD_FAILED. https://codereview.chromium.org/14060013/diff/1/components/autofill/browser/a... File components/autofill/browser/autocheckout/whitelist_manager.h (right): https://codereview.chromium.org/14060013/diff/1/components/autofill/browser/a... components/autofill/browser/autocheckout/whitelist_manager.h:56: return metrics_logger_; nit: virtual method implementations belong in the .cc file https://codereview.chromium.org/14060013/diff/1/components/autofill/browser/a... File components/autofill/browser/autocheckout/whitelist_manager_unittest.cc (right): https://codereview.chromium.org/14060013/diff/1/components/autofill/browser/a... components/autofill/browser/autocheckout/whitelist_manager_unittest.cc:126: .Times(1); nit: This is pretty hard to read. Could you break this into multiple statements by introducing some intermediate variables? https://codereview.chromium.org/14060013/diff/1/components/autofill/browser/a... File components/autofill/browser/autofill_metrics.h (right): https://codereview.chromium.org/14060013/diff/1/components/autofill/browser/a... components/autofill/browser/autofill_metrics.h:355: // The download was failed. nit: This comment is almost verbatim identical to the enum constant name, so I think you can just drop it. Ditto for the comment below. https://codereview.chromium.org/14060013/diff/1/components/autofill/browser/a... components/autofill/browser/autofill_metrics.h:447: const base::TimeDelta& duration, WhitelistDownloadStatus status) const; nit: One parameter per line, please, if they have to wrap.
https://codereview.chromium.org/14060013/diff/1/components/autofill/browser/a... File components/autofill/browser/autocheckout/whitelist_manager.cc (right): https://codereview.chromium.org/14060013/diff/1/components/autofill/browser/a... components/autofill/browser/autocheckout/whitelist_manager.cc:113: AutofillMetrics::WHITELIST_DOWNLOAD_FAILED); On 2013/04/17 00:18:01, Ilya Sherman wrote: > nit: Would be nice to move most of this code out of the if/else, and just have a > variable that is set to either AutofillMetrics::WHITELIST_DOWNLOAD_SUCCEEDED or > AutofillMetrics::WHITELIST_DOWNLOAD_FAILED. In the If block, I want to track UMA before calling BuildWhitelist(data), so that we do not include parsing time. If we want to move the code out of if/else, it should be before the "if" statement, it will require performing another (source->GetResponseCode() == net::HTTP_OK) test, it won't be much nicer. I will leave it as is for now, please let me know if think otherwise. https://codereview.chromium.org/14060013/diff/1/components/autofill/browser/a... File components/autofill/browser/autocheckout/whitelist_manager.h (right): https://codereview.chromium.org/14060013/diff/1/components/autofill/browser/a... components/autofill/browser/autocheckout/whitelist_manager.h:53: // Returns the |AutofillMetrics| instance that should be used for logging On 2013/04/16 17:56:54, ahutter wrote: > Take a look at autocheckout_manager.h. Might be good to be consistent with how > that deals with the metrics logger. I was following autocheckout_request_manager.h and autofill_dialog_controller_impl.h. I like this better because it does not require me add setter which is only for tests, and I don't need to worry about NULL pointers and object ownership etc. https://codereview.chromium.org/14060013/diff/1/components/autofill/browser/a... components/autofill/browser/autocheckout/whitelist_manager.h:56: return metrics_logger_; On 2013/04/17 00:18:01, Ilya Sherman wrote: > nit: virtual method implementations belong in the .cc file Done. https://codereview.chromium.org/14060013/diff/1/components/autofill/browser/a... components/autofill/browser/autocheckout/whitelist_manager.h:93: // Metrics Logging stuff. On 2013/04/16 17:56:54, ahutter wrote: > "Logger for UMA metrics" or something like that. Done. https://codereview.chromium.org/14060013/diff/1/components/autofill/browser/a... components/autofill/browser/autocheckout/whitelist_manager.h:94: AutofillMetrics metrics_logger_; On 2013/04/16 17:56:54, ahutter wrote: > new line after this. Done. https://codereview.chromium.org/14060013/diff/1/components/autofill/browser/a... File components/autofill/browser/autocheckout/whitelist_manager_unittest.cc (right): https://codereview.chromium.org/14060013/diff/1/components/autofill/browser/a... components/autofill/browser/autocheckout/whitelist_manager_unittest.cc:126: .Times(1); On 2013/04/17 00:18:01, Ilya Sherman wrote: > nit: This is pretty hard to read. Could you break this into multiple statements > by introducing some intermediate variables? Done. https://codereview.chromium.org/14060013/diff/1/components/autofill/browser/a... File components/autofill/browser/autofill_metrics.cc (right): https://codereview.chromium.org/14060013/diff/1/components/autofill/browser/a... components/autofill/browser/autofill_metrics.cc:464: LogUMAHistogramLongTimes("Autocheckout.WhitelistDownloadDuration", duration); On 2013/04/16 17:56:54, ahutter wrote: > I think you want to use LogUMAHistogramTimes since this isn't going to be a long > running call. Done. https://codereview.chromium.org/14060013/diff/1/components/autofill/browser/a... File components/autofill/browser/autofill_metrics.h (right): https://codereview.chromium.org/14060013/diff/1/components/autofill/browser/a... components/autofill/browser/autofill_metrics.h:354: enum WhitelistDownloadStatus { On 2013/04/16 17:56:54, ahutter wrote: > Probably want to throw Autocheckout on the front of this. Done. https://codereview.chromium.org/14060013/diff/1/components/autofill/browser/a... components/autofill/browser/autofill_metrics.h:355: // The download was failed. On 2013/04/17 00:18:01, Ilya Sherman wrote: > nit: This comment is almost verbatim identical to the enum constant name, so I > think you can just drop it. Ditto for the comment below. Done. https://codereview.chromium.org/14060013/diff/1/components/autofill/browser/a... components/autofill/browser/autofill_metrics.h:356: WHITELIST_DOWNLOAD_FAILED, On 2013/04/16 17:56:54, ahutter wrote: > Ditto about AUTOCHECKOUT Done. https://codereview.chromium.org/14060013/diff/1/components/autofill/browser/a... components/autofill/browser/autofill_metrics.h:357: // The download was succeeded. On 2013/04/16 17:56:54, ahutter wrote: > The download was successful. Done. https://codereview.chromium.org/14060013/diff/1/components/autofill/browser/a... components/autofill/browser/autofill_metrics.h:358: WHITELIST_DOWNLOAD_SUCCEEDED, On 2013/04/16 17:56:54, ahutter wrote: > ditto about AUTOCHECKOUT. Done. https://codereview.chromium.org/14060013/diff/1/components/autofill/browser/a... components/autofill/browser/autofill_metrics.h:446: virtual void LogAutocheckoutWhitelistDownloadDuration( On 2013/04/16 17:56:54, ahutter wrote: > Docs. Done. https://codereview.chromium.org/14060013/diff/1/components/autofill/browser/a... components/autofill/browser/autofill_metrics.h:447: const base::TimeDelta& duration, WhitelistDownloadStatus status) const; On 2013/04/17 00:18:01, Ilya Sherman wrote: > nit: One parameter per line, please, if they have to wrap. Done. https://codereview.chromium.org/14060013/diff/1/tools/metrics/histograms/hist... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/14060013/diff/1/tools/metrics/histograms/hist... tools/metrics/histograms/histograms.xml:164: Measures the duration for download Autocheckout whitelist file. On 2013/04/16 17:56:54, ahutter wrote: > Measures time taken to download the... > > To be consistent with Raman's summaries. Done. https://codereview.chromium.org/14060013/diff/1/tools/metrics/histograms/hist... tools/metrics/histograms/histograms.xml:168: <histogram name="Autocheckout.WhitelistDownloadDuration.Failed" units="ms"> On 2013/04/16 18:53:14, jar wrote: > nit: better would be to define a <field_trial...> below with the two possible > groups "Failed" and "Succeeded" as well as a separator="". > > This way you don't need to repeat the bulk of the prose 3 times. Can you please point me to a doc talks about how to use <field_trial...> and how this xml block should look like? base/metrics/field_trial.h only tells me how to modify the code, but not the xml file. And it seems like it is not trivial to change the code, I'm not sure if it worth to trade the code change with a few lines of xml, unless there are other benefits of using <field_trial>. BTW, there are bunch of other status in this xml file similar to this, should we convert them all together to make them consistent? https://codereview.chromium.org/14060013/diff/1/tools/metrics/histograms/hist... tools/metrics/histograms/histograms.xml:170: Measures the duration for download Autocheckout whitelist file in case the On 2013/04/16 17:56:54, ahutter wrote: > Ditto. Done. https://codereview.chromium.org/14060013/diff/1/tools/metrics/histograms/hist... tools/metrics/histograms/histograms.xml:177: Measures the duration for download Autocheckout whitelist file in case the On 2013/04/16 17:56:54, ahutter wrote: > Ditto. Done.
https://codereview.chromium.org/14060013/diff/1/components/autofill/browser/a... File components/autofill/browser/autocheckout/whitelist_manager.cc (right): https://codereview.chromium.org/14060013/diff/1/components/autofill/browser/a... components/autofill/browser/autocheckout/whitelist_manager.cc:113: AutofillMetrics::WHITELIST_DOWNLOAD_FAILED); On 2013/04/17 01:51:30, benquan wrote: > On 2013/04/17 00:18:01, Ilya Sherman wrote: > > nit: Would be nice to move most of this code out of the if/else, and just have > a > > variable that is set to either AutofillMetrics::WHITELIST_DOWNLOAD_SUCCEEDED > or > > AutofillMetrics::WHITELIST_DOWNLOAD_FAILED. > > In the If block, I want to track UMA before calling BuildWhitelist(data), so > that we do not include parsing time. If we want to move the code out of if/else, > it should be before the "if" statement, it will require performing another > (source->GetResponseCode() == net::HTTP_OK) test, it won't be much nicer. I will > leave it as is for now, please let me know if think otherwise. You can compute the duration outside of the if/else as well. IMO that's still cleaner, but it's not critical to change this. https://codereview.chromium.org/14060013/diff/1/tools/metrics/histograms/hist... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/14060013/diff/1/tools/metrics/histograms/hist... tools/metrics/histograms/histograms.xml:168: <histogram name="Autocheckout.WhitelistDownloadDuration.Failed" units="ms"> On 2013/04/17 01:51:30, benquan wrote: > On 2013/04/16 18:53:14, jar wrote: > > nit: better would be to define a <field_trial...> below with the two possible > > groups "Failed" and "Succeeded" as well as a separator="". > > > > This way you don't need to repeat the bulk of the prose 3 times. > > Can you please point me to a doc talks about how to use <field_trial...> and how > this xml block should look like? base/metrics/field_trial.h only tells me how to > modify the code, but not the xml file. And it seems like it is not trivial to > change the code, I'm not sure if it worth to trade the code change with a few > lines of xml, unless there are other benefits of using <field_trial>. > > BTW, there are bunch of other status in this xml file similar to this, should we > convert them all together to make them consistent? Take a look at the top of this file, namely lines 26 through 49: Jim's suggestion does not require any changes to code outside of histograms.xml (and the <fieldtrial> element is rather misleadingly named at this point). That said, for such short histogram descriptions, IMO it's nicer to just do what you've done and write all of the histogram names out explicitly, because it's easier to see what histograms, exactly, are being logged that way. Since Jim and I disagree here, I'd recommend just going with whatever seems cleaner to you, or whatever seems more consistent with the surrounding "code". https://codereview.chromium.org/14060013/diff/12001/components/autofill/brows... File components/autofill/browser/autocheckout/whitelist_manager.cc (right): https://codereview.chromium.org/14060013/diff/12001/components/autofill/brows... components/autofill/browser/autocheckout/whitelist_manager.cc:60: } nit: Please order this so that its sorted the same in the header and in the implementation file.
Oh, and LGTM with the nits addressed.
https://codereview.chromium.org/14060013/diff/1/components/autofill/browser/a... File components/autofill/browser/autocheckout/whitelist_manager.cc (right): https://codereview.chromium.org/14060013/diff/1/components/autofill/browser/a... components/autofill/browser/autocheckout/whitelist_manager.cc:113: AutofillMetrics::WHITELIST_DOWNLOAD_FAILED); On 2013/04/17 05:01:48, Ilya Sherman wrote: > On 2013/04/17 01:51:30, benquan wrote: > > On 2013/04/17 00:18:01, Ilya Sherman wrote: > > > nit: Would be nice to move most of this code out of the if/else, and just > have > > a > > > variable that is set to either AutofillMetrics::WHITELIST_DOWNLOAD_SUCCEEDED > > or > > > AutofillMetrics::WHITELIST_DOWNLOAD_FAILED. > > > > In the If block, I want to track UMA before calling BuildWhitelist(data), so > > that we do not include parsing time. If we want to move the code out of > if/else, > > it should be before the "if" statement, it will require performing another > > (source->GetResponseCode() == net::HTTP_OK) test, it won't be much nicer. I > will > > leave it as is for now, please let me know if think otherwise. > > You can compute the duration outside of the if/else as well. IMO that's still > cleaner, but it's not critical to change this. Done. https://codereview.chromium.org/14060013/diff/1/components/autofill/browser/a... File components/autofill/browser/autofill_metrics.h (right): https://codereview.chromium.org/14060013/diff/1/components/autofill/browser/a... components/autofill/browser/autofill_metrics.h:355: // The download was failed. On 2013/04/16 17:56:54, ahutter wrote: > The download failed. Done. https://codereview.chromium.org/14060013/diff/1/tools/metrics/histograms/hist... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/14060013/diff/1/tools/metrics/histograms/hist... tools/metrics/histograms/histograms.xml:168: <histogram name="Autocheckout.WhitelistDownloadDuration.Failed" units="ms"> On 2013/04/17 05:01:48, Ilya Sherman wrote: > On 2013/04/17 01:51:30, benquan wrote: > > On 2013/04/16 18:53:14, jar wrote: > > > nit: better would be to define a <field_trial...> below with the two > possible > > > groups "Failed" and "Succeeded" as well as a separator="". > > > > > > This way you don't need to repeat the bulk of the prose 3 times. > > > > Can you please point me to a doc talks about how to use <field_trial...> and > how > > this xml block should look like? base/metrics/field_trial.h only tells me how > to > > modify the code, but not the xml file. And it seems like it is not trivial to > > change the code, I'm not sure if it worth to trade the code change with a few > > lines of xml, unless there are other benefits of using <field_trial>. > > > > BTW, there are bunch of other status in this xml file similar to this, should > we > > convert them all together to make them consistent? > > Take a look at the top of this file, namely lines 26 through 49: Jim's > suggestion does not require any changes to code outside of histograms.xml (and > the <fieldtrial> element is rather misleadingly named at this point). That > said, for such short histogram descriptions, IMO it's nicer to just do what > you've done and write all of the histogram names out explicitly, because it's > easier to see what histograms, exactly, are being logged that way. Since Jim > and I disagree here, I'd recommend just going with whatever seems cleaner to > you, or whatever seems more consistent with the surrounding "code". I will go with "consistency", Jim, please let me know if you strongly disagree. https://codereview.chromium.org/14060013/diff/12001/components/autofill/brows... File components/autofill/browser/autocheckout/whitelist_manager.cc (right): https://codereview.chromium.org/14060013/diff/12001/components/autofill/brows... components/autofill/browser/autocheckout/whitelist_manager.cc:60: } On 2013/04/17 05:01:48, Ilya Sherman wrote: > nit: Please order this so that its sorted the same in the header and in the > implementation file. Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benquan@chromium.org/14060013/15002
Message was sent while issue was closed.
Change committed as 194777 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
