Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1818613002:
Implement UMA log throttling for cellular connections (Closed)
|
Created:
4 years, 9 months ago by gayane -on leave until 09-2017 Modified:
4 years, 8 months ago CC:
chromium-reviews, droger+watchlist_chromium.org, blundell+watchlist_chromium.org, sdefresne+watchlist_chromium.org, jam, darin-cc_chromium.org, asvitkine+watch_chromium.org, bengr Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement UMA log throttling for cellular connections.
For enabling UMA on cellular, a throttling logic is implemented that takes into account the weekly data usage of UMA on cellular as well as the ratio of UMA data usage over overall Chrome data traffic.
For tracking UMA data use and overall Chrome data traffic we use DataUseMeasurement class which already collects and reports similar data.
The allowed weekly UMA quota and UMA ratio are passed through as variation param.
BUG=599586
Committed: https://crrev.com/0b46091cb37b8fdd73a02675e2ab86850d51cb09
Cr-Commit-Position: refs/heads/master@{#385845}
Patch Set 1 #
Total comments: 18
Patch Set 2 : singleton pattern removed #
Total comments: 28
Patch Set 3 : improved design #Patch Set 4 : sync #Patch Set 5 : fix build for gn, fix unittests, add comments #
Total comments: 18
Patch Set 6 : Get callback through metrics service #Patch Set 7 : fix param order #
Total comments: 42
Patch Set 8 : #
Total comments: 26
Patch Set 9 : #Patch Set 10 : #
Total comments: 14
Patch Set 11 : #
Total comments: 15
Patch Set 12 : sync #Patch Set 13 : #
Total comments: 4
Patch Set 14 : nits #
Total comments: 12
Patch Set 15 : #
Total comments: 18
Patch Set 16 : #
Total comments: 10
Patch Set 17 : #Messages
Total messages: 79 (29 generated)
Patchset #1 (id:1) has been deleted
gayane@chromium.org changed reviewers: + asvitkine@chromium.org
Could you please have a look on high level design.
https://codereview.chromium.org/1818613002/diff/20001/chrome/browser/browser_... File chrome/browser/browser_process_impl.cc (right): https://codereview.chromium.org/1818613002/diff/20001/chrome/browser/browser_... chrome/browser/browser_process_impl.cc:850: registry->RegisterDictionaryPref(metrics::prefs::kUmaCellDataUse); Suggest adding a MetricsDataUseMeasurements::RegisterPrefs function and doing this from there. https://codereview.chromium.org/1818613002/diff/20001/components/data_use_mea... File components/data_use_measurement/content/data_use_measurement.cc (right): https://codereview.chromium.org/1818613002/diff/20001/components/data_use_mea... components/data_use_measurement/content/data_use_measurement.cc:99: metrics::MetricsDataUseMeasurements::GetInstance()->UpdateMetricsUsagePrefs( We can discuss more offline, but I would prefer the dependency be passed it to the ctor. Looks like the DataUseMeasurement object is owned by ChromeNetworkDelegate, which is constructed here: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro... So we can probably pass in a pointer to MetricsDataUseMeasurements() to it - that would be owned by MetricsServicesManager. We can chat more about this on Monday. You can wait on these changes until then so we make sure we're clear on what needs to be done here. https://codereview.chromium.org/1818613002/diff/20001/components/metrics/metr... File components/metrics/metrics_data_use_measurements.cc (right): https://codereview.chromium.org/1818613002/diff/20001/components/metrics/metr... components/metrics/metrics_data_use_measurements.cc:29: date_for_testing_(""), Nit: Can remove - std::string has a default ctor. https://codereview.chromium.org/1818613002/diff/20001/components/metrics/metr... components/metrics/metrics_data_use_measurements.cc:34: void MetricsDataUseMeasurements::SetMeasurementDateForTesting(std::string date) { const std::string& https://codereview.chromium.org/1818613002/diff/20001/components/metrics/metr... components/metrics/metrics_data_use_measurements.cc:39: if(date_for_testing_ != "") Nit: !.empty() https://codereview.chromium.org/1818613002/diff/20001/components/metrics/metr... components/metrics/metrics_data_use_measurements.cc:101: int MetricsDataUseMeasurements::TotalDataUse(std::string pref_name) { Nit: Compute* https://codereview.chromium.org/1818613002/diff/20001/components/metrics/metr... components/metrics/metrics_data_use_measurements.cc:106: for (base::DictionaryValue::Iterator it(*pref_dict); !it.IsAtEnd(); What's stopping this from considering data outside the week? https://codereview.chromium.org/1818613002/diff/20001/components/metrics/metr... File components/metrics/metrics_data_use_measurements.h (right): https://codereview.chromium.org/1818613002/diff/20001/components/metrics/metr... components/metrics/metrics_data_use_measurements.h:19: void UpdateMetricsUsagePrefs(const std::string& service_name, int message_size); Suggest running "git cl format" on your CL. https://codereview.chromium.org/1818613002/diff/20001/components/metrics/metr... components/metrics/metrics_data_use_measurements.h:42: base::WeakPtrFactory<MetricsDataUseMeasurements> self_ptr_factory_; Nit: weak_ptr_factory_
Patchset #2 (id:40001) has been deleted
This still needs a lot of polish, but its in a working state of the design that we discussed earlier. Could you have a quick look ? https://codereview.chromium.org/1818613002/diff/20001/chrome/browser/browser_... File chrome/browser/browser_process_impl.cc (right): https://codereview.chromium.org/1818613002/diff/20001/chrome/browser/browser_... chrome/browser/browser_process_impl.cc:850: registry->RegisterDictionaryPref(metrics::prefs::kUmaCellDataUse); On 2016/03/18 22:23:29, asvitkine (no revs until Thu) wrote: > Suggest adding a MetricsDataUseMeasurements::RegisterPrefs function and doing > this from there. Done. https://codereview.chromium.org/1818613002/diff/20001/components/data_use_mea... File components/data_use_measurement/content/data_use_measurement.cc (right): https://codereview.chromium.org/1818613002/diff/20001/components/data_use_mea... components/data_use_measurement/content/data_use_measurement.cc:99: metrics::MetricsDataUseMeasurements::GetInstance()->UpdateMetricsUsagePrefs( On 2016/03/18 22:23:29, Alexei Svitkine wrote: > We can discuss more offline, but I would prefer the dependency be passed it to > the ctor. > > Looks like the DataUseMeasurement object is owned by ChromeNetworkDelegate, > which is constructed here: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro... > > So we can probably pass in a pointer to MetricsDataUseMeasurements() to it - > that would be owned by MetricsServicesManager. We can chat more about this on > Monday. You can wait on these changes until then so we make sure we're clear on > what needs to be done here. Done. https://codereview.chromium.org/1818613002/diff/20001/components/metrics/metr... File components/metrics/metrics_data_use_measurements.cc (right): https://codereview.chromium.org/1818613002/diff/20001/components/metrics/metr... components/metrics/metrics_data_use_measurements.cc:29: date_for_testing_(""), On 2016/03/18 22:23:30, Alexei Svitkine (very slow) wrote: > Nit: Can remove - std::string has a default ctor. Done. https://codereview.chromium.org/1818613002/diff/20001/components/metrics/metr... components/metrics/metrics_data_use_measurements.cc:34: void MetricsDataUseMeasurements::SetMeasurementDateForTesting(std::string date) { On 2016/03/18 22:23:30, Alexei Svitkine (very slow) wrote: > const std::string& Done. https://codereview.chromium.org/1818613002/diff/20001/components/metrics/metr... components/metrics/metrics_data_use_measurements.cc:39: if(date_for_testing_ != "") On 2016/03/18 22:23:30, Alexei Svitkine (very slow) wrote: > Nit: !.empty() Done. https://codereview.chromium.org/1818613002/diff/20001/components/metrics/metr... components/metrics/metrics_data_use_measurements.cc:101: int MetricsDataUseMeasurements::TotalDataUse(std::string pref_name) { On 2016/03/18 22:23:30, Alexei Svitkine (very slow) wrote: > Nit: Compute* Done. https://codereview.chromium.org/1818613002/diff/20001/components/metrics/metr... components/metrics/metrics_data_use_measurements.cc:106: for (base::DictionaryValue::Iterator it(*pref_dict); !it.IsAtEnd(); On 2016/03/18 22:23:30, Alexei Svitkine (very slow) wrote: > What's stopping this from considering data outside the week? nothing. its just this function should be called after RemoveExpiredEntries(); you think it should be called in this function? https://codereview.chromium.org/1818613002/diff/20001/components/metrics/metr... File components/metrics/metrics_data_use_measurements.h (right): https://codereview.chromium.org/1818613002/diff/20001/components/metrics/metr... components/metrics/metrics_data_use_measurements.h:19: void UpdateMetricsUsagePrefs(const std::string& service_name, int message_size); On 2016/03/18 22:23:30, Alexei Svitkine (very slow) wrote: > Suggest running "git cl format" on your CL. hmm, didn't notice it didn't apply to new files https://codereview.chromium.org/1818613002/diff/20001/components/metrics/metr... components/metrics/metrics_data_use_measurements.h:42: base::WeakPtrFactory<MetricsDataUseMeasurements> self_ptr_factory_; On 2016/03/18 22:23:30, Alexei Svitkine (very slow) wrote: > Nit: weak_ptr_factory_ Done.
https://codereview.chromium.org/1818613002/diff/60001/chrome/browser/profiles... File chrome/browser/profiles/profile_io_data.cc (right): https://codereview.chromium.org/1818613002/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_io_data.cc:550: data_use_tracker_ = g_browser_process->metrics_service()->GetDataUseTracker(); I was suggesting to actually store the Callback itself - not the data_use_tracker_. https://codereview.chromium.org/1818613002/diff/60001/components/data_use_mea... File components/data_use_measurement/content/data_use_measurement.cc (right): https://codereview.chromium.org/1818613002/diff/60001/components/data_use_mea... components/data_use_measurement/content/data_use_measurement.cc:102: if (is_cellular) Nit: Combine this with if below. https://codereview.chromium.org/1818613002/diff/60001/components/data_use_mea... File components/data_use_measurement/content/data_use_measurement.h (right): https://codereview.chromium.org/1818613002/diff/60001/components/data_use_mea... components/data_use_measurement/content/data_use_measurement.h:104: DISALLOW_COPY_AND_ASSIGN(DataUseMeasurement); Add an empty line above it. https://codereview.chromium.org/1818613002/diff/60001/components/metrics/data... File components/metrics/data_use_tracker.cc (right): https://codereview.chromium.org/1818613002/diff/60001/components/metrics/data... components/metrics/data_use_tracker.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. 2016 https://codereview.chromium.org/1818613002/diff/60001/components/metrics/data... components/metrics/data_use_tracker.cc:19: static scoped_ptr<metrics::DataUseTracker> instance_; This is not needed anymore, right? https://codereview.chromium.org/1818613002/diff/60001/components/metrics/data... File components/metrics/data_use_tracker.h (right): https://codereview.chromium.org/1818613002/diff/60001/components/metrics/data... components/metrics/data_use_tracker.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. 2016 https://codereview.chromium.org/1818613002/diff/60001/components/metrics/data... components/metrics/data_use_tracker.h:26: base::WeakPtr<DataUseTracker> data_use_tracker); This doesn't need to be exposed in the header - so you can make it a free standing function in the anon namespace. https://codereview.chromium.org/1818613002/diff/60001/components/metrics/data... components/metrics/data_use_tracker.h:66: bool GetUmaQouta(int* qouta); Qouta -> Quota https://codereview.chromium.org/1818613002/diff/60001/components/metrics/data... File components/metrics/data_use_tracker_unittest.cc (right): https://codereview.chromium.org/1818613002/diff/60001/components/metrics/data... components/metrics/data_use_tracker_unittest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. 2016 https://codereview.chromium.org/1818613002/diff/60001/components/metrics/metr... File components/metrics/metrics_pref_names.cc (right): https://codereview.chromium.org/1818613002/diff/60001/components/metrics/metr... components/metrics/metrics_pref_names.cc:178: const char kUserCellDataUse[] = "user_call_datause"; Please prefix these with "user_experience_metrics.". Also, what does the "call" stand for? Did you mean "cell"? https://codereview.chromium.org/1818613002/diff/60001/components/metrics/metr... File components/metrics/metrics_pref_names.h (right): https://codereview.chromium.org/1818613002/diff/60001/components/metrics/metr... components/metrics/metrics_pref_names.h:60: } // namespace prefs Add an empty line above. https://codereview.chromium.org/1818613002/diff/60001/components/metrics/metr... File components/metrics/metrics_service.h (right): https://codereview.chromium.org/1818613002/diff/60001/components/metrics/metr... components/metrics/metrics_service.h:481: base::Callback<void(bool*)> cellular_callback_; Put these members below the friend declarations. https://codereview.chromium.org/1818613002/diff/60001/components/metrics/metr... File components/metrics/metrics_service_unittest.cc (right): https://codereview.chromium.org/1818613002/diff/60001/components/metrics/metr... components/metrics/metrics_service_unittest.cc:31: namespace { Add an empty line below. https://codereview.chromium.org/1818613002/diff/60001/components/metrics/prof... File components/metrics/profiler/profiler_metrics_provider.cc (right): https://codereview.chromium.org/1818613002/diff/60001/components/metrics/prof... components/metrics/profiler/profiler_metrics_provider.cc:129: if (!cellular_callback_.is_null()) { Nit: No {}'s for 1-liner ifs.
Patchset #5 (id:120001) has been deleted
Patchset #5 (id:140001) has been deleted
Patchset #5 (id:160001) has been deleted
Patchset #5 (id:180001) has been deleted
Patchset #6 (id:220001) has been deleted
Patchset #5 (id:200001) has been deleted
Seems like all the bits are passing now. Could you have one more look now. https://codereview.chromium.org/1818613002/diff/60001/chrome/browser/profiles... File chrome/browser/profiles/profile_io_data.cc (right): https://codereview.chromium.org/1818613002/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_io_data.cc:550: data_use_tracker_ = g_browser_process->metrics_service()->GetDataUseTracker(); On 2016/03/29 16:29:10, Alexei Svitkine wrote: > I was suggesting to actually store the Callback itself - not the > data_use_tracker_. Done. https://codereview.chromium.org/1818613002/diff/60001/components/data_use_mea... File components/data_use_measurement/content/data_use_measurement.cc (right): https://codereview.chromium.org/1818613002/diff/60001/components/data_use_mea... components/data_use_measurement/content/data_use_measurement.cc:102: if (is_cellular) On 2016/03/29 16:29:10, Alexei Svitkine wrote: > Nit: Combine this with if below. Done. https://codereview.chromium.org/1818613002/diff/60001/components/data_use_mea... File components/data_use_measurement/content/data_use_measurement.h (right): https://codereview.chromium.org/1818613002/diff/60001/components/data_use_mea... components/data_use_measurement/content/data_use_measurement.h:104: DISALLOW_COPY_AND_ASSIGN(DataUseMeasurement); On 2016/03/29 16:29:10, Alexei Svitkine wrote: > Add an empty line above it. Done. https://codereview.chromium.org/1818613002/diff/60001/components/metrics/data... File components/metrics/data_use_tracker.cc (right): https://codereview.chromium.org/1818613002/diff/60001/components/metrics/data... components/metrics/data_use_tracker.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2016/03/29 16:29:10, Alexei Svitkine wrote: > 2016 Done. https://codereview.chromium.org/1818613002/diff/60001/components/metrics/data... components/metrics/data_use_tracker.cc:19: static scoped_ptr<metrics::DataUseTracker> instance_; On 2016/03/29 16:29:10, Alexei Svitkine wrote: > This is not needed anymore, right? Done. https://codereview.chromium.org/1818613002/diff/60001/components/metrics/data... File components/metrics/data_use_tracker.h (right): https://codereview.chromium.org/1818613002/diff/60001/components/metrics/data... components/metrics/data_use_tracker.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2016/03/29 16:29:10, Alexei Svitkine wrote: > 2016 Done. https://codereview.chromium.org/1818613002/diff/60001/components/metrics/data... components/metrics/data_use_tracker.h:26: base::WeakPtr<DataUseTracker> data_use_tracker); On 2016/03/29 16:29:10, Alexei Svitkine wrote: > This doesn't need to be exposed in the header - so you can make it a free > standing function in the anon namespace. Done. https://codereview.chromium.org/1818613002/diff/60001/components/metrics/data... components/metrics/data_use_tracker.h:66: bool GetUmaQouta(int* qouta); On 2016/03/29 16:29:10, Alexei Svitkine wrote: > Qouta -> Quota Done. https://codereview.chromium.org/1818613002/diff/60001/components/metrics/data... File components/metrics/data_use_tracker_unittest.cc (right): https://codereview.chromium.org/1818613002/diff/60001/components/metrics/data... components/metrics/data_use_tracker_unittest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2016/03/29 16:29:10, Alexei Svitkine wrote: > 2016 Done. https://codereview.chromium.org/1818613002/diff/60001/components/metrics/metr... File components/metrics/metrics_pref_names.cc (right): https://codereview.chromium.org/1818613002/diff/60001/components/metrics/metr... components/metrics/metrics_pref_names.cc:178: const char kUserCellDataUse[] = "user_call_datause"; On 2016/03/29 16:29:10, Alexei Svitkine wrote: > Please prefix these with "user_experience_metrics.". > > Also, what does the "call" stand for? Did you mean "cell"? Done. https://codereview.chromium.org/1818613002/diff/60001/components/metrics/metr... File components/metrics/metrics_pref_names.h (right): https://codereview.chromium.org/1818613002/diff/60001/components/metrics/metr... components/metrics/metrics_pref_names.h:60: } // namespace prefs On 2016/03/29 16:29:10, Alexei Svitkine wrote: > Add an empty line above. Done. https://codereview.chromium.org/1818613002/diff/60001/components/metrics/metr... File components/metrics/metrics_service.h (right): https://codereview.chromium.org/1818613002/diff/60001/components/metrics/metr... components/metrics/metrics_service.h:481: base::Callback<void(bool*)> cellular_callback_; On 2016/03/29 16:29:10, Alexei Svitkine wrote: > Put these members below the friend declarations. Done. https://codereview.chromium.org/1818613002/diff/60001/components/metrics/metr... File components/metrics/metrics_service_unittest.cc (right): https://codereview.chromium.org/1818613002/diff/60001/components/metrics/metr... components/metrics/metrics_service_unittest.cc:31: namespace { On 2016/03/29 16:29:10, Alexei Svitkine wrote: > Add an empty line below. Done. https://codereview.chromium.org/1818613002/diff/60001/components/metrics/prof... File components/metrics/profiler/profiler_metrics_provider.cc (right): https://codereview.chromium.org/1818613002/diff/60001/components/metrics/prof... components/metrics/profiler/profiler_metrics_provider.cc:129: if (!cellular_callback_.is_null()) { On 2016/03/29 16:29:11, Alexei Svitkine wrote: > Nit: No {}'s for 1-liner ifs. Done.
+bengr FYI/heads up For enabling UMA on cellular we'll be adding throttling logic to UMA uploads over cellular that take overall Chrome bandwidth use into account. To support this, this CL is hooking into the data_use_measurement.cc code that your team owns for logging the DataUse. histograms in order to keep track of UMA and overall data usage. We're hoping to get this landed before the M51 branch, which is next week. So giving you a heads up since we'll probably add you as a reviewer on this CL soonish. (At the moment, I'm still doing the initial review on this, so you don't need to start looking just yet, but we'll send it your way for components/data_use_measurement/ OWNERS review once it's ready.)
https://codereview.chromium.org/1818613002/diff/240001/chrome/browser/io_thre... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/1818613002/diff/240001/chrome/browser/io_thre... chrome/browser/io_thread.cc:518: g_browser_process->metrics_service()->GetDataUseTracker(), Instead of having a static GetForwardingCallback() on the DataUseTracker and passing g_browser_process->metrics_service()->GetDataUseTracker() to it, how about doing: g_browser_process->metrics_service()->GetDataUseForwardingCallback(...) Then, this call site (and the other one) only needs to depend on MetricsService and one API function we expose, instead of two. https://codereview.chromium.org/1818613002/diff/240001/chrome/browser/io_thre... File chrome/browser/io_thread.h (right): https://codereview.chromium.org/1818613002/diff/240001/chrome/browser/io_thre... chrome/browser/io_thread.h:603: // thread and passed to // |ChromeNetworkDelegate|. Nit: Remove extra // https://codereview.chromium.org/1818613002/diff/240001/chrome/browser/net/chr... File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/1818613002/diff/240001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate.cc:308: data_use_measurement_.SetMetricsDataUseForwarder(metrics_data_use_forwarder); Can this be a ctor param for that object instead of having a setter? https://codereview.chromium.org/1818613002/diff/240001/chrome/browser/net/chr... File chrome/browser/net/chrome_network_delegate.h (right): https://codereview.chromium.org/1818613002/diff/240001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate.h:74: metrics::UpdateUsagePrefCallbackType metrics_data_use_forwarder); Pass by const ref. Do the same in other places you pass the callback as a param. https://codereview.chromium.org/1818613002/diff/240001/chrome/browser/profile... File chrome/browser/profiles/profile_io_data.h (right): https://codereview.chromium.org/1818613002/diff/240001/chrome/browser/profile... chrome/browser/profiles/profile_io_data.h:601: // thread and passed to // |ChromeNetworkDelegate|. Nit: Remove extra // https://codereview.chromium.org/1818613002/diff/240001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.cc (right): https://codereview.chromium.org/1818613002/diff/240001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:101: net::NetworkChangeNotifier::GetConnectionType()); This already gets queried inside the GetHistogramName() calls above. Can you move this up above that code and make GetHistogramName() take the boolean as a param so this is only done once? https://codereview.chromium.org/1818613002/diff/240001/components/metrics/dat... File components/metrics/data_use_tracker.h (right): https://codereview.chromium.org/1818613002/diff/240001/components/metrics/dat... components/metrics/data_use_tracker.h:19: class DataUseTracker { Add a comment. https://codereview.chromium.org/1818613002/diff/240001/components/metrics/dat... components/metrics/data_use_tracker.h:36: DataUseTracker(PrefService* local_state); explicit https://codereview.chromium.org/1818613002/diff/240001/components/metrics/dat... components/metrics/data_use_tracker.h:37: ~DataUseTracker(); Put ctor and dtor at the top.
Getting callback through metrics service, but metrics service still calls data_use_tracker to get the callback. https://codereview.chromium.org/1818613002/diff/240001/chrome/browser/io_thre... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/1818613002/diff/240001/chrome/browser/io_thre... chrome/browser/io_thread.cc:518: g_browser_process->metrics_service()->GetDataUseTracker(), On 2016/03/31 04:32:25, Alexei Svitkine wrote: > Instead of having a static GetForwardingCallback() on the DataUseTracker and > passing g_browser_process->metrics_service()->GetDataUseTracker() to it, how > about doing: > > g_browser_process->metrics_service()->GetDataUseForwardingCallback(...) > > Then, this call site (and the other one) only needs to depend on MetricsService > and one API function we expose, instead of two. Done. https://codereview.chromium.org/1818613002/diff/240001/chrome/browser/io_thre... File chrome/browser/io_thread.h (right): https://codereview.chromium.org/1818613002/diff/240001/chrome/browser/io_thre... chrome/browser/io_thread.h:603: // thread and passed to // |ChromeNetworkDelegate|. On 2016/03/31 04:32:25, Alexei Svitkine wrote: > Nit: Remove extra // Done. https://codereview.chromium.org/1818613002/diff/240001/chrome/browser/net/chr... File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/1818613002/diff/240001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate.cc:308: data_use_measurement_.SetMetricsDataUseForwarder(metrics_data_use_forwarder); On 2016/03/31 04:32:25, Alexei Svitkine wrote: > Can this be a ctor param for that object instead of having a setter? Done. https://codereview.chromium.org/1818613002/diff/240001/chrome/browser/net/chr... File chrome/browser/net/chrome_network_delegate.h (right): https://codereview.chromium.org/1818613002/diff/240001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate.h:74: metrics::UpdateUsagePrefCallbackType metrics_data_use_forwarder); On 2016/03/31 04:32:25, Alexei Svitkine wrote: > Pass by const ref. Do the same in other places you pass the callback as a param. Done. https://codereview.chromium.org/1818613002/diff/240001/chrome/browser/profile... File chrome/browser/profiles/profile_io_data.h (right): https://codereview.chromium.org/1818613002/diff/240001/chrome/browser/profile... chrome/browser/profiles/profile_io_data.h:601: // thread and passed to // |ChromeNetworkDelegate|. On 2016/03/31 04:32:25, Alexei Svitkine wrote: > Nit: Remove extra // Done. https://codereview.chromium.org/1818613002/diff/240001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.cc (right): https://codereview.chromium.org/1818613002/diff/240001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:101: net::NetworkChangeNotifier::GetConnectionType()); On 2016/03/31 04:32:25, Alexei Svitkine wrote: > This already gets queried inside the GetHistogramName() calls above. Can you > move this up above that code and make GetHistogramName() take the boolean as a > param so this is only done once? Done. https://codereview.chromium.org/1818613002/diff/240001/components/metrics/dat... File components/metrics/data_use_tracker.h (right): https://codereview.chromium.org/1818613002/diff/240001/components/metrics/dat... components/metrics/data_use_tracker.h:19: class DataUseTracker { On 2016/03/31 04:32:25, Alexei Svitkine wrote: > Add a comment. Done. https://codereview.chromium.org/1818613002/diff/240001/components/metrics/dat... components/metrics/data_use_tracker.h:36: DataUseTracker(PrefService* local_state); On 2016/03/31 04:32:25, Alexei Svitkine wrote: > explicit Done. https://codereview.chromium.org/1818613002/diff/240001/components/metrics/dat... components/metrics/data_use_tracker.h:37: ~DataUseTracker(); On 2016/03/31 04:32:25, Alexei Svitkine wrote: > Put ctor and dtor at the top. Done.
Yep - that's what I meant. :) Can you file a crbug to track the work and update the CL description?
Patchset #6 (id:260001) has been deleted
Description was changed from ========== Initial draft for UMA throttling ========== to ========== Implement UMA log throttling for cellular connections. For enabling UMA on cellular implement throttling logic to UMA uploads for cellular connections that take into account the weekly data usage of UMA on cellular as well as the ratio of UMA data usage over overall Chrome data traffic. For tracking UMA data use and overall Chrome data traffic we use connected to DataUseMeasurement class which already collects and reports similar data. BUG=599586 ==========
Description was changed from ========== Implement UMA log throttling for cellular connections. For enabling UMA on cellular implement throttling logic to UMA uploads for cellular connections that take into account the weekly data usage of UMA on cellular as well as the ratio of UMA data usage over overall Chrome data traffic. For tracking UMA data use and overall Chrome data traffic we use connected to DataUseMeasurement class which already collects and reports similar data. BUG=599586 ========== to ========== Implement UMA log throttling for cellular connections. For enabling UMA on cellular implement throttling logic to UMA uploads for cellular connections that take into account the weekly data usage of UMA on cellular as well as the ratio of UMA data usage over overall Chrome data traffic. For tracking UMA data use and overall Chrome data traffic we use connected to DataUseMeasurement class which already collects and reports similar data. The allowed weekly UMA quota and UMA ratio are passed through Finch experiment. BUG=599586 ==========
https://codereview.chromium.org/1818613002/diff/300001/components/metrics/dat... File components/metrics/data_use_tracker.cc (right): https://codereview.chromium.org/1818613002/diff/300001/components/metrics/dat... components/metrics/data_use_tracker.cc:18: void UpdateMetricsUsagePrefs( Add a 1-line comment. Also, put empty lines within the namespace. https://codereview.chromium.org/1818613002/diff/300001/components/metrics/dat... components/metrics/data_use_tracker.cc:53: RemoveExpiredEntries(); Nit: Add a blank line below. https://codereview.chromium.org/1818613002/diff/300001/components/metrics/dat... components/metrics/data_use_tracker.cc:56: bool is_quota_specified = GetUmaQuota(&uma_quota); Nit: Inline this into the if below and have a separate if for the math. When you have a separate if for the math, you can move line 54 to just be above that if. https://codereview.chromium.org/1818613002/diff/300001/components/metrics/dat... components/metrics/data_use_tracker.cc:61: bool is_ratio_specified = GetUmaRatio(&uma_ratio); Nit: Just inline this into the if below. https://codereview.chromium.org/1818613002/diff/300001/components/metrics/dat... components/metrics/data_use_tracker.cc:67: (double)(log_bytes + user_total_data_use) <= static_cast<double> https://codereview.chromium.org/1818613002/diff/300001/components/metrics/dat... components/metrics/data_use_tracker.cc:73: int message_size) { Add a ThreadChecker member in this class and DCHECK on it here and in the other non-static methods. https://codereview.chromium.org/1818613002/diff/300001/components/metrics/dat... components/metrics/data_use_tracker.cc:84: local_state_->GetDictionary(pref_name)->GetInteger(todays_key, Is there any chance local_state_->GetDictionary(pref_name) is null? I notice you check for it in the function below. Also, please add a comment below about what cases it can be null in. https://codereview.chromium.org/1818613002/diff/300001/components/metrics/dat... components/metrics/data_use_tracker.cc:173: void DataUseTracker::SetMeasurementDateForTesting(const std::string& date) { Instead of these setters, I suggest just making the Get*() methods virtual and overriding them in a Test subclass of DataUseTracker. Then, you can simplify this code a lot (no need to have the members and the ifs. https://codereview.chromium.org/1818613002/diff/300001/components/metrics/dat... File components/metrics/data_use_tracker_unittest.cc (right): https://codereview.chromium.org/1818613002/diff/300001/components/metrics/dat... components/metrics/data_use_tracker_unittest.cc:15: namespace { Add an empty line below. https://codereview.chromium.org/1818613002/diff/300001/components/metrics/dat... components/metrics/data_use_tracker_unittest.cc:34: void SetPrefTestValuesOverRatio(PrefService* local_state) { Add a 1-line comment to these methods. https://codereview.chromium.org/1818613002/diff/300001/components/metrics/met... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/1818613002/diff/300001/components/metrics/met... components/metrics/metrics_service.cc:559: UpdateUsagePrefCallbackType MetricsService::GetDataUseForwardingCallback() { Add a thread check here please. (DCHECK(IsSingleThreaded()); ) https://codereview.chromium.org/1818613002/diff/300001/components/metrics/met... components/metrics/metrics_service.cc:896: scheduler_->UploadCancelled(); This logic doesn't seem correct. What if we're not cellular - this will always cancel the upload, no? https://codereview.chromium.org/1818613002/diff/300001/components/metrics/met... File components/metrics/metrics_service.h (right): https://codereview.chromium.org/1818613002/diff/300001/components/metrics/met... components/metrics/metrics_service.h:85: // Creates the MetricsService as above with given |cellular_callback| Instead of having a callback as a ctor param, can't it just be a function of the client interface? Isn't it already? https://codereview.chromium.org/1818613002/diff/300001/components/metrics/met... components/metrics/metrics_service.h:225: // |data_use_tracker| weak pointer. Should be called on UI thread. This description reads like implementation detail. Can you instead talk about the purpose of this. Also, mention that the callback should be safe to use from any thread (don't just say its only for UI thread), although this function should be called on UI thread. https://codereview.chromium.org/1818613002/diff/300001/ios/chrome/browser/met... File ios/chrome/browser/metrics/ios_chrome_metrics_service_client.cc (right): https://codereview.chromium.org/1818613002/diff/300001/ios/chrome/browser/met... ios/chrome/browser/metrics/ios_chrome_metrics_service_client.cc:61: // is assigned to experimental group for enabled cellular uploads. What's the reason for this change? Seems like this is only used directly below and for ProfilerMetricsProvider - neither of which seem like they need the change. Am I missing something? Is it because you're not constructing metrics service with the new callback? Does it really need the version of the callback that has an out param?
https://codereview.chromium.org/1818613002/diff/300001/components/components_... File components/components_tests.gyp (left): https://codereview.chromium.org/1818613002/diff/300001/components/components_... components/components_tests.gyp:246: ], What happened here? https://codereview.chromium.org/1818613002/diff/300001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.h (right): https://codereview.chromium.org/1818613002/diff/300001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:37: DataUseMeasurement( Add explicit https://codereview.chromium.org/1818613002/diff/300001/components/metrics/dat... File components/metrics/data_use_tracker.cc (right): https://codereview.chromium.org/1818613002/diff/300001/components/metrics/dat... components/metrics/data_use_tracker.cc:113: if (key_date > week_ago) Can you just add this logic to ComputeTotalDataUse() and remove the need for the extra function? i.e. it can sum up the ones that are in range and remove the others. https://codereview.chromium.org/1818613002/diff/300001/components/metrics/dat... File components/metrics/data_use_tracker.h (right): https://codereview.chromium.org/1818613002/diff/300001/components/metrics/dat... components/metrics/data_use_tracker.h:39: base::WeakPtr<DataUseTracker> GetWeakPtr(); Is this needed anymore? https://codereview.chromium.org/1818613002/diff/300001/components/metrics/dat... components/metrics/data_use_tracker.h:85: }; Add DISALLOW_COPY_AND_ASSIGN()
Thanks Alexei for all the comments. please have one more look https://codereview.chromium.org/1818613002/diff/300001/components/components_... File components/components_tests.gyp (left): https://codereview.chromium.org/1818613002/diff/300001/components/components_... components/components_tests.gyp:246: ], On 2016/03/31 20:59:31, Alexei Svitkine wrote: > What happened here? i think this was automatically done. but then i renamed the file and deleted it as I think a separate source for one file is not necessary. https://codereview.chromium.org/1818613002/diff/300001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.h (right): https://codereview.chromium.org/1818613002/diff/300001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:37: DataUseMeasurement( On 2016/03/31 20:59:31, Alexei Svitkine wrote: > Add explicit Done. https://codereview.chromium.org/1818613002/diff/300001/components/metrics/dat... File components/metrics/data_use_tracker.cc (right): https://codereview.chromium.org/1818613002/diff/300001/components/metrics/dat... components/metrics/data_use_tracker.cc:18: void UpdateMetricsUsagePrefs( On 2016/03/31 20:00:39, Alexei Svitkine wrote: > Add a 1-line comment. > > Also, put empty lines within the namespace. Done. https://codereview.chromium.org/1818613002/diff/300001/components/metrics/dat... components/metrics/data_use_tracker.cc:53: RemoveExpiredEntries(); On 2016/03/31 20:00:39, Alexei Svitkine wrote: > Nit: Add a blank line below. Done. https://codereview.chromium.org/1818613002/diff/300001/components/metrics/dat... components/metrics/data_use_tracker.cc:56: bool is_quota_specified = GetUmaQuota(&uma_quota); On 2016/03/31 20:00:39, Alexei Svitkine wrote: > Nit: Inline this into the if below and have a separate if for the math. When you > have a separate if for the math, you can move line 54 to just be above that if. I am not sure I got it right. https://codereview.chromium.org/1818613002/diff/300001/components/metrics/dat... components/metrics/data_use_tracker.cc:61: bool is_ratio_specified = GetUmaRatio(&uma_ratio); On 2016/03/31 20:00:39, Alexei Svitkine wrote: > Nit: Just inline this into the if below. Done. https://codereview.chromium.org/1818613002/diff/300001/components/metrics/dat... components/metrics/data_use_tracker.cc:67: (double)(log_bytes + user_total_data_use) <= On 2016/03/31 20:00:39, Alexei Svitkine wrote: > static_cast<double> Done. https://codereview.chromium.org/1818613002/diff/300001/components/metrics/dat... components/metrics/data_use_tracker.cc:73: int message_size) { On 2016/03/31 20:00:39, Alexei Svitkine wrote: > Add a ThreadChecker member in this class and DCHECK on it here and in the other > non-static methods. Done. https://codereview.chromium.org/1818613002/diff/300001/components/metrics/dat... components/metrics/data_use_tracker.cc:84: local_state_->GetDictionary(pref_name)->GetInteger(todays_key, On 2016/03/31 20:00:39, Alexei Svitkine wrote: > Is there any chance local_state_->GetDictionary(pref_name) is null? I notice you > check for it in the function below. > > Also, please add a comment below about what cases it can be null in. added the check https://codereview.chromium.org/1818613002/diff/300001/components/metrics/dat... components/metrics/data_use_tracker.cc:113: if (key_date > week_ago) On 2016/03/31 20:59:31, Alexei Svitkine wrote: > Can you just add this logic to ComputeTotalDataUse() and remove the need for the > extra function? > > i.e. it can sum up the ones that are in range and remove the others. I guess I can do that, but then I need to name the function something like FilterAndComputeTotalDataUse and I cannot test them separately anymore https://codereview.chromium.org/1818613002/diff/300001/components/metrics/dat... components/metrics/data_use_tracker.cc:173: void DataUseTracker::SetMeasurementDateForTesting(const std::string& date) { On 2016/03/31 20:00:39, Alexei Svitkine wrote: > Instead of these setters, I suggest just making the Get*() methods virtual and > overriding them in a Test subclass of DataUseTracker. Then, you can simplify > this code a lot (no need to have the members and the ifs. Done. https://codereview.chromium.org/1818613002/diff/300001/components/metrics/dat... File components/metrics/data_use_tracker.h (right): https://codereview.chromium.org/1818613002/diff/300001/components/metrics/dat... components/metrics/data_use_tracker.h:39: base::WeakPtr<DataUseTracker> GetWeakPtr(); On 2016/03/31 20:59:31, Alexei Svitkine wrote: > Is this needed anymore? Done. https://codereview.chromium.org/1818613002/diff/300001/components/metrics/dat... components/metrics/data_use_tracker.h:85: }; On 2016/03/31 20:59:31, Alexei Svitkine wrote: > Add DISALLOW_COPY_AND_ASSIGN() Done. https://codereview.chromium.org/1818613002/diff/300001/components/metrics/dat... File components/metrics/data_use_tracker_unittest.cc (right): https://codereview.chromium.org/1818613002/diff/300001/components/metrics/dat... components/metrics/data_use_tracker_unittest.cc:15: namespace { On 2016/03/31 20:00:39, Alexei Svitkine wrote: > Add an empty line below. Done. https://codereview.chromium.org/1818613002/diff/300001/components/metrics/dat... components/metrics/data_use_tracker_unittest.cc:34: void SetPrefTestValuesOverRatio(PrefService* local_state) { On 2016/03/31 20:00:39, Alexei Svitkine wrote: > Add a 1-line comment to these methods. Done. https://codereview.chromium.org/1818613002/diff/300001/components/metrics/met... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/1818613002/diff/300001/components/metrics/met... components/metrics/metrics_service.cc:559: UpdateUsagePrefCallbackType MetricsService::GetDataUseForwardingCallback() { On 2016/03/31 20:00:40, Alexei Svitkine wrote: > Add a thread check here please. (DCHECK(IsSingleThreaded()); ) Done. https://codereview.chromium.org/1818613002/diff/300001/components/metrics/met... components/metrics/metrics_service.cc:896: scheduler_->UploadCancelled(); On 2016/03/31 20:00:40, Alexei Svitkine wrote: > This logic doesn't seem correct. What if we're not cellular - this will always > cancel the upload, no? Done. https://codereview.chromium.org/1818613002/diff/300001/components/metrics/met... File components/metrics/metrics_service.h (right): https://codereview.chromium.org/1818613002/diff/300001/components/metrics/met... components/metrics/metrics_service.h:85: // Creates the MetricsService as above with given |cellular_callback| On 2016/03/31 20:00:40, Alexei Svitkine wrote: > Instead of having a callback as a ctor param, can't it just be a function of the > client interface? Isn't it already? I think we tried to do this once and I think metrics service cannot depend on client. https://codereview.chromium.org/1818613002/diff/300001/components/metrics/met... components/metrics/metrics_service.h:225: // |data_use_tracker| weak pointer. Should be called on UI thread. On 2016/03/31 20:00:40, Alexei Svitkine wrote: > This description reads like implementation detail. Can you instead talk about > the purpose of this. Also, mention that the callback should be safe to use from > any thread (don't just say its only for UI thread), although this function > should be called on UI thread. Done. https://codereview.chromium.org/1818613002/diff/300001/ios/chrome/browser/met... File ios/chrome/browser/metrics/ios_chrome_metrics_service_client.cc (right): https://codereview.chromium.org/1818613002/diff/300001/ios/chrome/browser/met... ios/chrome/browser/metrics/ios_chrome_metrics_service_client.cc:61: // is assigned to experimental group for enabled cellular uploads. On 2016/03/31 20:00:40, Alexei Svitkine wrote: > What's the reason for this change? Seems like this is only used directly below > and for ProfilerMetricsProvider - neither of which seem like they need the > change. Am I missing something? > > Is it because you're not constructing metrics service with the new callback? > Does it really need the version of the callback that has an out param? I changed this function not to have a return value because I moved it from anon namespace to be a member function and had to bind with weakptr. But actually I don't remember why I needed it to be a member function.
https://codereview.chromium.org/1818613002/diff/300001/components/metrics/dat... File components/metrics/data_use_tracker.cc (right): https://codereview.chromium.org/1818613002/diff/300001/components/metrics/dat... components/metrics/data_use_tracker.cc:113: if (key_date > week_ago) On 2016/03/31 21:55:09, gayane wrote: > On 2016/03/31 20:59:31, Alexei Svitkine wrote: > > Can you just add this logic to ComputeTotalDataUse() and remove the need for > the > > extra function? > > > > i.e. it can sum up the ones that are in range and remove the others. > > I guess I can do that, but then I need to name the function something like > FilterAndComputeTotalDataUse > and I cannot test them separately anymore Okay, keeping them separate for tests makes sense. https://codereview.chromium.org/1818613002/diff/300001/components/metrics/met... File components/metrics/metrics_service.h (right): https://codereview.chromium.org/1818613002/diff/300001/components/metrics/met... components/metrics/metrics_service.h:85: // Creates the MetricsService as above with given |cellular_callback| On 2016/03/31 21:55:10, gayane wrote: > On 2016/03/31 20:00:40, Alexei Svitkine wrote: > > Instead of having a callback as a ctor param, can't it just be a function of > the > > client interface? Isn't it already? > > I think we tried to do this once and I think metrics service cannot depend on > client. I think it should work. MetricsService already depends on client for a number of other things (e.g. client->GetRegistryBackupKey() right in its ctor). https://codereview.chromium.org/1818613002/diff/320001/components/metrics/dat... File components/metrics/data_use_tracker.cc (right): https://codereview.chromium.org/1818613002/diff/320001/components/metrics/dat... components/metrics/data_use_tracker.cc:60: int uma_quota; Nit: uma_weekly_quota_bytes https://codereview.chromium.org/1818613002/diff/320001/components/metrics/dat... components/metrics/data_use_tracker.cc:65: if (log_bytes + uma_total_data_use <= uma_quota) Nit: Make a var for log_bytes + log_bytes + uma_total_data_use since you use this expression below too. https://codereview.chromium.org/1818613002/diff/320001/components/metrics/dat... components/metrics/data_use_tracker.cc:72: int user_total_data_use = ComputeTotalDataUse(prefs::kUserCellDataUse); Please add a comment above both this and the check above explaining the reasoning. https://codereview.chromium.org/1818613002/diff/320001/components/metrics/dat... components/metrics/data_use_tracker.cc:96: if (user_pref_dict) { Nit: No {}'s Add a comment explaining when it will be null. https://codereview.chromium.org/1818613002/diff/320001/components/metrics/dat... components/metrics/data_use_tracker.cc:122: base::Time week_ago = current_date - base::TimeDelta::FromDays(7); Move these lines outside of the for loop and make the vars const. https://codereview.chromium.org/1818613002/diff/320001/components/metrics/dat... components/metrics/data_use_tracker.cc:172: DCHECK(thread_checker_.CalledOnValidThread()); Nit: Add an empty line after each line like this. https://codereview.chromium.org/1818613002/diff/320001/components/metrics/dat... File components/metrics/data_use_tracker.h (right): https://codereview.chromium.org/1818613002/diff/320001/components/metrics/dat... components/metrics/data_use_tracker.h:36: // Returns whether a log with provided |log_bytes| can be uploaded according Remove extra space after Returns https://codereview.chromium.org/1818613002/diff/320001/components/metrics/dat... components/metrics/data_use_tracker.h:37: // to data use ratio and UMA quota provided by finch experiment. Finch is an internal codename - don't use it in code. https://codereview.chromium.org/1818613002/diff/320001/components/metrics/dat... components/metrics/data_use_tracker.h:38: bool CanUploadUMALog(int log_bytes); Nit: "ShouldUploadLogOnCellular" https://codereview.chromium.org/1818613002/diff/320001/components/metrics/dat... File components/metrics/data_use_tracker_unittest.cc (right): https://codereview.chromium.org/1818613002/diff/320001/components/metrics/dat... components/metrics/data_use_tracker_unittest.cc:73: class MockDataUseTracker : public DataUseTracker { This looks like a Fake, not a Mock. http://stackoverflow.com/questions/346372/whats-the-difference-between-faking... https://codereview.chromium.org/1818613002/diff/320001/components/metrics/dat... components/metrics/data_use_tracker_unittest.cc:94: }; Move this class to the anon namespace above. Probably above the two free standing functions. https://codereview.chromium.org/1818613002/diff/320001/components/metrics/met... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/1818613002/diff/320001/components/metrics/met... components/metrics/metrics_service.cc:895: log_manager_.staged_log_hash().size())) Nit: Add {}'s https://codereview.chromium.org/1818613002/diff/320001/components/metrics/met... File components/metrics/metrics_service.h (right): https://codereview.chromium.org/1818613002/diff/320001/components/metrics/met... components/metrics/metrics_service.h:225: // from any thread, but this function should be called on UI thread. Nit: Remove extra space before "this"
Patchset #10 (id:360001) has been deleted
Patchset #9 (id:340001) has been deleted
I have addressed all the comments and also removed IsCellularLogicEnabled function from MetricsService construtor https://codereview.chromium.org/1818613002/diff/320001/components/metrics/dat... File components/metrics/data_use_tracker.cc (right): https://codereview.chromium.org/1818613002/diff/320001/components/metrics/dat... components/metrics/data_use_tracker.cc:60: int uma_quota; On 2016/03/31 22:18:03, Alexei Svitkine wrote: > Nit: uma_weekly_quota_bytes Done. https://codereview.chromium.org/1818613002/diff/320001/components/metrics/dat... components/metrics/data_use_tracker.cc:65: if (log_bytes + uma_total_data_use <= uma_quota) On 2016/03/31 22:18:03, Alexei Svitkine wrote: > Nit: Make a var for log_bytes + log_bytes + uma_total_data_use since you use > this expression below too. Done. https://codereview.chromium.org/1818613002/diff/320001/components/metrics/dat... components/metrics/data_use_tracker.cc:72: int user_total_data_use = ComputeTotalDataUse(prefs::kUserCellDataUse); On 2016/03/31 22:18:02, Alexei Svitkine wrote: > Please add a comment above both this and the check above explaining the > reasoning. Done. https://codereview.chromium.org/1818613002/diff/320001/components/metrics/dat... components/metrics/data_use_tracker.cc:96: if (user_pref_dict) { On 2016/03/31 22:18:03, Alexei Svitkine wrote: > Nit: No {}'s > > Add a comment explaining when it will be null. Done. https://codereview.chromium.org/1818613002/diff/320001/components/metrics/dat... components/metrics/data_use_tracker.cc:122: base::Time week_ago = current_date - base::TimeDelta::FromDays(7); On 2016/03/31 22:18:03, Alexei Svitkine wrote: > Move these lines outside of the for loop and make the vars const. Done. https://codereview.chromium.org/1818613002/diff/320001/components/metrics/dat... components/metrics/data_use_tracker.cc:172: DCHECK(thread_checker_.CalledOnValidThread()); On 2016/03/31 22:18:03, Alexei Svitkine wrote: > Nit: Add an empty line after each line like this. Done. https://codereview.chromium.org/1818613002/diff/320001/components/metrics/dat... File components/metrics/data_use_tracker.h (right): https://codereview.chromium.org/1818613002/diff/320001/components/metrics/dat... components/metrics/data_use_tracker.h:36: // Returns whether a log with provided |log_bytes| can be uploaded according On 2016/03/31 22:18:03, Alexei Svitkine wrote: > Remove extra space after Returns Done. https://codereview.chromium.org/1818613002/diff/320001/components/metrics/dat... components/metrics/data_use_tracker.h:37: // to data use ratio and UMA quota provided by finch experiment. On 2016/03/31 22:18:03, Alexei Svitkine wrote: > Finch is an internal codename - don't use it in code. Done. https://codereview.chromium.org/1818613002/diff/320001/components/metrics/dat... components/metrics/data_use_tracker.h:38: bool CanUploadUMALog(int log_bytes); On 2016/03/31 22:18:03, Alexei Svitkine wrote: > Nit: "ShouldUploadLogOnCellular" Done. https://codereview.chromium.org/1818613002/diff/320001/components/metrics/dat... File components/metrics/data_use_tracker_unittest.cc (right): https://codereview.chromium.org/1818613002/diff/320001/components/metrics/dat... components/metrics/data_use_tracker_unittest.cc:73: class MockDataUseTracker : public DataUseTracker { On 2016/03/31 22:18:03, Alexei Svitkine wrote: > This looks like a Fake, not a Mock. > > http://stackoverflow.com/questions/346372/whats-the-difference-between-faking... Done. https://codereview.chromium.org/1818613002/diff/320001/components/metrics/dat... components/metrics/data_use_tracker_unittest.cc:94: }; On 2016/03/31 22:18:03, Alexei Svitkine wrote: > Move this class to the anon namespace above. Probably above the two free > standing functions. Done. https://codereview.chromium.org/1818613002/diff/320001/components/metrics/met... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/1818613002/diff/320001/components/metrics/met... components/metrics/metrics_service.cc:895: log_manager_.staged_log_hash().size())) On 2016/03/31 22:18:03, Alexei Svitkine wrote: > Nit: Add {}'s Done. https://codereview.chromium.org/1818613002/diff/320001/components/metrics/met... File components/metrics/metrics_service.h (right): https://codereview.chromium.org/1818613002/diff/320001/components/metrics/met... components/metrics/metrics_service.h:225: // from any thread, but this function should be called on UI thread. On 2016/03/31 22:18:03, Alexei Svitkine wrote: > Nit: Remove extra space before "this" Done.
Can you also proofread the CL description? For example, the second sentence is a bit run-on and I think the one after that reads a bit weirdly. Please rephrase to make them a bit clearer. Thanks! https://codereview.chromium.org/1818613002/diff/380001/components/metrics/dat... File components/metrics/data_use_tracker.cc (right): https://codereview.chromium.org/1818613002/diff/380001/components/metrics/dat... components/metrics/data_use_tracker.cc:79: // then the log should be uploaded and vice verse. verse -> versa https://codereview.chromium.org/1818613002/diff/380001/components/metrics/dat... components/metrics/data_use_tracker.cc:80: return (new_uma_total_data_use) / Nit: No need for ()'s here now. https://codereview.chromium.org/1818613002/diff/380001/components/metrics/dat... components/metrics/data_use_tracker.cc:105: // The pref should exists as long as they are properly registered. That doesn't tell me when it *shouldn't* exist. i.e. why is the if necessary? When wouldn't they be properly registered? https://codereview.chromium.org/1818613002/diff/380001/components/metrics/dat... File components/metrics/data_use_tracker_unittest.cc (right): https://codereview.chromium.org/1818613002/diff/380001/components/metrics/dat... components/metrics/data_use_tracker_unittest.cc:56: }; Nit: add private: DISALLOW_COPY_AND_ASSIGN() Same for the class above. https://codereview.chromium.org/1818613002/diff/380001/components/metrics/met... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/1818613002/diff/380001/components/metrics/met... components/metrics/metrics_service.cc:883: cellular_callback_.Run(&is_cellular); This should be using the client interface, right? Also, remove the member.
Patchset #9 (id:380001) has been deleted
Description was changed from ========== Implement UMA log throttling for cellular connections. For enabling UMA on cellular implement throttling logic to UMA uploads for cellular connections that take into account the weekly data usage of UMA on cellular as well as the ratio of UMA data usage over overall Chrome data traffic. For tracking UMA data use and overall Chrome data traffic we use connected to DataUseMeasurement class which already collects and reports similar data. The allowed weekly UMA quota and UMA ratio are passed through Finch experiment. BUG=599586 ========== to ========== Implement UMA log throttling for cellular connections. For enabling UMA on cellular throttling logic is implemented that takes into account the weekly data usage of UMA on cellular as well as the ratio of UMA data usage over overall Chrome data traffic. For tracking UMA data use and overall Chrome data traffic we use DataUseMeasurement class which already collects and reports similar data. The allowed weekly UMA quota and UMA ratio are passed through as variation param. BUG=599586 ==========
Unfortunately I realized I forgot to change metrics service to use the client and uploaded a new patch and deleted the other one. I thought I have done it fast enough, but you were faster. So your comments are not on the patch. Please see them bellow. Can you also proofread the CL description? For example, the second sentence is a bit run-on and I think the one after that reads a bit weirdly. Please rephrase to make them a bit clearer. Thanks! Done. https://codereview.chromium.org/1818613002/diff/380001/components/metrics/dat... File components/metrics/data_use_tracker.cc (right): https://codereview.chromium.org/1818613002/diff/380001/components/metrics/dat... components/metrics/data_use_tracker.cc:79: // then the log should be uploaded and vice verse. verse -> versa Done. https://codereview.chromium.org/1818613002/diff/380001/components/metrics/dat... components/metrics/data_use_tracker.cc:80: return (new_uma_total_data_use) / Nit: No need for ()'s here now. Done. https://codereview.chromium.org/1818613002/diff/380001/components/metrics/dat... components/metrics/data_use_tracker.cc:105: // The pref should exists as long as they are properly registered. That doesn't tell me when it *shouldn't* exist. i.e. why is the if necessary? When wouldn't they be properly registered? I added the ifs just in case. I can remove them. https://codereview.chromium.org/1818613002/diff/380001/components/metrics/dat... File components/metrics/data_use_tracker_unittest.cc (right): https://codereview.chromium.org/1818613002/diff/380001/components/metrics/dat... components/metrics/data_use_tracker_unittest.cc:56: }; Nit: add private: DISALLOW_COPY_AND_ASSIGN() Same for the class above. Done. https://codereview.chromium.org/1818613002/diff/380001/components/metrics/met... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/1818613002/diff/380001/components/metrics/met... components/metrics/metrics_service.cc:883: cellular_callback_.Run(&is_cellular); This should be using the client interface, right? Also, remove the member. Done.
Description was changed from ========== Implement UMA log throttling for cellular connections. For enabling UMA on cellular throttling logic is implemented that takes into account the weekly data usage of UMA on cellular as well as the ratio of UMA data usage over overall Chrome data traffic. For tracking UMA data use and overall Chrome data traffic we use DataUseMeasurement class which already collects and reports similar data. The allowed weekly UMA quota and UMA ratio are passed through as variation param. BUG=599586 ========== to ========== Implement UMA log throttling for cellular connections. For enabling UMA on cellular, a throttling logic is implemented that takes into account the weekly data usage of UMA on cellular as well as the ratio of UMA data usage over overall Chrome data traffic. For tracking UMA data use and overall Chrome data traffic we use DataUseMeasurement class which already collects and reports similar data. The allowed weekly UMA quota and UMA ratio are passed through as variation param. BUG=599586 ==========
On Fri, Apr 1, 2016 at 4:31 PM, <gayane@chromium.org> wrote: > Unfortunately I realized I forgot to change metrics service to use the > client > and uploaded a new patch and deleted the other one. I thought I have done > it > fast enough, but you were faster. So your comments are not on the patch. > Please > see them bellow. > > > Can you also proofread the CL description? For example, the second > sentence is a > bit run-on and I think the one after that reads a bit weirdly. Please > rephrase > to make them a bit clearer. Thanks! > > Done. > > > https://codereview.chromium.org/1818613002/diff/380001/components/metrics/dat... > File components/metrics/data_use_tracker.cc (right): > > > https://codereview.chromium.org/1818613002/diff/380001/components/metrics/dat... > components/metrics/data_use_tracker.cc:79: // then the log should be > uploaded and vice verse. > verse -> versa > > Done. > > > https://codereview.chromium.org/1818613002/diff/380001/components/metrics/dat... > components/metrics/data_use_tracker.cc:80: return > (new_uma_total_data_use) / > Nit: No need for ()'s here now. > > Done. > > > https://codereview.chromium.org/1818613002/diff/380001/components/metrics/dat... > components/metrics/data_use_tracker.cc:105: // The pref should exists as > long as they are properly registered. > That doesn't tell me when it *shouldn't* exist. > > i.e. why is the if necessary? When wouldn't they be properly registered? > > I added the ifs just in case. I can remove them. > I suggest removing them, then if we don't think there's a real case where they shouldn't be there. > > > https://codereview.chromium.org/1818613002/diff/380001/components/metrics/dat... > File components/metrics/data_use_tracker_unittest.cc (right): > > > https://codereview.chromium.org/1818613002/diff/380001/components/metrics/dat... > components/metrics/data_use_tracker_unittest.cc:56: }; > Nit: add > > private: > DISALLOW_COPY_AND_ASSIGN() > > Same for the class above. > > Done. > > > https://codereview.chromium.org/1818613002/diff/380001/components/metrics/met... > File components/metrics/metrics_service.cc (right): > > > https://codereview.chromium.org/1818613002/diff/380001/components/metrics/met... > components/metrics/metrics_service.cc:883: > cellular_callback_.Run(&is_cellular); > This should be using the client interface, right? Also, remove the > member. > > Done. > > https://codereview.chromium.org/1818613002/ > -- 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.
https://codereview.chromium.org/1818613002/diff/420001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.h (right): https://codereview.chromium.org/1818613002/diff/420001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:17: #include "components/metrics/data_use_tracker.h" The only reason we have this dependency is because I use metrics::UpdateUsagePrefCallbackType here. If I just use base::Callback<void(const std::string&, int)> instead we can remove the dependency. What you think ?
asvitkine@chromium.org changed reviewers: + bengr@chromium.org
Thanks, looks good now. Adding Ben for owners review of components/data_use_measurement. https://codereview.chromium.org/1818613002/diff/420001/components/components_... File components/components_tests.gyp (left): https://codereview.chromium.org/1818613002/diff/420001/components/components_... components/components_tests.gyp:246: ], Please restore this and line 962 below. (This is the existing test in components/data_use_measurement). https://codereview.chromium.org/1818613002/diff/420001/components/metrics/dat... File components/metrics/data_use_tracker.h (right): https://codereview.chromium.org/1818613002/diff/420001/components/metrics/dat... components/metrics/data_use_tracker.h:32: // |data_use_tracker| weak pointer. Should be called on UI thread. |data_use_tracker| is not a thing. Please update the comment. https://codereview.chromium.org/1818613002/diff/420001/components/metrics/dat... components/metrics/data_use_tracker.h:66: virtual bool GetUmaWeeaklyQuota(int* uma_weekly_quota_bytes); Comments please. https://codereview.chromium.org/1818613002/diff/420001/components/metrics/dat... File components/metrics/data_use_tracker_unittest.cc (right): https://codereview.chromium.org/1818613002/diff/420001/components/metrics/dat... components/metrics/data_use_tracker_unittest.cc:33: private: Add an empty line above private: Same below.
https://codereview.chromium.org/1818613002/diff/420001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.h (right): https://codereview.chromium.org/1818613002/diff/420001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:17: #include "components/metrics/data_use_tracker.h" On 2016/04/01 20:34:44, gayane wrote: > The only reason we have this dependency is because I use > metrics::UpdateUsagePrefCallbackType here. If I just use > base::Callback<void(const std::string&, int)> instead we can remove the > dependency. What you think ? Yeah I thought of this too. I'm ok with this, but let's see what Ben says as OWNER.
asvitkine@chromium.org changed reviewers: + mmenke@chromium.org
Also looping in +mmenke for the changes in: chrome/browser/io_thread.* chrome/browser/net/* chrome/browser/profiles/*
Patchset #11 (id:440001) has been deleted
https://codereview.chromium.org/1818613002/diff/420001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.cc (right): https://codereview.chromium.org/1818613002/diff/420001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:110: total_upload_bytes + total_received_bytes); This seems like way too many posted tasks for too little gain - one for every redirect, one for every URLRequest. Can we just jump over to the IOThread and grab this whenever we need it, or accumulate data and only periodically post a task or something? The are certainly issues related to the fact that this is a per-profile class.
gayane@chromium.org changed reviewers: - bengr@chromium.org, mmenke@chromium.org
Done. https://codereview.chromium.org/1818613002/diff/420001/components/components_... File components/components_tests.gyp (left): https://codereview.chromium.org/1818613002/diff/420001/components/components_... components/components_tests.gyp:246: ], On 2016/04/01 20:38:58, Alexei Svitkine wrote: > Please restore this and line 962 below. > > (This is the existing test in components/data_use_measurement). Done. https://codereview.chromium.org/1818613002/diff/420001/components/metrics/dat... File components/metrics/data_use_tracker.h (right): https://codereview.chromium.org/1818613002/diff/420001/components/metrics/dat... components/metrics/data_use_tracker.h:32: // |data_use_tracker| weak pointer. Should be called on UI thread. On 2016/04/01 20:38:58, Alexei Svitkine wrote: > |data_use_tracker| is not a thing. Please update the comment. Done. https://codereview.chromium.org/1818613002/diff/420001/components/metrics/dat... components/metrics/data_use_tracker.h:66: virtual bool GetUmaWeeaklyQuota(int* uma_weekly_quota_bytes); On 2016/04/01 20:38:58, Alexei Svitkine wrote: > Comments please. Done. https://codereview.chromium.org/1818613002/diff/420001/components/metrics/dat... File components/metrics/data_use_tracker_unittest.cc (right): https://codereview.chromium.org/1818613002/diff/420001/components/metrics/dat... components/metrics/data_use_tracker_unittest.cc:33: private: On 2016/04/01 20:38:58, Alexei Svitkine wrote: > Add an empty line above private: > > Same below. Done.
https://codereview.chromium.org/1818613002/diff/420001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.cc (right): https://codereview.chromium.org/1818613002/diff/420001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:110: total_upload_bytes + total_received_bytes); On 2016/04/01 20:59:35, mmenke wrote: > This seems like way too many posted tasks for too little gain - one for every > redirect, one for every URLRequest. Can we just jump over to the IOThread and > grab this whenever we need it, or accumulate data and only periodically post a > task or something? The are certainly issues related to the fact that this is a > per-profile class. Is that really a lot? I suspect that the ratio of URL requests to posted tasks in general is already something tiny (i.e. URL requests are very infrequent compared to posted tasks).
asvitkine@chromium.org changed reviewers: + bengr@chromium.org, mmenke@chromium.org
https://codereview.chromium.org/1818613002/diff/420001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.cc (right): https://codereview.chromium.org/1818613002/diff/420001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:110: total_upload_bytes + total_received_bytes); On 2016/04/01 21:07:30, Alexei Svitkine wrote: > On 2016/04/01 20:59:35, mmenke wrote: > > This seems like way too many posted tasks for too little gain - one for every > > redirect, one for every URLRequest. Can we just jump over to the IOThread and > > grab this whenever we need it, or accumulate data and only periodically post a > > task or something? The are certainly issues related to the fact that this is > a > > per-profile class. > > Is that really a lot? I suspect that the ratio of URL requests to posted tasks > in general is already something tiny (i.e. URL requests are very infrequent > compared to posted tasks). (mmenke: Not sure if you saw this reply - as I think you accidentally got removed from the R list earlier when I sent it.)
https://codereview.chromium.org/1818613002/diff/420001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.cc (right): https://codereview.chromium.org/1818613002/diff/420001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:110: total_upload_bytes + total_received_bytes); On 2016/04/04 16:01:00, Alexei Svitkine wrote: > On 2016/04/01 21:07:30, Alexei Svitkine wrote: > > On 2016/04/01 20:59:35, mmenke wrote: > > > This seems like way too many posted tasks for too little gain - one for > every > > > redirect, one for every URLRequest. Can we just jump over to the IOThread > and > > > grab this whenever we need it, or accumulate data and only periodically post > a > > > task or something? The are certainly issues related to the fact that this > is > > a > > > per-profile class. > > > > Is that really a lot? I suspect that the ratio of URL requests to posted tasks > > in general is already something tiny (i.e. URL requests are very infrequent > > compared to posted tasks). > > (mmenke: Not sure if you saw this reply - as I think you accidentally got > removed from the R list earlier when I sent it.) You may be right. I'm generally fairly skeptical of doing significant work for purely metrics-related reasons. https://codereview.chromium.org/1818613002/diff/460001/chrome/browser/profile... File chrome/browser/profiles/profile_io_data.cc (right): https://codereview.chromium.org/1818613002/diff/460001/chrome/browser/profile... chrome/browser/profiles/profile_io_data.cc:552: g_browser_process->metrics_service()->GetDataUseForwardingCallback(); Can we just grab the callback from the IOThread? Also, it's a bug that this is after setting initialialized_on_UI_thread to true (And more importantly, after BrowserContext::EnsureResourceContextInitialized) https://codereview.chromium.org/1818613002/diff/460001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.cc (right): https://codereview.chromium.org/1818613002/diff/460001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:107: if (is_conn_cellular && !metrics_data_use_forwarder_.is_null()) { Should this class really know that metrics_data_use_forwarder_ doesn't care about non-cellular use? https://codereview.chromium.org/1818613002/diff/460001/components/metrics/dat... File components/metrics/data_use_tracker.cc (right): https://codereview.chromium.org/1818613002/diff/460001/components/metrics/dat... components/metrics/data_use_tracker.cc:90: UpdateUsagePref(prefs::kUmaCellDataUse, message_size); Recording UMA and non-UMA data use through a method in components/metrics with a magic call from data_use_measurement seems a bit funky. What if some other service wants similar data? Add another daily use_tracker for that component, and introduce a dependency on that component to data_use_measurement, too? Wonder if there's some way we could fairly easily standardize this, so any service could pull their usage from the MetricsService, without the service depending on them. And don't think "kUserCellDataUse" should go here at all. https://codereview.chromium.org/1818613002/diff/460001/components/metrics/dat... components/metrics/data_use_tracker.cc:94: int message_size) { All this does is write to a pref from another thread? PrefMember supports that directly - seems like re-inventing the wheel a bit here. https://codereview.chromium.org/1818613002/diff/460001/components/metrics/dat... components/metrics/data_use_tracker.cc:129: local_state_->Set(pref_name, user_pref_new_dict); DataReductionProxyCompressionStats has its own magic that may or may not do something similar for recording / aggregating daily stats. I don't suppose we could share code? (Disclaimer: I haven't dug into it, just notice they're dealing with similar issues to get daily stats) https://codereview.chromium.org/1818613002/diff/460001/components/metrics/dat... components/metrics/data_use_tracker.cc:147: bool DataUseTracker::GetUmaWeeaklyQuota(int* uma_weekly_quota_bytes) { Weeakly -> Weekly
For decreasing posting task from IO to UI we can implement a mechanism to buffer the update tasks and post one to UI with some frequency. That will require either leaking some of that logic to the DataUserMeasurement class or having to change the callback to an object that keeps track of this. Would you be OK if add a TODO for addressing this later as well as we are trying to manage for M51 https://codereview.chromium.org/1818613002/diff/460001/chrome/browser/profile... File chrome/browser/profiles/profile_io_data.cc (right): https://codereview.chromium.org/1818613002/diff/460001/chrome/browser/profile... chrome/browser/profiles/profile_io_data.cc:552: g_browser_process->metrics_service()->GetDataUseForwardingCallback(); On 2016/04/04 17:26:23, mmenke wrote: > Can we just grab the callback from the IOThread? > > Also, it's a bug that this is after setting initialialized_on_UI_thread to true > (And more importantly, after BrowserContext::EnsureResourceContextInitialized) Did you mean to get from IOThread object ? If so, Done. https://codereview.chromium.org/1818613002/diff/460001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.cc (right): https://codereview.chromium.org/1818613002/diff/460001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:107: if (is_conn_cellular && !metrics_data_use_forwarder_.is_null()) { On 2016/04/04 17:26:23, mmenke wrote: > Should this class really know that metrics_data_use_forwarder_ doesn't care > about non-cellular use? Done. https://codereview.chromium.org/1818613002/diff/460001/components/metrics/dat... File components/metrics/data_use_tracker.cc (right): https://codereview.chromium.org/1818613002/diff/460001/components/metrics/dat... components/metrics/data_use_tracker.cc:90: UpdateUsagePref(prefs::kUmaCellDataUse, message_size); On 2016/04/04 17:26:23, mmenke wrote: > Recording UMA and non-UMA data use through a method in components/metrics with a > magic call from data_use_measurement seems a bit funky. What if some other > service wants similar data? Add another daily use_tracker for that component, > and introduce a dependency on that component to data_use_measurement.h, too? > > Wonder if there's some way we could fairly easily standardize this, so any > service could pull their usage from the MetricsService, without the service > depending on them. And don't think "kUserCellDataUse" should go here at all. For now we would like to manage to get to M51 which is this Friday, because a lot of people are waiting for UMA on cellular. For future we are planning to do the same for IOS which already will require significant refactoring. So we can include the suggested ideas at that time. For now I have added a comment on data_use_measurement to warn other components which will decide to do the same. For kUserCellDataUse, this is a pref which is part of our logic. I don't see why this doesn't belong here. https://codereview.chromium.org/1818613002/diff/460001/components/metrics/dat... components/metrics/data_use_tracker.cc:94: int message_size) { On 2016/04/04 17:26:23, mmenke wrote: > All this does is write to a pref from another thread? PrefMember supports that > directly - seems like re-inventing the wheel a bit here. For using PrefMember we can have pass PrefMember object, which can be modified on IO thread to data_use_measurment instead of the the current callback. What do you think? https://codereview.chromium.org/1818613002/diff/460001/components/metrics/dat... components/metrics/data_use_tracker.cc:129: local_state_->Set(pref_name, user_pref_new_dict); On 2016/04/04 17:26:23, mmenke wrote: > DataReductionProxyCompressionStats has its own magic that may or may not do > something similar for recording / aggregating daily stats. I don't suppose we > could share code? (Disclaimer: I haven't dug into it, just notice they're > dealing with similar issues to get daily stats) I had a quick look but It didn't seem to me similar enough, it is not even using data_use_measurement component. https://codereview.chromium.org/1818613002/diff/460001/components/metrics/dat... components/metrics/data_use_tracker.cc:147: bool DataUseTracker::GetUmaWeeaklyQuota(int* uma_weekly_quota_bytes) { On 2016/04/04 17:26:23, mmenke wrote: > Weeakly -> Weekly Done.
https://codereview.chromium.org/1818613002/diff/500001/chrome/browser/io_thre... File chrome/browser/io_thread.h (right): https://codereview.chromium.org/1818613002/diff/500001/chrome/browser/io_thre... chrome/browser/io_thread.h:294: const metrics::UpdateUsagePrefCallbackType GetMetricsDataUseForwarder(); Nit: Return by const ref. https://codereview.chromium.org/1818613002/diff/500001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.h (right): https://codereview.chromium.org/1818613002/diff/500001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:93: // solution. Nit: "refactor the existing solution" is pretty vague - could you be more precise? e.g. "would be to refactor this class to support registering arbitrary observers."
Addressed comments by @asvitkine https://codereview.chromium.org/1818613002/diff/500001/chrome/browser/io_thre... File chrome/browser/io_thread.h (right): https://codereview.chromium.org/1818613002/diff/500001/chrome/browser/io_thre... chrome/browser/io_thread.h:294: const metrics::UpdateUsagePrefCallbackType GetMetricsDataUseForwarder(); On 2016/04/05 15:58:57, Alexei Svitkine wrote: > Nit: Return by const ref. Done. https://codereview.chromium.org/1818613002/diff/500001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.h (right): https://codereview.chromium.org/1818613002/diff/500001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:93: // solution. On 2016/04/05 15:58:57, Alexei Svitkine wrote: > Nit: "refactor the existing solution" is pretty vague - could you be more > precise? e.g. "would be to refactor this class to support registering arbitrary > observers." Done.
Addressed comments by @asvitkine
On 2016/04/05 14:26:46, gayane wrote: > For decreasing posting task from IO to UI we can implement a mechanism to buffer > the update tasks and post one to UI with some frequency. That will require > either leaking some of that logic to the DataUserMeasurement class or having to > change the callback to an object that keeps track of this. Would you be OK if > add a TODO for addressing this later as well as we are trying to manage for M51 It's up to you - I'm certainly not going to block on this. I may just be over paranoid here, and it may not be worth the effort. Worth noting that sending a BrowserThread::PostTask every 100 ms for every tab during startup did show up in histograms as being a significant cause of jank - this code should hopefully call it less than that, on average, but when pages load, could call it more than that for a while. You could at one of the calls that makes it show up in the performance profiler to double-check, and then remove the call if it turns out to be a non-issue.
io_thread.h, profiles/, chrome/browser/net LGTM (Despite the comments elsewhere, did not do a full review of other code - just skimmed it). https://codereview.chromium.org/1818613002/diff/460001/components/metrics/dat... File components/metrics/data_use_tracker.cc (right): https://codereview.chromium.org/1818613002/diff/460001/components/metrics/dat... components/metrics/data_use_tracker.cc:90: UpdateUsagePref(prefs::kUmaCellDataUse, message_size); On 2016/04/05 14:26:46, gayane wrote: > On 2016/04/04 17:26:23, mmenke wrote: > > Recording UMA and non-UMA data use through a method in components/metrics with > a > > magic call from data_use_measurement seems a bit funky. What if some other > > service wants similar data? Add another daily use_tracker for that component, > > and introduce a dependency on that component to data_use_measurement.h, too? > > > > Wonder if there's some way we could fairly easily standardize this, so any > > service could pull their usage from the MetricsService, without the service > > depending on them. And don't think "kUserCellDataUse" should go here at all. > > For now we would like to manage to get to M51 which is this Friday, because a > lot of people are waiting for UMA on cellular. For future we are planning to do > the same for IOS which already will require significant refactoring. So we can > include the suggested ideas at that time. > For now I have added a comment on data_use_measurement to warn other components > which will decide to do the same. > For kUserCellDataUse, this is a pref which is part of our logic. I don't see why > this doesn't belong here. Not going to block on this, but my general feeling is if we're going to keep this around, it's best to do it right the first time than not. You'll either be stuck with the designed, which becomes progressively harder and harder to change over time, or you'll have to go through review yet again, wasting everyone's time. https://codereview.chromium.org/1818613002/diff/460001/components/metrics/dat... components/metrics/data_use_tracker.cc:94: int message_size) { On 2016/04/05 14:26:46, gayane wrote: > On 2016/04/04 17:26:23, mmenke wrote: > > All this does is write to a pref from another thread? PrefMember supports > that > > directly - seems like re-inventing the wheel a bit here. > > For using PrefMember we can have pass PrefMember object, which can be modified > on IO thread to data_use_measurment instead of the the current callback. What do > you think? Could DataUseTracker::GetDataUseForwardingCallback do something like: scoped_ptr<PrefMember> user_pref(...); // Create it from local_state_. user_pre user_pref->MoveToThread(ui); ... same for uma_pref. Callback = base::Bind(&UpdatePrefsOnIO, base::Owned(user_pref), base::Owned(uma_pref)); And then just do them all on the IO thread? That doesn't actually save on PostTasks, which I suggested elsewhere, but it does leave it all in PrefMember's camp, at least. Anyhow, not a huge savings in complexity, I suppose, since we still need stuff on the UI thread. https://codereview.chromium.org/1818613002/diff/460001/components/metrics/dat... components/metrics/data_use_tracker.cc:129: local_state_->Set(pref_name, user_pref_new_dict); On 2016/04/05 14:26:46, gayane wrote: > On 2016/04/04 17:26:23, mmenke wrote: > > DataReductionProxyCompressionStats has its own magic that may or may not do > > something similar for recording / aggregating daily stats. I don't suppose we > > could share code? (Disclaimer: I haven't dug into it, just notice they're > > dealing with similar issues to get daily stats) > > I had a quick look but It didn't seem to me similar enough, it is not even using > data_use_measurement component. Right...which means things would have to move around a bit to share code, which is preferable to code duplication. I'd suggest you at least test this with a lot of cases (Time going backwards a day and a year, various amount of days in cache). https://codereview.chromium.org/1818613002/diff/520001/components/metrics/dat... File components/metrics/data_use_tracker.h (right): https://codereview.chromium.org/1818613002/diff/520001/components/metrics/dat... components/metrics/data_use_tracker.h:17: typedef base::Callback<void(const std::string&, int, bool)> include string https://codereview.chromium.org/1818613002/diff/520001/components/metrics/dat... components/metrics/data_use_tracker.h:34: scoped_refptr<base::SequencedTaskRunner> ui_task_runner); include ref_counted.h. Should also include the sequenced task runner header, or forward declare it here and include it in the cc file. https://codereview.chromium.org/1818613002/diff/520001/components/metrics/dat... components/metrics/data_use_tracker.h:65: int ComputeTotalDataUse(std::string pref_name); const std::string& https://codereview.chromium.org/1818613002/diff/520001/components/metrics/dat... components/metrics/data_use_tracker.h:68: virtual bool GetUmaWeeklyQuota(int* uma_weekly_quota_bytes); These getters should presumably all be const (think it's better to make stuff mutable in the unit tests, if needed, than to make code non-const in production code when it logically is const) https://codereview.chromium.org/1818613002/diff/520001/components/metrics/dat... components/metrics/data_use_tracker.h:74: virtual base::Time GetCurrentMeasurementDate(); include time https://codereview.chromium.org/1818613002/diff/520001/components/metrics/dat... components/metrics/data_use_tracker.h:85: DISALLOW_COPY_AND_ASSIGN(DataUseTracker); include base/macros
> On 2016/04/05 14:26:46, gayane wrote: > > For decreasing posting task from IO to UI we can implement a mechanism to > buffer > > the update tasks and post one to UI with some frequency. That will require > > either leaking some of that logic to the DataUserMeasurement class or having > to > > change the callback to an object that keeps track of this. Would you be OK if > > add a TODO for addressing this later as well as we are trying to manage for > M51 > > It's up to you - I'm certainly not going to block on this. I may just be over > paranoid here, and it may not be worth the effort. Worth noting that sending a > BrowserThread::PostTask every 100 ms for every tab during startup did show up in > histograms as being a significant cause of jank - this code should hopefully > call it less than that, on average, but when pages load, could call it more than > that for a while. You could at one of the calls that makes it show up in the > performance profiler to double-check, and then remove the call if it turns out > to be a non-issue. We discussed this with asvitkine@ but we think that jank detection with profiler might not help us in this case. I will do another analysis to see with what frequency we post tasks. > https://codereview.chromium.org/1818613002/diff/460001/components/metrics/dat... > components/metrics/data_use_tracker.cc:94: int message_size) { > On 2016/04/05 14:26:46, gayane wrote: > > On 2016/04/04 17:26:23, mmenke wrote: > > > All this does is write to a pref from another thread? PrefMember supports > > that > > > directly - seems like re-inventing the wheel a bit here. > > > > For using PrefMember we can have pass PrefMember object, which can be modified > > on IO thread to data_use_measurment instead of the the current callback. What > do > > you think? > > Could DataUseTracker::GetDataUseForwardingCallback do something like: > > scoped_ptr<PrefMember> user_pref(...); // Create it from local_state_. > user_pre > user_pref->MoveToThread(ui); > ... same for uma_pref. > > Callback = base::Bind(&UpdatePrefsOnIO, base::Owned(user_pref), > base::Owned(uma_pref)); > > And then just do them all on the IO thread? That doesn't actually save on > PostTasks, which I suggested elsewhere, but it does leave it all in PrefMember's > camp, at least. > > Anyhow, not a huge savings in complexity, I suppose, since we still need stuff > on the UI thread. In this case it seems like if we want to update both prefs then it will be two post tasks, instead of current one. https://codereview.chromium.org/1818613002/diff/520001/components/metrics/dat... File components/metrics/data_use_tracker.h (right): https://codereview.chromium.org/1818613002/diff/520001/components/metrics/dat... components/metrics/data_use_tracker.h:17: typedef base::Callback<void(const std::string&, int, bool)> On 2016/04/05 20:18:32, mmenke wrote: > include string Done. https://codereview.chromium.org/1818613002/diff/520001/components/metrics/dat... components/metrics/data_use_tracker.h:34: scoped_refptr<base::SequencedTaskRunner> ui_task_runner); On 2016/04/05 20:18:32, mmenke wrote: > include ref_counted.h. Should also include the sequenced task runner header, or > forward declare it here and include it in the cc file. Done. https://codereview.chromium.org/1818613002/diff/520001/components/metrics/dat... components/metrics/data_use_tracker.h:65: int ComputeTotalDataUse(std::string pref_name); On 2016/04/05 20:18:32, mmenke wrote: > const std::string& Done. https://codereview.chromium.org/1818613002/diff/520001/components/metrics/dat... components/metrics/data_use_tracker.h:68: virtual bool GetUmaWeeklyQuota(int* uma_weekly_quota_bytes); On 2016/04/05 20:18:32, mmenke wrote: > These getters should presumably all be const (think it's better to make stuff > mutable in the unit tests, if needed, than to make code non-const in production > code when it logically is const) Done. https://codereview.chromium.org/1818613002/diff/520001/components/metrics/dat... components/metrics/data_use_tracker.h:74: virtual base::Time GetCurrentMeasurementDate(); On 2016/04/05 20:18:32, mmenke wrote: > include time Done. https://codereview.chromium.org/1818613002/diff/520001/components/metrics/dat... components/metrics/data_use_tracker.h:85: DISALLOW_COPY_AND_ASSIGN(DataUseTracker); On 2016/04/05 20:18:32, mmenke wrote: > include base/macros Done.
gayane@chromium.org changed reviewers: - mmenke@chromium.org
bengr@ ping
https://codereview.chromium.org/1818613002/diff/540001/components/metrics/met... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/1818613002/diff/540001/components/metrics/met... components/metrics/metrics_service.cc:310: data_use_tracker_.reset(new DataUseTracker(local_state_)); We only want to do this logic if enabled via variations params, right? So, to avoid doing it on other platforms where we don't need it - I suggest not constructing this object at all. To do that, I suggest moving the creation of the object to DataUseTracker file itself - i.e. via a Create() function that will check the variations params. Then, in GetDataUseForwardingCallback(), you can return a nil callback if the object doesn't exist.
gayane@chromium.org changed reviewers: + asargent@chromium.org, blundell@chromium.org, mmenke@chromium.org
asargent@chromium.org: Please review changes in chrome/browser/extensions/api/web_request/web_request_api_unittest.cc blundell@chromium.org: Please review changes in components/metrics.gypi
web_request_api_unittest.cc lgtm
https://codereview.chromium.org/1818613002/diff/540001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.cc (right): https://codereview.chromium.org/1818613002/diff/540001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:81: bool is_conn_cellular = net::NetworkChangeNotifier::IsConnectionCellular( conn -> connection. is_cellular and is_connection_cellular are both fine, but change the name everywhere. https://codereview.chromium.org/1818613002/diff/540001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:96: DataUseUserData::ServiceName service_name = Why did you move this out of the loop? It doesn't appear to be needed in this scope. https://codereview.chromium.org/1818613002/diff/540001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.h (right): https://codereview.chromium.org/1818613002/diff/540001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:71: bool is_conn_cellular) const; nit: is_connection_cellular https://codereview.chromium.org/1818613002/diff/540001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:91: // Note: If similar mechanism would need be used for components other than If -> If a https://codereview.chromium.org/1818613002/diff/540001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:93: // support registering arbitrary observers. Add a TODO(rajendrant), and reference crbug.com/601185 https://codereview.chromium.org/1818613002/diff/540001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement_unittest.cc (right): https://codereview.chromium.org/1818613002/diff/540001/components/data_use_me... components/data_use_measurement/content/data_use_measurement_unittest.cc:31: : data_use_measurement_(metrics::UpdateUsagePrefCallbackType()) { Please test that the callback gets called appropriately.
What am I being asked to review on this CL? Sorry if I missed it.
On 2016/04/07 07:29:33, blundell wrote: > What am I being asked to review on this CL? Sorry if I missed it. blundell@ Please review changes in components/metrics.gypi
asvitkine@ and bengr@ comments addressed https://codereview.chromium.org/1818613002/diff/540001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.cc (right): https://codereview.chromium.org/1818613002/diff/540001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:81: bool is_conn_cellular = net::NetworkChangeNotifier::IsConnectionCellular( On 2016/04/06 20:57:11, bengr wrote: > conn -> connection. is_cellular and is_connection_cellular are both fine, but > change the name everywhere. Done. https://codereview.chromium.org/1818613002/diff/540001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:96: DataUseUserData::ServiceName service_name = On 2016/04/06 20:57:11, bengr wrote: > Why did you move this out of the loop? It doesn't appear to be needed in this > scope. It is passed to the callback later line 109. https://codereview.chromium.org/1818613002/diff/540001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.h (right): https://codereview.chromium.org/1818613002/diff/540001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:71: bool is_conn_cellular) const; On 2016/04/06 20:57:11, bengr wrote: > nit: is_connection_cellular Done. https://codereview.chromium.org/1818613002/diff/540001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:91: // Note: If similar mechanism would need be used for components other than On 2016/04/06 20:57:11, bengr wrote: > If -> If a Done. https://codereview.chromium.org/1818613002/diff/540001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:93: // support registering arbitrary observers. On 2016/04/06 20:57:11, bengr wrote: > Add a TODO(rajendrant), and reference crbug.com/601185 Done. https://codereview.chromium.org/1818613002/diff/540001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement_unittest.cc (right): https://codereview.chromium.org/1818613002/diff/540001/components/data_use_me... components/data_use_measurement/content/data_use_measurement_unittest.cc:31: : data_use_measurement_(metrics::UpdateUsagePrefCallbackType()) { On 2016/04/06 20:57:11, bengr wrote: > Please test that the callback gets called appropriately. Done. https://codereview.chromium.org/1818613002/diff/540001/components/metrics/met... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/1818613002/diff/540001/components/metrics/met... components/metrics/metrics_service.cc:310: data_use_tracker_.reset(new DataUseTracker(local_state_)); On 2016/04/06 18:37:43, Alexei Svitkine wrote: > We only want to do this logic if enabled via variations params, right? > > So, to avoid doing it on other platforms where we don't need it - I suggest not > constructing this object at all. To do that, I suggest moving the creation of > the object to DataUseTracker file itself - i.e. via a Create() function that > will check the variations params. Then, in GetDataUseForwardingCallback(), you > can return a nil callback if the object doesn't exist. Done.
On 2016/04/07 15:10:41, gayane wrote: > On 2016/04/07 07:29:33, blundell wrote: > > What am I being asked to review on this CL? Sorry if I missed it. > > blundell@ Please review changes in components/metrics.gypi Alexei's LG should be enough for that one, as //components/OWNERS redirects metrics.gypi to //components/metrics/OWNERS.
lgtm % comments https://codereview.chromium.org/1818613002/diff/540001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.h (right): https://codereview.chromium.org/1818613002/diff/540001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:71: bool is_conn_cellular) const; On 2016/04/07 15:30:39, gayane wrote: > On 2016/04/06 20:57:11, bengr wrote: > > nit: is_connection_cellular > > Done. Seems you still have some places that use the old name. Please search the patch and update all references. https://codereview.chromium.org/1818613002/diff/540001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:93: // support registering arbitrary observers. On 2016/04/07 15:30:39, gayane wrote: > On 2016/04/06 20:57:11, bengr wrote: > > Add a TODO(rajendrant), and reference crbug.com/601185 > > Done. I don't see this being done? https://codereview.chromium.org/1818613002/diff/560001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement_unittest.cc (right): https://codereview.chromium.org/1818613002/diff/560001/components/data_use_me... components/data_use_measurement/content/data_use_measurement_unittest.cc:39: void SendRequest(bool is_user_request) { Add a comment and mention what the param means. https://codereview.chromium.org/1818613002/diff/560001/components/metrics/dat... File components/metrics/data_use_tracker.cc (right): https://codereview.chromium.org/1818613002/diff/560001/components/metrics/dat... components/metrics/data_use_tracker.cc:44: "Enabled") == "true") { Should it be based on having Uma_Quota & Uma_Ratio params? https://codereview.chromium.org/1818613002/diff/560001/components/metrics/dat... File components/metrics/data_use_tracker.h (right): https://codereview.chromium.org/1818613002/diff/560001/components/metrics/dat... components/metrics/data_use_tracker.h:34: // Returns an instance of |DataUseTracker| with provided |local_state|. Add a sentence about why it may not create one. https://codereview.chromium.org/1818613002/diff/560001/components/metrics/met... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/1818613002/diff/560001/components/metrics/met... components/metrics/metrics_service.cc:310: data_use_tracker_ = DataUseTracker::Create(local_state_); Nit: Move this to the initializer list above. https://codereview.chromium.org/1818613002/diff/560001/components/metrics/met... components/metrics/metrics_service.cc:554: if (data_use_tracker_) Nit: {}
Patchset #17 (id:580001) has been deleted
lgtm
asvitkine@ comments addressed. asvitkine@ blundell@ according to my review tool components/metrics.gypi is not covered. https://codereview.chromium.org/1818613002/diff/540001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.h (right): https://codereview.chromium.org/1818613002/diff/540001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:71: bool is_conn_cellular) const; On 2016/04/07 15:43:29, Alexei Svitkine wrote: > On 2016/04/07 15:30:39, gayane wrote: > > On 2016/04/06 20:57:11, bengr wrote: > > > nit: is_connection_cellular > > > > Done. > > Seems you still have some places that use the old name. Please search the patch > and update all references. Done. https://codereview.chromium.org/1818613002/diff/540001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:93: // support registering arbitrary observers. On 2016/04/07 15:43:29, Alexei Svitkine wrote: > On 2016/04/07 15:30:39, gayane wrote: > > On 2016/04/06 20:57:11, bengr wrote: > > > Add a TODO(rajendrant), and reference crbug.com/601185 > > > > Done. > > I don't see this being done? Done. https://codereview.chromium.org/1818613002/diff/560001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement_unittest.cc (right): https://codereview.chromium.org/1818613002/diff/560001/components/data_use_me... components/data_use_measurement/content/data_use_measurement_unittest.cc:39: void SendRequest(bool is_user_request) { On 2016/04/07 15:43:29, Alexei Svitkine wrote: > Add a comment and mention what the param means. Done. https://codereview.chromium.org/1818613002/diff/560001/components/metrics/dat... File components/metrics/data_use_tracker.cc (right): https://codereview.chromium.org/1818613002/diff/560001/components/metrics/dat... components/metrics/data_use_tracker.cc:44: "Enabled") == "true") { On 2016/04/07 15:43:29, Alexei Svitkine wrote: > Should it be based on having Uma_Quota & Uma_Ratio params? Done. https://codereview.chromium.org/1818613002/diff/560001/components/metrics/dat... File components/metrics/data_use_tracker.h (right): https://codereview.chromium.org/1818613002/diff/560001/components/metrics/dat... components/metrics/data_use_tracker.h:34: // Returns an instance of |DataUseTracker| with provided |local_state|. On 2016/04/07 15:43:29, Alexei Svitkine wrote: > Add a sentence about why it may not create one. Done. https://codereview.chromium.org/1818613002/diff/560001/components/metrics/met... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/1818613002/diff/560001/components/metrics/met... components/metrics/metrics_service.cc:310: data_use_tracker_ = DataUseTracker::Create(local_state_); On 2016/04/07 15:43:29, Alexei Svitkine wrote: > Nit: Move this to the initializer list above. Done. https://codereview.chromium.org/1818613002/diff/560001/components/metrics/met... components/metrics/metrics_service.cc:554: if (data_use_tracker_) On 2016/04/07 15:43:29, Alexei Svitkine wrote: > Nit: {} Done.
lgtm
The CQ bit was checked by gayane@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org, asargent@chromium.org, bengr@chromium.org Link to the patchset: https://codereview.chromium.org/1818613002/#ps600001 (title: " ")
"asvitkine@ blundell@ according to my review tool components/metrics.gypi is not covered." I think the extension for adding owners info to code review doesn't undestand OWNERS files that use file: includes. On Thu, Apr 7, 2016 at 2:57 PM, <asvitkine@chromium.org> wrote: > lgtm > > > > https://codereview.chromium.org/1818613002/ > -- 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.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1818613002/600001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1818613002/600001
Message was sent while issue was closed.
Description was changed from ========== Implement UMA log throttling for cellular connections. For enabling UMA on cellular, a throttling logic is implemented that takes into account the weekly data usage of UMA on cellular as well as the ratio of UMA data usage over overall Chrome data traffic. For tracking UMA data use and overall Chrome data traffic we use DataUseMeasurement class which already collects and reports similar data. The allowed weekly UMA quota and UMA ratio are passed through as variation param. BUG=599586 ========== to ========== Implement UMA log throttling for cellular connections. For enabling UMA on cellular, a throttling logic is implemented that takes into account the weekly data usage of UMA on cellular as well as the ratio of UMA data usage over overall Chrome data traffic. For tracking UMA data use and overall Chrome data traffic we use DataUseMeasurement class which already collects and reports similar data. The allowed weekly UMA quota and UMA ratio are passed through as variation param. BUG=599586 ==========
Message was sent while issue was closed.
Committed patchset #17 (id:600001)
Message was sent while issue was closed.
Description was changed from ========== Implement UMA log throttling for cellular connections. For enabling UMA on cellular, a throttling logic is implemented that takes into account the weekly data usage of UMA on cellular as well as the ratio of UMA data usage over overall Chrome data traffic. For tracking UMA data use and overall Chrome data traffic we use DataUseMeasurement class which already collects and reports similar data. The allowed weekly UMA quota and UMA ratio are passed through as variation param. BUG=599586 ========== to ========== Implement UMA log throttling for cellular connections. For enabling UMA on cellular, a throttling logic is implemented that takes into account the weekly data usage of UMA on cellular as well as the ratio of UMA data usage over overall Chrome data traffic. For tracking UMA data use and overall Chrome data traffic we use DataUseMeasurement class which already collects and reports similar data. The allowed weekly UMA quota and UMA ratio are passed through as variation param. BUG=599586 Committed: https://crrev.com/0b46091cb37b8fdd73a02675e2ab86850d51cb09 Cr-Commit-Position: refs/heads/master@{#385845} ==========
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/0b46091cb37b8fdd73a02675e2ab86850d51cb09 Cr-Commit-Position: refs/heads/master@{#385845} |
