Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2882693002:
Custom Tabs: add speculation status histograms. (Closed)
DescriptionCustom Tabs: add speculation status histograms.
Add status histograms for speculation in custom tabs, recording the
status of speculation at start time and if the speculation is later
swapped in. The histograms added are CustomTabs.SpeculationStatusOnStart
and CustomTabs.SpeculationStatusOnSwap.
BUG=710720
Review-Url: https://codereview.chromium.org/2882693002
Cr-Commit-Position: refs/heads/master@{#477628}
Committed: https://chromium.googlesource.com/chromium/src/+/e91423967c739606e96cd4120c91a49be697bbca
Patch Set 1 #
Total comments: 6
Patch Set 2 : mor enums #
Total comments: 2
Patch Set 3 : data reduction name tweak #
Total comments: 7
Patch Set 4 : split histos #
Total comments: 16
Patch Set 5 : histogram comments #
Total comments: 6
Patch Set 6 : comments #Patch Set 7 : rebase #
Total comments: 16
Patch Set 8 : comments #Patch Set 9 : comments #
Total comments: 4
Patch Set 10 : comments #Patch Set 11 : rebase problem #
Messages
Total messages: 44 (16 generated)
mattcary@chromium.org changed reviewers: + lizeb@chromium.org, pasko@chromium.org
Follow-on to the previous cl.
Description was changed from ========== Custom Tabs: add speculation abort reason histograms. This adds the CustomTabs.SpeculationAborted enumeration histogram and updates it appropriately for speculative URL launching in CustomTabsConnection. ========== to ========== Custom Tabs: add speculation abort reason histograms. This adds the CustomTabs.SpeculationAborted enumeration histogram and updates it appropriately for speculative URL launching in CustomTabsConnection. BUG=710720 ==========
thanks! looks potentially useful, but it makes me want to request more :) see comments https://codereview.chromium.org/2882693002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (right): https://codereview.chromium.org/2882693002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:964: RecordHistogram.recordEnumeratedHistogram("CustomTabs.SpeculationAborted", recording successes would be cheap, potentially useful, would avoid repeating the string and the call to RecordHistogram - hence less error prone. like: boolean maySpeculate(CustomTabsSessionToken session) { int speculation_result = maySpeculateImplWithResult(session); RecordHistogram.recordEnumeratedHistogram("CustomTabs.MaySpeculateResult", speculation_result, SPECULATION_ABORTED_MAX); if (speculation_result == SPECULATION_ALLOWED) return true; return false; } https://codereview.chromium.org/2882693002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:977: if (!prefs.getNetworkPredictionEnabled()) return false; not recording this reason? why? https://codereview.chromium.org/2882693002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:978: if (DataReductionProxySettings.getInstance().isDataReductionProxyEnabled()) return false; ditto
https://codereview.chromium.org/2882693002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (right): https://codereview.chromium.org/2882693002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:964: RecordHistogram.recordEnumeratedHistogram("CustomTabs.SpeculationAborted", On 2017/05/12 16:19:32, pasko wrote: > recording successes would be cheap, potentially useful, would avoid repeating > the string and the call to RecordHistogram - hence less error prone. > > like: > > boolean maySpeculate(CustomTabsSessionToken session) { > int speculation_result = maySpeculateImplWithResult(session); > RecordHistogram.recordEnumeratedHistogram("CustomTabs.MaySpeculateResult", > speculation_result, SPECULATION_ABORTED_MAX); > if (speculation_result == SPECULATION_ALLOWED) return true; > return false; > } > Great point, that makes analysis a lot easier as well to have all relevant numbers in the same histogram. I implemented a slightly expanded version of your idea, LMK. Note that I did not count if a background tab was requested, but the feature was not enabled. Also note that the counts to the histogram are in some cases overlapping. It seemed to me better to get a single histogram with complete information, than several related histograms that each partitioned the space. I also only added prerender status that seemed directly comparable to that of background tab. There's probably lots of room for discussion there as well. https://codereview.chromium.org/2882693002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:977: if (!prefs.getNetworkPredictionEnabled()) return false; On 2017/05/12 16:19:32, pasko wrote: > not recording this reason? why? Done. https://codereview.chromium.org/2882693002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:978: if (DataReductionProxySettings.getInstance().isDataReductionProxyEnabled()) return false; On 2017/05/12 16:19:32, pasko wrote: > ditto Done.
lgtm, thanks! https://codereview.chromium.org/2882693002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (right): https://codereview.chromium.org/2882693002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:105: private static final int SPECULATION_STATUS_ABORTED_DATA_REDUCTION_DISABLED = 12; nit: Data reduction enabled? Both are correct I guess, as the speculation is disabled due to data reduction being enabled, but I was a bit surprised reading this name.
https://codereview.chromium.org/2882693002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (right): https://codereview.chromium.org/2882693002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:105: private static final int SPECULATION_STATUS_ABORTED_DATA_REDUCTION_DISABLED = 12; On 2017/05/15 09:20:33, Benoit L wrote: > nit: Data reduction enabled? > Both are correct I guess, as the speculation is disabled due to data reduction > being enabled, but I was a bit surprised reading this name. Arg, thanks. Too many negations to resolve. I guess my nesting limit is.. er.. 1.
Please separate into 2 histograms to better reflect the time of recording in each. See the comment below. https://codereview.chromium.org/2882693002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (right): https://codereview.chromium.org/2882693002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:91: private static final int SPECULATION_STATUS_BACKGROUND_TAB = 3; Putting these into the same histogram makes it hard for interpretation because: 1. need to sum up a select of buckets to see discrepancy caused by browser kills between allowing and taking. 2. it is not clear about the time of histogram recording. Describing when each histogram is recorded is very important for histogram maintenance, otherwise there are a lot of confusions and mis-interpretation of the data, we need to keep these descriptions short and somewhat intuitive. For the reasons above I am leaning towards having two histograms: 1. CustomTabs.SpeculationStatusOnStart SPECULATION_STATUS_ALLOWED (not necessary since it is PREFETCH+PRERENDER+BACKGROUND_TAB) PREFETCH PRERENDER BACKGROUND_TAB ABORTED_DEVICE_CLASS ABORTED_BLOCK_3RD_PARTY_COOKIES NETWORK_PREDICTION_DISABLED // nit: s/NETWORK/ABORTED_NETWORK/ DATA_REDUCTION_ENABLED // nit: s/DATA/ABORTED_DATA/ ABORTED_NETWORK_METERED 2. CustomTabs.SpeculationStatusOnSwap remaining Having ALLOWED seems more readable, and the overhead is just one bucket, so let's have it for these reasons. Having to sum up 3 buckets on the dashboard to get the number is not hard, this option would have been okay as well (modulo a longer description and knowing what to sum up). Note: we did not include prerender timeouts and other FinalStatus-es, so the total counts would not match easily with those on the Prerender level. Seems like not a big problem, just wanted to bring it in case you have thoughts on why it may complicate interpretation. mattcary@ wrote: > It seemed to me better to get a single histogram with complete information, > than several related histograms that each partitioned the space. Clear separation of the _time_ when the histogram is recorded is critically important. Having 2 histograms does not partition too much, one can observe them in one dashboard page, with almost the same UX as one histogram, but with extra good separation in naming and clearer descriptions. Please do. https://codereview.chromium.org/2882693002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:645: RecordHistogram.recordEnumeratedHistogram(SPECULATION_STATUS_TAG, it would be nice to shorten this common call as something like RecordSpeculationStatus(SPECULATION_STATUS_PRERENDER_TAKEN). This would be shorter and less error prone with the use of TAG and MAX. https://codereview.chromium.org/2882693002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:998: // TODO(yusufo): The check for prerender in PrivacyManager now checks for the network I think Yusuf wanted to say 'PrivacyPreferencesManager', please change it this way
Thanks, implementing your suggests. One minor question below while I do that. https://codereview.chromium.org/2882693002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (right): https://codereview.chromium.org/2882693002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:91: private static final int SPECULATION_STATUS_BACKGROUND_TAB = 3; > 1. need to sum up a select of buckets to see discrepancy caused by browser kills > between allowing and taking. > What exactly do you mean here? Can you provide an example?
https://codereview.chromium.org/2882693002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (right): https://codereview.chromium.org/2882693002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:91: private static final int SPECULATION_STATUS_BACKGROUND_TAB = 3; On 2017/05/15 13:47:19, mattcary wrote: > > 1. need to sum up a select of buckets to see discrepancy caused by browser > kills > > between allowing and taking. > > > What exactly do you mean here? Can you provide an example? we have two potentially distant moments in time: 1. the call to maySpeculate() 2. the call to takePrerender() // or its analogy for other types if the browser were never not killed in between those, we could say 'hey, the count(maySpeculate) - count(takePrerender) associates with renderer OOM / timeouts, other FinalStatus-es', which we would then be able to split by reason using flavors of Prerender.FinalStatus. But we would have a discrepancy when (1) happens, but (2) does not in quite a few cases .. like chrome going to bg, being killed there, having time to successfully flush the uma logs with info from (1), but loosing either the logs from (2) or actually not executing it. Now given that we have two separate histograms for the 2 moments, we would have a good way to tell how many maySpeculate()s did not follow up with a takePrerender(). Neat?
On 2017/05/15 15:26:26, pasko wrote: > https://codereview.chromium.org/2882693002/diff/40001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java > (right): > > https://codereview.chromium.org/2882693002/diff/40001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:91: > private static final int SPECULATION_STATUS_BACKGROUND_TAB = 3; > On 2017/05/15 13:47:19, mattcary wrote: > > > 1. need to sum up a select of buckets to see discrepancy caused by browser > > kills > > > between allowing and taking. > > > > > What exactly do you mean here? Can you provide an example? > > we have two potentially distant moments in time: > 1. the call to maySpeculate() > 2. the call to takePrerender() // or its analogy for other types > > if the browser were never not killed in between those, we could say 'hey, the > count(maySpeculate) - count(takePrerender) associates with renderer OOM / > timeouts, other FinalStatus-es', which we would then be able to split by reason > using flavors of Prerender.FinalStatus. But we would have a discrepancy when (1) > happens, but (2) does not in quite a few cases .. like chrome going to bg, being > killed there, having time to successfully flush the uma logs with info from (1), > but loosing either the logs from (2) or actually not executing it. > > Now given that we have two separate histograms for the 2 moments, we would have > a good way to tell how many maySpeculate()s did not follow up with a > takePrerender(). Neat? By looking at the total counts of the two histograms? That would be easier than summing up the appropriate enum values, I agree. But I'm not getting something: what is the difference in having one vs two histograms in counting when there is a crash between (1) and (2)? Wouldn't chrome flush (or not be able to flush) the uma logs from (1) vs loosing them from (2), whether or not the histograms were the same or not? I'm not actually arguing with the idea of splitting into two, there's several good reasons to do that. I'm just trying to understand the subtleties of UMA collection.
On 2017/05/15 15:51:28, mattcary wrote: > On 2017/05/15 15:26:26, pasko wrote: > > Now given that we have two separate histograms for the 2 moments, we would > > have a good way to tell how many maySpeculate()s did not follow up with a > > takePrerender(). Neat? > > By looking at the total counts of the two histograms? That would be easier than > summing up the appropriate enum values, I agree. > > But I'm not getting something: what is the difference in having one vs two > histograms in counting when there is a crash between (1) and (2)? Wouldn't > chrome flush (or not be able to flush) the uma logs from (1) vs loosing them > from (2), whether or not the histograms were the same or not? > > I'm not actually arguing with the idea of splitting into two, there's several > good reasons to do that. I'm just trying to understand the subtleties of UMA > collection. To the best of my knowledge there is no difference between having 1 and 2 histograms wrt to how much data we'd loose due to kills/crashes. This argument is only about being able to compare counts and concluding that, for example, background kills are-or-are-not a significant factor skewing the takePrerender kinds.
Split the histos, PTAL. https://codereview.chromium.org/2882693002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (right): https://codereview.chromium.org/2882693002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:645: RecordHistogram.recordEnumeratedHistogram(SPECULATION_STATUS_TAG, On 2017/05/15 12:56:14, pasko wrote: > it would be nice to shorten this common call as something like > RecordSpeculationStatus(SPECULATION_STATUS_PRERENDER_TAKEN). This would be > shorter and less error prone with the use of TAG and MAX. Done. https://codereview.chromium.org/2882693002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:998: // TODO(yusufo): The check for prerender in PrivacyManager now checks for the network On 2017/05/15 12:56:14, pasko wrote: > I think Yusuf wanted to say 'PrivacyPreferencesManager', please change it this > way Done.
code looks good, asking for more verbosity in the dashboard descriptions https://codereview.chromium.org/2882693002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (right): https://codereview.chromium.org/2882693002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:90: private static final int SPECULATION_STATUS_ON_START_BACKGROUND_TAB = 4; nit: can we swap the order of PRERENDER_NOT_STARTED and BACKGROUND_TAB? The latter starts a prerender and the former is close to the meaning of the aborted series (though we do not have the visibility into why it was aborted" https://codereview.chromium.org/2882693002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/enums.xml (right): https://codereview.chromium.org/2882693002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/enums.xml:6434: +<enum name="CustomTabsSpeculationStatusOnStart" type="int"> let me suggest fuller descriptions below. I am not firmly standing behind them, those are up to you and the UMA reviewer. https://codereview.chromium.org/2882693002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/enums.xml:6436: + <int value="1" label="Prefetch"/> Allowed by CustomTabs and is a prefetch kind https://codereview.chromium.org/2882693002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/enums.xml:6437: + <int value="2" label="Prerender"/> Allowed by CustomTabs and is a prerender kind https://codereview.chromium.org/2882693002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/enums.xml:6438: + <int value="3" label="Prerender Not Started"/> Allowed by CustomTabs early, but later throttled or improper request was made https://codereview.chromium.org/2882693002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/enums.xml:6439: + <int value="4" label="Background Tab"/> Allowed by CustomTabs and is a request to create a background tab https://codereview.chromium.org/2882693002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2882693002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:9814: + Android: How a speculation was started or why it was aborted. CustomTabs is Android-only, so there is no extra bit of information. We should document it more verbosely, as this text will be seen on the dashboard. I've left a few length increases in enums.xml. Here we should mention something like: "Recorded when a loading speculation is requested from the CustomTabs service." and also a few words about "Allowed" being a sum of X, Y and Z. https://codereview.chromium.org/2882693002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:9822: + Android: What happened when a speculation was attempted to be swapped. Same as above, please write explicitly "Recorded when ..."
https://codereview.chromium.org/2882693002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (right): https://codereview.chromium.org/2882693002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:90: private static final int SPECULATION_STATUS_ON_START_BACKGROUND_TAB = 4; On 2017/05/16 14:35:50, pasko wrote: > nit: can we swap the order of PRERENDER_NOT_STARTED and BACKGROUND_TAB? The > latter starts a prerender and the former is close to the meaning of the aborted > series (though we do not have the visibility into why it was aborted" Sure, although personally I find it confusing to have the prerender statuses spread around. https://codereview.chromium.org/2882693002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/enums.xml (right): https://codereview.chromium.org/2882693002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/enums.xml:6434: +<enum name="CustomTabsSpeculationStatusOnStart" type="int"> On 2017/05/16 14:35:50, pasko wrote: > let me suggest fuller descriptions below. I am not firmly standing behind them, > those are up to you and the UMA reviewer. Done, see what you think of my interpretation of your comments. https://codereview.chromium.org/2882693002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/enums.xml:6436: + <int value="1" label="Prefetch"/> On 2017/05/16 14:35:50, pasko wrote: > Allowed by CustomTabs and is a prefetch kind Done. https://codereview.chromium.org/2882693002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/enums.xml:6437: + <int value="2" label="Prerender"/> On 2017/05/16 14:35:50, pasko wrote: > Allowed by CustomTabs and is a prerender kind Done. https://codereview.chromium.org/2882693002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/enums.xml:6438: + <int value="3" label="Prerender Not Started"/> On 2017/05/16 14:35:50, pasko wrote: > Allowed by CustomTabs early, but later throttled or improper request was made Done. https://codereview.chromium.org/2882693002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/enums.xml:6439: + <int value="4" label="Background Tab"/> On 2017/05/16 14:35:50, pasko wrote: > Allowed by CustomTabs and is a request to create a background tab Done. https://codereview.chromium.org/2882693002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2882693002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:9814: + Android: How a speculation was started or why it was aborted. On 2017/05/16 14:35:50, pasko wrote: > CustomTabs is Android-only, so there is no extra bit of information. > The other histograms mention android, so I was being consistent. Also, not everyone may know that custom tabs implies android, so it seems useful even if it's redundant. > We should document it more verbosely, as this text will be seen on the > dashboard. I've left a few length increases in enums.xml. Here we should mention > something like: > > "Recorded when a loading speculation is requested from the CustomTabs service." > and also a few words about "Allowed" being a sum of X, Y and Z. Done. https://codereview.chromium.org/2882693002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:9822: + Android: What happened when a speculation was attempted to be swapped. On 2017/05/16 14:35:50, pasko wrote: > Same as above, please write explicitly "Recorded when ..." Done.
still lgtm, thanks. https://codereview.chromium.org/2882693002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (right): https://codereview.chromium.org/2882693002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:1200: private void recordSpeculationStatusOnStart(int status) { nit: static? https://codereview.chromium.org/2882693002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:1205: private void recordSpeculationStatusOnSwap(int status) { nit: static? https://codereview.chromium.org/2882693002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/enums.xml (right): https://codereview.chromium.org/2882693002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/enums.xml:6438: + <int value="1" label="Prefetch kind of speculation started"/> This is not NoState Prefetch, right?
https://codereview.chromium.org/2882693002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (right): https://codereview.chromium.org/2882693002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:1200: private void recordSpeculationStatusOnStart(int status) { On 2017/05/22 12:44:01, Benoit L wrote: > nit: static? Done. https://codereview.chromium.org/2882693002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:1205: private void recordSpeculationStatusOnSwap(int status) { On 2017/05/22 12:44:01, Benoit L wrote: > nit: static? Done. https://codereview.chromium.org/2882693002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/enums.xml (right): https://codereview.chromium.org/2882693002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/enums.xml:6438: + <int value="1" label="Prefetch kind of speculation started"/> On 2017/05/22 12:44:02, Benoit L wrote: > This is not NoState Prefetch, right? I'm pretty sure it's spec prefetch (it leads to resource_prefetch_predictor). But mostly I just wanted to count the codepath.
The CQ bit was checked by mattcary@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lizeb@chromium.org Link to the patchset: https://codereview.chromium.org/2882693002/#ps100001 (title: "comments")
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
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) 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_...)
mattcary@chromium.org changed reviewers: + mpearson@google.com
+mpearson for histograms
mpearson@chromium.org changed reviewers: + mpearson@chromium.org
By the way, the changelist description only bears limits resemblance to the code here. --mark https://codereview.chromium.org/2882693002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/enums.xml (right): https://codereview.chromium.org/2882693002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/enums.xml:6473: + <int value="0" label="Background Tab Taken"/> Taken == swapped-in? https://codereview.chromium.org/2882693002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/enums.xml:6474: + <int value="1" label="Background Tab Not Matched"/> "not matched" == ? no longer needed? https://codereview.chromium.org/2882693002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2882693002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:9967: + and, if allowed, also when the specific kind of speculation is started. The semantics for this seem weird. Can only one type of speculation be started? When do you record "allowed but not started" (upon request, as the description implies)? Will it be started? When are aborted recorded? At the same time (upon request) or later? If at the same time, then why don't you record "requested but not allowed" as opposed "started and aborted"? Please rethink this a bit and get back to me. https://codereview.chromium.org/2882693002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:9976: + into a visible tab. Is this true, that the browser speculation decides when to swap-in, not the browser itself? Is a speculation always eligible to be swapped in? (E.g., what if the user switched tabs; what does a speculation request to swap-in get recorded as?).
Description was changed from ========== Custom Tabs: add speculation abort reason histograms. This adds the CustomTabs.SpeculationAborted enumeration histogram and updates it appropriately for speculative URL launching in CustomTabsConnection. BUG=710720 ========== to ========== Custom Tabs: add speculation status histograms. Add status histograms for speculation in custom tabs, recording the status of speculation at start time and if the speculation is later swapped in. The histograms added are CustomTabs.SpeculationStatusOnStart and CustomTabs.SpeculationStatusOnSwap. BUG=710720 ==========
Description was changed from ========== Custom Tabs: add speculation status histograms. Add status histograms for speculation in custom tabs, recording the status of speculation at start time and if the speculation is later swapped in. The histograms added are CustomTabs.SpeculationStatusOnStart and CustomTabs.SpeculationStatusOnSwap. BUG=710720 ========== to ========== Custom Tabs: add speculation status histograms. Add status histograms for speculation in custom tabs, recording the status of speculation at start time and if the speculation is later swapped in. The histograms added are CustomTabs.SpeculationStatusOnStart and CustomTabs.SpeculationStatusOnSwap. BUG=710720 ==========
Updated the CL description as well, sorry about that. https://codereview.chromium.org/2882693002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/enums.xml (right): https://codereview.chromium.org/2882693002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/enums.xml:6473: + <int value="0" label="Background Tab Taken"/> On 2017/05/30 22:09:53, Mark P wrote: > Taken == swapped-in? Done. https://codereview.chromium.org/2882693002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/enums.xml:6474: + <int value="1" label="Background Tab Not Matched"/> On 2017/05/30 22:09:53, Mark P wrote: > "not matched" == ? > no longer needed? Currently a background tab that's not matched is discarded, but that might not be true in the future. I think the current wording is more accurate and future-proof. https://codereview.chromium.org/2882693002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2882693002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:9967: + and, if allowed, also when the specific kind of speculation is started. On 2017/05/30 22:09:54, Mark P wrote: > The semantics for this seem weird. Can only one type of speculation be started? > When do you record "allowed but not started" (upon request, as the description > implies)? Will it be started? When are aborted recorded? At the same time > (upon request) or later? If at the same time, then why don't you record > "requested but not allowed" as opposed "started and aborted"? > > Please rethink this a bit and get back to me. I agree that the semantics are a bit weird. There are general reasons why speculation can't be done at all (all the "aborted" statuses). For prerender, there are additional edge cases where prerender is not started that are only discovered when the prerender is actually created (see CustomTabsConnection.prerenderUrl). https://codereview.chromium.org/2882693002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:9976: + into a visible tab. On 2017/05/30 22:09:54, Mark P wrote: > Is this true, that the browser speculation decides when to swap-in, not the > browser itself? Is a speculation always eligible to be swapped in? (E.g., what > if the user switched tabs; what does a speculation request to swap-in get > recorded as?). Both. CustomTabs decides to launch a URL, and the current speculation, if any, decides if that URL is a match for it. A speculation is currently only eligible to be swapped in if the URL matches; if it doesn't match we record the "not matched" histograms.
https://codereview.chromium.org/2882693002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/enums.xml (right): https://codereview.chromium.org/2882693002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/enums.xml:6474: + <int value="1" label="Background Tab Not Matched"/> On 2017/05/31 08:15:40, mattcary wrote: > On 2017/05/30 22:09:53, Mark P wrote: > > "not matched" == ? > > no longer needed? > > Currently a background tab that's not matched is discarded, but that might not > be true in the future. I think the current wording is more accurate and > future-proof. I find the meaning of "not matched" unintuitive. What wrong with "not swapped in"? https://codereview.chromium.org/2882693002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2882693002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:9967: + and, if allowed, also when the specific kind of speculation is started. On 2017/05/31 08:15:40, mattcary wrote: > On 2017/05/30 22:09:54, Mark P wrote: > > The semantics for this seem weird. Can only one type of speculation be > started? > > When do you record "allowed but not started" (upon request, as the > description > > implies)? Will it be started? When are aborted recorded? At the same time > > (upon request) or later? If at the same time, then why don't you record > > "requested but not allowed" as opposed "started and aborted"? > > > > Please rethink this a bit and get back to me. > > I agree that the semantics are a bit weird. There are general reasons why > speculation can't be done at all (all the "aborted" statuses). For prerender, > there are additional edge cases where prerender is not started that are only > discovered when the prerender is actually created (see > CustomTabsConnection.prerenderUrl). You answered some of my question but not all, and did not revise the description at all. I understand this is complicated, which is why it's important to write a good description while this is all clear in your mind so your future self and your coworkers can remember the nuances here. In particular, please have the description answer: * Can only one type of speculation be started? * When do you record "allowed but not started" (upon request, as the description implies)? Will it be started? * When are aborted recorded? At the same time (upon request) or later or earlier? * If at the same time, then why don't you record "requested but not allowed" as opposed "started and aborted"? * IF earlier, can it be recorded without a corresponding "started" call? * Does every abort call have an (emitted) started call associated with it? Also, more broadly (answer me, not in the histograms.xml description) is it useful to have a single histogram that lumps together aborts for different kinds of speculations without any way to sort out how common each is for each? I'd think separate aborted histograms would make sense for distinct types of speculations...
Thanks for your comments & review. https://codereview.chromium.org/2882693002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/enums.xml (right): https://codereview.chromium.org/2882693002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/enums.xml:6474: + <int value="1" label="Background Tab Not Matched"/> On 2017/06/01 05:19:35, Mark P wrote: > On 2017/05/31 08:15:40, mattcary wrote: > > On 2017/05/30 22:09:53, Mark P wrote: > > > "not matched" == ? > > > no longer needed? > > > > Currently a background tab that's not matched is discarded, but that might not > > be true in the future. I think the current wording is more accurate and > > future-proof. > > I find the meaning of "not matched" unintuitive. What wrong with "not swapped > in"? There may in the future be multiple reasons for a speculation to not be swapped in; currently not matching is the only reason but in the future there may be more. With the current wording we would not have to change the enum label if more failure-to-swap reasons are added. Let me know if it would be preferable to do it that way. https://codereview.chromium.org/2882693002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2882693002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:9967: + and, if allowed, also when the specific kind of speculation is started. On 2017/06/01 05:19:35, Mark P wrote: > On 2017/05/31 08:15:40, mattcary wrote: > > On 2017/05/30 22:09:54, Mark P wrote: > > > The semantics for this seem weird. Can only one type of speculation be > > started? > > > When do you record "allowed but not started" (upon request, as the > > description > > > implies)? Will it be started? When are aborted recorded? At the same time > > > (upon request) or later? If at the same time, then why don't you record > > > "requested but not allowed" as opposed "started and aborted"? > > > > > > Please rethink this a bit and get back to me. > > > > I agree that the semantics are a bit weird. There are general reasons why > > speculation can't be done at all (all the "aborted" statuses). For prerender, > > there are additional edge cases where prerender is not started that are only > > discovered when the prerender is actually created (see > > CustomTabsConnection.prerenderUrl). > > You answered some of my question but not all, and did not revise the description > at all. I understand this is complicated, which is why it's important to write > a good description while this is all clear in your mind so your future self and > your coworkers can remember the nuances here. In particular, please have the > description answer: > * Can only one type of speculation be started? > * When do you record "allowed but not started" (upon request, as the description > implies)? Will it be started? > * When are aborted recorded? At the same time (upon request) or later or > earlier? > * If at the same time, then why don't you record "requested but not allowed" > as opposed "started and aborted"? > * IF earlier, can it be recorded without a corresponding "started" call? > * Does every abort call have an (emitted) started call associated with it? > > Also, more broadly (answer me, not in the histograms.xml description) is it > useful to have a single histogram that lumps together aborts for different kinds > of speculations without any way to sort out how common each is for each? I'd > think separate aborted histograms would make sense for distinct types of > speculations... Updated, LMK. The reason why these values appear in the same histogram is that their values are only useful relative to all counts: for example, abort counts or start counts relative to total number of speculations allowed. Just a, for example, "abort" histogram isn't informative or actionable by itself. It seems less error-prone to have this information in a single histogram, then to expect users to always query for a particular set of histograms.
metrics xml generally lgtm with some concerns below --mark https://codereview.chromium.org/2882693002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/enums.xml (right): https://codereview.chromium.org/2882693002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/enums.xml:6474: + <int value="1" label="Background Tab Not Matched"/> On 2017/06/06 14:39:23, mattcary wrote: > On 2017/06/01 05:19:35, Mark P wrote: > > On 2017/05/31 08:15:40, mattcary wrote: > > > On 2017/05/30 22:09:53, Mark P wrote: > > > > "not matched" == ? > > > > no longer needed? > > > > > > Currently a background tab that's not matched is discarded, but that might > not > > > be true in the future. I think the current wording is more accurate and > > > future-proof. > > > > I find the meaning of "not matched" unintuitive. What wrong with "not swapped > > in"? > > There may in the future be multiple reasons for a speculation to not be swapped > in; currently not matching is the only reason but in the future there may be > more. With the current wording we would not have to change the enum label if > more failure-to-swap reasons are added. Let me know if it would be preferable to > do it that way. Okay. https://codereview.chromium.org/2882693002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2882693002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:9967: + and, if allowed, also when the specific kind of speculation is started. On 2017/06/06 14:39:23, mattcary wrote: > On 2017/06/01 05:19:35, Mark P wrote: > > On 2017/05/31 08:15:40, mattcary wrote: > > > On 2017/05/30 22:09:54, Mark P wrote: > > > > The semantics for this seem weird. Can only one type of speculation be > > > started? > > > > When do you record "allowed but not started" (upon request, as the > > > description > > > > implies)? Will it be started? When are aborted recorded? At the same > time > > > > (upon request) or later? If at the same time, then why don't you record > > > > "requested but not allowed" as opposed "started and aborted"? > > > > > > > > Please rethink this a bit and get back to me. > > > > > > I agree that the semantics are a bit weird. There are general reasons why > > > speculation can't be done at all (all the "aborted" statuses). For > prerender, > > > there are additional edge cases where prerender is not started that are only > > > discovered when the prerender is actually created (see > > > CustomTabsConnection.prerenderUrl). > > > > You answered some of my question but not all, and did not revise the > description > > at all. I understand this is complicated, which is why it's important to > write > > a good description while this is all clear in your mind so your future self > and > > your coworkers can remember the nuances here. In particular, please have the > > description answer: > > * Can only one type of speculation be started? > > * When do you record "allowed but not started" (upon request, as the > description > > implies)? Will it be started? > > * When are aborted recorded? At the same time (upon request) or later or > > earlier? > > * If at the same time, then why don't you record "requested but not > allowed" > > as opposed "started and aborted"? > > * IF earlier, can it be recorded without a corresponding "started" call? > > * Does every abort call have an (emitted) started call associated with it? > > > > Also, more broadly (answer me, not in the histograms.xml description) is it > > useful to have a single histogram that lumps together aborts for different > kinds > > of speculations without any way to sort out how common each is for each? I'd > > think separate aborted histograms would make sense for distinct types of > > speculations... > > Updated, LMK. Thanks. Left some comments on the latest patchset. > The reason why these values appear in the same histogram is that their values > are only useful relative to all counts: for example, abort counts or start > counts relative to total number of speculations allowed. Just a, for example, > "abort" histogram isn't informative or actionable by itself. It seems less > error-prone to have this information in a single histogram, then to expect users > to always query for a particular set of histograms. Understood about the usefulness of putting all this in one histogram. A thought for the future: perhaps it would be useful to have separate "abort" histograms for the different types of speculations. Maybe the types of requests that trigger, say, prerender speculation might be prohibited for a different set of reasons (more often device type, for example) than the types of requests that trigger prefetch speculations. Maybe those are more often blocked due to, say, data reduction enabled. https://codereview.chromium.org/2882693002/diff/160001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2882693002/diff/160001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:10190: + with either a "Speculation allowed" value, or an aborted value. If nit: "aborted", in my understanding of English, typically means started but cancelled at some point. As I understand this histogram, the value for "aborted" mean "the reason this was not started". Can you instead use a word such as "prohibited" or "prevented" that doesn't imply there it was originally "started"? (Also, revise the enums accordingly.) https://codereview.chromium.org/2882693002/diff/160001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:10194: + started". The latter case is when a prerender is abandoned during nit: latter -> last
https://codereview.chromium.org/2882693002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/enums.xml (right): https://codereview.chromium.org/2882693002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/enums.xml:6474: + <int value="1" label="Background Tab Not Matched"/> On 2017/06/06 18:25:50, Mark P wrote: > On 2017/06/06 14:39:23, mattcary wrote: > > On 2017/06/01 05:19:35, Mark P wrote: > > > On 2017/05/31 08:15:40, mattcary wrote: > > > > On 2017/05/30 22:09:53, Mark P wrote: > > > > > "not matched" == ? > > > > > no longer needed? > > > > > > > > Currently a background tab that's not matched is discarded, but that might > > not > > > > be true in the future. I think the current wording is more accurate and > > > > future-proof. > > > > > > I find the meaning of "not matched" unintuitive. What wrong with "not > swapped > > > in"? > > > > There may in the future be multiple reasons for a speculation to not be > swapped > > in; currently not matching is the only reason but in the future there may be > > more. With the current wording we would not have to change the enum label if > > more failure-to-swap reasons are added. Let me know if it would be preferable > to > > do it that way. > > Okay. Acknowledged. https://codereview.chromium.org/2882693002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2882693002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:9967: + and, if allowed, also when the specific kind of speculation is started. On 2017/06/06 18:25:50, Mark P wrote: > On 2017/06/06 14:39:23, mattcary wrote: > > On 2017/06/01 05:19:35, Mark P wrote: > > > On 2017/05/31 08:15:40, mattcary wrote: > > > > On 2017/05/30 22:09:54, Mark P wrote: > > > > > The semantics for this seem weird. Can only one type of speculation be > > > > started? > > > > > When do you record "allowed but not started" (upon request, as the > > > > description > > > > > implies)? Will it be started? When are aborted recorded? At the same > > time > > > > > (upon request) or later? If at the same time, then why don't you record > > > > > "requested but not allowed" as opposed "started and aborted"? > > > > > > > > > > Please rethink this a bit and get back to me. > > > > > > > > I agree that the semantics are a bit weird. There are general reasons why > > > > speculation can't be done at all (all the "aborted" statuses). For > > prerender, > > > > there are additional edge cases where prerender is not started that are > only > > > > discovered when the prerender is actually created (see > > > > CustomTabsConnection.prerenderUrl). > > > > > > You answered some of my question but not all, and did not revise the > > description > > > at all. I understand this is complicated, which is why it's important to > > write > > > a good description while this is all clear in your mind so your future self > > and > > > your coworkers can remember the nuances here. In particular, please have > the > > > description answer: > > > * Can only one type of speculation be started? > > > * When do you record "allowed but not started" (upon request, as the > > description > > > implies)? Will it be started? > > > * When are aborted recorded? At the same time (upon request) or later or > > > earlier? > > > * If at the same time, then why don't you record "requested but not > > allowed" > > > as opposed "started and aborted"? > > > * IF earlier, can it be recorded without a corresponding "started" call? > > > * Does every abort call have an (emitted) started call associated with it? > > > > > > Also, more broadly (answer me, not in the histograms.xml description) is it > > > useful to have a single histogram that lumps together aborts for different > > kinds > > > of speculations without any way to sort out how common each is for each? > I'd > > > think separate aborted histograms would make sense for distinct types of > > > speculations... > > > > Updated, LMK. > > Thanks. Left some comments on the latest patchset. > > > The reason why these values appear in the same histogram is that their values > > are only useful relative to all counts: for example, abort counts or start > > counts relative to total number of speculations allowed. Just a, for example, > > "abort" histogram isn't informative or actionable by itself. It seems less > > error-prone to have this information in a single histogram, then to expect > users > > to always query for a particular set of histograms. > > Understood about the usefulness of putting all this in one histogram. > > A thought for the future: perhaps it would be useful to have separate "abort" > histograms for the different types of speculations. Maybe the types of requests > that trigger, say, prerender speculation might be prohibited for a different set > of reasons (more often device type, for example) than the types of requests that > trigger prefetch speculations. Maybe those are more often blocked due to, say, > data reduction enabled. That's a good point. For now, the abort is decided independently of the speculation type. https://codereview.chromium.org/2882693002/diff/160001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2882693002/diff/160001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:10190: + with either a "Speculation allowed" value, or an aborted value. If On 2017/06/06 18:25:50, Mark P wrote: > nit: "aborted", in my understanding of English, typically means started but > cancelled at some point. As I understand this histogram, the value for > "aborted" mean "the reason this was not started". Can you instead use a word > such as "prohibited" or "prevented" that doesn't imply there it was originally > "started"? > > (Also, revise the enums accordingly.) Done. https://codereview.chromium.org/2882693002/diff/160001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:10194: + started". The latter case is when a prerender is abandoned during On 2017/06/06 18:25:50, Mark P wrote: > nit: latter -> last Done.
The CQ bit was checked by mattcary@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lizeb@chromium.org, mpearson@chromium.org Link to the patchset: https://codereview.chromium.org/2882693002/#ps180001 (title: "comments")
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
Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
The CQ bit was checked by mattcary@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lizeb@chromium.org, mpearson@chromium.org Link to the patchset: https://codereview.chromium.org/2882693002/#ps200001 (title: "rebase problem")
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": 200001, "attempt_start_ts": 1496838732057080,
"parent_rev": "c563b1bc5cf4e536ddb2a06504cca698fa7a986b", "commit_rev":
"e91423967c739606e96cd4120c91a49be697bbca"}
Message was sent while issue was closed.
Description was changed from ========== Custom Tabs: add speculation status histograms. Add status histograms for speculation in custom tabs, recording the status of speculation at start time and if the speculation is later swapped in. The histograms added are CustomTabs.SpeculationStatusOnStart and CustomTabs.SpeculationStatusOnSwap. BUG=710720 ========== to ========== Custom Tabs: add speculation status histograms. Add status histograms for speculation in custom tabs, recording the status of speculation at start time and if the speculation is later swapped in. The histograms added are CustomTabs.SpeculationStatusOnStart and CustomTabs.SpeculationStatusOnSwap. BUG=710720 Review-Url: https://codereview.chromium.org/2882693002 Cr-Commit-Position: refs/heads/master@{#477628} Committed: https://chromium.googlesource.com/chromium/src/+/e91423967c739606e96cd4120c91... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://chromium.googlesource.com/chromium/src/+/e91423967c739606e96cd4120c91... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
