Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2808063002:
Add DownloadStatus metric to FaviconHandler (Closed)
DescriptionAdd DownloadStatus metric to FaviconHandler
This metric records how often single icon downloads fail with 404 and
how often 404'ing icons can be skipped because they were already known.
BUG=694956
Review-Url: https://codereview.chromium.org/2808063002
Cr-Commit-Position: refs/heads/master@{#465545}
Committed: https://chromium.googlesource.com/chromium/src/+/be0a6071e24bc1e0f432533042d3d813bbe6ed3f
Patch Set 1 #
Total comments: 4
Patch Set 2 : Rename histogram enum descriptions #
Total comments: 8
Patch Set 3 : Simplify tests and adress comments #
Total comments: 8
Patch Set 4 : Optimize UMA macro usage #
Total comments: 2
Patch Set 5 : Remove unnecessary cast #Patch Set 6 : Rebase #
Messages
Total messages: 30 (16 generated)
fhorschig@chromium.org changed reviewers: + jkrcal@chromium.org
Hi Jan, would you please have a look at the third and last metric for the FaviconHandler?
lgtm % nits https://codereview.chromium.org/2808063002/diff/1/components/favicon/core/fav... File components/favicon/core/favicon_handler.h (right): https://codereview.chromium.org/2808063002/diff/1/components/favicon/core/fav... components/favicon/core/favicon_handler.h:81: // These values must stay in sync with the FaviconCacheStatus enum nit: FaviconDownloadStatus https://codereview.chromium.org/2808063002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2808063002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:92645: + </int> nit: Can you make the strings more coherent/analogous? E.g. Download succeeded. Download failed. Download skipped because the URL was known to have failed previously.
Thanks! https://codereview.chromium.org/2808063002/diff/1/components/favicon/core/fav... File components/favicon/core/favicon_handler.h (right): https://codereview.chromium.org/2808063002/diff/1/components/favicon/core/fav... components/favicon/core/favicon_handler.h:81: // These values must stay in sync with the FaviconCacheStatus enum On 2017/04/10 12:29:38, jkrcal wrote: > nit: FaviconDownloadStatus Done. https://codereview.chromium.org/2808063002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2808063002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:92645: + </int> On 2017/04/10 12:29:38, jkrcal wrote: > nit: Can you make the strings more coherent/analogous? E.g. > Download succeeded. > Download failed. > Download skipped because the URL was known to have failed previously. Done. Thank you for the wording.
fhorschig@chromium.org changed reviewers: + pkotwicz@chromium.org
Hi Peter, would you please take a look at the newest, least controversial FaviconHandler metric?
https://codereview.chromium.org/2808063002/diff/20001/components/favicon/core... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2808063002/diff/20001/components/favicon/core... components/favicon/core/favicon_handler.cc:382: } else if (http_status_code == 200) { Can we record this whenever !bitmaps.empty() https://codereview.chromium.org/2808063002/diff/20001/components/favicon/core... File components/favicon/core/favicon_handler.h (right): https://codereview.chromium.org/2808063002/diff/20001/components/favicon/core... components/favicon/core/favicon_handler.h:78: // Outcome of a single favicon download. Nit: Remove "single". I think that the word in unnecessary https://codereview.chromium.org/2808063002/diff/20001/components/favicon/core... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2808063002/diff/20001/components/favicon/core... components/favicon/core/favicon_handler_unittest.cc:1140: TEST_F(FaviconHandlerTest, TestRecordSkippedDownloadAttemptAfterFailure) { Can you please split this test into two? - One test for when UnableToDownloadFavicon(k404IconURL) returns true - One test for when UnableToDownloadFavicon(k404IconURL) returns false https://codereview.chromium.org/2808063002/diff/20001/components/favicon/core... components/favicon/core/favicon_handler_unittest.cc:1149: RunHandlerWithCandidates( Can you use RunHandlerWithSimpleFaviconCandidates() ?
https://codereview.chromium.org/2808063002/diff/20001/components/favicon/core... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2808063002/diff/20001/components/favicon/core... components/favicon/core/favicon_handler.cc:382: } else if (http_status_code == 200) { On 2017/04/10 18:30:59, pkotwicz wrote: > Can we record this whenever !bitmaps.empty() Done. https://codereview.chromium.org/2808063002/diff/20001/components/favicon/core... File components/favicon/core/favicon_handler.h (right): https://codereview.chromium.org/2808063002/diff/20001/components/favicon/core... components/favicon/core/favicon_handler.h:78: // Outcome of a single favicon download. On 2017/04/10 18:30:59, pkotwicz wrote: > Nit: Remove "single". I think that the word in unnecessary Gone. https://codereview.chromium.org/2808063002/diff/20001/components/favicon/core... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2808063002/diff/20001/components/favicon/core... components/favicon/core/favicon_handler_unittest.cc:1140: TEST_F(FaviconHandlerTest, TestRecordSkippedDownloadAttemptAfterFailure) { On 2017/04/10 18:30:59, pkotwicz wrote: > Can you please split this test into two? > - One test for when UnableToDownloadFavicon(k404IconURL) returns true > - One test for when UnableToDownloadFavicon(k404IconURL) returns false Done. https://codereview.chromium.org/2808063002/diff/20001/components/favicon/core... components/favicon/core/favicon_handler_unittest.cc:1149: RunHandlerWithCandidates( On 2017/04/10 18:30:59, pkotwicz wrote: > Can you use RunHandlerWithSimpleFaviconCandidates() ? Done.
LGTM
fhorschig@chromium.org changed reviewers: + isherman@chromium.org
Hi Ilya, would you please have a look at the DownloadStatus metric in histograms.xml?
https://codereview.chromium.org/2808063002/diff/40001/components/favicon/core... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2808063002/diff/40001/components/favicon/core... components/favicon/core/favicon_handler.cc:385: UMA_HISTOGRAM_COUNTS_100("Favicons.DownloadOutcome", Could this be UMA_HISTOGRAM_SPARSE_SLOWLY or UMA_HISTOGRAM_ENUMERATION? UMA_HISTOGRAM_COUNTS_100 does not seem appropriate for this purpose. https://codereview.chromium.org/2808063002/diff/40001/components/favicon/core... components/favicon/core/favicon_handler.cc:394: DownloadOutcome::SUCCEEDED); Please create a small wrapper function for emitting to this histogram, so that you do not need to repeat the macro invocation several times. The macro expands to a fair bit of code, so it's nice to help the compiler avoid doing that multiple times. https://codereview.chromium.org/2808063002/diff/40001/components/favicon/core... File components/favicon/core/favicon_handler.h (right): https://codereview.chromium.org/2808063002/diff/40001/components/favicon/core... components/favicon/core/favicon_handler.h:82: // in histograms.xml. nit: Please also document that this enum should be treated as append-only, since it backs an UMA histogram. https://codereview.chromium.org/2808063002/diff/40001/components/favicon/core... components/favicon/core/favicon_handler.h:83: enum DownloadOutcome { SUCCEEDED = 0, FAILED = 1, SKIPPED = 2 }; nit: Could this be an enum class?
https://codereview.chromium.org/2808063002/diff/40001/components/favicon/core... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2808063002/diff/40001/components/favicon/core... components/favicon/core/favicon_handler.cc:385: UMA_HISTOGRAM_COUNTS_100("Favicons.DownloadOutcome", On 2017/04/12 23:22:13, Ilya Sherman wrote: > Could this be UMA_HISTOGRAM_SPARSE_SLOWLY or UMA_HISTOGRAM_ENUMERATION? > UMA_HISTOGRAM_COUNTS_100 does not seem appropriate for this purpose. Done. https://codereview.chromium.org/2808063002/diff/40001/components/favicon/core... components/favicon/core/favicon_handler.cc:394: DownloadOutcome::SUCCEEDED); On 2017/04/12 23:22:13, Ilya Sherman wrote: > Please create a small wrapper function for emitting to this histogram, so that > you do not need to repeat the macro invocation several times. The macro expands > to a fair bit of code, so it's nice to help the compiler avoid doing that > multiple times. Done, thanks for the explanation! https://codereview.chromium.org/2808063002/diff/40001/components/favicon/core... File components/favicon/core/favicon_handler.h (right): https://codereview.chromium.org/2808063002/diff/40001/components/favicon/core... components/favicon/core/favicon_handler.h:82: // in histograms.xml. On 2017/04/12 23:22:14, Ilya Sherman wrote: > nit: Please also document that this enum should be treated as append-only, since > it backs an UMA histogram. Done. https://codereview.chromium.org/2808063002/diff/40001/components/favicon/core... components/favicon/core/favicon_handler.h:83: enum DownloadOutcome { SUCCEEDED = 0, FAILED = 1, SKIPPED = 2 }; On 2017/04/12 23:22:14, Ilya Sherman wrote: > nit: Could this be an enum class? Done.
The CQ bit was checked by fhorschig@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: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Metrics LGTM, thanks. https://codereview.chromium.org/2808063002/diff/60001/components/favicon/core... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2808063002/diff/60001/components/favicon/core... components/favicon/core/favicon_handler.cc:84: "Favicons.DownloadOutcome", static_cast<int>(outcome), nit: The static_cast should no longer be necessary. Are you getting a compile error without it?
https://codereview.chromium.org/2808063002/diff/60001/components/favicon/core... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2808063002/diff/60001/components/favicon/core... components/favicon/core/favicon_handler.cc:84: "Favicons.DownloadOutcome", static_cast<int>(outcome), On 2017/04/13 21:24:43, Ilya Sherman wrote: > nit: The static_cast should no longer be necessary. Are you getting a compile > error without it? Removed. Nice to know that this works!
The CQ bit was checked by fhorschig@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.
The CQ bit was checked by fhorschig@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jkrcal@chromium.org, pkotwicz@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2808063002/#ps100001 (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": 100001, "attempt_start_ts": 1492596758599230,
"parent_rev": "e3ab8efcc3b78b2b5bf9dabdc94ab81d7770481e", "commit_rev":
"be0a6071e24bc1e0f432533042d3d813bbe6ed3f"}
Message was sent while issue was closed.
Description was changed from ========== Add DownloadStatus metric to FaviconHandler This metric records how often single icon downloads fail with 404 and how often 404'ing icons can be skipped because they were already known. BUG=672939 ========== to ========== Add DownloadStatus metric to FaviconHandler This metric records how often single icon downloads fail with 404 and how often 404'ing icons can be skipped because they were already known. BUG=672939 Review-Url: https://codereview.chromium.org/2808063002 Cr-Commit-Position: refs/heads/master@{#465545} Committed: https://chromium.googlesource.com/chromium/src/+/be0a6071e24bc1e0f432533042d3... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/be0a6071e24bc1e0f432533042d3...
Message was sent while issue was closed.
Description was changed from ========== Add DownloadStatus metric to FaviconHandler This metric records how often single icon downloads fail with 404 and how often 404'ing icons can be skipped because they were already known. BUG=672939 Review-Url: https://codereview.chromium.org/2808063002 Cr-Commit-Position: refs/heads/master@{#465545} Committed: https://chromium.googlesource.com/chromium/src/+/be0a6071e24bc1e0f432533042d3... ========== to ========== Add DownloadStatus metric to FaviconHandler This metric records how often single icon downloads fail with 404 and how often 404'ing icons can be skipped because they were already known. BUG=694956 Review-Url: https://codereview.chromium.org/2808063002 Cr-Commit-Position: refs/heads/master@{#465545} Committed: https://chromium.googlesource.com/chromium/src/+/be0a6071e24bc1e0f432533042d3... ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
