Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2812243002:
[Remote suggestions] Log favicon fetch result for both code paths (Closed)
Description[Remote suggestions] Log favicon fetch result for both code paths
We have two code paths that download favicons from different favicon
servers. We can switch between them based on a field trial feature.
This CL adds logging of success rates for both the code-paths.
BUG=709498
Review-Url: https://codereview.chromium.org/2812243002
Cr-Commit-Position: refs/heads/master@{#464395}
Committed: https://chromium.googlesource.com/chromium/src/+/08d79b5a7dd7bbaaec2f4ec5ad68de3ed63d58c1
Patch Set 1 #Patch Set 2 : Add fetch time histogram #
Total comments: 12
Patch Set 3 : Comments #
Total comments: 2
Messages
Total messages: 26 (17 generated)
jkrcal@chromium.org changed reviewers: + isherman@chromium.org, mvanouwerkerk@chromium.org
Michael, could you PTAL at this CL? Ilya, could you PTAL at histograms.xml?
https://codereview.chromium.org/2812243002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2812243002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:96: private long mFaviconFetchStartTime; nit: rename to mFaviconFetchStartTimeMs https://codereview.chromium.org/2812243002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:209: mFaviconFetchStartTime = SystemClock.elapsedRealtime(); As onBindViewHolder can be called more than once, it seems better to scope the start time as a member of the callback for the favicon request. https://codereview.chromium.org/2812243002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:413: recordFaviconFetchTime(); This will record the time for all results, whether they are from cache, null, or success or failure from the network. Would it be useful to also log those cases separately? https://codereview.chromium.org/2812243002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:421: RecordHistogram.recordTimesHistogram( recordTimesHistogram maxes out at 10 seconds, I think we'll exceed that often for network requests in emerging markets. Please use recordMediumTimesHistogram which allows for 3 minutes. https://codereview.chromium.org/2812243002/diff/20001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2812243002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:201: if (!result.image.IsEmpty()) { Should there be an else clause to log FaviconFetchResult::FAILURE ?
The CQ bit was checked by mvanouwerkerk@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.
https://codereview.chromium.org/2812243002/diff/20001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2812243002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:41: RESULT_MAX = 3 Optional nit: I'd name this "COUNT" rather than "MAX" -- "max" is typically used as an alias for the largest actual value in an enum.
Sorry, metrics LGTM % the optional nit. Please let me know if you end up adding more histograms, as I'd like to take another look in that case.
Thanks! Michael, could you please take another look? https://codereview.chromium.org/2812243002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2812243002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:96: private long mFaviconFetchStartTime; On 2017/04/12 13:20:30, Michael van Ouwerkerk wrote: > nit: rename to mFaviconFetchStartTimeMs Done. https://codereview.chromium.org/2812243002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:209: mFaviconFetchStartTime = SystemClock.elapsedRealtime(); On 2017/04/12 13:20:31, Michael van Ouwerkerk wrote: > As onBindViewHolder can be called more than once, it seems better to scope the > start time as a member of the callback for the favicon request. Thanks for spotting this! Done. The parameter is now all around but I see no better simple way how to do it. https://codereview.chromium.org/2812243002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:413: recordFaviconFetchTime(); On 2017/04/12 13:20:31, Michael van Ouwerkerk wrote: > This will record the time for all results, whether they are from cache, null, or > success or failure from the network. Would it be useful to also log those cases > separately? I believe that the simple solution is here good enough. I believe we will see different peaks in the distribution for results from cache and for results from the network. The purpose of this histogram is to check that there is no major speed regression which should be visible from the mixed histogram. (Logging it separately would need duplicate code in java, c++, similarly to the fetch result histogram) WDYT? https://codereview.chromium.org/2812243002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:421: RecordHistogram.recordTimesHistogram( On 2017/04/12 13:20:31, Michael van Ouwerkerk wrote: > recordTimesHistogram maxes out at 10 seconds, I think we'll exceed that often > for network requests in emerging markets. Please use recordMediumTimesHistogram > which allows for 3 minutes. Done. https://codereview.chromium.org/2812243002/diff/20001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2812243002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:41: RESULT_MAX = 3 On 2017/04/12 23:18:32, Ilya Sherman wrote: > Optional nit: I'd name this "COUNT" rather than "MAX" -- "max" is typically used > as an alias for the largest actual value in an enum. Good point, done. https://codereview.chromium.org/2812243002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:201: if (!result.image.IsEmpty()) { On 2017/04/12 13:20:31, Michael van Ouwerkerk wrote: > Should there be an else clause to log FaviconFetchResult::FAILURE ? The failure is recorded on line 218 / line 240. The execution flow is - check from cache (potentially report success_cached) - check from google server (potentially report failure if we cannot send the request - 218 - or if the response is empty - 240) - get the newly fetched result from the cache (report success_fetched)
lgtm % one comment for your consideration https://codereview.chromium.org/2812243002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2812243002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:208: long faviconFetchStartTimeMs = SystemClock.elapsedRealtime(); One way to slightly reduce the omnipresense of this variable would be to move this statement into both fetchFaviconFromLocalCache and fetchFaviconFromLocalCacheOrGoogleServer so they won't have it as an argument and you can delete the variable here. Fwiw :-)
The CQ bit was checked by jkrcal@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...
Thanks! https://codereview.chromium.org/2812243002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2812243002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:208: long faviconFetchStartTimeMs = SystemClock.elapsedRealtime(); On 2017/04/13 10:16:22, Michael van Ouwerkerk wrote: > One way to slightly reduce the omnipresense of this variable would be to move > this statement into both fetchFaviconFromLocalCache and > fetchFaviconFromLocalCacheOrGoogleServer so they won't have it as an argument > and you can delete the variable here. Fwiw :-) This does not work for fetchFaviconFromLocalCache as this can be (similarly to the c++ flow) called twice per request and I need to drag the same timestamp across these two calls.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jkrcal@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 jkrcal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2812243002/#ps40001 (title: "Comments")
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": 1492091653392160,
"parent_rev": "ab15c62867108abf92582f3f6c41a766b10324c0", "commit_rev":
"08d79b5a7dd7bbaaec2f4ec5ad68de3ed63d58c1"}
Message was sent while issue was closed.
Description was changed from ========== [Remote suggestions] Log favicon fetch result for both code paths We have two code paths that download favicons from different favicon servers. We can switch between them based on a field trial feature. This CL adds logging of success rates for both the code-paths. BUG=709498 ========== to ========== [Remote suggestions] Log favicon fetch result for both code paths We have two code paths that download favicons from different favicon servers. We can switch between them based on a field trial feature. This CL adds logging of success rates for both the code-paths. BUG=709498 Review-Url: https://codereview.chromium.org/2812243002 Cr-Commit-Position: refs/heads/master@{#464395} Committed: https://chromium.googlesource.com/chromium/src/+/08d79b5a7dd7bbaaec2f4ec5ad68... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/08d79b5a7dd7bbaaec2f4ec5ad68... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
