Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2524363003:
Support experiment configurations with/without stability metrics. (Closed)
|
Created:
4 years ago by bcwhite Modified:
4 years ago Reviewers:
Alexei Svitkine (slow) CC:
chromium-reviews, asvitkine+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSupport experiment configurations with/without stability metrics.
Also: Ensure all HasInitialStabilityMetrics methods get called as they
may do other setup.
BUG=546019
Committed: https://crrev.com/02ca7451aa852e2eea5ec7a3ffc3285f58eafd2f
Cr-Commit-Position: refs/heads/master@{#435084}
Patch Set 1 #
Total comments: 8
Patch Set 2 : addressed review comments by Alexei #
Total comments: 4
Patch Set 3 : move experiment check into FileMetricsProvider #
Messages
Total messages: 37 (26 generated)
The CQ bit was checked by bcwhite@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_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
bcwhite@chromium.org changed reviewers: + asvitkine@chromium.org
https://codereview.chromium.org/2524363003/diff/1/chrome/browser/chrome_brows... File chrome/browser/chrome_browser_field_trials.cc (right): https://codereview.chromium.org/2524363003/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_field_trials.cc:89: unreported == "yes"); This would change the meaning of the existing groups. How about making the new param be in the negative (e.g. disable_stability_uploads or something)? https://codereview.chromium.org/2524363003/diff/1/components/metrics/file_met... File components/metrics/file_metrics_provider.cc (right): https://codereview.chromium.org/2524363003/diff/1/components/metrics/file_met... components/metrics/file_metrics_provider.cc:109: bool FileMetricsProvider::stability_metrics_enabled_ = true; I don't see where you're using this? https://codereview.chromium.org/2524363003/diff/1/components/metrics/file_met... File components/metrics/file_metrics_provider.h (right): https://codereview.chromium.org/2524363003/diff/1/components/metrics/file_met... components/metrics/file_metrics_provider.h:107: stability_metrics_enabled_ = enabled; Nit: Either use hacker_style for the function name or move its body to the .cc. https://codereview.chromium.org/2524363003/diff/1/components/metrics/metrics_... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/2524363003/diff/1/components/metrics/metrics_... components/metrics/metrics_service.cc:919: has_stability_metrics |= provider->HasInitialStabilityMetrics(); Please add a comment above why this doesn't do an early return.
The CQ bit was checked by bcwhite@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...
https://codereview.chromium.org/2524363003/diff/1/chrome/browser/chrome_brows... File chrome/browser/chrome_browser_field_trials.cc (right): https://codereview.chromium.org/2524363003/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_field_trials.cc:89: unreported == "yes"); On 2016/11/24 21:27:58, Alexei Svitkine (slow) wrote: > This would change the meaning of the existing groups. > > How about making the new param be in the negative (e.g. > disable_stability_uploads or something)? I've never liked the double negatives that occur that way but there's an easy fix: != "no". https://codereview.chromium.org/2524363003/diff/1/components/metrics/file_met... File components/metrics/file_metrics_provider.cc (right): https://codereview.chromium.org/2524363003/diff/1/components/metrics/file_met... components/metrics/file_metrics_provider.cc:109: bool FileMetricsProvider::stability_metrics_enabled_ = true; On 2016/11/24 21:27:58, Alexei Svitkine (slow) wrote: > I don't see where you're using this? Doh! Unsaved file after testing. https://codereview.chromium.org/2524363003/diff/1/components/metrics/file_met... File components/metrics/file_metrics_provider.h (right): https://codereview.chromium.org/2524363003/diff/1/components/metrics/file_met... components/metrics/file_metrics_provider.h:107: stability_metrics_enabled_ = enabled; On 2016/11/24 21:27:58, Alexei Svitkine (slow) wrote: > Nit: Either use hacker_style for the function name or move its body to the .cc. Done. https://codereview.chromium.org/2524363003/diff/1/components/metrics/metrics_... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/2524363003/diff/1/components/metrics/metrics_... components/metrics/metrics_service.cc:919: has_stability_metrics |= provider->HasInitialStabilityMetrics(); On 2016/11/24 21:27:58, Alexei Svitkine (slow) wrote: > Please add a comment above why this doesn't do an early return. Done.
https://codereview.chromium.org/2524363003/diff/20001/components/metrics/file... File components/metrics/file_metrics_provider.cc (right): https://codereview.chromium.org/2524363003/diff/20001/components/metrics/file... components/metrics/file_metrics_provider.cc:475: if (!stability_metrics_enabled_) Instead of the extra boolean, why not check the param directly here? (You can use a helper function in the anon namespace to make the call site simple). This way, it avoids extra complexity of a static and having to set it from somewhere else.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2524363003/diff/20001/components/metrics/file... File components/metrics/file_metrics_provider.cc (right): https://codereview.chromium.org/2524363003/diff/20001/components/metrics/file... components/metrics/file_metrics_provider.cc:475: if (!stability_metrics_enabled_) On 2016/11/25 19:44:42, Alexei Svitkine (slow) wrote: > Instead of the extra boolean, why not check the param directly here? (You can > use a helper function in the anon namespace to make the call site simple). > > This way, it avoids extra complexity of a static and having to set it from > somewhere else. I could. I thought it would be better to keep experiment access in one place. There's nothing else in this class that knows about experiments.
https://codereview.chromium.org/2524363003/diff/20001/components/metrics/file... File components/metrics/file_metrics_provider.cc (right): https://codereview.chromium.org/2524363003/diff/20001/components/metrics/file... components/metrics/file_metrics_provider.cc:475: if (!stability_metrics_enabled_) On 2016/11/28 15:35:46, bcwhite wrote: > On 2016/11/25 19:44:42, Alexei Svitkine (slow) wrote: > > Instead of the extra boolean, why not check the param directly here? (You can > > use a helper function in the anon namespace to make the call site simple). > > > > This way, it avoids extra complexity of a static and having to set it from > > somewhere else. > > I could. I thought it would be better to keep experiment access in one place. > There's nothing else in this class that knows about experiments. All other things being equal, I agree. However, here it's a trade off between making the API surface more complex vs. having the code to check experiment params be in separate files. I think it would be better to keep the API surface better at the cost of checking the params in two different places.
The CQ bit was checked by bcwhite@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_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Patchset #3 (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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #3 (id:60001) has been deleted
The CQ bit was checked by bcwhite@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...
https://codereview.chromium.org/2524363003/diff/20001/components/metrics/file... File components/metrics/file_metrics_provider.cc (right): https://codereview.chromium.org/2524363003/diff/20001/components/metrics/file... components/metrics/file_metrics_provider.cc:475: if (!stability_metrics_enabled_) On 2016/11/28 16:08:49, Alexei Svitkine (slow) wrote: > On 2016/11/28 15:35:46, bcwhite wrote: > > On 2016/11/25 19:44:42, Alexei Svitkine (slow) wrote: > > > Instead of the extra boolean, why not check the param directly here? (You > can > > > use a helper function in the anon namespace to make the call site simple). > > > > > > This way, it avoids extra complexity of a static and having to set it from > > > somewhere else. > > > > I could. I thought it would be better to keep experiment access in one place. > > > There's nothing else in this class that knows about experiments. > > All other things being equal, I agree. > > However, here it's a trade off between making the API surface more complex vs. > having the code to check experiment params be in separate files. > > I think it would be better to keep the API surface better at the cost of > checking the params in two different places. Done.
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 bcwhite@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1480452870904730,
"parent_rev": "a9fe38b8fce80f0a13117584dbf5c1184e3cb470", "commit_rev":
"edb1f289a32295117c6217cd3c27f261078491b2"}
Message was sent while issue was closed.
Committed patchset #3 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Support experiment configurations with/without stability metrics. Also: Ensure all HasInitialStabilityMetrics methods get called as they may do other setup. BUG=546019 ========== to ========== Support experiment configurations with/without stability metrics. Also: Ensure all HasInitialStabilityMetrics methods get called as they may do other setup. BUG=546019 Committed: https://crrev.com/02ca7451aa852e2eea5ec7a3ffc3285f58eafd2f Cr-Commit-Position: refs/heads/master@{#435084} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/02ca7451aa852e2eea5ec7a3ffc3285f58eafd2f Cr-Commit-Position: refs/heads/master@{#435084} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
