|
|
Created:
4 years, 10 months ago by Donn Denman Modified:
4 years, 9 months ago CC:
chromium-reviews, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Contextual Search] Add metrics for Resolve and View timing
Add metrics to record the time between starting a contextual search and the Resolve completing.
Add metrics for the time between starting a contextual search and the results being viewable in the overlay.
Also do some minor code cleanup: Rename some variables used for timing that had ambiguous names, remove an unused method.
BUG=526291
Committed: https://crrev.com/8fe933a0ea4f518cffa061009985d814e8797b21
Cr-Commit-Position: refs/heads/master@{#378640}
Patch Set 1 #
Total comments: 16
Patch Set 2 : Updated some method names. #
Total comments: 11
Patch Set 3 : Switched to using a separate timer. #
Messages
Total messages: 27 (10 generated)
donnd@chromium.org changed reviewers: + pedrosimonetti@chromium.org
Pedro, PTAL at the first three files in this CL. Thanks!
https://codereview.chromium.org/1736203002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java (right): https://codereview.chromium.org/1736203002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:389: public void onSearchResultsViewable(boolean didResolve) { Does this mean the results were viewed, or that they simple could have been viewed (because they were preteched)? Assuming the latter, wouldn't it be easier to understand if this was called onSearchResultsPrefetched()? https://codereview.chromium.org/1736203002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelMetrics.java (right): https://codereview.chromium.org/1736203002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelMetrics.java:190: public void onSearchTermResolved() { Nit: JavaDoc https://codereview.chromium.org/1736203002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelMetrics.java:195: public void onSearchResultsViewable(boolean didResolve) { Nit: JavaDoc https://codereview.chromium.org/1736203002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java (right): https://codereview.chromium.org/1736203002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java:712: public static void logSearchTermResolution(long durationMs) { Nit: logSearchTermResolution --> logSearchTermResolutionDuration? https://codereview.chromium.org/1736203002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java:725: : "Search.ContextualSearchLiteralResultsViewableDuration"; I thought "literal" / verbatim results were never prefetched. Isn't this true? If that's true, then passing the didResolve boolean is not necessary. https://codereview.chromium.org/1736203002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java:731: * Logs whether the promo was seen. Shouldn't we also log the time it takes to establish a selection? Currently, the selection is the first feedback we give to users that something is happening, and with the single request model, we'll postpone that until we get single request response. I'd be interested to know for how long we'll delay that feedback in the new model.
Thanks Pedro, PTAL. https://codereview.chromium.org/1736203002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java (right): https://codereview.chromium.org/1736203002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:389: public void onSearchResultsViewable(boolean didResolve) { On 2016/02/26 22:30:09, pedrosimonetti wrote: > Does this mean the results were viewed, or that they simple could have been > viewed (because they were preteched)? > > Assuming the latter, wouldn't it be easier to understand if this was called > onSearchResultsPrefetched()? Humm, onSearchResultsPrefetched seems confusing too because we when we just did the prefetch loading. How about onPrefetchedSearchNavigated? This ties it in with the navigation event and that it should only be called for prefetched requests. https://codereview.chromium.org/1736203002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelMetrics.java (right): https://codereview.chromium.org/1736203002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelMetrics.java:190: public void onSearchTermResolved() { On 2016/02/26 22:30:09, pedrosimonetti wrote: > Nit: JavaDoc Done. https://codereview.chromium.org/1736203002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelMetrics.java:195: public void onSearchResultsViewable(boolean didResolve) { On 2016/02/26 22:30:09, pedrosimonetti wrote: > Nit: JavaDoc Done. https://codereview.chromium.org/1736203002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java (right): https://codereview.chromium.org/1736203002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java:712: public static void logSearchTermResolution(long durationMs) { On 2016/02/26 22:30:09, pedrosimonetti wrote: > Nit: logSearchTermResolution --> logSearchTermResolutionDuration? Done. https://codereview.chromium.org/1736203002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java:725: : "Search.ContextualSearchLiteralResultsViewableDuration"; On 2016/02/26 22:30:10, pedrosimonetti wrote: > I thought "literal" / verbatim results were never prefetched. Isn't this true? > If that's true, then passing the didResolve boolean is not necessary. No, with the opt-out on HTTPS pages we usually do a literal search but we want to prefetch the results for a quick-viewing experience. https://codereview.chromium.org/1736203002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java:731: * Logs whether the promo was seen. On 2016/02/26 22:30:09, pedrosimonetti wrote: > Shouldn't we also log the time it takes to establish a selection? Currently, the > selection is the first feedback we give to users that something is happening, > and with the single request model, we'll postpone that until we get single > request response. I'd be interested to know for how long we'll delay that > feedback in the new model. Are you asking about the initial selection that happens when you tap, or the time until we extend the selection to the search term? The former is not affected by Single-request. The latter should be exactly the same as the resolve timing, because the selection adjustment is computed from the resolved term. Am I missing something?
lgtm https://codereview.chromium.org/1736203002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java (right): https://codereview.chromium.org/1736203002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:389: public void onSearchResultsViewable(boolean didResolve) { On 2016/02/27 00:18:39, Donn Denman wrote: > On 2016/02/26 22:30:09, pedrosimonetti wrote: > > Does this mean the results were viewed, or that they simple could have been > > viewed (because they were preteched)? > > > > Assuming the latter, wouldn't it be easier to understand if this was called > > onSearchResultsPrefetched()? > > Humm, onSearchResultsPrefetched seems confusing too because we when we just did > the prefetch loading. How about onPrefetchedSearchNavigated? This ties it in > with the navigation event and that it should only be called for prefetched > requests. Good point. onPrefetchedSearchNavigated() seems good to me. Clearly more intelligible than onSearchResultsViewable() https://codereview.chromium.org/1736203002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java (right): https://codereview.chromium.org/1736203002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java:725: : "Search.ContextualSearchLiteralResultsViewableDuration"; On 2016/02/27 00:18:39, Donn Denman wrote: > On 2016/02/26 22:30:10, pedrosimonetti wrote: > > I thought "literal" / verbatim results were never prefetched. Isn't this true? > > If that's true, then passing the didResolve boolean is not necessary. > > No, with the opt-out on HTTPS pages we usually do a literal search but we want > to prefetch the results for a quick-viewing experience. I see. In this case, nevermind. https://codereview.chromium.org/1736203002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java:731: * Logs whether the promo was seen. On 2016/02/27 00:18:39, Donn Denman wrote: > On 2016/02/26 22:30:09, pedrosimonetti wrote: > > Shouldn't we also log the time it takes to establish a selection? Currently, > the > > selection is the first feedback we give to users that something is happening, > > and with the single request model, we'll postpone that until we get single > > request response. I'd be interested to know for how long we'll delay that > > feedback in the new model. > > Are you asking about the initial selection that happens when you tap, or the > time until we extend the selection to the search term? The former is not > affected by Single-request. The latter should be exactly the same as the > resolve timing, because the selection adjustment is computed from the resolved > term. Am I missing something? Yes, I'm asking about the initial selection. I thought we were considering not doing the initial selection, so we could prevent it from triggering a search depending on the server's response.
https://codereview.chromium.org/1736203002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java (right): https://codereview.chromium.org/1736203002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java:731: * Logs whether the promo was seen. On 2016/02/27 00:39:04, pedrosimonetti wrote: > On 2016/02/27 00:18:39, Donn Denman wrote: > > On 2016/02/26 22:30:09, pedrosimonetti wrote: > > > Shouldn't we also log the time it takes to establish a selection? Currently, > > the > > > selection is the first feedback we give to users that something is > happening, > > > and with the single request model, we'll postpone that until we get single > > > request response. I'd be interested to know for how long we'll delay that > > > feedback in the new model. > > > > Are you asking about the initial selection that happens when you tap, or the > > time until we extend the selection to the search term? The former is not > > affected by Single-request. The latter should be exactly the same as the > > resolve timing, because the selection adjustment is computed from the resolved > > term. Am I missing something? > > Yes, I'm asking about the initial selection. I thought we were considering not > doing the initial selection, so we could prevent it from triggering a search > depending on the server's response. The idea of not establishing an initial selection is part of the "New Tap Feedback", which still has not been prototyped, so we're a long way from building that. No harm checking though!
donnd@chromium.org changed reviewers: + twellington@chromium.org
Theresa, PTAL. Thanks!
donnd@chromium.org changed reviewers: + rkaplow@chromium.org
Robert, PTAL at histograms.xml for OWNERS. Thanks!
https://codereview.chromium.org/1736203002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java (right): https://codereview.chromium.org/1736203002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:386: * Notifies the panel that navigation to prefetched Search Results are now viewable. I was confused about what "navigation" meant until reading the docstrings in ContextualSearchUma. Initially I was thinking this should read "Notifies the panel that prefetched Search Results are now viewable", but it looks like this method isn't notifying the panel, it's notifying panel metrics. Maybe something like "Called after the panel has navigated to prefetched Search Results" or something like that. In the same vein, I think the method name is confusing. onNavigateToPrefetchedSearch() makes more sense to me or onPanelNavigatedToPrefetchedSearch() https://codereview.chromium.org/1736203002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelMetrics.java (right): https://codereview.chromium.org/1736203002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelMetrics.java:196: long durationMs = (System.nanoTime() - mSearchViewStartTimeNs) / 1000000; mSearchViewStartTimeNs is set in onSeachPanelFirstView() which only gets called when exiting from peeked. I think we may want mSearchStartTimeNs, which is set to System.nanoTime() if a new search is starting. https://codereview.chromium.org/1736203002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelMetrics.java:202: * Currently this just logs time-till-navigated into the uma metrics. I think the wording on this docstring and "time-till-navigated" is confusing too. It might be more clear if the comment said something about the panel navigating to search results resulting in the search results being viewable. Initially I was wondering what/who navigated to the search results. https://codereview.chromium.org/1736203002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java (right): https://codereview.chromium.org/1736203002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java:709: * Logs the duration from starting a search until the Search Term is Resolved. Capitalization for some words is inconsistent between files. e.g. in ContextualSearchPanelMetrics "search term" and "resolved" are lower case. I could see an argument for "search term" being a proper noun in the Contextual Search context, but in general English the phrase is not capitalized. https://codereview.chromium.org/1736203002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java:719: * and they start becoming viewable. Should be called only for searches that are prefetched. This docstring is really clear!
lgtm histogram part lgtm
Description was changed from ========== [Contextual Search] Add metrics for Resolve and View timing Add metrics to record the time between starting a contextual search and the Resolve completing. Also add metrics for the time between starting a contextual search and the results being viewable in the overlay. BUG=526291 ========== to ========== [Contextual Search] Add metrics for Resolve and View timing Add metrics to record the time between starting a contextual search and the Resolve completing. Add metrics for the time between starting a contextual search and the results being viewable in the overlay. Also do some minor code cleanup: Rename some variables used for timing that had ambiguous names, remove an unused method. BUG=526291 ==========
Theresa and/or Pedro, PTAL, this needed some rework. Theresa pointed out that I was using the wrong timer, so some rework was needed. I also decided to clean up the current timer names, since they were confusing to me. https://codereview.chromium.org/1736203002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java (right): https://codereview.chromium.org/1736203002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:386: * Notifies the panel that navigation to prefetched Search Results are now viewable. On 2016/02/29 19:42:44, Theresa Wellington wrote: > I was confused about what "navigation" meant until reading the docstrings in > ContextualSearchUma. > > Initially I was thinking this should read "Notifies the panel that prefetched > Search Results are now viewable", but it looks like this method isn't notifying > the panel, it's notifying panel metrics. Maybe something like "Called after the > panel has navigated to prefetched Search Results" or something like that. > > In the same vein, I think the method name is confusing. > onNavigateToPrefetchedSearch() makes more sense to me or > onPanelNavigatedToPrefetchedSearch() OK, I adopted your suggestions for "onPanelNavigatedToPrefetchedSearch" as the method name and updated the description as you suggested. https://codereview.chromium.org/1736203002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelMetrics.java (right): https://codereview.chromium.org/1736203002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelMetrics.java:196: long durationMs = (System.nanoTime() - mSearchViewStartTimeNs) / 1000000; On 2016/02/29 19:42:44, Theresa Wellington wrote: > mSearchViewStartTimeNs is set in onSeachPanelFirstView() which only gets called > when exiting from peeked. I think we may want mSearchStartTimeNs, which is set > to System.nanoTime() if a new search is starting. Wow, that was a big mistake. Thanks for catching that! https://codereview.chromium.org/1736203002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelMetrics.java:202: * Currently this just logs time-till-navigated into the uma metrics. On 2016/02/29 19:42:44, Theresa Wellington wrote: > I think the wording on this docstring and "time-till-navigated" is confusing > too. It might be more clear if the comment said something about the panel > navigating to search results resulting in the search results being viewable. > Initially I was wondering what/who navigated to the search results. Done. https://codereview.chromium.org/1736203002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java (right): https://codereview.chromium.org/1736203002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java:709: * Logs the duration from starting a search until the Search Term is Resolved. On 2016/02/29 19:42:44, Theresa Wellington wrote: > Capitalization for some words is inconsistent between files. e.g. in > ContextualSearchPanelMetrics "search term" and "resolved" are lower case. I > could see an argument for "search term" being a proper noun in the Contextual > Search context, but in general English the phrase is not capitalized. Thanks for noticing this inconsistency. I think we should leave "Search Term" and "Search Results" capitalized since they have special meaning for us. Lowered "resolved" here since that's just normal English. https://codereview.chromium.org/1736203002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java:719: * and they start becoming viewable. Should be called only for searches that are prefetched. On 2016/02/29 19:42:44, Theresa Wellington wrote: > This docstring is really clear! Acknowledged.
lgtm The new timer names are great! https://codereview.chromium.org/1736203002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java (right): https://codereview.chromium.org/1736203002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java:709: * Logs the duration from starting a search until the Search Term is Resolved. On 2016/03/01 21:18:07, Donn Denman wrote: > On 2016/02/29 19:42:44, Theresa Wellington wrote: > > Capitalization for some words is inconsistent between files. e.g. in > > ContextualSearchPanelMetrics "search term" and "resolved" are lower case. I > > could see an argument for "search term" being a proper noun in the Contextual > > Search context, but in general English the phrase is not capitalized. > > Thanks for noticing this inconsistency. > > I think we should leave "Search Term" and "Search Results" capitalized since > they have special meaning for us. Lowered "resolved" here since that's just > normal English. sg :)
The CQ bit was checked by donnd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rkaplow@chromium.org, pedrosimonetti@chromium.org Link to the patchset: https://codereview.chromium.org/1736203002/#ps40001 (title: "Switched to using a separate timer.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1736203002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1736203002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by donnd@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1736203002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1736203002/40001
Message was sent while issue was closed.
Description was changed from ========== [Contextual Search] Add metrics for Resolve and View timing Add metrics to record the time between starting a contextual search and the Resolve completing. Add metrics for the time between starting a contextual search and the results being viewable in the overlay. Also do some minor code cleanup: Rename some variables used for timing that had ambiguous names, remove an unused method. BUG=526291 ========== to ========== [Contextual Search] Add metrics for Resolve and View timing Add metrics to record the time between starting a contextual search and the Resolve completing. Add metrics for the time between starting a contextual search and the results being viewable in the overlay. Also do some minor code cleanup: Rename some variables used for timing that had ambiguous names, remove an unused method. BUG=526291 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [Contextual Search] Add metrics for Resolve and View timing Add metrics to record the time between starting a contextual search and the Resolve completing. Add metrics for the time between starting a contextual search and the results being viewable in the overlay. Also do some minor code cleanup: Rename some variables used for timing that had ambiguous names, remove an unused method. BUG=526291 ========== to ========== [Contextual Search] Add metrics for Resolve and View timing Add metrics to record the time between starting a contextual search and the Resolve completing. Add metrics for the time between starting a contextual search and the results being viewable in the overlay. Also do some minor code cleanup: Rename some variables used for timing that had ambiguous names, remove an unused method. BUG=526291 Committed: https://crrev.com/8fe933a0ea4f518cffa061009985d814e8797b21 Cr-Commit-Position: refs/heads/master@{#378640} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/8fe933a0ea4f518cffa061009985d814e8797b21 Cr-Commit-Position: refs/heads/master@{#378640} |