Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2439303002:
Record UMA on number of days since data reduction proxy is enabled. (Closed)
DescriptionRecord UMA on number of days since data reduction proxy is enabled.
BUG=658347
Committed: https://crrev.com/579a409d89d9da9a10a924e3d4704fc1e1144d77
Cr-Commit-Position: refs/heads/master@{#427368}
Patch Set 1 : ps #
Total comments: 22
Patch Set 2 : sclittle comments #
Total comments: 8
Patch Set 3 : addressed sclittle comments #Patch Set 4 : fix compile error #Messages
Total messages: 54 (41 generated)
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
tbansal@chromium.org changed reviewers: + rajendrant@chromium.org
rajendrant: ptal. thanks.
Description was changed from ========== Record UMA on number of days since data reduction proxy is enabled. BUG= ========== to ========== Record UMA on number of days since data reduction proxy is enabled. BUG=658347 ==========
On 2016/10/24 17:15:06, tbansal1 wrote: > rajendrant: ptal. thanks. This UMA is recorded on every chrome startup. Wondering if this would give more weight to users opening chrome multiple times. How about recording this somewhere in DataReductionProxyCompressionStats::RecordRequestSizePrefs(), where Net.DailyContentLength.* UMA is recorded only once per day.
On 2016/10/24 18:21:21, Raj wrote: > On 2016/10/24 17:15:06, tbansal1 wrote: > > rajendrant: ptal. thanks. > > This UMA is recorded on every chrome startup. > Wondering if this would give more weight to users opening chrome multiple times. > How about recording this somewhere in > DataReductionProxyCompressionStats::RecordRequestSizePrefs(), where > Net.DailyContentLength.* UMA is recorded only once per day. One of the goals is to debug the user feedback reports (see the bug description). If we record once per day, then we may not be able to capture data from this histogram in the user feedback report. If somebody wants to analyze per-user, then they can group the bucket values by the client ID and day to remove bias from users that open Chrome multiple times.
On 2016/10/24 18:31:19, tbansal1 wrote: > On 2016/10/24 18:21:21, Raj wrote: > > On 2016/10/24 17:15:06, tbansal1 wrote: > > > rajendrant: ptal. thanks. > > > > This UMA is recorded on every chrome startup. > > Wondering if this would give more weight to users opening chrome multiple > times. > > How about recording this somewhere in > > DataReductionProxyCompressionStats::RecordRequestSizePrefs(), where > > Net.DailyContentLength.* UMA is recorded only once per day. > > One of the goals is to debug the user feedback reports (see the bug > description). > If we record once per day, then we may not be able to capture data from this > histogram > in the user feedback report. > > If somebody wants to analyze per-user, then they can group the bucket values by > the > client ID and day to remove bias from users that open Chrome multiple times. ok lgtm
tbansal@chromium.org changed reviewers: + sclittle@chromium.org
sclittle: ptal. thanks.
https://codereview.chromium.org/2439303002/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc (right): https://codereview.chromium.org/2439303002/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc:253: clock_ = (std::move(clock)); nit: the outer parentheses around the std::move are unnecessary. https://codereview.chromium.org/2439303002/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc:269: prefs->GetInt64(prefs::kDataReductionProxyLastEnabledTime); Why is this being stored as milliseconds? Could you just save/load this pref using ToInternalValue/FromInternalValue? That would simplify this, plus it would cut down on the number of floating point operations that are performed, since JS times are represented as doubles. https://codereview.chromium.org/2439303002/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h (right): https://codereview.chromium.org/2439303002/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h:12: #include <vector> nit: Is this <vector> include necessary? Can it be removed? https://codereview.chromium.org/2439303002/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h:265: nit: Could you remove the empty lines here for consistency? https://codereview.chromium.org/2439303002/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h:289: void SetClockForTesting(std::unique_ptr<base::Clock> clock); optional nit: Have you considered setting the base::Clock in the constructor, and creating a private constructor that tests are allowed to use that takes in a std::unique_ptr<base::Clock>? The public no-args constructor could just call the private constructor with a default clock, and the private constructor could do all the actual initialization that the public constructor does now. Test classes would then always use a test clock. That seems a bit cleaner to me then exposing a method SetClockForTesting to tests, but up to you. https://codereview.chromium.org/2439303002/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc (right): https://codereview.chromium.org/2439303002/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc:652: double time_now = base::Time::Now().ToJsTime(); Could you just use the test clock here instead? That way, you can expect the exact value of the test clock instead of expecting that it's after time_now. https://codereview.chromium.org/2439303002/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc:690: clock_ptr->SetNow(base::Time::Now()); nit: Could you use some constant instead of base::Time::Now()? It seems a bit cleaner that way, since you have the test clock already. https://codereview.chromium.org/2439303002/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc:754: const char kUMAEnabledState[] = "DataReductionProxy.DaysSinceEnabled"; nit: This is referenced exactly once below - could the string literal just be inlined? https://codereview.chromium.org/2439303002/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc:755: std::unique_ptr<base::SimpleTestClock> clock(new base::SimpleTestClock()); nit: |clock| is never referenced below. Could you remove it?
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Patchset #2 (id:40001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
sclittle: ptal. thanks. https://codereview.chromium.org/2439303002/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc (right): https://codereview.chromium.org/2439303002/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc:253: clock_ = (std::move(clock)); On 2016/10/24 21:00:36, sclittle wrote: > nit: the outer parentheses around the std::move are unnecessary. Done. https://codereview.chromium.org/2439303002/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc:269: prefs->GetInt64(prefs::kDataReductionProxyLastEnabledTime); On 2016/10/24 21:00:36, sclittle wrote: > Why is this being stored as milliseconds? Could you just save/load this pref > using ToInternalValue/FromInternalValue? That would simplify this, plus it would > cut down on the number of floating point operations that are performed, since JS > times are represented as doubles. Done. https://codereview.chromium.org/2439303002/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h (right): https://codereview.chromium.org/2439303002/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h:12: #include <vector> On 2016/10/24 21:00:36, sclittle wrote: > nit: Is this <vector> include necessary? Can it be removed? Done. https://codereview.chromium.org/2439303002/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h:265: On 2016/10/24 21:00:36, sclittle wrote: > nit: Could you remove the empty lines here for consistency? Done. https://codereview.chromium.org/2439303002/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h:289: void SetClockForTesting(std::unique_ptr<base::Clock> clock); On 2016/10/24 21:00:36, sclittle wrote: > optional nit: Have you considered setting the base::Clock in the constructor, > and creating a private constructor that tests are allowed to use that takes in a > std::unique_ptr<base::Clock>? > > The public no-args constructor could just call the private constructor with a > default clock, and the private constructor could do all the actual > initialization that the public constructor does now. > > Test classes would then always use a test clock. That seems a bit cleaner to me > then exposing a method SetClockForTesting to tests, but up to you. Changing the clock_ variable directly. This is similar to how other tests change the value of other private variables. https://codereview.chromium.org/2439303002/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc (right): https://codereview.chromium.org/2439303002/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc:652: double time_now = base::Time::Now().ToJsTime(); On 2016/10/24 21:00:36, sclittle wrote: > Could you just use the test clock here instead? That way, you can expect the > exact value of the test clock instead of expecting that it's after time_now. tHAT's what the next test does. Here, I wanted to check with original clock. I can remove this if it is not too meaningful. https://codereview.chromium.org/2439303002/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc:690: clock_ptr->SetNow(base::Time::Now()); On 2016/10/24 21:00:36, sclittle wrote: > nit: Could you use some constant instead of base::Time::Now()? It seems a bit > cleaner that way, since you have the test clock already. Done. https://codereview.chromium.org/2439303002/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc:754: const char kUMAEnabledState[] = "DataReductionProxy.DaysSinceEnabled"; On 2016/10/24 21:00:36, sclittle wrote: > nit: This is referenced exactly once below - could the string literal just be > inlined? Done. https://codereview.chromium.org/2439303002/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc:755: std::unique_ptr<base::SimpleTestClock> clock(new base::SimpleTestClock()); On 2016/10/24 21:00:36, sclittle wrote: > nit: |clock| is never referenced below. Could you remove it? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
lgtm % nits https://codereview.chromium.org/2439303002/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h (right): https://codereview.chromium.org/2439303002/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h:289: void SetClockForTesting(std::unique_ptr<base::Clock> clock); On 2016/10/25 00:10:03, tbansal1 wrote: > On 2016/10/24 21:00:36, sclittle wrote: > > optional nit: Have you considered setting the base::Clock in the constructor, > > and creating a private constructor that tests are allowed to use that takes in > a > > std::unique_ptr<base::Clock>? > > > > The public no-args constructor could just call the private constructor with a > > default clock, and the private constructor could do all the actual > > initialization that the public constructor does now. > > > > Test classes would then always use a test clock. That seems a bit cleaner to > me > > then exposing a method SetClockForTesting to tests, but up to you. > > Changing the clock_ variable directly. This is similar to how other tests change > the value of other private variables. Could you add a comment on |clock_| saying that it can be modified by tests, and that it must never be null? https://codereview.chromium.org/2439303002/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc (right): https://codereview.chromium.org/2439303002/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc:652: double time_now = base::Time::Now().ToJsTime(); On 2016/10/25 00:10:03, tbansal1 wrote: > On 2016/10/24 21:00:36, sclittle wrote: > > Could you just use the test clock here instead? That way, you can expect the > > exact value of the test clock instead of expecting that it's after time_now. > > tHAT's what the next test does. Here, I wanted to check with original clock. I > can remove this if it is not too meaningful. Ok. Could you remove this test then? https://codereview.chromium.org/2439303002/diff/60001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc (right): https://codereview.chromium.org/2439303002/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc:262: int64_t milliseconds = It's not milliseconds anymore, could you rename this to last_enabled_time or something? https://codereview.chromium.org/2439303002/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc:286: static_cast<int64_t>(clock_->Now().ToInternalValue())); nit: remove unnecessary static_cast https://codereview.chromium.org/2439303002/diff/60001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_test_utils.cc (right): https://codereview.chromium.org/2439303002/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_test_utils.cc:57: //AddProxyToCommandLine(); nit: could you remove this dead commented code? https://codereview.chromium.org/2439303002/diff/60001/components/data_reducti... File components/data_reduction_proxy/core/common/data_reduction_proxy_pref_names.cc (right): https://codereview.chromium.org/2439303002/diff/60001/components/data_reducti... components/data_reduction_proxy/core/common/data_reduction_proxy_pref_names.cc:188: // An integer pref that contains the time (in milliseconds since the epoch) when nit: it's not milliseconds anymore, could you update the comment?
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:80001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Patchset #3 (id:100001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
tbansal@chromium.org changed reviewers: + rkaplow@chromium.org
rkaplow: ptal at histograms.xml. Thanks. https://codereview.chromium.org/2439303002/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h (right): https://codereview.chromium.org/2439303002/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h:289: void SetClockForTesting(std::unique_ptr<base::Clock> clock); On 2016/10/25 01:12:39, sclittle wrote: > On 2016/10/25 00:10:03, tbansal1 wrote: > > On 2016/10/24 21:00:36, sclittle wrote: > > > optional nit: Have you considered setting the base::Clock in the > constructor, > > > and creating a private constructor that tests are allowed to use that takes > in > > a > > > std::unique_ptr<base::Clock>? > > > > > > The public no-args constructor could just call the private constructor with > a > > > default clock, and the private constructor could do all the actual > > > initialization that the public constructor does now. > > > > > > Test classes would then always use a test clock. That seems a bit cleaner to > > me > > > then exposing a method SetClockForTesting to tests, but up to you. > > > > Changing the clock_ variable directly. This is similar to how other tests > change > > the value of other private variables. > > Could you add a comment on |clock_| saying that it can be modified by tests, and > that it must never be null? Done. I did not add "can be modified by tests" since there are other variables in this class which are also modified by the tests. https://codereview.chromium.org/2439303002/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc (right): https://codereview.chromium.org/2439303002/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc:652: double time_now = base::Time::Now().ToJsTime(); On 2016/10/25 01:12:39, sclittle wrote: > On 2016/10/25 00:10:03, tbansal1 wrote: > > On 2016/10/24 21:00:36, sclittle wrote: > > > Could you just use the test clock here instead? That way, you can expect the > > > exact value of the test clock instead of expecting that it's after time_now. > > > > tHAT's what the next test does. Here, I wanted to check with original clock. I > > can remove this if it is not too meaningful. > > Ok. Could you remove this test then? Done. https://codereview.chromium.org/2439303002/diff/60001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc (right): https://codereview.chromium.org/2439303002/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc:262: int64_t milliseconds = On 2016/10/25 01:12:39, sclittle wrote: > It's not milliseconds anymore, could you rename this to last_enabled_time or > something? Done. https://codereview.chromium.org/2439303002/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc:286: static_cast<int64_t>(clock_->Now().ToInternalValue())); On 2016/10/25 01:12:39, sclittle wrote: > nit: remove unnecessary static_cast Done. https://codereview.chromium.org/2439303002/diff/60001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_test_utils.cc (right): https://codereview.chromium.org/2439303002/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_test_utils.cc:57: //AddProxyToCommandLine(); On 2016/10/25 01:12:39, sclittle wrote: > nit: could you remove this dead commented code? Done. https://codereview.chromium.org/2439303002/diff/60001/components/data_reducti... File components/data_reduction_proxy/core/common/data_reduction_proxy_pref_names.cc (right): https://codereview.chromium.org/2439303002/diff/60001/components/data_reducti... components/data_reduction_proxy/core/common/data_reduction_proxy_pref_names.cc:188: // An integer pref that contains the time (in milliseconds since the epoch) when On 2016/10/25 01:12:39, sclittle wrote: > nit: it's not milliseconds anymore, could you update the comment? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #4 (id:140001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by tbansal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rajendrant@chromium.org, sclittle@chromium.org Link to the patchset: https://codereview.chromium.org/2439303002/#ps160001 (title: "fix compile error")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Record UMA on number of days since data reduction proxy is enabled. BUG=658347 ========== to ========== Record UMA on number of days since data reduction proxy is enabled. BUG=658347 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Record UMA on number of days since data reduction proxy is enabled. BUG=658347 ========== to ========== Record UMA on number of days since data reduction proxy is enabled. BUG=658347 Committed: https://crrev.com/579a409d89d9da9a10a924e3d4704fc1e1144d77 Cr-Commit-Position: refs/heads/master@{#427368} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/579a409d89d9da9a10a924e3d4704fc1e1144d77 Cr-Commit-Position: refs/heads/master@{#427368} |
