Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1130993005:
Reset metrics if cellular uploads logic should be applied. (Closed)
|
Created:
5 years, 7 months ago by gayane -on leave until 09-2017 Modified:
5 years, 7 months ago Reviewers:
Alexei Svitkine (slow) CC:
chromium-reviews, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReset metrics if cellular uploads logic should be applied.
In order to avoid large quantity of uploads after always enabling cellular uploads on Android reset metrics. This should happen only once.
BUG=455847
Committed: https://crrev.com/09e9a5f4b8dbc7c599a3ffab70762f49f28d0777
Cr-Commit-Position: refs/heads/master@{#329661}
Patch Set 1 #
Total comments: 15
Patch Set 2 : Added anon function #Patch Set 3 : fix comments #
Total comments: 8
Patch Set 4 : Fix function name and nits #
Total comments: 2
Patch Set 5 : fix namespace comment #Patch Set 6 : fix #Patch Set 7 : fix for win #Messages
Total messages: 27 (11 generated)
gayane@chromium.org changed reviewers: + asvitkine@chromium.org
Please have a look.
https://codereview.chromium.org/1130993005/diff/1/chrome/browser/metrics/chro... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/1130993005/diff/1/chrome/browser/metrics/chro... chrome/browser/metrics/chrome_metrics_service_client.cc:282: Nit: No blank line at start of function. https://codereview.chromium.org/1130993005/diff/1/chrome/browser/metrics/chro... chrome/browser/metrics/chrome_metrics_service_client.cc:287: if (!local_state->HasPrefPath(prefs::kMetricsReportingEnabled) && Can you remind me where this pref will be initialized on Android? https://codereview.chromium.org/1130993005/diff/1/chrome/browser/metrics/chro... chrome/browser/metrics/chrome_metrics_service_client.cc:288: variations::GetVariationParamValue("UMA_EnableCellularLogUpload", Nit: Align. https://codereview.chromium.org/1130993005/diff/1/chrome/browser/metrics/chro... chrome/browser/metrics/chrome_metrics_service_client.cc:384: "Enabled") == "true") { You're repeating this in two places - make a helper function at the top in an anon namespace.
Please have one more look. https://codereview.chromium.org/1130993005/diff/1/chrome/browser/metrics/chro... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/1130993005/diff/1/chrome/browser/metrics/chro... chrome/browser/metrics/chrome_metrics_service_client.cc:282: On 2015/05/07 21:06:53, Alexei Svitkine wrote: > Nit: No blank line at start of function. Done. https://codereview.chromium.org/1130993005/diff/1/chrome/browser/metrics/chro... chrome/browser/metrics/chrome_metrics_service_client.cc:287: if (!local_state->HasPrefPath(prefs::kMetricsReportingEnabled) && On 2015/05/07 21:06:53, Alexei Svitkine wrote: > Can you remind me where this pref will be initialized on Android? This pref will be initialized as soon as new session for UMA logging will start. https://cs.corp.google.com/#clankium/src/chrome/android/java/src/org/chromium... and that happens after this code. https://codereview.chromium.org/1130993005/diff/1/chrome/browser/metrics/chro... chrome/browser/metrics/chrome_metrics_service_client.cc:288: variations::GetVariationParamValue("UMA_EnableCellularLogUpload", On 2015/05/07 21:06:53, Alexei Svitkine wrote: > Nit: Align. Done. https://codereview.chromium.org/1130993005/diff/1/chrome/browser/metrics/chro... chrome/browser/metrics/chrome_metrics_service_client.cc:384: "Enabled") == "true") { On 2015/05/07 21:06:53, Alexei Svitkine wrote: > You're repeating this in two places - make a helper function at the top in an > anon namespace. Done.
https://codereview.chromium.org/1130993005/diff/1/chrome/browser/metrics/chro... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/1130993005/diff/1/chrome/browser/metrics/chro... chrome/browser/metrics/chrome_metrics_service_client.cc:284: // log manager is initialized correctly. This comment only explains why you're doing this here - and not *why* this is being done, please mention the purpose of doing this. https://codereview.chromium.org/1130993005/diff/1/chrome/browser/metrics/chro... chrome/browser/metrics/chrome_metrics_service_client.cc:287: if (!local_state->HasPrefPath(prefs::kMetricsReportingEnabled) && On 2015/05/07 21:25:58, gayane wrote: > On 2015/05/07 21:06:53, Alexei Svitkine wrote: > > Can you remind me where this pref will be initialized on Android? > > This pref will be initialized as soon as new session for UMA logging will start. > https://cs.corp.google.com/#clankium/src/chrome/android/java/src/org/chromium... > > and that happens after this code. I see - it's unfortunate that the two places are so decoupled. But I guess it's hard to avoid given the current design - so please at least add a comment mentioning what you said here (i.e. where the pref is initialized). Can we do this from UmaSessionStats instead? In updatePreferences(), we already check the state of the trials. https://codereview.chromium.org/1130993005/diff/1/chrome/browser/metrics/chro... chrome/browser/metrics/chrome_metrics_service_client.cc:380: // Reset stability metrics after all providers are registered. Expand comment to mention *why* it's done after all providers are registered. It's obvious from the code that it's done after all of them are registered.
Fixed comments + discussion for pref reset location. https://codereview.chromium.org/1130993005/diff/1/chrome/browser/metrics/chro... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/1130993005/diff/1/chrome/browser/metrics/chro... chrome/browser/metrics/chrome_metrics_service_client.cc:284: // log manager is initialized correctly. On 2015/05/07 22:04:17, Alexei Svitkine wrote: > This comment only explains why you're doing this here - and not *why* this is > being done, please mention the purpose of doing this. Done. https://codereview.chromium.org/1130993005/diff/1/chrome/browser/metrics/chro... chrome/browser/metrics/chrome_metrics_service_client.cc:287: if (!local_state->HasPrefPath(prefs::kMetricsReportingEnabled) && On 2015/05/07 22:04:17, Alexei Svitkine wrote: > On 2015/05/07 21:25:58, gayane wrote: > > On 2015/05/07 21:06:53, Alexei Svitkine wrote: > > > Can you remind me where this pref will be initialized on Android? > > > > This pref will be initialized as soon as new session for UMA logging will > start. > > > https://cs.corp.google.com/#clankium/src/chrome/android/java/src/org/chromium... > > > > and that happens after this code. > > I see - it's unfortunate that the two places are so decoupled. But I guess it's > hard to avoid given the current design - so please at least add a comment > mentioning what you said here (i.e. where the pref is initialized). > > Can we do this from UmaSessionStats instead? In updatePreferences(), we already > check the state of the trials. I guess we could do this in UmaSessionStats if I could refresh queues in MetricsLogManager, because its constructor uses kMetricsInitialLogs and kMetricsOngoingLogs to init queues. I could reset the prefs and call MetricsLogManager.DiscardCurrentLog() if I understood correctly. Does this sound reasonable to you? https://codereview.chromium.org/1130993005/diff/1/chrome/browser/metrics/chro... chrome/browser/metrics/chrome_metrics_service_client.cc:380: // Reset stability metrics after all providers are registered. On 2015/05/07 22:04:17, Alexei Svitkine wrote: > Expand comment to mention *why* it's done after all providers are registered. > It's obvious from the code that it's done after all of them are registered. Done.
https://codereview.chromium.org/1130993005/diff/1/chrome/browser/metrics/chro... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/1130993005/diff/1/chrome/browser/metrics/chro... chrome/browser/metrics/chrome_metrics_service_client.cc:287: if (!local_state->HasPrefPath(prefs::kMetricsReportingEnabled) && On 2015/05/11 23:02:05, gayane wrote: > On 2015/05/07 22:04:17, Alexei Svitkine wrote: > > On 2015/05/07 21:25:58, gayane wrote: > > > On 2015/05/07 21:06:53, Alexei Svitkine wrote: > > > > Can you remind me where this pref will be initialized on Android? > > > > > > This pref will be initialized as soon as new session for UMA logging will > > start. > > > > > > https://cs.corp.google.com/#clankium/src/chrome/android/java/src/org/chromium... > > > > > > and that happens after this code. > > > > I see - it's unfortunate that the two places are so decoupled. But I guess > it's > > hard to avoid given the current design - so please at least add a comment > > mentioning what you said here (i.e. where the pref is initialized). > > > > Can we do this from UmaSessionStats instead? In updatePreferences(), we > already > > check the state of the trials. > > I guess we could do this in UmaSessionStats if I could refresh queues in > MetricsLogManager, because its constructor uses kMetricsInitialLogs and > kMetricsOngoingLogs to init queues. > > I could reset the prefs and call MetricsLogManager.DiscardCurrentLog() if I > understood correctly. > > Does this sound reasonable to you? That seems more complicated. I'm OK with the current approach now that you've added the comment about where the pref is reset. https://codereview.chromium.org/1130993005/diff/40001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/1130993005/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_service_client.cc:109: bool needReset() { Please use a better name. Capitalize first letter, but also NeedReset() isn't a good name - please make it more descriptive, e.g. ShouldClearSavedMetrics(). https://codereview.chromium.org/1130993005/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_service_client.cc:116: } // namespace Nit: add a blank line after this. https://codereview.chromium.org/1130993005/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_service_client.cc:297: #if defined(OS_ANDROID) Instead of having the ifdefs in 3 places, how about making the function return false on non-Android platforms, so that you can remove the ifdefs from 2 of the places. https://codereview.chromium.org/1130993005/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_service_client.cc:393: if (needReset()) { Nit: No {}'s
Patchset #4 (id:60001) has been deleted
Patchset #4 (id:80001) has been deleted
Changed the function name + nits. Please have one more look. https://codereview.chromium.org/1130993005/diff/40001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/1130993005/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_service_client.cc:109: bool needReset() { On 2015/05/12 16:40:00, Alexei Svitkine wrote: > Please use a better name. > > Capitalize first letter, but also NeedReset() isn't a good name - please make it > more descriptive, e.g. ShouldClearSavedMetrics(). Done. https://codereview.chromium.org/1130993005/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_service_client.cc:116: } // namespace On 2015/05/12 16:40:00, Alexei Svitkine wrote: > Nit: add a blank line after this. Done. https://codereview.chromium.org/1130993005/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_service_client.cc:297: #if defined(OS_ANDROID) On 2015/05/12 16:40:00, Alexei Svitkine wrote: > Instead of having the ifdefs in 3 places, how about making the function return > false on non-Android platforms, so that you can remove the ifdefs from 2 of the > places. Done. https://codereview.chromium.org/1130993005/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_service_client.cc:393: if (needReset()) { On 2015/05/12 16:40:00, Alexei Svitkine wrote: > Nit: No {}'s Done.
lgtm https://codereview.chromium.org/1130993005/diff/100001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/1130993005/diff/100001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_service_client.cc:118: } // anonymous namespace Nit: No need to say "anonymous", just " // namespace" is fine.
Fixed namesapce comment. Submitting now. https://codereview.chromium.org/1130993005/diff/100001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/1130993005/diff/100001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_service_client.cc:118: } // anonymous namespace On 2015/05/13 14:36:02, Alexei Svitkine wrote: > Nit: No need to say "anonymous", just " // namespace" is fine. Done.
The CQ bit was checked by gayane@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/1130993005/#ps120001 (title: "fix namespace comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1130993005/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by gayane@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/1130993005/#ps140001 (title: "fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1130993005/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win8_chromium_rel on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
The CQ bit was checked by gayane@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/1130993005/#ps150001 (title: "fix for win")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1130993005/150001
Message was sent while issue was closed.
Committed patchset #7 (id:150001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/09e9a5f4b8dbc7c599a3ffab70762f49f28d0777 Cr-Commit-Position: refs/heads/master@{#329661} |
