Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1309143004:
Add UMA metrics to record the size of internal memory available on a user's device when the Update … (Closed)
DescriptionAdd UMA metrics to record the size of internal memory available on a user's device when the Update InfoBar is shown.
BUG=517839
Committed: https://crrev.com/0e93d6cbb4d81de7e204f5c36cd450f9a1804974
Cr-Commit-Position: refs/heads/master@{#346409}
Patch Set 1 #
Total comments: 3
Patch Set 2 : #
Total comments: 2
Patch Set 3 : Update APIs used. #Patch Set 4 : Add conditional use of deprecated API. #
Total comments: 4
Patch Set 5 : Updating Statfs function used. #Patch Set 6 : Adding RecordLinearHistogram. #
Total comments: 3
Patch Set 7 : Fix indent #
Messages
Total messages: 23 (4 generated)
khushalsagar@chromium.org changed reviewers: + yfriedman@chromium.org
https://codereview.chromium.org/1309143004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaUpdateInfobar.java (right): https://codereview.chromium.org/1309143004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaUpdateInfobar.java:96: // Only record the case where available size is less than 100 MB. Why cap it at 100MB? It will skew what the population looks like. We can have it just report the max bucket.
https://codereview.chromium.org/1309143004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaUpdateInfobar.java (right): https://codereview.chromium.org/1309143004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaUpdateInfobar.java:96: // Only record the case where available size is less than 100 MB. On 2015/08/27 14:32:04, Yaron wrote: > Why cap it at 100MB? It will skew what the population looks like. We can have it > just report the max bucket. I am not sure what to pick as the max value. Users could have as much as 64 GB of free space. Also this will determine the number of buckets we choose. The buckets will be of uniform size but the update size has mostly been less than 60-70 MB in the past, so we'd want more granularity for those buckets to accurately predict whether the update would've failed for a user. For sizes >100 MB we can always be sure that the update would not fail because of insufficient storage.
asvitkine@chromium.org changed reviewers: + asvitkine@chromium.org
https://codereview.chromium.org/1309143004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaUpdateInfobar.java (right): https://codereview.chromium.org/1309143004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaUpdateInfobar.java:96: // Only record the case where available size is less than 100 MB. On 2015/08/27 15:09:01, Khushal wrote: > On 2015/08/27 14:32:04, Yaron wrote: > > Why cap it at 100MB? It will skew what the population looks like. We can have > it > > just report the max bucket. > > I am not sure what to pick as the max value. Users could have as much as 64 GB > of free space. Also this will determine the number of buckets we choose. The > buckets will be of uniform size but the update size has mostly been less than > 60-70 MB in the past, so we'd want more granularity for those buckets to > accurately predict whether the update would've failed for a user. For sizes >100 > MB we can always be sure that the update would not fail because of insufficient > storage. FYI: UMA histograms don't do uniform sized buckets by default. So in fact you'll get an exponential distribution of buckets.
On 2015/08/27 15:15:01, Alexei Svitkine wrote: > https://codereview.chromium.org/1309143004/diff/1/chrome/android/java/src/org... > File > chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaUpdateInfobar.java > (right): > > https://codereview.chromium.org/1309143004/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaUpdateInfobar.java:96: > // Only record the case where available size is less than 100 MB. > On 2015/08/27 15:09:01, Khushal wrote: > > On 2015/08/27 14:32:04, Yaron wrote: > > > Why cap it at 100MB? It will skew what the population looks like. We can > have > > it > > > just report the max bucket. > > > > I am not sure what to pick as the max value. Users could have as much as 64 GB > > of free space. Also this will determine the number of buckets we choose. The > > buckets will be of uniform size but the update size has mostly been less than > > 60-70 MB in the past, so we'd want more granularity for those buckets to > > accurately predict whether the update would've failed for a user. For sizes > >100 > > MB we can always be sure that the update would not fail because of > insufficient > > storage. > > FYI: UMA histograms don't do uniform sized buckets by default. So in fact you'll > get an exponential distribution of buckets. Thanks Alexel. I have changed it to recordCount100Histogram, it fits the case.
https://codereview.chromium.org/1309143004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaUpdateInfobar.java (right): https://codereview.chromium.org/1309143004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaUpdateInfobar.java:96: "GoogleUpdate.InfoBar.InternalStorageSizeAvailable", size); 100MB seems pretty low for this. If you said that 64gb is the max, I'd just use that with a custom counts. (e.g. 64000).
On 2015/08/27 18:21:29, Alexei Svitkine wrote: > https://codereview.chromium.org/1309143004/diff/20001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaUpdateInfobar.java > (right): > > https://codereview.chromium.org/1309143004/diff/20001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaUpdateInfobar.java:96: > "GoogleUpdate.InfoBar.InternalStorageSizeAvailable", size); > 100MB seems pretty low for this. > > If you said that 64gb is the max, I'd just use that with a custom counts. (e.g. > 64000). The max would be the max internal storage on a user's device, which is hard to cap. And since the aim is to know if the user can not update the application due to insufficient storage, we should be able to confidently say that for values above 100 MB this would not be the case. From what I understand this recording would place all cases beyond 100 MB in a single bucket. Would it be better to use that if that's what we are trying to measure?
https://codereview.chromium.org/1309143004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaUpdateInfobar.java (right): https://codereview.chromium.org/1309143004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaUpdateInfobar.java:92: int blockSize = statFs.getBlockSize(); These APIs were deprecated in api level 18. Can you conditionally use the new ones?
On 2015/08/27 18:58:45, Yaron wrote: > https://codereview.chromium.org/1309143004/diff/20001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaUpdateInfobar.java > (right): > > https://codereview.chromium.org/1309143004/diff/20001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaUpdateInfobar.java:92: > int blockSize = statFs.getBlockSize(); > These APIs were deprecated in api level 18. Can you conditionally use the new > ones? Done.
lgtm https://codereview.chromium.org/1309143004/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaUpdateInfobar.java (right): https://codereview.chromium.org/1309143004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaUpdateInfobar.java:108: private int getSizeUpdatedApi(StatFs statFs) { nit: make these both static https://codereview.chromium.org/1309143004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaUpdateInfobar.java:109: long blockSize = statFs.getBlockSizeLong(); just use getAvailableBytes in this case
Done. Thanks Yaron. https://codereview.chromium.org/1309143004/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaUpdateInfobar.java (right): https://codereview.chromium.org/1309143004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaUpdateInfobar.java:108: private int getSizeUpdatedApi(StatFs statFs) { On 2015/08/27 21:53:02, Yaron wrote: > nit: make these both static Done. https://codereview.chromium.org/1309143004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaUpdateInfobar.java:109: long blockSize = statFs.getBlockSizeLong(); On 2015/08/27 21:53:02, Yaron wrote: > just use getAvailableBytes in this case Done.
On 2015/08/27 18:46:16, Khushal wrote: > On 2015/08/27 18:21:29, Alexei Svitkine wrote: > > > https://codereview.chromium.org/1309143004/diff/20001/chrome/android/java/src... > > File > > > chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaUpdateInfobar.java > > (right): > > > > > https://codereview.chromium.org/1309143004/diff/20001/chrome/android/java/src... > > > chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaUpdateInfobar.java:96: > > "GoogleUpdate.InfoBar.InternalStorageSizeAvailable", size); > > 100MB seems pretty low for this. > > > > If you said that 64gb is the max, I'd just use that with a custom counts. > (e.g. > > 64000). > > The max would be the max internal storage on a user's device, which is hard to > cap. And since the aim is to know if the user can not update the application due > to insufficient storage, we should be able to confidently say that for values > above 100 MB this would not be the case. > From what I understand this recording would place all cases beyond 100 MB in a > single bucket. Would it be better to use that if that's what we are trying to > measure? Do you mean "for values above 100 MB the user would be able to update the application"? If so, it seems you're baking in an assumption that the app size will always be <100MB which doesn't seem obvious to me. At the very least, I'd extend the range to 1G if you're trying to measure this. However, if what you're really after is the boolean of whether user has sufficient storage, measuring that separately makes sense to me. (Or do both, so that you get the boolean which you can easily track over time, etc - and the actual break down to inform you what the target APK size should be to aim for.)
On 2015/08/28 15:30:58, Alexei Svitkine wrote: > On 2015/08/27 18:46:16, Khushal wrote: > > On 2015/08/27 18:21:29, Alexei Svitkine wrote: > > > > > > https://codereview.chromium.org/1309143004/diff/20001/chrome/android/java/src... > > > File > > > > > > chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaUpdateInfobar.java > > > (right): > > > > > > > > > https://codereview.chromium.org/1309143004/diff/20001/chrome/android/java/src... > > > > > > chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaUpdateInfobar.java:96: > > > "GoogleUpdate.InfoBar.InternalStorageSizeAvailable", size); > > > 100MB seems pretty low for this. > > > > > > If you said that 64gb is the max, I'd just use that with a custom counts. > > (e.g. > > > 64000). > > > > The max would be the max internal storage on a user's device, which is hard to > > cap. And since the aim is to know if the user can not update the application > due > > to insufficient storage, we should be able to confidently say that for values > > above 100 MB this would not be the case. > > From what I understand this recording would place all cases beyond 100 MB in a > > single bucket. Would it be better to use that if that's what we are trying to > > measure? > > Do you mean "for values above 100 MB the user would be able to update the > application"? If so, it seems you're baking in an assumption that the app size > will always be <100MB which doesn't seem obvious to me. > > At the very least, I'd extend the range to 1G if you're trying to measure this. > > However, if what you're really after is the boolean of whether user has > sufficient storage, measuring that separately makes sense to me. (Or do both, so > that you get the boolean which you can easily track over time, etc - and the > actual break down to inform you what the target APK size should be to aim for.) As discussed, I've added the java function for using a linear histogram for this case with max value of 200M and 100 buckets. Please take a look.
lgtm https://codereview.chromium.org/1309143004/diff/100001/base/android/record_hi... File base/android/record_histogram.cc (right): https://codereview.chromium.org/1309143004/diff/100001/base/android/record_hi... base/android/record_histogram.cc:89: jstring j_histogram_name, nit: fix indentation https://codereview.chromium.org/1309143004/diff/100001/base/android/record_hi... base/android/record_histogram.cc:104: std::string histogram_name = ConvertJavaStringToUTF8(env, j_histogram_name); 80 cols https://codereview.chromium.org/1309143004/diff/100001/base/android/record_hi... base/android/record_histogram.cc:107: HistogramBase::kUmaTargetedHistogramFlag); nit: indentation
On 2015/08/31 13:53:21, Yaron wrote: > lgtm > > https://codereview.chromium.org/1309143004/diff/100001/base/android/record_hi... > File base/android/record_histogram.cc (right): > > https://codereview.chromium.org/1309143004/diff/100001/base/android/record_hi... > base/android/record_histogram.cc:89: jstring j_histogram_name, > nit: fix indentation > > https://codereview.chromium.org/1309143004/diff/100001/base/android/record_hi... > base/android/record_histogram.cc:104: std::string histogram_name = > ConvertJavaStringToUTF8(env, j_histogram_name); > 80 cols > > https://codereview.chromium.org/1309143004/diff/100001/base/android/record_hi... > base/android/record_histogram.cc:107: HistogramBase::kUmaTargetedHistogramFlag); > nit: indentation Done.
lgtm
The CQ bit was checked by khushalsagar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yfriedman@chromium.org Link to the patchset: https://codereview.chromium.org/1309143004/#ps120001 (title: "Fix indent")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1309143004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1309143004/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/0e93d6cbb4d81de7e204f5c36cd450f9a1804974 Cr-Commit-Position: refs/heads/master@{#346409} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
