Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2248243002:
Enabling sampling of UMA and crash reports on Android. (Closed)
|
Created:
4 years, 4 months ago by jwd Modified:
4 years, 3 months ago CC:
chromium-reviews, kalyank, sadrul, asvitkine+watch_chromium.org, gayane+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnabling sampling of UMA and crash reports on Android.
This has a side effect of causing metrics reporting to be enabled during the session that the pref is changed.
BUG=609987
Committed: https://crrev.com/0c8e0974b783b3e22e09ae2560524c03e5518461
Cr-Commit-Position: refs/heads/master@{#414482}
Patch Set 1 #Patch Set 2 : Enabling sampling of UMA and crash reports on Android. #Patch Set 3 : Enabling sampling of UMA and crash reports on Android. #Patch Set 4 : Enabling sampling of UMA and crash reports on Android. #
Total comments: 15
Patch Set 5 : Initial comments w/o new param change #Patch Set 6 : Adding metrics service updates when consent changes #Patch Set 7 : fix test #Patch Set 8 : Some missing changes #
Total comments: 27
Patch Set 9 : Some of Ilya's comments #
Total comments: 6
Patch Set 10 : Ilya + Gayane comments #Patch Set 11 : fixing compile and reverting feature disable change #
Total comments: 4
Patch Set 12 : Gayane's comments #
Total comments: 2
Patch Set 13 : Ilya's comments #
Total comments: 10
Patch Set 14 : Theresa's and dfalcantara's comments with sync #Messages
Total messages: 40 (13 generated)
The CQ bit was checked by jwd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
jwd@chromium.org changed reviewers: + asvitkine@chromium.org, gayane@chromium.org
https://codereview.chromium.org/2248243002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadCallable.java (right): https://codereview.chromium.org/2248243002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadCallable.java:113: return UPLOAD_FAILURE; Can you add a new enum for this case. I think it's confusing to return UPLOAD_FAILURE here. You can make the caller(s) of this handle it the same way as UPLOAD_FAILURE, if that's what makes the most sense, but I think having the distinction makes sense. https://codereview.chromium.org/2248243002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaSessionStats.java (right): https://codereview.chromium.org/2248243002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaSessionStats.java:197: // Update the metrics sampling state. Expand comment to mention why this is being done - i.e. "so that it can be used by Java code before native is initialized.". https://codereview.chromium.org/2248243002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaUtils.java (right): https://codereview.chromium.org/2248243002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaUtils.java:62: return nativeIsClientInMetricsReportingSample(); Please make the native* name consistent with the method name. i.e. either make it nativeIsClientInSample or make the other mthod isClientInMetricsReportingSample. https://codereview.chromium.org/2248243002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java (right): https://codereview.chromium.org/2248243002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:32: private static final String PREF_METRICS_IN_SAMPLE = "metrics_in_sample"; Nit: Move this right below PREF_METRICS_REPORTING https://codereview.chromium.org/2248243002/diff/60001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/crash/MinidumpUploadCallableTest.java (right): https://codereview.chromium.org/2248243002/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/crash/MinidumpUploadCallableTest.java:126: mIsInSample = isInSample; Sigh, I really hate this mock class, as it makes it completely not obvious what each param does during initialization. I think you can do better by making the members protected and using brace initializer syntax at the callsite. e.g. new MockCrashreportingPermissionManager() {{ mIsInSample = true; }}; I think the current state of this code is extremely unreadable - and with this CL you're incrementally making it worse, so I suggest just making a default ctor for this and changing the tests to use the above syntax. Given your current patch is already changing all the call sites, I suggest just cleaning things up and migrating to the above syntax, so it's easy to tell what each test is doing. https://codereview.chromium.org/2248243002/diff/60001/chrome/browser/android/... File chrome/browser/android/metrics/uma_session_stats.cc (right): https://codereview.chromium.org/2248243002/diff/60001/chrome/browser/android/... chrome/browser/android/metrics/uma_session_stats.cc:108: // treat the startup path as a change when consent has been given. Hmm, I am bit confused by this. In my CL that added UpdateMetricsPrefsOnPermissionChange(), I was assuming this would only be called on a user initiated change of the pref. But this comment implies this actually changes in a startup case. If so, I think the UpdateMetricsPrefsOnPermissionChange() is not correct. As it assumes the pref is changing (and for example, when enabling, clears "old" crash counts). If that is not the case, then (my) logic is wrong. :( How can we detect that the pref is actually changing here. Should we have a separate param from the Java side? (Also, separately, it's not obvious to me how why you're changing the logic here - and why the change to making it static makes a difference to the old logic. Can you explain?)
https://codereview.chromium.org/2248243002/diff/60001/chrome/browser/android/... File chrome/browser/android/metrics/uma_session_stats.cc (right): https://codereview.chromium.org/2248243002/diff/60001/chrome/browser/android/... chrome/browser/android/metrics/uma_session_stats.cc:108: // treat the startup path as a change when consent has been given. On 2016/08/18 06:16:39, Alexei Svitkine (OOO) wrote: > Hmm, I am bit confused by this. > > In my CL that added UpdateMetricsPrefsOnPermissionChange(), I was assuming this > would only be called on a user initiated change of the pref. But this comment > implies this actually changes in a startup case. > > If so, I think the UpdateMetricsPrefsOnPermissionChange() is not correct. As it > assumes the pref is changing (and for example, when enabling, clears "old" crash > counts). > > If that is not the case, then (my) logic is wrong. :( Shoot, yeah, should have caught that. But I do believe this is on the startup path. Gayane, can maybe confirm. > > How can we detect that the pref is actually changing here. Should we have a > separate param from the Java side? So, the problem is that the UpdateMetricsServiceState function is called with no arguments from the java side (on connection change, on startNewSession, onResume, and NOT when the setting is changed), and is strictly independent of the function that syncs android and chrome prefs. The only way we currently have of knowing if the update is the first since consent change is maybe from the GoogleUpdateSettings::GetCollectStatsConsent, which does file ops. I could have some initialization that checks that once a session. It'll be annoying since it'll need to be a posted task, but it should work. I think it only means an extra file op on first run and whenever the pref changes, since I think the old code was doing extra file ops anyway. idea (1) is do that idea (2): Create an "is dirty" android pref, whenever we change the consent pref, we mark the is dirty pref as true, whenever we call UpdateMetricsServiceState we pass in the is dirty value no the native version, and set it to false after. idea (3): Only synchronize the the android and chrome prefs in the UpdateMtricsServiceState call, that way we know when there's a change, since they disagree. idea (4): Rework a lot of the Java code to be more similar to the native code, i.e. Something similar to ChangeMetricsReportingState in metrics_reporting_state. That would be called from the settings change and the first run flow (or something would be from there, since SetCollectStatsConsent needs to be called before ForceClientIdCreation, otherwise the consent file won't get written...) > > (Also, separately, it's not obvious to me how why you're changing the logic here > - and why the change to making it static makes a difference to the old logic. > Can you explain?) It's different from the old logic in the case of sampling. may_record can be true, while metrics aren't recording. With the old logic, it would always look like it's switching on. Not VERY wrong, since the client isn't recording and we probably don't care about the "old" crash counts, but it would do extra file ops.
https://codereview.chromium.org/2248243002/diff/60001/chrome/browser/android/... File chrome/browser/android/metrics/uma_session_stats.cc (right): https://codereview.chromium.org/2248243002/diff/60001/chrome/browser/android/... chrome/browser/android/metrics/uma_session_stats.cc:108: // treat the startup path as a change when consent has been given. On 2016/08/18 15:47:56, jwd wrote: > On 2016/08/18 06:16:39, Alexei Svitkine (OOO) wrote: > > Hmm, I am bit confused by this. > > > > In my CL that added UpdateMetricsPrefsOnPermissionChange(), I was assuming > this > > would only be called on a user initiated change of the pref. But this comment > > implies this actually changes in a startup case. > > > > If so, I think the UpdateMetricsPrefsOnPermissionChange() is not correct. As > it > > assumes the pref is changing (and for example, when enabling, clears "old" > crash > > counts). > > > > If that is not the case, then (my) logic is wrong. :( > Shoot, yeah, should have caught that. But I do believe this is on the startup > path. > Gayane, can maybe confirm. > > > > How can we detect that the pref is actually changing here. Should we have a > > separate param from the Java side? > So, the problem is that the UpdateMetricsServiceState function is called with no > arguments from the java side (on connection change, on startNewSession, > onResume, and NOT when the setting is changed), and is strictly independent of > the function that syncs android and chrome prefs. > > The only way we currently have of knowing if the update is the first since > consent change is maybe from the GoogleUpdateSettings::GetCollectStatsConsent, > which does file ops. I could have some initialization that checks that once a > session. It'll be annoying since it'll need to be a posted task, but it should > work. I think it only means an extra file op on first run and whenever the pref > changes, since I think the old code was doing extra file ops anyway. > > idea (1) is do that > > idea (2): > Create an "is dirty" android pref, whenever we change the consent pref, we mark > the is dirty pref as true, whenever we call UpdateMetricsServiceState we pass in > the is dirty value no the native version, and set it to false after. > > idea (3): > Only synchronize the the android and chrome prefs in the > UpdateMtricsServiceState call, that way we know when there's a change, since > they disagree. > > idea (4): > Rework a lot of the Java code to be more similar to the native code, i.e. > Something similar to ChangeMetricsReportingState in metrics_reporting_state. > That would be called from the settings change and the first run flow (or > something would be from there, since SetCollectStatsConsent needs to be called > before ForceClientIdCreation, otherwise the consent file won't get written...) > > > > > (Also, separately, it's not obvious to me how why you're changing the logic > here > > - and why the change to making it static makes a difference to the old logic. > > Can you explain?) > It's different from the old logic in the case of sampling. may_record can be > true, while metrics aren't recording. With the old logic, it would always look > like it's switching on. Not VERY wrong, since the client isn't recording and we > probably don't care about the "old" crash counts, but it would do extra file > ops. > I think we should try to get to (4) eventually - making Java and native code more similar. In the mean time, how hard would it be to simply add a new param to the Java & JNI API that specifies whether the pref is actually changing as a result of a user toggling the pref? Then, only do the metrics prefs stuff when that is true.
On 2016/08/18 16:00:30, Alexei Svitkine (OOO) wrote: > https://codereview.chromium.org/2248243002/diff/60001/chrome/browser/android/... > File chrome/browser/android/metrics/uma_session_stats.cc (right): > > https://codereview.chromium.org/2248243002/diff/60001/chrome/browser/android/... > chrome/browser/android/metrics/uma_session_stats.cc:108: // treat the startup > path as a change when consent has been given. > On 2016/08/18 15:47:56, jwd wrote: > > On 2016/08/18 06:16:39, Alexei Svitkine (OOO) wrote: > > > Hmm, I am bit confused by this. > > > > > > In my CL that added UpdateMetricsPrefsOnPermissionChange(), I was assuming > > this > > > would only be called on a user initiated change of the pref. But this > comment > > > implies this actually changes in a startup case. > > > > > > If so, I think the UpdateMetricsPrefsOnPermissionChange() is not correct. As > > it > > > assumes the pref is changing (and for example, when enabling, clears "old" > > crash > > > counts). > > > > > > If that is not the case, then (my) logic is wrong. :( > > Shoot, yeah, should have caught that. But I do believe this is on the startup > > path. > > Gayane, can maybe confirm. > > > > > > How can we detect that the pref is actually changing here. Should we have a > > > separate param from the Java side? > > So, the problem is that the UpdateMetricsServiceState function is called with > no > > arguments from the java side (on connection change, on startNewSession, > > onResume, and NOT when the setting is changed), and is strictly independent of > > the function that syncs android and chrome prefs. > > > > The only way we currently have of knowing if the update is the first since > > consent change is maybe from the GoogleUpdateSettings::GetCollectStatsConsent, > > which does file ops. I could have some initialization that checks that once a > > session. It'll be annoying since it'll need to be a posted task, but it should > > work. I think it only means an extra file op on first run and whenever the > pref > > changes, since I think the old code was doing extra file ops anyway. > > > > idea (1) is do that > > > > idea (2): > > Create an "is dirty" android pref, whenever we change the consent pref, we > mark > > the is dirty pref as true, whenever we call UpdateMetricsServiceState we pass > in > > the is dirty value no the native version, and set it to false after. > > > > idea (3): > > Only synchronize the the android and chrome prefs in the > > UpdateMtricsServiceState call, that way we know when there's a change, since > > they disagree. > > > > idea (4): > > Rework a lot of the Java code to be more similar to the native code, i.e. > > Something similar to ChangeMetricsReportingState in metrics_reporting_state. > > That would be called from the settings change and the first run flow (or > > something would be from there, since SetCollectStatsConsent needs to be called > > before ForceClientIdCreation, otherwise the consent file won't get written...) > > > > > > > > (Also, separately, it's not obvious to me how why you're changing the logic > > here > > > - and why the change to making it static makes a difference to the old > logic. > > > Can you explain?) > > It's different from the old logic in the case of sampling. may_record can be > > true, while metrics aren't recording. With the old logic, it would always look > > like it's switching on. Not VERY wrong, since the client isn't recording and > we > > probably don't care about the "old" crash counts, but it would do extra file > > ops. > > > > I think we should try to get to (4) eventually - making Java and native code > more similar. > > In the mean time, how hard would it be to simply add a new param to the Java & > JNI API that specifies whether the pref is actually changing as a result of a > user toggling the pref? Then, only do the metrics prefs stuff when that is true. Currently, non-trivial, since the Update call is done independently of pref changes. Either we'd need to call it when we make a pref change and pass is_change=true there, and pass false at all the other call sites; or we'd need to do (2) so that the Java side Update method can know.
repeating reply in inline comment https://codereview.chromium.org/2248243002/diff/60001/chrome/browser/android/... File chrome/browser/android/metrics/uma_session_stats.cc (right): https://codereview.chromium.org/2248243002/diff/60001/chrome/browser/android/... chrome/browser/android/metrics/uma_session_stats.cc:108: // treat the startup path as a change when consent has been given. On 2016/08/18 16:00:30, Alexei Svitkine (OOO) wrote: > On 2016/08/18 15:47:56, jwd wrote: > > On 2016/08/18 06:16:39, Alexei Svitkine (OOO) wrote: > > > Hmm, I am bit confused by this. > > > > > > In my CL that added UpdateMetricsPrefsOnPermissionChange(), I was assuming > > this > > > would only be called on a user initiated change of the pref. But this > comment > > > implies this actually changes in a startup case. > > > > > > If so, I think the UpdateMetricsPrefsOnPermissionChange() is not correct. As > > it > > > assumes the pref is changing (and for example, when enabling, clears "old" > > crash > > > counts). > > > > > > If that is not the case, then (my) logic is wrong. :( > > Shoot, yeah, should have caught that. But I do believe this is on the startup > > path. > > Gayane, can maybe confirm. > > > > > > How can we detect that the pref is actually changing here. Should we have a > > > separate param from the Java side? > > So, the problem is that the UpdateMetricsServiceState function is called with > no > > arguments from the java side (on connection change, on startNewSession, > > onResume, and NOT when the setting is changed), and is strictly independent of > > the function that syncs android and chrome prefs. > > > > The only way we currently have of knowing if the update is the first since > > consent change is maybe from the GoogleUpdateSettings::GetCollectStatsConsent, > > which does file ops. I could have some initialization that checks that once a > > session. It'll be annoying since it'll need to be a posted task, but it should > > work. I think it only means an extra file op on first run and whenever the > pref > > changes, since I think the old code was doing extra file ops anyway. > > > > idea (1) is do that > > > > idea (2): > > Create an "is dirty" android pref, whenever we change the consent pref, we > mark > > the is dirty pref as true, whenever we call UpdateMetricsServiceState we pass > in > > the is dirty value no the native version, and set it to false after. > > > > idea (3): > > Only synchronize the the android and chrome prefs in the > > UpdateMtricsServiceState call, that way we know when there's a change, since > > they disagree. > > > > idea (4): > > Rework a lot of the Java code to be more similar to the native code, i.e. > > Something similar to ChangeMetricsReportingState in metrics_reporting_state. > > That would be called from the settings change and the first run flow (or > > something would be from there, since SetCollectStatsConsent needs to be called > > before ForceClientIdCreation, otherwise the consent file won't get written...) > > > > > > > > (Also, separately, it's not obvious to me how why you're changing the logic > > here > > > - and why the change to making it static makes a difference to the old > logic. > > > Can you explain?) > > It's different from the old logic in the case of sampling. may_record can be > > true, while metrics aren't recording. With the old logic, it would always look > > like it's switching on. Not VERY wrong, since the client isn't recording and > we > > probably don't care about the "old" crash counts, but it would do extra file > > ops. > > > > I think we should try to get to (4) eventually - making Java and native code > more similar. > > In the mean time, how hard would it be to simply add a new param to the Java & > JNI API that specifies whether the pref is actually changing as a result of a > user toggling the pref? Then, only do the metrics prefs stuff when that is true. Currently, non-trivial, since the Update call is done independently of pref changes. Either we'd need to call it when we make a pref change and pass is_change=true there, and pass false at all the other call sites; or we'd need to do (2) so that the Java side Update method can know.
On 2016/08/18 19:49:53, jwd wrote: > On 2016/08/18 16:00:30, Alexei Svitkine (OOO) wrote: > > > https://codereview.chromium.org/2248243002/diff/60001/chrome/browser/android/... > > File chrome/browser/android/metrics/uma_session_stats.cc (right): > > > > > https://codereview.chromium.org/2248243002/diff/60001/chrome/browser/android/... > > chrome/browser/android/metrics/uma_session_stats.cc:108: // treat the startup > > path as a change when consent has been given. > > On 2016/08/18 15:47:56, jwd wrote: > > > On 2016/08/18 06:16:39, Alexei Svitkine (OOO) wrote: > > > > Hmm, I am bit confused by this. > > > > > > > > In my CL that added UpdateMetricsPrefsOnPermissionChange(), I was assuming > > > this > > > > would only be called on a user initiated change of the pref. But this > > comment > > > > implies this actually changes in a startup case. > > > > > > > > If so, I think the UpdateMetricsPrefsOnPermissionChange() is not correct. > As > > > it > > > > assumes the pref is changing (and for example, when enabling, clears "old" > > > crash > > > > counts). > > > > > > > > If that is not the case, then (my) logic is wrong. :( > > > Shoot, yeah, should have caught that. But I do believe this is on the > startup > > > path. > > > Gayane, can maybe confirm. > > > > > > > > How can we detect that the pref is actually changing here. Should we have > a > > > > separate param from the Java side? > > > So, the problem is that the UpdateMetricsServiceState function is called > with > > no > > > arguments from the java side (on connection change, on startNewSession, > > > onResume, and NOT when the setting is changed), and is strictly independent > of > > > the function that syncs android and chrome prefs. > > > > > > The only way we currently have of knowing if the update is the first since > > > consent change is maybe from the > GoogleUpdateSettings::GetCollectStatsConsent, > > > which does file ops. I could have some initialization that checks that once > a > > > session. It'll be annoying since it'll need to be a posted task, but it > should > > > work. I think it only means an extra file op on first run and whenever the > > pref > > > changes, since I think the old code was doing extra file ops anyway. > > > > > > idea (1) is do that > > > > > > idea (2): > > > Create an "is dirty" android pref, whenever we change the consent pref, we > > mark > > > the is dirty pref as true, whenever we call UpdateMetricsServiceState we > pass > > in > > > the is dirty value no the native version, and set it to false after. > > > > > > idea (3): > > > Only synchronize the the android and chrome prefs in the > > > UpdateMtricsServiceState call, that way we know when there's a change, since > > > they disagree. > > > > > > idea (4): > > > Rework a lot of the Java code to be more similar to the native code, i.e. > > > Something similar to ChangeMetricsReportingState in metrics_reporting_state. > > > That would be called from the settings change and the first run flow (or > > > something would be from there, since SetCollectStatsConsent needs to be > called > > > before ForceClientIdCreation, otherwise the consent file won't get > written...) > > > > > > > > > > > (Also, separately, it's not obvious to me how why you're changing the > logic > > > here > > > > - and why the change to making it static makes a difference to the old > > logic. > > > > Can you explain?) > > > It's different from the old logic in the case of sampling. may_record can be > > > true, while metrics aren't recording. With the old logic, it would always > look > > > like it's switching on. Not VERY wrong, since the client isn't recording and > > we > > > probably don't care about the "old" crash counts, but it would do extra file > > > ops. > > > > > > > I think we should try to get to (4) eventually - making Java and native code > > more similar. > > > > In the mean time, how hard would it be to simply add a new param to the Java & > > JNI API that specifies whether the pref is actually changing as a result of a > > user toggling the pref? Then, only do the metrics prefs stuff when that is > true. > Currently, non-trivial, since the Update call is done independently of pref > changes. Either we'd need to call it when we make a pref change and pass > is_change=true there, and pass false at all the other call sites; or we'd need > to do (2) so that the Java side Update method can know. Catching up with the discussion. I don't like option (2) very much because eventually we get stuck with android prefs that takes time to remove. However Jesse's other suggestion to pass is_change=false from where this function is getting called now and call this function when the pref actually gets called with is_change=true will work. The new ON/OFF switch code is here https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... I think we can skip the similar change for old 3-option setting. What you think ?
On 2016/08/18 21:09:01, gayane wrote: > On 2016/08/18 19:49:53, jwd wrote: > > On 2016/08/18 16:00:30, Alexei Svitkine (OOO) wrote: > > > > > > https://codereview.chromium.org/2248243002/diff/60001/chrome/browser/android/... > > > File chrome/browser/android/metrics/uma_session_stats.cc (right): > > > > > > > > > https://codereview.chromium.org/2248243002/diff/60001/chrome/browser/android/... > > > chrome/browser/android/metrics/uma_session_stats.cc:108: // treat the > startup > > > path as a change when consent has been given. > > > On 2016/08/18 15:47:56, jwd wrote: > > > > On 2016/08/18 06:16:39, Alexei Svitkine (OOO) wrote: > > > > > Hmm, I am bit confused by this. > > > > > > > > > > In my CL that added UpdateMetricsPrefsOnPermissionChange(), I was > assuming > > > > this > > > > > would only be called on a user initiated change of the pref. But this > > > comment > > > > > implies this actually changes in a startup case. > > > > > > > > > > If so, I think the UpdateMetricsPrefsOnPermissionChange() is not > correct. > > As > > > > it > > > > > assumes the pref is changing (and for example, when enabling, clears > "old" > > > > crash > > > > > counts). > > > > > > > > > > If that is not the case, then (my) logic is wrong. :( > > > > Shoot, yeah, should have caught that. But I do believe this is on the > > startup > > > > path. > > > > Gayane, can maybe confirm. > > > > > > > > > > How can we detect that the pref is actually changing here. Should we > have > > a > > > > > separate param from the Java side? > > > > So, the problem is that the UpdateMetricsServiceState function is called > > with > > > no > > > > arguments from the java side (on connection change, on startNewSession, > > > > onResume, and NOT when the setting is changed), and is strictly > independent > > of > > > > the function that syncs android and chrome prefs. > > > > > > > > The only way we currently have of knowing if the update is the first since > > > > consent change is maybe from the > > GoogleUpdateSettings::GetCollectStatsConsent, > > > > which does file ops. I could have some initialization that checks that > once > > a > > > > session. It'll be annoying since it'll need to be a posted task, but it > > should > > > > work. I think it only means an extra file op on first run and whenever the > > > pref > > > > changes, since I think the old code was doing extra file ops anyway. > > > > > > > > idea (1) is do that > > > > > > > > idea (2): > > > > Create an "is dirty" android pref, whenever we change the consent pref, we > > > mark > > > > the is dirty pref as true, whenever we call UpdateMetricsServiceState we > > pass > > > in > > > > the is dirty value no the native version, and set it to false after. > > > > > > > > idea (3): > > > > Only synchronize the the android and chrome prefs in the > > > > UpdateMtricsServiceState call, that way we know when there's a change, > since > > > > they disagree. > > > > > > > > idea (4): > > > > Rework a lot of the Java code to be more similar to the native code, i.e. > > > > Something similar to ChangeMetricsReportingState in > metrics_reporting_state. > > > > That would be called from the settings change and the first run flow (or > > > > something would be from there, since SetCollectStatsConsent needs to be > > called > > > > before ForceClientIdCreation, otherwise the consent file won't get > > written...) > > > > > > > > > > > > > > (Also, separately, it's not obvious to me how why you're changing the > > logic > > > > here > > > > > - and why the change to making it static makes a difference to the old > > > logic. > > > > > Can you explain?) > > > > It's different from the old logic in the case of sampling. may_record can > be > > > > true, while metrics aren't recording. With the old logic, it would always > > look > > > > like it's switching on. Not VERY wrong, since the client isn't recording > and > > > we > > > > probably don't care about the "old" crash counts, but it would do extra > file > > > > ops. > > > > > > > > > > I think we should try to get to (4) eventually - making Java and native code > > > more similar. > > > > > > In the mean time, how hard would it be to simply add a new param to the Java > & > > > JNI API that specifies whether the pref is actually changing as a result of > a > > > user toggling the pref? Then, only do the metrics prefs stuff when that is > > true. > > Currently, non-trivial, since the Update call is done independently of pref > > changes. Either we'd need to call it when we make a pref change and pass > > is_change=true there, and pass false at all the other call sites; or we'd need > > to do (2) so that the Java side Update method can know. > > Catching up with the discussion. I don't like option (2) very much because > eventually > we get stuck with android prefs that takes time to remove. However Jesse's other > > suggestion to pass is_change=false from where this function is getting called > now and > call this function when the pref actually gets called with is_change=true will > work. > The new ON/OFF switch code is here > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... > I think we can skip the similar change for old 3-option setting. What you think > ? Alright, so maybe non-trivial was an overstatement. I'm going to go with adding the argument and extra callsite when prefs change. Also changes for the other comments.
Sounds good. I'll be around until lunch time if you want me to do a review pass. Otherwise, happy to have others (e.g. Gayane and Ilya) review this. On Fri, Aug 19, 2016 at 6:32 AM, <jwd@chromium.org> wrote: > On 2016/08/18 21:09:01, gayane wrote: > > On 2016/08/18 19:49:53, jwd wrote: > > > On 2016/08/18 16:00:30, Alexei Svitkine (OOO) wrote: > > > > > > > > > > https://codereview.chromium.org/2248243002/diff/60001/ > chrome/browser/android/metrics/uma_session_stats.cc > > > > File chrome/browser/android/metrics/uma_session_stats.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2248243002/diff/60001/ > chrome/browser/android/metrics/uma_session_stats.cc#newcode108 > > > > chrome/browser/android/metrics/uma_session_stats.cc:108: // treat > the > > startup > > > > path as a change when consent has been given. > > > > On 2016/08/18 15:47:56, jwd wrote: > > > > > On 2016/08/18 06:16:39, Alexei Svitkine (OOO) wrote: > > > > > > Hmm, I am bit confused by this. > > > > > > > > > > > > In my CL that added UpdateMetricsPrefsOnPermissionChange(), I > was > > assuming > > > > > this > > > > > > would only be called on a user initiated change of the pref. But > this > > > > comment > > > > > > implies this actually changes in a startup case. > > > > > > > > > > > > If so, I think the UpdateMetricsPrefsOnPermissionChange() is not > > correct. > > > As > > > > > it > > > > > > assumes the pref is changing (and for example, when enabling, > clears > > "old" > > > > > crash > > > > > > counts). > > > > > > > > > > > > If that is not the case, then (my) logic is wrong. :( > > > > > Shoot, yeah, should have caught that. But I do believe this is on > the > > > startup > > > > > path. > > > > > Gayane, can maybe confirm. > > > > > > > > > > > > How can we detect that the pref is actually changing here. > Should we > > have > > > a > > > > > > separate param from the Java side? > > > > > So, the problem is that the UpdateMetricsServiceState function is > called > > > with > > > > no > > > > > arguments from the java side (on connection change, on > startNewSession, > > > > > onResume, and NOT when the setting is changed), and is strictly > > independent > > > of > > > > > the function that syncs android and chrome prefs. > > > > > > > > > > The only way we currently have of knowing if the update is the > first > since > > > > > consent change is maybe from the > > > GoogleUpdateSettings::GetCollectStatsConsent, > > > > > which does file ops. I could have some initialization that checks > that > > once > > > a > > > > > session. It'll be annoying since it'll need to be a posted task, > but it > > > should > > > > > work. I think it only means an extra file op on first run and > whenever > the > > > > pref > > > > > changes, since I think the old code was doing extra file ops > anyway. > > > > > > > > > > idea (1) is do that > > > > > > > > > > idea (2): > > > > > Create an "is dirty" android pref, whenever we change the consent > pref, > we > > > > mark > > > > > the is dirty pref as true, whenever we call > UpdateMetricsServiceState we > > > pass > > > > in > > > > > the is dirty value no the native version, and set it to false > after. > > > > > > > > > > idea (3): > > > > > Only synchronize the the android and chrome prefs in the > > > > > UpdateMtricsServiceState call, that way we know when there's a > change, > > since > > > > > they disagree. > > > > > > > > > > idea (4): > > > > > Rework a lot of the Java code to be more similar to the native > code, > i.e. > > > > > Something similar to ChangeMetricsReportingState in > > metrics_reporting_state. > > > > > That would be called from the settings change and the first run > flow (or > > > > > something would be from there, since SetCollectStatsConsent needs > to be > > > called > > > > > before ForceClientIdCreation, otherwise the consent file won't get > > > written...) > > > > > > > > > > > > > > > > > (Also, separately, it's not obvious to me how why you're > changing the > > > logic > > > > > here > > > > > > - and why the change to making it static makes a difference to > the old > > > > logic. > > > > > > Can you explain?) > > > > > It's different from the old logic in the case of sampling. > may_record > can > > be > > > > > true, while metrics aren't recording. With the old logic, it would > always > > > look > > > > > like it's switching on. Not VERY wrong, since the client isn't > recording > > and > > > > we > > > > > probably don't care about the "old" crash counts, but it would do > extra > > file > > > > > ops. > > > > > > > > > > > > > I think we should try to get to (4) eventually - making Java and > native > code > > > > more similar. > > > > > > > > In the mean time, how hard would it be to simply add a new param to > the > Java > > & > > > > JNI API that specifies whether the pref is actually changing as a > result > of > > a > > > > user toggling the pref? Then, only do the metrics prefs stuff when > that is > > > true. > > > Currently, non-trivial, since the Update call is done independently of > pref > > > changes. Either we'd need to call it when we make a pref change and > pass > > > is_change=true there, and pass false at all the other call sites; or > we'd > need > > > to do (2) so that the Java side Update method can know. > > > > Catching up with the discussion. I don't like option (2) very much > because > > eventually > > we get stuck with android prefs that takes time to remove. However > Jesse's > other > > > > suggestion to pass is_change=false from where this function is getting > called > > now and > > call this function when the pref actually gets called with > is_change=true will > > work. > > The new ON/OFF switch code is here > > > https://cs.chromium.org/chromium/src/chrome/android/ > java/src/org/chromium/chrome/browser/preferences/privacy/ > UsageAndCrashReportsPreferenceFragment.java?type=cs&q= > usageandcrash&sq=package:chromium&l=39 > > I think we can skip the similar change for old 3-option setting. What you > think > > ? > > Alright, so maybe non-trivial was an overstatement. I'm going to go with > adding > the argument and extra callsite when prefs change. Also changes for the > other > comments. > > https://codereview.chromium.org/2248243002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Description was changed from ========== Enabling sampling of UMA and crash reports on Android. BUG= ========== to ========== Enabling sampling of UMA and crash reports on Android. This has a side effect of causing metrics reporting to be enabled during the session that the pref is changed. BUG=609987 ==========
jwd@chromium.org changed reviewers: + isherman@chromium.org
+isherman@ for metrics reviews. https://codereview.chromium.org/2248243002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadCallable.java (right): https://codereview.chromium.org/2248243002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadCallable.java:113: return UPLOAD_FAILURE; On 2016/08/18 06:16:39, Alexei Svitkine (OOO) wrote: > Can you add a new enum for this case. I think it's confusing to return > UPLOAD_FAILURE here. > > You can make the caller(s) of this handle it the same way as UPLOAD_FAILURE, if > that's what makes the most sense, but I think having the distinction makes > sense. Done. https://codereview.chromium.org/2248243002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaSessionStats.java (right): https://codereview.chromium.org/2248243002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaSessionStats.java:197: // Update the metrics sampling state. On 2016/08/18 06:16:39, Alexei Svitkine (OOO) wrote: > Expand comment to mention why this is being done - i.e. "so that it can be used > by Java code before native is initialized.". Done. https://codereview.chromium.org/2248243002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaUtils.java (right): https://codereview.chromium.org/2248243002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaUtils.java:62: return nativeIsClientInMetricsReportingSample(); On 2016/08/18 06:16:39, Alexei Svitkine (OOO) wrote: > Please make the native* name consistent with the method name. > > i.e. either make it nativeIsClientInSample or make the other mthod > isClientInMetricsReportingSample. Done. https://codereview.chromium.org/2248243002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java (right): https://codereview.chromium.org/2248243002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:32: private static final String PREF_METRICS_IN_SAMPLE = "metrics_in_sample"; On 2016/08/18 06:16:39, Alexei Svitkine (OOO) wrote: > Nit: Move this right below PREF_METRICS_REPORTING Done. https://codereview.chromium.org/2248243002/diff/60001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/crash/MinidumpUploadCallableTest.java (right): https://codereview.chromium.org/2248243002/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/crash/MinidumpUploadCallableTest.java:126: mIsInSample = isInSample; On 2016/08/18 06:16:39, Alexei Svitkine (OOO) wrote: > Sigh, I really hate this mock class, as it makes it completely not obvious what > each param does during initialization. > > I think you can do better by making the members protected and using brace > initializer syntax at the callsite. > > e.g. > > new MockCrashreportingPermissionManager() {{ > mIsInSample = true; > }}; > > I think the current state of this code is extremely unreadable - and with this > CL you're incrementally making it worse, so I suggest just making a default ctor > for this and changing the tests to use the above syntax. Given your current > patch is already changing all the call sites, I suggest just cleaning things up > and migrating to the above syntax, so it's easy to tell what each test is doing. Done. https://codereview.chromium.org/2248243002/diff/60001/chrome/browser/android/... File chrome/browser/android/metrics/uma_session_stats.cc (right): https://codereview.chromium.org/2248243002/diff/60001/chrome/browser/android/... chrome/browser/android/metrics/uma_session_stats.cc:108: // treat the startup path as a change when consent has been given. On 2016/08/18 21:00:47, jwd wrote: > On 2016/08/18 16:00:30, Alexei Svitkine (OOO) wrote: > > On 2016/08/18 15:47:56, jwd wrote: > > > On 2016/08/18 06:16:39, Alexei Svitkine (OOO) wrote: > > > > Hmm, I am bit confused by this. > > > > > > > > In my CL that added UpdateMetricsPrefsOnPermissionChange(), I was assuming > > > this > > > > would only be called on a user initiated change of the pref. But this > > comment > > > > implies this actually changes in a startup case. > > > > > > > > If so, I think the UpdateMetricsPrefsOnPermissionChange() is not correct. > As > > > it > > > > assumes the pref is changing (and for example, when enabling, clears "old" > > > crash > > > > counts). > > > > > > > > If that is not the case, then (my) logic is wrong. :( > > > Shoot, yeah, should have caught that. But I do believe this is on the > startup > > > path. > > > Gayane, can maybe confirm. > > > > > > > > How can we detect that the pref is actually changing here. Should we have > a > > > > separate param from the Java side? > > > So, the problem is that the UpdateMetricsServiceState function is called > with > > no > > > arguments from the java side (on connection change, on startNewSession, > > > onResume, and NOT when the setting is changed), and is strictly independent > of > > > the function that syncs android and chrome prefs. > > > > > > The only way we currently have of knowing if the update is the first since > > > consent change is maybe from the > GoogleUpdateSettings::GetCollectStatsConsent, > > > which does file ops. I could have some initialization that checks that once > a > > > session. It'll be annoying since it'll need to be a posted task, but it > should > > > work. I think it only means an extra file op on first run and whenever the > > pref > > > changes, since I think the old code was doing extra file ops anyway. > > > > > > idea (1) is do that > > > > > > idea (2): > > > Create an "is dirty" android pref, whenever we change the consent pref, we > > mark > > > the is dirty pref as true, whenever we call UpdateMetricsServiceState we > pass > > in > > > the is dirty value no the native version, and set it to false after. > > > > > > idea (3): > > > Only synchronize the the android and chrome prefs in the > > > UpdateMtricsServiceState call, that way we know when there's a change, since > > > they disagree. > > > > > > idea (4): > > > Rework a lot of the Java code to be more similar to the native code, i.e. > > > Something similar to ChangeMetricsReportingState in metrics_reporting_state. > > > That would be called from the settings change and the first run flow (or > > > something would be from there, since SetCollectStatsConsent needs to be > called > > > before ForceClientIdCreation, otherwise the consent file won't get > written...) > > > > > > > > > > > (Also, separately, it's not obvious to me how why you're changing the > logic > > > here > > > > - and why the change to making it static makes a difference to the old > > logic. > > > > Can you explain?) > > > It's different from the old logic in the case of sampling. may_record can be > > > true, while metrics aren't recording. With the old logic, it would always > look > > > like it's switching on. Not VERY wrong, since the client isn't recording and > > we > > > probably don't care about the "old" crash counts, but it would do extra file > > > ops. > > > > > > > I think we should try to get to (4) eventually - making Java and native code > > more similar. > > > > In the mean time, how hard would it be to simply add a new param to the Java & > > JNI API that specifies whether the pref is actually changing as a result of a > > user toggling the pref? Then, only do the metrics prefs stuff when that is > true. > > Currently, non-trivial, since the Update call is done independently of pref > changes. Either we'd need to call it when we make a pref change and pass > is_change=true there, and pass false at all the other call sites; or we'd need > to do (2) so that the Java side Update method can know. Added the is_change parameter, and made it so that this gets called when the prefs are changed.
I haven't yet looked at UmaSessionStats.java and UsageAndCrashReportsPreferenceFragment.java (will look more later tonight), but here are comments on the rest: https://codereview.chromium.org/2248243002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadCallable.java (right): https://codereview.chromium.org/2248243002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadCallable.java:114: Log.i(TAG, "Minidump cannot currently be uploaded due to sampling."); nit: "currently" seems somewhat misleading -- or do clients flipflop in and out of the sample? https://codereview.chromium.org/2248243002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadCallable.java:115: return UPLOAD_DISABLED_BY_SAMPLING; What code handles this return value? Will the system just keep retrying this upload indefinitely? https://codereview.chromium.org/2248243002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadCallable.java:116: } Optional: This kind of feels like it belongs before the "isLimited" check. I don't think the ultimate logic actually changes; but it just seems, conceptually, like it should be checked earlier. https://codereview.chromium.org/2248243002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java (left): https://codereview.chromium.org/2248243002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:329: * This should be called only once, the first time Chrome is launched. Hmm, why did you remove this line? What are the semantics of "init" / "initial" now? https://codereview.chromium.org/2248243002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java (right): https://codereview.chromium.org/2248243002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:295: return mSharedPreferences.getBoolean(PREF_METRICS_IN_SAMPLE, true); Hmm, the default value is true? That seems riskier than the default value being false. Or, am I misunderstanding something? (Hopefully, the default value case should actually never be reached. Is there a way to assert that?) https://codereview.chromium.org/2248243002/diff/140001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/crash/MinidumpUploadCallableTest.java (right): https://codereview.chromium.org/2248243002/diff/140001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/crash/MinidumpUploadCallableTest.java:292: assertFalse(mExpectedFileAfterUpload.exists()); Could you please also add an assertion for what it is that *does* happen to the file? https://codereview.chromium.org/2248243002/diff/140001/chrome/browser/android... File chrome/browser/android/metrics/uma_session_stats.cc (right): https://codereview.chromium.org/2248243002/diff/140001/chrome/browser/android... chrome/browser/android/metrics/uma_session_stats.cc:102: // * Logs are neither being recorded or uploaded. Could you please document |consent_change| as well? https://codereview.chromium.org/2248243002/diff/140001/chrome/browser/android... chrome/browser/android/metrics/uma_session_stats.cc:102: // * Logs are neither being recorded or uploaded. This list sounds like there ought to be an assertion: DCHECK(may_record || !may_upload);
https://codereview.chromium.org/2248243002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadCallable.java (right): https://codereview.chromium.org/2248243002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadCallable.java:115: return UPLOAD_DISABLED_BY_SAMPLING; On 2016/08/23 21:32:17, Ilya Sherman wrote: > What code handles this return value? Will the system just keep retrying this > upload indefinitely? Ok, looking at this a little bit more, I see that for example in case of UPLOAD_USER_DISABLED they call cleanupMinidumpFile() so that it doesn't get retried. https://codereview.chromium.org/2248243002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaUtils.java (right): https://codereview.chromium.org/2248243002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaUtils.java:58: * Determines if this client is eligible to send metrics. If they are, and there was user This affects crashes as well. Names get long, but it would be good to mention crashes as well. https://codereview.chromium.org/2248243002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/CrashReportingPermissionManager.java (right): https://codereview.chromium.org/2248243002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/CrashReportingPermissionManager.java:17: public boolean isClientInMetricsSample(); maybe just isClientInSample
https://codereview.chromium.org/2248243002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadCallable.java (right): https://codereview.chromium.org/2248243002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadCallable.java:114: Log.i(TAG, "Minidump cannot currently be uploaded due to sampling."); On 2016/08/23 21:32:17, Ilya Sherman wrote: > nit: "currently" seems somewhat misleading -- or do clients flipflop in and out > of the sample? It's not ideal to have it change, but it's not strictly permanent. I'm fine changing it though. It's certainly not as meaningful as it is for the upload limited version. https://codereview.chromium.org/2248243002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadCallable.java:115: return UPLOAD_DISABLED_BY_SAMPLING; On 2016/08/23 21:32:17, Ilya Sherman wrote: > What code handles this return value? Will the system just keep retrying this > upload indefinitely? Hm, yes, I believe it will... I can make not do that by renaming or deleting the file. Maybe for now, deleting it is the correct thing to do, since we don't have the manual upload functionality, and it's non trivial to get these minidumps off the filesystem. The simplest thing would be to treat it the same as a failure, but that would still try to re-upload a few times, so it's not so good. Thoughts? https://codereview.chromium.org/2248243002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadCallable.java:116: } On 2016/08/23 21:32:17, Ilya Sherman wrote: > Optional: This kind of feels like it belongs before the "isLimited" check. I > don't think the ultimate logic actually changes; but it just seems, > conceptually, like it should be checked earlier. Done. https://codereview.chromium.org/2248243002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java (left): https://codereview.chromium.org/2248243002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:329: * This should be called only once, the first time Chrome is launched. On 2016/08/23 21:32:17, Ilya Sherman wrote: > Hmm, why did you remove this line? What are the semantics of "init" / "initial" > now? I should change the first line too, and maybe the name... I removed the line because it's wrong. It IS being called from other places at the moment, and it's safe to do so in the new on/off uploads preferences world (as opposed to the on/only on wifi/off old world). Gayane is going to be doing a cleanup soon to get rid of the old pref related stuff. https://codereview.chromium.org/2248243002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java (right): https://codereview.chromium.org/2248243002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:295: return mSharedPreferences.getBoolean(PREF_METRICS_IN_SAMPLE, true); On 2016/08/23 21:32:17, Ilya Sherman wrote: > Hmm, the default value is true? That seems riskier than the default value being > false. Or, am I misunderstanding something? (Hopefully, the default value case > should actually never be reached. Is there a way to assert that?) The default is that clients are in sample, so if there is consent, they will send metrics/crashes. If it's false, we'll potentially lose crashes that occur early in first-run, before native code is initialized. So, the default value case is actually intended to be reachable. https://codereview.chromium.org/2248243002/diff/140001/chrome/browser/android... File chrome/browser/android/metrics/uma_session_stats.cc (right): https://codereview.chromium.org/2248243002/diff/140001/chrome/browser/android... chrome/browser/android/metrics/uma_session_stats.cc:102: // * Logs are neither being recorded or uploaded. On 2016/08/23 21:32:17, Ilya Sherman wrote: > This list sounds like there ought to be an assertion: DCHECK(may_record || > !may_upload); Done. https://codereview.chromium.org/2248243002/diff/140001/chrome/browser/android... chrome/browser/android/metrics/uma_session_stats.cc:102: // * Logs are neither being recorded or uploaded. On 2016/08/23 21:32:17, Ilya Sherman wrote: > Could you please document |consent_change| as well? Re-worked some of the code to avoid that parameter.
Hokay, I've now also looked at UmaSessionStats.java ... but quite frankly I don't fully understand it. I think there are important subtleties that I should be paying attention to within that code, but I am not well versed enough in the Android/C++ bridge code, and the two parallel sets of preferences, to fully follow the logic and be sure I'm not missing anything subtle. So, hopefully Gayane feels comfortable reviewing that code... =) https://codereview.chromium.org/2248243002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadCallable.java (right): https://codereview.chromium.org/2248243002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadCallable.java:115: return UPLOAD_DISABLED_BY_SAMPLING; On 2016/08/23 23:00:58, jwd wrote: > On 2016/08/23 21:32:17, Ilya Sherman wrote: > > What code handles this return value? Will the system just keep retrying this > > upload indefinitely? > > Hm, yes, I believe it will... > I can make not do that by renaming or deleting the file. > Maybe for now, deleting it is the correct thing to do, since we don't have the > manual upload functionality, and it's non trivial to get these minidumps off the > filesystem. The simplest thing would be to treat it the same as a failure, but > that would still try to re-upload a few times, so it's not so good. Thoughts? I think Gayane's suggestion, i.e. call cleanupMinidumpFile(), makes some sense, since it parallels the case where the upload is not permitted by the user. It might be even better to somehow distinguish files that were not uploaded because they were not allowed by the user vs. because they were sampled out, but that's probably best to save for a follow-up CL. https://codereview.chromium.org/2248243002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java (right): https://codereview.chromium.org/2248243002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:295: return mSharedPreferences.getBoolean(PREF_METRICS_IN_SAMPLE, true); On 2016/08/23 23:00:58, jwd wrote: > On 2016/08/23 21:32:17, Ilya Sherman wrote: > > Hmm, the default value is true? That seems riskier than the default value > being > > false. Or, am I misunderstanding something? (Hopefully, the default value > case > > should actually never be reached. Is there a way to assert that?) > > The default is that clients are in sample, so if there is consent, they will > send metrics/crashes. If it's false, we'll potentially lose crashes that occur > early in first-run, before native code is initialized. So, the default value > case is actually intended to be reachable. Okay, that makes sense. Could you please add a brief comment here documenting that reasoning? https://codereview.chromium.org/2248243002/diff/140001/chrome/browser/android... File chrome/browser/android/metrics/uma_session_stats.cc (right): https://codereview.chromium.org/2248243002/diff/140001/chrome/browser/android... chrome/browser/android/metrics/uma_session_stats.cc:102: // * Logs are neither being recorded or uploaded. On 2016/08/23 23:00:59, jwd wrote: > On 2016/08/23 21:32:17, Ilya Sherman wrote: > > Could you please document |consent_change| as well? > > Re-worked some of the code to avoid that parameter. Now may_record is also unused. Is it still needed here? Is it important that the code in ChangeMetricsReportingConsent() happens before the code in UpdateMetricsServiceState()? If so, is there something that guarantees that ordering? It would be good to document, as well, if there is an ordering constraint. https://codereview.chromium.org/2248243002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadCallable.java (right): https://codereview.chromium.org/2248243002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadCallable.java:56: UPLOAD_DISABLED_BY_SAMPLING}) Could you please revert the format change here? Or is this the "correct" formatting? The previous one was certainly more scannable. https://codereview.chromium.org/2248243002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadCallable.java:103: Log.i(TAG, "Minidump cannot be uploaded due to sampling."); nit: This is super nitpicky, but I'd log something more like "Minidump upload skipped due to sampling. Marking file as uploaded for cleanup to prevent future uploads." That second sentence should be modified according to what we actually do with the file. https://codereview.chromium.org/2248243002/diff/160001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_services_manager_client.cc (right): https://codereview.chromium.org/2248243002/diff/160001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_services_manager_client.cc:37: base::FEATURE_DISABLED_BY_DEFAULT}; Hmm, why disabled by default?
https://codereview.chromium.org/2248243002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadCallable.java (right): https://codereview.chromium.org/2248243002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadCallable.java:115: return UPLOAD_DISABLED_BY_SAMPLING; On 2016/08/24 05:02:50, Ilya Sherman wrote: > On 2016/08/23 23:00:58, jwd wrote: > > On 2016/08/23 21:32:17, Ilya Sherman wrote: > > > What code handles this return value? Will the system just keep retrying > this > > > upload indefinitely? > > > > Hm, yes, I believe it will... > > I can make not do that by renaming or deleting the file. > > Maybe for now, deleting it is the correct thing to do, since we don't have the > > manual upload functionality, and it's non trivial to get these minidumps off > the > > filesystem. The simplest thing would be to treat it the same as a failure, but > > that would still try to re-upload a few times, so it's not so good. Thoughts? > > I think Gayane's suggestion, i.e. call cleanupMinidumpFile(), makes some sense, > since it parallels the case where the upload is not permitted by the user. > > It might be even better to somehow distinguish files that were not uploaded > because they were not allowed by the user vs. because they were sampled out, but > that's probably best to save for a follow-up CL. Done. https://codereview.chromium.org/2248243002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaUtils.java (right): https://codereview.chromium.org/2248243002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaUtils.java:58: * Determines if this client is eligible to send metrics. If they are, and there was user On 2016/08/23 22:43:51, gayane wrote: > This affects crashes as well. Names get long, but it would be good to mention > crashes as well. Done. https://codereview.chromium.org/2248243002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/CrashReportingPermissionManager.java (right): https://codereview.chromium.org/2248243002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/CrashReportingPermissionManager.java:17: public boolean isClientInMetricsSample(); On 2016/08/23 22:43:51, gayane wrote: > maybe just isClientInSample I put Metrics in the name because it's implemented in PrivacyPreferencesManager, and I wanted to keep the connection to metrics stuff there. https://codereview.chromium.org/2248243002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java (right): https://codereview.chromium.org/2248243002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:295: return mSharedPreferences.getBoolean(PREF_METRICS_IN_SAMPLE, true); On 2016/08/24 05:02:50, Ilya Sherman wrote: > On 2016/08/23 23:00:58, jwd wrote: > > On 2016/08/23 21:32:17, Ilya Sherman wrote: > > > Hmm, the default value is true? That seems riskier than the default value > > being > > > false. Or, am I misunderstanding something? (Hopefully, the default value > > case > > > should actually never be reached. Is there a way to assert that?) > > > > The default is that clients are in sample, so if there is consent, they will > > send metrics/crashes. If it's false, we'll potentially lose crashes that occur > > early in first-run, before native code is initialized. So, the default value > > case is actually intended to be reachable. > > Okay, that makes sense. Could you please add a brief comment here documenting > that reasoning? Done. https://codereview.chromium.org/2248243002/diff/140001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/crash/MinidumpUploadCallableTest.java (right): https://codereview.chromium.org/2248243002/diff/140001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/crash/MinidumpUploadCallableTest.java:292: assertFalse(mExpectedFileAfterUpload.exists()); On 2016/08/23 21:32:17, Ilya Sherman wrote: > Could you please also add an assertion for what it is that *does* happen to the > file? Done. https://codereview.chromium.org/2248243002/diff/140001/chrome/browser/android... File chrome/browser/android/metrics/uma_session_stats.cc (right): https://codereview.chromium.org/2248243002/diff/140001/chrome/browser/android... chrome/browser/android/metrics/uma_session_stats.cc:102: // * Logs are neither being recorded or uploaded. On 2016/08/24 05:02:50, Ilya Sherman wrote: > On 2016/08/23 23:00:59, jwd wrote: > > On 2016/08/23 21:32:17, Ilya Sherman wrote: > > > Could you please document |consent_change| as well? > > > > Re-worked some of the code to avoid that parameter. > > Now may_record is also unused. Is it still needed here? Is it important that > the code in ChangeMetricsReportingConsent() happens before the code in > UpdateMetricsServiceState()? If so, is there something that guarantees that > ordering? It would be good to document, as well, if there is an ordering > constraint. Well, it's needed for the DCHECK ;P. Note, there is a deeper place in MetricsServicesManager to move that DCHECK, but I'm not going to because it violates how we use C++ side api. There is an ordering constraint, if ChangeMetricsReportingConsent() gets called, UpdateMetricsServiceState() should be called after it to affect the metrics services. That's currently enforced on the Java side by changeMetricsReportingConsent() calling updateMetricsServiceState() at the end. I will add something to the comment for Change...(). https://codereview.chromium.org/2248243002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadCallable.java (right): https://codereview.chromium.org/2248243002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadCallable.java:56: UPLOAD_DISABLED_BY_SAMPLING}) On 2016/08/24 05:02:50, Ilya Sherman wrote: > Could you please revert the format change here? Or is this the "correct" > formatting? The previous one was certainly more scannable. Yeah, it's the result of git cl format. I reverted it once, because I prefered the manual version, but I didn't want to keep fighting with the formatter. https://codereview.chromium.org/2248243002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadCallable.java:103: Log.i(TAG, "Minidump cannot be uploaded due to sampling."); On 2016/08/24 05:02:50, Ilya Sherman wrote: > nit: This is super nitpicky, but I'd log something more like "Minidump upload > skipped due to sampling. Marking file as uploaded for cleanup to prevent future > uploads." > > That second sentence should be modified according to what we actually do with > the file. Done. https://codereview.chromium.org/2248243002/diff/160001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_services_manager_client.cc (right): https://codereview.chromium.org/2248243002/diff/160001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_services_manager_client.cc:37: base::FEATURE_DISABLED_BY_DEFAULT}; On 2016/08/24 05:02:50, Ilya Sherman wrote: > Hmm, why disabled by default? Oops, was doing that for manual testing, slipped into the upload.
jwd@chromium.org changed reviewers: + dfalcantara@chromium.org, twellington@chromium.org
+twellington@ for prefs changes and higher up android owners +dfalcantara@ for metrics and also higher up android owners Adding Java side owners.
No major comments. Here are a few nits. https://codereview.chromium.org/2248243002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java (right): https://codereview.chromium.org/2248243002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:29: private static final String PREF_METRICS_IN_SAMPLE = "metrics_in_sample"; in_metrics_sample? https://codereview.chromium.org/2248243002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:296: // bee initialized on first run. We'd rather have some extra crashes than none from that been initialized
https://codereview.chromium.org/2248243002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java (right): https://codereview.chromium.org/2248243002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:29: private static final String PREF_METRICS_IN_SAMPLE = "metrics_in_sample"; On 2016/08/24 19:18:53, gayane wrote: > in_metrics_sample? Done. https://codereview.chromium.org/2248243002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:296: // bee initialized on first run. We'd rather have some extra crashes than none from that On 2016/08/24 19:18:53, gayane wrote: > been initialized Done.
Everything outside of UmaSessionStats.java LGTM -- I defer to other reviewers for that file, as I'm not well versed enough in that code. https://codereview.chromium.org/2248243002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaUtils.java (right): https://codereview.chromium.org/2248243002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaUtils.java:60: * consent, then metrics and crashes would be reported. nit: Please re-wrap this text onto just two lines =)
https://codereview.chromium.org/2248243002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaUtils.java (right): https://codereview.chromium.org/2248243002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaUtils.java:60: * consent, then metrics and crashes would be reported. On 2016/08/24 20:02:02, Ilya Sherman wrote: > nit: Please re-wrap this text onto just two lines =) Done.
lgtm https://chromiumcodereview.appspot.com/2248243002/diff/240001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaSessionStats.java (right): https://chromiumcodereview.appspot.com/2248243002/diff/240001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaSessionStats.java:235: private static native long nativeInit(); nit: put the newline above back in. https://chromiumcodereview.appspot.com/2248243002/diff/240001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java (right): https://chromiumcodereview.appspot.com/2248243002/diff/240001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:282: * consent, then metrics and crashes would be reported. A rough version of this pair of sentences is repeated at least 4 times (% determines, checks, sets). Is there one place you could refer to with a "See {@link thisplace}" to avoid further divergence?
https://codereview.chromium.org/2248243002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadCallable.java (left): https://codereview.chromium.org/2248243002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadCallable.java:59: UPLOAD_COMMANDLINE_DISABLED nit: I think this was more readable before https://codereview.chromium.org/2248243002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaSessionStats.java (right): https://codereview.chromium.org/2248243002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaSessionStats.java:176: * Chrome Local State prefs for metrics reporting should be in sync before calling this "should be in sync" through what mechanism? I assume by calling #changeMetricsReportingConsent() before this is called, and if so, can this comment be updated to explicitly state that? https://codereview.chromium.org/2248243002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java (right): https://codereview.chromium.org/2248243002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:294: public boolean isClientInMetricsSample() { The method name doesn't really go with the description (which doesn't talk about sampling). Maybe isClientEligbleForMetricsReporting() or something like that? Or update the description to indicate that eligibility is dependent on whether the client is in the metrics sample group.
lgtm
Patchset #14 (id:260001) has been deleted
https://codereview.chromium.org/2248243002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadCallable.java (left): https://codereview.chromium.org/2248243002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadCallable.java:59: UPLOAD_COMMANDLINE_DISABLED On 2016/08/24 23:54:13, Theresa Wellington wrote: > nit: I think this was more readable before Heh, yes, it was git cl format who did this change. I'll put it back before submitting. https://codereview.chromium.org/2248243002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaSessionStats.java (right): https://codereview.chromium.org/2248243002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaSessionStats.java:176: * Chrome Local State prefs for metrics reporting should be in sync before calling this On 2016/08/24 23:54:13, Theresa Wellington wrote: > "should be in sync" through what mechanism? I assume by calling > #changeMetricsReportingConsent() before this is called, and if so, can this > comment be updated to explicitly state that? There's nothing that currently strictly enforces it, I was more intending for it to be part of the contract of this method. It is currently the case that that contract is being satisfied either by #changeMetricsReportingConsent() or by calling syncUsageAndCrashReportingPrefs() during session start before this gets called. It's an important that they are in sync though, so I'm adding an explicit sync call here. https://codereview.chromium.org/2248243002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaSessionStats.java:235: private static native long nativeInit(); On 2016/08/24 23:22:29, dfalcantara wrote: > nit: put the newline above back in. Done. https://codereview.chromium.org/2248243002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java (right): https://codereview.chromium.org/2248243002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:282: * consent, then metrics and crashes would be reported. On 2016/08/24 23:22:29, dfalcantara wrote: > A rough version of this pair of sentences is repeated at least 4 times (% > determines, checks, sets). Is there one place you could refer to with a "See > {@link thisplace}" to avoid further divergence? Done. https://codereview.chromium.org/2248243002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java:294: public boolean isClientInMetricsSample() { On 2016/08/24 23:54:13, Theresa Wellington wrote: > The method name doesn't really go with the description (which doesn't talk about > sampling). Maybe isClientEligbleForMetricsReporting() or something like that? Or > update the description to indicate that eligibility is dependent on whether the > client is in the metrics sample group. Done.
The CQ bit was checked by jwd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, dfalcantara@chromium.org, twellington@chromium.org Link to the patchset: https://codereview.chromium.org/2248243002/#ps280001 (title: "Theresa's and dfalcantara's comments with sync")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Enabling sampling of UMA and crash reports on Android. This has a side effect of causing metrics reporting to be enabled during the session that the pref is changed. BUG=609987 ========== to ========== Enabling sampling of UMA and crash reports on Android. This has a side effect of causing metrics reporting to be enabled during the session that the pref is changed. BUG=609987 ==========
Message was sent while issue was closed.
Committed patchset #14 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== Enabling sampling of UMA and crash reports on Android. This has a side effect of causing metrics reporting to be enabled during the session that the pref is changed. BUG=609987 ========== to ========== Enabling sampling of UMA and crash reports on Android. This has a side effect of causing metrics reporting to be enabled during the session that the pref is changed. BUG=609987 Committed: https://crrev.com/0c8e0974b783b3e22e09ae2560524c03e5518461 Cr-Commit-Position: refs/heads/master@{#414482} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/0c8e0974b783b3e22e09ae2560524c03e5518461 Cr-Commit-Position: refs/heads/master@{#414482} |
