Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2781093002:
Add attempt count metric to FaviconHandler (Closed)
DescriptionAdd attempt count metric to FaviconHandler
The histograms this metric introduces aggregates how many download
attempts were needed to determine the best-fitting favicon.
This is stored in two metrics in order to separate requests for large
icons.
BUG=694956
Review-Url: https://codereview.chromium.org/2781093002
Cr-Commit-Position: refs/heads/master@{#465526}
Committed: https://chromium.googlesource.com/chromium/src/+/a9fb2c3cf6bc9b4877d9c21f9f12094fb5c26ecd
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add third metric #Patch Set 3 : Require metric for every notification icon type #
Total comments: 19
Patch Set 4 : Record 404s as attempts and add separate tests for recording #Patch Set 5 : Readded a different metric for touch icons #
Total comments: 12
Patch Set 6 : Use histogram suffix instead of completely new histogram #Patch Set 7 : Use dedicated variable for request count and simplify tests #
Total comments: 6
Patch Set 8 : Verify expired results are recorded exactly once. #
Total comments: 10
Patch Set 9 : Correct test and restrict histogram #
Total comments: 2
Patch Set 10 : Use public sparse_slowly #
Dependent Patchsets: Messages
Total messages: 48 (20 generated)
fhorschig@chromium.org changed reviewers: + jkrcal@chromium.org
Hi Jan, Is this metric something you would consider useful? (You proposed it a while back but I am not sure about the granularity you had in mind.)
Yep, this CL seems useful, indeed! https://codereview.chromium.org/2781093002/diff/1/components/favicon/core/fav... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2781093002/diff/1/components/favicon/core/fav... components/favicon/core/favicon_handler.cc:481: UMA_HISTOGRAM_COUNTS_100("Favicons.LargeIconDownloadAttempts", Can we actually have three histogram per each FaviconDriverObserver::NotificationIconType? (can you also have the naming of the histograms consistent with the naming of the enum?)
https://codereview.chromium.org/2781093002/diff/1/components/favicon/core/fav... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2781093002/diff/1/components/favicon/core/fav... components/favicon/core/favicon_handler.cc:481: UMA_HISTOGRAM_COUNTS_100("Favicons.LargeIconDownloadAttempts", On 2017/03/30 07:53:28, jkrcal wrote: > Can we actually have three histogram per each > FaviconDriverObserver::NotificationIconType? > (can you also have the naming of the histograms consistent with the naming of > the enum?) I added a third historgram but it's slightly different: If we ever add new type to the enum without new metric, we would lose the total number of attempts. If we have the overall count and counts for Large and Touch icon downloads, we can calculate the number of attempts for the small favicons. We could also add a fourth type for small icons, but I don't think it's important enough to record duplicate metrics. WDYT?
On 2017/04/03 09:56:49, fhorschig wrote: > https://codereview.chromium.org/2781093002/diff/1/components/favicon/core/fav... > File components/favicon/core/favicon_handler.cc (right): > > https://codereview.chromium.org/2781093002/diff/1/components/favicon/core/fav... > components/favicon/core/favicon_handler.cc:481: > UMA_HISTOGRAM_COUNTS_100("Favicons.LargeIconDownloadAttempts", > On 2017/03/30 07:53:28, jkrcal wrote: > > Can we actually have three histogram per each > > FaviconDriverObserver::NotificationIconType? > > (can you also have the naming of the histograms consistent with the naming of > > the enum?) > > I added a third historgram but it's slightly different: > If we ever add new type to the enum without new metric, we would lose the total > number of attempts. If we have the overall count and counts for Large and Touch > icon downloads, we can calculate the number of attempts for the small favicons. > > We could also add a fourth type for small icons, but I don't think it's > important > enough to record duplicate metrics. WDYT? Why not having a switch statement over the enum, instead? (having the 3 types as I suggested). This is more future-proof because this way, you would enforce that whoever changes the enum would be forces to evaluate whether / how to extend the UMA logging.
On 2017/04/03 10:12:58, jkrcal wrote: > On 2017/04/03 09:56:49, fhorschig wrote: > > > https://codereview.chromium.org/2781093002/diff/1/components/favicon/core/fav... > > File components/favicon/core/favicon_handler.cc (right): > > > > > https://codereview.chromium.org/2781093002/diff/1/components/favicon/core/fav... > > components/favicon/core/favicon_handler.cc:481: > > UMA_HISTOGRAM_COUNTS_100("Favicons.LargeIconDownloadAttempts", > > On 2017/03/30 07:53:28, jkrcal wrote: > > > Can we actually have three histogram per each > > > FaviconDriverObserver::NotificationIconType? > > > (can you also have the naming of the histograms consistent with the naming > of > > > the enum?) > > > > I added a third historgram but it's slightly different: > > If we ever add new type to the enum without new metric, we would lose the > total > > number of attempts. If we have the overall count and counts for Large and > Touch > > icon downloads, we can calculate the number of attempts for the small > favicons. > > > > We could also add a fourth type for small icons, but I don't think it's > > important > > enough to record duplicate metrics. WDYT? > > Why not having a switch statement over the enum, instead? (having the 3 types as > I suggested). Done. > This is more future-proof because this way, you would enforce that whoever > changes the enum would be forces to evaluate whether / how to extend the UMA > logging. Sounds good.
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.
lgtm, thanks!
fhorschig@chromium.org changed reviewers: + pkotwicz@chromium.org
Hi Peter, would you please have a look at the new metrics for recording the favicon download attempt count?
https://codereview.chromium.org/2781093002/diff/40001/components/favicon/core... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2781093002/diff/40001/components/favicon/core... components/favicon/core/favicon_handler.cc:76: return; Can we use the same histogram for Favicons.LargeIconDownloadAttempts and Favicons.TouchIconDownloadAttempts mastiz@ is trying to consolidate FaviconHandlers for type FaviconDriverObserver::NON_TOUCH_LARGEST and FaviconDriverObserver::TOUCH_LARGEST in https://codereview.chromium.org/2795763004/ https://codereview.chromium.org/2781093002/diff/40001/components/favicon/core... components/favicon/core/favicon_handler.cc:79: return; Nit: The return here is redundant https://codereview.chromium.org/2781093002/diff/40001/components/favicon/core... components/favicon/core/favicon_handler.cc:425: RecordAttemptsForHandlerType(handler_type_, request_attempts_); Can you store |current_candidate_index_ + 1| instead? https://codereview.chromium.org/2781093002/diff/40001/components/favicon/core... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2781093002/diff/40001/components/favicon/core... components/favicon/core/favicon_handler_unittest.cc:347: if (!download_histogram_name_.empty() && delegate_.downloads().size() > 0) { We should check the histogram only for some tests not every test https://codereview.chromium.org/2781093002/diff/40001/components/favicon/core... components/favicon/core/favicon_handler_unittest.cc:428: EXPECT_THAT(delegate_.downloads(), IsEmpty()); This is probably a better test to check download_histogram_->ExpectTotalCount("Favicons.DownloadAttempts", 0); https://codereview.chromium.org/2781093002/diff/40001/components/favicon/core... components/favicon/core/favicon_handler_unittest.cc:644: download_histogram_->ExpectTotalCount("Favicons.DownloadAttempts", 0); I don't think that this is a good test for this check https://codereview.chromium.org/2781093002/diff/40001/components/favicon/core... components/favicon/core/favicon_handler_unittest.cc:684: download_histogram_->ExpectTotalCount("Favicons.FaviconDownloadAttempts", 0); I don't think that this is a good test for this check https://codereview.chromium.org/2781093002/diff/40001/components/favicon/core... components/favicon/core/favicon_handler_unittest.cc:714: /*sample=*/2, /*expected_count=*/1); I don't think that this is a good test for this check https://codereview.chromium.org/2781093002/diff/40001/components/favicon/core... components/favicon/core/favicon_handler_unittest.cc:983: kIconURLWithoutSize1, kIconURLWithoutSize2)); This is a good test to check download_histogram_>ExpectBucketCount("Favicons.LargeIconDownloadAttempts", /*sample=*/1, /*expected_count=*/5);
https://codereview.chromium.org/2781093002/diff/40001/components/favicon/core... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2781093002/diff/40001/components/favicon/core... components/favicon/core/favicon_handler.cc:76: return; Can we use the same histogram for Favicons.LargeIconDownloadAttempts and Favicons.TouchIconDownloadAttempts mastiz@ is trying to consolidate FaviconHandlers for type FaviconDriverObserver::NON_TOUCH_LARGEST and FaviconDriverObserver::TOUCH_LARGEST in https://codereview.chromium.org/2795763004/ https://codereview.chromium.org/2781093002/diff/40001/components/favicon/core... components/favicon/core/favicon_handler.cc:79: return; Nit: The return here is redundant https://codereview.chromium.org/2781093002/diff/40001/components/favicon/core... components/favicon/core/favicon_handler.cc:425: RecordAttemptsForHandlerType(handler_type_, request_attempts_); Can you store |current_candidate_index_ + 1| instead? https://codereview.chromium.org/2781093002/diff/40001/components/favicon/core... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2781093002/diff/40001/components/favicon/core... components/favicon/core/favicon_handler_unittest.cc:347: if (!download_histogram_name_.empty() && delegate_.downloads().size() > 0) { We should check the histogram only for some tests not every test https://codereview.chromium.org/2781093002/diff/40001/components/favicon/core... components/favicon/core/favicon_handler_unittest.cc:428: EXPECT_THAT(delegate_.downloads(), IsEmpty()); This is probably a better test to check download_histogram_->ExpectTotalCount("Favicons.DownloadAttempts", 0); https://codereview.chromium.org/2781093002/diff/40001/components/favicon/core... components/favicon/core/favicon_handler_unittest.cc:644: download_histogram_->ExpectTotalCount("Favicons.DownloadAttempts", 0); I don't think that this is a good test for this check https://codereview.chromium.org/2781093002/diff/40001/components/favicon/core... components/favicon/core/favicon_handler_unittest.cc:684: download_histogram_->ExpectTotalCount("Favicons.FaviconDownloadAttempts", 0); I don't think that this is a good test for this check https://codereview.chromium.org/2781093002/diff/40001/components/favicon/core... components/favicon/core/favicon_handler_unittest.cc:714: /*sample=*/2, /*expected_count=*/1); I don't think that this is a good test for this check https://codereview.chromium.org/2781093002/diff/40001/components/favicon/core... components/favicon/core/favicon_handler_unittest.cc:983: kIconURLWithoutSize1, kIconURLWithoutSize2)); This is a good test to check download_histogram_>ExpectBucketCount("Favicons.LargeIconDownloadAttempts", /*sample=*/1, /*expected_count=*/5);
https://codereview.chromium.org/2781093002/diff/40001/components/favicon/core... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2781093002/diff/40001/components/favicon/core... components/favicon/core/favicon_handler.cc:76: return; On 2017/04/04 13:04:37, pkotwicz wrote: > Can we use the same histogram for Favicons.LargeIconDownloadAttempts and > Favicons.TouchIconDownloadAttempts > > mastiz@ is trying to consolidate FaviconHandlers for type > FaviconDriverObserver::NON_TOUCH_LARGEST and > FaviconDriverObserver::TOUCH_LARGEST > > in https://codereview.chromium.org/2795763004/ Done. https://codereview.chromium.org/2781093002/diff/40001/components/favicon/core... components/favicon/core/favicon_handler.cc:79: return; On 2017/04/04 13:04:37, pkotwicz wrote: > Nit: The return here is redundant Gone. https://codereview.chromium.org/2781093002/diff/40001/components/favicon/core... components/favicon/core/favicon_handler.cc:425: RecordAttemptsForHandlerType(handler_type_, request_attempts_); On 2017/04/04 13:04:38, pkotwicz wrote: > Can you store |current_candidate_index_ + 1| instead? Done. This captures 404s as attempts as well. https://codereview.chromium.org/2781093002/diff/40001/components/favicon/core... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2781093002/diff/40001/components/favicon/core... components/favicon/core/favicon_handler_unittest.cc:347: if (!download_histogram_name_.empty() && delegate_.downloads().size() > 0) { On 2017/04/04 13:04:38, pkotwicz wrote: > We should check the histogram only for some tests not every test Done. https://codereview.chromium.org/2781093002/diff/40001/components/favicon/core... components/favicon/core/favicon_handler_unittest.cc:428: EXPECT_THAT(delegate_.downloads(), IsEmpty()); On 2017/04/04 13:04:38, pkotwicz wrote: > This is probably a better test to check > download_histogram_->ExpectTotalCount("Favicons.DownloadAttempts", 0); Added. (There are separate test cases, too, but this covers the cache-only case pretty well) https://codereview.chromium.org/2781093002/diff/40001/components/favicon/core... components/favicon/core/favicon_handler_unittest.cc:644: download_histogram_->ExpectTotalCount("Favicons.DownloadAttempts", 0); On 2017/04/04 13:04:38, pkotwicz wrote: > I don't think that this is a good test for this check Gone. https://codereview.chromium.org/2781093002/diff/40001/components/favicon/core... components/favicon/core/favicon_handler_unittest.cc:684: download_histogram_->ExpectTotalCount("Favicons.FaviconDownloadAttempts", 0); On 2017/04/04 13:04:38, pkotwicz wrote: > I don't think that this is a good test for this check Gone. https://codereview.chromium.org/2781093002/diff/40001/components/favicon/core... components/favicon/core/favicon_handler_unittest.cc:714: /*sample=*/2, /*expected_count=*/1); On 2017/04/04 13:04:38, pkotwicz wrote: > I don't think that this is a good test for this check Gone. https://codereview.chromium.org/2781093002/diff/40001/components/favicon/core... components/favicon/core/favicon_handler_unittest.cc:983: kIconURLWithoutSize1, kIconURLWithoutSize2)); On 2017/04/04 13:04:38, pkotwicz wrote: > This is a good test to check > > download_histogram_>ExpectBucketCount("Favicons.LargeIconDownloadAttempts", > /*sample=*/1, /*expected_count=*/5); Added a new test for multiple downloads. If you prefer, I can join them.
+mastiz (CC) as OWNER of the IconType consolidation CL I readded the third bucket for now. More info in-line. https://codereview.chromium.org/2781093002/diff/40001/components/favicon/core... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2781093002/diff/40001/components/favicon/core... components/favicon/core/favicon_handler.cc:76: return; On 2017/04/10 09:55:50, fhorschig wrote: > On 2017/04/04 13:04:37, pkotwicz wrote: > > Can we use the same histogram for Favicons.LargeIconDownloadAttempts and > > Favicons.TouchIconDownloadAttempts > > > > mastiz@ is trying to consolidate FaviconHandlers for type > > FaviconDriverObserver::NON_TOUCH_LARGEST and > > FaviconDriverObserver::TOUCH_LARGEST > > > > in https://codereview.chromium.org/2795763004/ > > Done. I was informed that we might have a large delay on that effort and we could potentially lose the recorded data if that change turns out to be pushed back. We discussed that it might make more sense to consolidate the metrics later, which is still wasy to do. Therefore, I readded the differentiation for now. WDYT?
https://codereview.chromium.org/2781093002/diff/80001/components/favicon/core... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2781093002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler.cc:525: RecordAttemptsForHandlerType(handler_type_, current_candidate_index_); Why are we recording Favicons.FaviconDownloadAttempts from here?
https://codereview.chromium.org/2781093002/diff/80001/components/favicon/core... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2781093002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler.cc:525: RecordAttemptsForHandlerType(handler_type_, current_candidate_index_); On 2017/04/10 14:58:11, pkotwicz wrote: > Why are we recording Favicons.FaviconDownloadAttempts from here? Because TestRecordDownloadAttemptsFinishedByCache would fail. We have multiple candidates, but the best candidate fails to load. Then the cached next candidate is used to complete the request. I considered it important to capture these attempts as well.
https://codereview.chromium.org/2781093002/diff/80001/components/favicon/core... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2781093002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler.cc:525: RecordAttemptsForHandlerType(handler_type_, current_candidate_index_); Sorry for the confusion earlier. I think that we need a dedicated variable which counts the number of downloads: |num_download_requests_| (because that is what the histogram is recording) We should probably only record the metric if |num_download_requests_| is non zero Sorry for flip flopping https://codereview.chromium.org/2781093002/diff/80001/components/favicon/core... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2781093002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler_unittest.cc:1050: {gfx::Size(16, 16), gfx::Size(512, 512)})}); - Are multiple FaviconURLs helpful for this test? - Are multiple sizes per FaviconURL helpful for this test? If not can you simplify the input? https://codereview.chromium.org/2781093002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler_unittest.cc:1070: Are multiple sizes per FaviconURL helpful for this test? https://codereview.chromium.org/2781093002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler_unittest.cc:1085: RunHandlerWithCandidates( Nit: Can you use RunHandlerWithSimpleCandidates() https://codereview.chromium.org/2781093002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler_unittest.cc:1113: const GURL kIconURL16_512("http://www.google.com/c"); Are multiple sizes per FaviconURL helpful for this test?
https://codereview.chromium.org/2781093002/diff/80001/components/favicon/core... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2781093002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler.cc:525: RecordAttemptsForHandlerType(handler_type_, current_candidate_index_); On 2017/04/10 17:26:10, pkotwicz wrote: > Sorry for the confusion earlier. I think that we need a dedicated variable which > counts the number of downloads: |num_download_requests_| (because that is what > the histogram is recording) I agree, It's more explicit and future-proof in we ever skip candidates. The recorded number is still exactly the same (incl. 404 as attempts). > We should probably only record the metric if |num_download_requests_| is non > zero Done. Added a check in |OnFaviconData| so |RecordAttempts...| does the action it implies. Added comment to |OnDidDownload...|. > Sorry for flip flopping Don't worry. https://codereview.chromium.org/2781093002/diff/80001/components/favicon/core... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2781093002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler_unittest.cc:1050: {gfx::Size(16, 16), gfx::Size(512, 512)})}); On 2017/04/10 17:26:10, pkotwicz wrote: > - Are multiple FaviconURLs helpful for this test? Yes, I want to log every single failed attempt. Commented. > - Are multiple sizes per FaviconURL helpful for this test? Nope, removed. > If not can you simplify the input? Done. https://codereview.chromium.org/2781093002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler_unittest.cc:1070: On 2017/04/10 17:26:10, pkotwicz wrote: > Are multiple sizes per FaviconURL helpful for this test? Gone. https://codereview.chromium.org/2781093002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler_unittest.cc:1085: RunHandlerWithCandidates( On 2017/04/10 17:26:10, pkotwicz wrote: > Nit: Can you use RunHandlerWithSimpleCandidates() No, I want to set the NotificationIconType to NON_TOUCH_LARGEST. https://codereview.chromium.org/2781093002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler_unittest.cc:1113: const GURL kIconURL16_512("http://www.google.com/c"); On 2017/04/10 17:26:10, pkotwicz wrote: > Are multiple sizes per FaviconURL helpful for this test? Gone.
https://codereview.chromium.org/2781093002/diff/120001/components/favicon/cor... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2781093002/diff/120001/components/favicon/cor... components/favicon/core/favicon_handler.cc:63: void RecordAttemptsForHandlerType( Maybe rename this method to: RecordDownloadAttemptsForHandlerType() https://codereview.chromium.org/2781093002/diff/120001/components/favicon/cor... components/favicon/core/favicon_handler.cc:426: // |num_download_requests_| can never be 0. Nit: OnDidDownloadFavicon -> OnDidDownloadFavicon() "invoking a request" -> "requesting a download" https://codereview.chromium.org/2781093002/diff/120001/components/favicon/cor... components/favicon/core/favicon_handler.cc:530: RecordAttemptsForHandlerType(handler_type_, current_candidate_index_); Don't you want to pass |num_download_requests_| here? You should record this in the else{} statement of if(has_expired_or_incomplete_result){} If the icon is expired the current code records the UMA metric twice
https://codereview.chromium.org/2781093002/diff/120001/components/favicon/cor... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2781093002/diff/120001/components/favicon/cor... components/favicon/core/favicon_handler.cc:63: void RecordAttemptsForHandlerType( On 2017/04/12 02:48:20, pkotwicz wrote: > Maybe rename this method to: RecordDownloadAttemptsForHandlerType() Done. https://codereview.chromium.org/2781093002/diff/120001/components/favicon/cor... components/favicon/core/favicon_handler.cc:426: // |num_download_requests_| can never be 0. On 2017/04/12 02:48:21, pkotwicz wrote: > Nit: OnDidDownloadFavicon -> OnDidDownloadFavicon() > > "invoking a request" -> "requesting a download" Done. https://codereview.chromium.org/2781093002/diff/120001/components/favicon/cor... components/favicon/core/favicon_handler.cc:530: RecordAttemptsForHandlerType(handler_type_, current_candidate_index_); On 2017/04/12 02:48:21, pkotwicz wrote: > Don't you want to pass |num_download_requests_| here? Yes, done. > You should record this in the else{} statement of > if(has_expired_or_incomplete_result){} > If the icon is expired the current code records the UMA metric twice Done. (It hasn't happened yet because num_download_requests_ was usually 0 at this point. There is a test verifying that now.)
https://codereview.chromium.org/2781093002/diff/140001/components/favicon/cor... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2781093002/diff/140001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1141: kPageURL, kIconURL16x16, In order to trigger the error case in FaviconHandler::OnFaviconData() that I mentioned in the previous code review, you would need to pass a page url != kPageUrl
LGTM with nit
LGTM with nit in comment #25
fhorschig@chromium.org changed reviewers: + isherman@chromium.org
Hi Ilya, could you please also take a look at this metric? It records the number of downloads we needed until we received a suitable favicon.
https://codereview.chromium.org/2781093002/diff/140001/components/favicon/cor... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2781093002/diff/140001/components/favicon/cor... components/favicon/core/favicon_handler.cc:69: return; How many download attempts do you expect to see, typically? I'm guessing << 10? Could these histograms use UMA_HISTOGRAM_SPARSE_SLOWLY with some modest sanity-checks in place? Allocating space for 50 buckets each seems excessive for these. https://codereview.chromium.org/2781093002/diff/140001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2781093002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:20430: +<histogram name="Favicons.DownloadAttempts"> nit: Please add base="true", as you never record to this histogram directly; only through suffixes. https://codereview.chromium.org/2781093002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:20430: +<histogram name="Favicons.DownloadAttempts"> nit: Please add units="attempts" https://codereview.chromium.org/2781093002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:20433: + Records the number of icon requested until the best-fitting candidate was nit: s/icon/icons (or s/requested/requests)
https://codereview.chromium.org/2781093002/diff/140001/components/favicon/cor... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2781093002/diff/140001/components/favicon/cor... components/favicon/core/favicon_handler.cc:69: return; On 2017/04/12 20:43:03, Ilya Sherman wrote: > How many download attempts do you expect to see, typically? I'm guessing << 10? It should indeed be << 10. > Could these histograms use UMA_HISTOGRAM_SPARSE_SLOWLY with some modest > sanity-checks in place? Allocating space for 50 buckets each seems excessive > for these. Used SPARSE_SLOWLY. I limited the minimal/maximal bucket count as sanity check and adjusted the histogram description accordingly. If I misinterpreted your intention, please tell me. https://codereview.chromium.org/2781093002/diff/140001/components/favicon/cor... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2781093002/diff/140001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1141: kPageURL, kIconURL16x16, On 2017/04/12 14:39:24, pkotwicz wrote: > In order to trigger the error case in FaviconHandler::OnFaviconData() that I > mentioned in the previous code review, you would need to pass a page url != > kPageUrl Correct, thanks! (I should be more careful while refactoring tests ...) https://codereview.chromium.org/2781093002/diff/140001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2781093002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:20430: +<histogram name="Favicons.DownloadAttempts"> On 2017/04/12 20:43:03, Ilya Sherman wrote: > nit: Please add base="true", as you never record to this histogram directly; > only through suffixes. Done. https://codereview.chromium.org/2781093002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:20430: +<histogram name="Favicons.DownloadAttempts"> On 2017/04/12 20:43:03, Ilya Sherman wrote: > nit: Please add units="attempts" Done. https://codereview.chromium.org/2781093002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:20433: + Records the number of icon requested until the best-fitting candidate was On 2017/04/12 20:43:03, Ilya Sherman wrote: > nit: s/icon/icons (or s/requested/requests) 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: This issue passed the CQ dry run.
Thanks. Metrics LGTM % a final comment: https://codereview.chromium.org/2781093002/diff/160001/components/favicon/cor... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2781093002/diff/160001/components/favicon/cor... components/favicon/core/favicon_handler.cc:72: INTERNAL_HISTOGRAM_SPARSE_SLOWLY("Favicons.DownloadAttempts.Favicons", Please use UMA_HISTOGRAM_SPARSE_SLOWLY. The INTERNAL macros are implementation details, not intended to be used as a public API.
https://codereview.chromium.org/2781093002/diff/160001/components/favicon/cor... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2781093002/diff/160001/components/favicon/cor... components/favicon/core/favicon_handler.cc:72: INTERNAL_HISTOGRAM_SPARSE_SLOWLY("Favicons.DownloadAttempts.Favicons", On 2017/04/13 21:17:59, Ilya Sherman wrote: > Please use UMA_HISTOGRAM_SPARSE_SLOWLY. The INTERNAL macros are implementation > details, not intended to be used as a public API. Done and thanks!
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/2781093002/#ps180001 (title: "Use public sparse_slowly")
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": 180001, "attempt_start_ts": 1492588562994760,
"parent_rev": "3408db33fff22d04e05f38c875258debb25967f5", "commit_rev":
"a9fb2c3cf6bc9b4877d9c21f9f12094fb5c26ecd"}
Message was sent while issue was closed.
Description was changed from ========== Add attempt count metric to FaviconHandler The histograms this metric introduces aggregates how many download attempts were needed to determine the best-fitting favicon. This is stored in two metrics in order to separate requests for large icons. BUG=672939 ========== to ========== Add attempt count metric to FaviconHandler The histograms this metric introduces aggregates how many download attempts were needed to determine the best-fitting favicon. This is stored in two metrics in order to separate requests for large icons. BUG=672939 Review-Url: https://codereview.chromium.org/2781093002 Cr-Commit-Position: refs/heads/master@{#465526} Committed: https://chromium.googlesource.com/chromium/src/+/a9fb2c3cf6bc9b4877d9c21f9f12... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/a9fb2c3cf6bc9b4877d9c21f9f12...
Message was sent while issue was closed.
Description was changed from ========== Add attempt count metric to FaviconHandler The histograms this metric introduces aggregates how many download attempts were needed to determine the best-fitting favicon. This is stored in two metrics in order to separate requests for large icons. BUG=672939 Review-Url: https://codereview.chromium.org/2781093002 Cr-Commit-Position: refs/heads/master@{#465526} Committed: https://chromium.googlesource.com/chromium/src/+/a9fb2c3cf6bc9b4877d9c21f9f12... ========== to ========== Add attempt count metric to FaviconHandler The histograms this metric introduces aggregates how many download attempts were needed to determine the best-fitting favicon. This is stored in two metrics in order to separate requests for large icons. BUG=694956 Review-Url: https://codereview.chromium.org/2781093002 Cr-Commit-Position: refs/heads/master@{#465526} Committed: https://chromium.googlesource.com/chromium/src/+/a9fb2c3cf6bc9b4877d9c21f9f12... ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
