Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2288853003:
Adding local field trial for metrics/crash reports sampling. (Closed)
DescriptionAdding local field trial for metrics/crash reports sampling.
This is to support the first-run case, if no first-run variations seed was available.
BUG=642086
Committed: https://crrev.com/34f77fad8128e9b51db959a4a1f9ab6bfcb08257
Cr-Commit-Position: refs/heads/master@{#415473}
Patch Set 1 #Patch Set 2 : Adding local field trial for sampling. #
Total comments: 8
Patch Set 3 : #Patch Set 4 : #
Total comments: 14
Patch Set 5 : #
Total comments: 2
Patch Set 6 : final comments #
Messages
Total messages: 24 (12 generated)
Description was changed from ========== Adding local field trial for sampling. BUG= ========== to ========== Adding local field trial for metrics/crash reports sampling. This is to support the first-run case, if no first-run variations seed was available. BUG=642086 ==========
jwd@chromium.org changed reviewers: + asvitkine@chromium.org
https://codereview.chromium.org/2288853003/diff/20001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_field_trials.cc (right): https://codereview.chromium.org/2288853003/diff/20001/chrome/browser/chrome_b... chrome/browser/chrome_browser_field_trials.cc:131: if (chrome::GetChannel() != version_info::Channel::UNKNOWN) Shouldn't you be checking for STABLE? https://codereview.chromium.org/2288853003/diff/20001/chrome/browser/chrome_b... chrome/browser/chrome_browser_field_trials.cc:162: #if defined(OS_WIN) || defined(OS_ANDROID) How about just moving the ifdefs to the body of the function you're calling. It's already named "IfNeeded", so the ifdefs can be part of that. https://codereview.chromium.org/2288853003/diff/20001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_field_trials.h (right): https://codereview.chromium.org/2288853003/diff/20001/chrome/browser/chrome_b... chrome/browser/chrome_browser_field_trials.h:26: void SetupFeatureControllingFieldTrials(bool was_a_seed_applied, Nit: I find |was_a_seed_applied| name kind of awkward. How about just |had_seed|. Also, explain the param in the comment. https://codereview.chromium.org/2288853003/diff/20001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_metrics_services_manager_client.cc (right): https://codereview.chromium.org/2288853003/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_services_manager_client.cc:121: const std::string kTrialName = "MetricsAndCrashSampling"; static const char[] same for the other ones below
https://codereview.chromium.org/2288853003/diff/20001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_field_trials.cc (right): https://codereview.chromium.org/2288853003/diff/20001/chrome/browser/chrome_b... chrome/browser/chrome_browser_field_trials.cc:131: if (chrome::GetChannel() != version_info::Channel::UNKNOWN) On 2016/08/29 19:38:42, Alexei Svitkine (slow) wrote: > Shouldn't you be checking for STABLE? Done. https://codereview.chromium.org/2288853003/diff/20001/chrome/browser/chrome_b... chrome/browser/chrome_browser_field_trials.cc:162: #if defined(OS_WIN) || defined(OS_ANDROID) On 2016/08/29 19:38:42, Alexei Svitkine (slow) wrote: > How about just moving the ifdefs to the body of the function you're calling. > > It's already named "IfNeeded", so the ifdefs can be part of that. Done. https://codereview.chromium.org/2288853003/diff/20001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_field_trials.h (right): https://codereview.chromium.org/2288853003/diff/20001/chrome/browser/chrome_b... chrome/browser/chrome_browser_field_trials.h:26: void SetupFeatureControllingFieldTrials(bool was_a_seed_applied, On 2016/08/29 19:38:42, Alexei Svitkine (slow) wrote: > Nit: I find |was_a_seed_applied| name kind of awkward. How about just > |had_seed|. Also, explain the param in the comment. Done. https://codereview.chromium.org/2288853003/diff/20001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_metrics_services_manager_client.cc (right): https://codereview.chromium.org/2288853003/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_services_manager_client.cc:121: const std::string kTrialName = "MetricsAndCrashSampling"; On 2016/08/29 19:38:42, Alexei Svitkine (slow) wrote: > static const char[] > > same for the other ones below Done.
LGTM % lots of nits https://codereview.chromium.org/2288853003/diff/60001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_field_trials.cc (right): https://codereview.chromium.org/2288853003/diff/60001/chrome/browser/chrome_b... chrome/browser/chrome_browser_field_trials.cc:117: // Create a field trial to controll metrics/crash sampling for Stable on Nit: "controll" -> "control" https://codereview.chromium.org/2288853003/diff/60001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2288853003/diff/60001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:910: bool was_a_seed_applied = false; Nit: has_seed https://codereview.chromium.org/2288853003/diff/60001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_metrics_services_manager_client.cc (right): https://codereview.chromium.org/2288853003/diff/60001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_services_manager_client.cc:67: // associated with a variation param for reporting samlping |rate| in per mille. Nit: "samlping" -> "sampling" https://codereview.chromium.org/2288853003/diff/60001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_services_manager_client.cc:68: void AppendSamplingTrialGroup(const std::string& name, Nit: name -> group_name https://codereview.chromium.org/2288853003/diff/60001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_services_manager_client.cc:127: 1, 1, base::FieldTrial::ONE_TIME_RANDOMIZED, NULL)); Nit: nullptr https://codereview.chromium.org/2288853003/diff/60001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_metrics_services_manager_client.h (right): https://codereview.chromium.org/2288853003/diff/60001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_services_manager_client.h:30: // Unconditionally attempts to create a field trial to controll client side Nit: controll -> control https://codereview.chromium.org/2288853003/diff/60001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_services_manager_client.h:33: // have variations first-run support. This should only be called when there is Nit: "variations first-run" -> "first-run variations"
https://codereview.chromium.org/2288853003/diff/60001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_field_trials.cc (right): https://codereview.chromium.org/2288853003/diff/60001/chrome/browser/chrome_b... chrome/browser/chrome_browser_field_trials.cc:117: // Create a field trial to controll metrics/crash sampling for Stable on On 2016/08/30 18:17:05, Alexei Svitkine (slow) wrote: > Nit: "controll" -> "control" Done. https://codereview.chromium.org/2288853003/diff/60001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2288853003/diff/60001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:910: bool was_a_seed_applied = false; On 2016/08/30 18:17:05, Alexei Svitkine (slow) wrote: > Nit: has_seed Done. https://codereview.chromium.org/2288853003/diff/60001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_metrics_services_manager_client.cc (right): https://codereview.chromium.org/2288853003/diff/60001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_services_manager_client.cc:67: // associated with a variation param for reporting samlping |rate| in per mille. On 2016/08/30 18:17:05, Alexei Svitkine (slow) wrote: > Nit: "samlping" -> "sampling" Done. https://codereview.chromium.org/2288853003/diff/60001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_services_manager_client.cc:68: void AppendSamplingTrialGroup(const std::string& name, On 2016/08/30 18:17:05, Alexei Svitkine (slow) wrote: > Nit: name -> group_name Done. https://codereview.chromium.org/2288853003/diff/60001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_services_manager_client.cc:127: 1, 1, base::FieldTrial::ONE_TIME_RANDOMIZED, NULL)); On 2016/08/30 18:17:05, Alexei Svitkine (slow) wrote: > Nit: nullptr Done. https://codereview.chromium.org/2288853003/diff/60001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_metrics_services_manager_client.h (right): https://codereview.chromium.org/2288853003/diff/60001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_services_manager_client.h:30: // Unconditionally attempts to create a field trial to controll client side On 2016/08/30 18:17:05, Alexei Svitkine (slow) wrote: > Nit: controll -> control Done. https://codereview.chromium.org/2288853003/diff/60001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_services_manager_client.h:33: // have variations first-run support. This should only be called when there is On 2016/08/30 18:17:05, Alexei Svitkine (slow) wrote: > Nit: "variations first-run" -> "first-run variations" Done.
https://codereview.chromium.org/2288853003/diff/60001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_field_trials.cc (right): https://codereview.chromium.org/2288853003/diff/60001/chrome/browser/chrome_b... chrome/browser/chrome_browser_field_trials.cc:117: // Create a field trial to controll metrics/crash sampling for Stable on On 2016/08/30 18:17:05, Alexei Svitkine (slow) wrote: > Nit: "controll" -> "control" Done. https://codereview.chromium.org/2288853003/diff/60001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2288853003/diff/60001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:910: bool was_a_seed_applied = false; On 2016/08/30 18:17:05, Alexei Svitkine (slow) wrote: > Nit: has_seed Done. https://codereview.chromium.org/2288853003/diff/60001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_metrics_services_manager_client.cc (right): https://codereview.chromium.org/2288853003/diff/60001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_services_manager_client.cc:67: // associated with a variation param for reporting samlping |rate| in per mille. On 2016/08/30 18:17:05, Alexei Svitkine (slow) wrote: > Nit: "samlping" -> "sampling" Done. https://codereview.chromium.org/2288853003/diff/60001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_services_manager_client.cc:68: void AppendSamplingTrialGroup(const std::string& name, On 2016/08/30 18:17:05, Alexei Svitkine (slow) wrote: > Nit: name -> group_name Done. https://codereview.chromium.org/2288853003/diff/60001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_services_manager_client.cc:127: 1, 1, base::FieldTrial::ONE_TIME_RANDOMIZED, NULL)); On 2016/08/30 18:17:05, Alexei Svitkine (slow) wrote: > Nit: nullptr Done. https://codereview.chromium.org/2288853003/diff/60001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_metrics_services_manager_client.h (right): https://codereview.chromium.org/2288853003/diff/60001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_services_manager_client.h:30: // Unconditionally attempts to create a field trial to controll client side On 2016/08/30 18:17:05, Alexei Svitkine (slow) wrote: > Nit: controll -> control Done. https://codereview.chromium.org/2288853003/diff/60001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_services_manager_client.h:33: // have variations first-run support. This should only be called when there is On 2016/08/30 18:17:05, Alexei Svitkine (slow) wrote: > Nit: "variations first-run" -> "first-run variations" Done.
The CQ bit was checked by jwd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
jwd@chromium.org changed reviewers: + thestig@chromium.org
+thestig@ for chrome_browser_main.cc change.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2288853003/diff/80001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2288853003/diff/80001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:910: bool has_seed = false; How about: bool has_seed = variations_service && variations_service->Foo(); and then "let git cl format" make it pretty?
https://codereview.chromium.org/2288853003/diff/80001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2288853003/diff/80001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:910: bool has_seed = false; On 2016/08/30 21:31:28, Lei Zhang wrote: > How about: > > bool has_seed = variations_service && variations_service->Foo(); > > and then "let git cl format" make it pretty? Done.
The CQ bit was checked by jwd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2288853003/#ps100001 (title: "final comments")
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 ========== Adding local field trial for metrics/crash reports sampling. This is to support the first-run case, if no first-run variations seed was available. BUG=642086 ========== to ========== Adding local field trial for metrics/crash reports sampling. This is to support the first-run case, if no first-run variations seed was available. BUG=642086 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Adding local field trial for metrics/crash reports sampling. This is to support the first-run case, if no first-run variations seed was available. BUG=642086 ========== to ========== Adding local field trial for metrics/crash reports sampling. This is to support the first-run case, if no first-run variations seed was available. BUG=642086 Committed: https://crrev.com/34f77fad8128e9b51db959a4a1f9ab6bfcb08257 Cr-Commit-Position: refs/heads/master@{#415473} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/34f77fad8128e9b51db959a4a1f9ab6bfcb08257 Cr-Commit-Position: refs/heads/master@{#415473}
Message was sent while issue was closed.
Patchset #7 (id:120001) has been deleted |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
