Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2691033002:
Collect field trial information from the stability file (Closed)
|
Created:
3 years, 10 months ago by manzagop (departed) Modified:
3 years, 10 months ago CC:
chromium-reviews, blundell+watchlist_chromium.org, sdefresne+watchlist_chromium.org, droger+watchlist_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCollect field trial information from the stability file
Field trial information is recorded using the global user data
mechanism (https://crrev.com/2666653002/). This CL revises global user
data collection such that field trial data is processed (from strings
to finch identifiers) and collected to a new field trial field in the
report.
BUG=691595
Review-Url: https://codereview.chromium.org/2691033002
Cr-Commit-Position: refs/heads/master@{#451625}
Committed: https://chromium.googlesource.com/chromium/src/+/4904876279fbe19be093f6a669fd71905a4ac14c
Patch Set 1 #Patch Set 2 : signed/unsigned #Patch Set 3 : Merge #
Total comments: 8
Patch Set 4 : Address bcwhite's comments #Patch Set 5 : Merge #Patch Set 6 : Fix test post merge #
Total comments: 8
Patch Set 7 : Address rkaplow's comments #
Messages
Total messages: 42 (28 generated)
The CQ bit was checked by manzagop@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...
manzagop@chromium.org changed reviewers: + bcwhite@chromium.org
This revises global data collection to handle field trials. Please have a look!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by manzagop@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 manzagop@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: This issue passed the CQ dry run.
https://codereview.chromium.org/2691033002/diff/40001/components/browser_watc... File components/browser_watcher/postmortem_report_collector.cc (right): https://codereview.chromium.org/2691033002/diff/40001/components/browser_watc... components/browser_watcher/postmortem_report_collector.cc:70: if (report && base::StartsWith(key, kFieldTrialKeyPrefix, To be clear, you want field-trial strings to go into the collected values if no report is requested? https://codereview.chromium.org/2691033002/diff/40001/components/browser_watc... components/browser_watcher/postmortem_report_collector.cc:83: collected_value.set_string_value(value); I believe set_string_value will accept (char*, len) which would be more efficient that creating a std::string from the returned StringPiece. That's probably also true for set_bytes_value and set_char_value.
The CQ bit was checked by manzagop@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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by manzagop@chromium.org to run a CQ dry run
Ready for another look! https://codereview.chromium.org/2691033002/diff/40001/components/browser_watc... File components/browser_watcher/postmortem_report_collector.cc (right): https://codereview.chromium.org/2691033002/diff/40001/components/browser_watc... components/browser_watcher/postmortem_report_collector.cc:70: if (report && base::StartsWith(key, kFieldTrialKeyPrefix, On 2017/02/14 15:42:14, bcwhite wrote: > To be clear, you want field-trial strings to go into the collected values if no > report is requested? Yeah, this is clunky. I'm using this function to collect both the global data, which may contain finch data, and activity data, which should not. In the first case I pass in the report, in the second I don't have one. https://codereview.chromium.org/2691033002/diff/40001/components/browser_watc... components/browser_watcher/postmortem_report_collector.cc:83: collected_value.set_string_value(value); On 2017/02/14 15:42:14, bcwhite wrote: > I believe set_string_value will accept (char*, len) which would be more > efficient that creating a std::string from the returned StringPiece. > > That's probably also true for set_bytes_value and set_char_value. Nice! Ah, but MakeActiveGroupId wants a string. Still changed it, if that function ever changes. Also changed the other cases.
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.
https://codereview.chromium.org/2691033002/diff/40001/components/browser_watc... File components/browser_watcher/postmortem_report_collector.cc (right): https://codereview.chromium.org/2691033002/diff/40001/components/browser_watc... components/browser_watcher/postmortem_report_collector.cc:70: if (report && base::StartsWith(key, kFieldTrialKeyPrefix, On 2017/02/14 19:21:15, manzagop wrote: > On 2017/02/14 15:42:14, bcwhite wrote: > > To be clear, you want field-trial strings to go into the collected values if > no > > report is requested? > > Yeah, this is clunky. I'm using this function to collect both the global data, > which may contain finch data, and activity data, which should not. In the first > case I pass in the report, in the second I don't have one. On the second pass, do you want the field trials to be ignored or included with other global data?
https://codereview.chromium.org/2691033002/diff/40001/components/browser_watc... File components/browser_watcher/postmortem_report_collector.cc (right): https://codereview.chromium.org/2691033002/diff/40001/components/browser_watc... components/browser_watcher/postmortem_report_collector.cc:70: if (report && base::StartsWith(key, kFieldTrialKeyPrefix, On 2017/02/14 21:05:16, bcwhite wrote: > On 2017/02/14 19:21:15, manzagop wrote: > > On 2017/02/14 15:42:14, bcwhite wrote: > > > To be clear, you want field-trial strings to go into the collected values if > > no > > > report is requested? > > > > Yeah, this is clunky. I'm using this function to collect both the global data, > > which may contain finch data, and activity data, which should not. In the > first > > case I pass in the report, in the second I don't have one. > > On the second pass, do you want the field trials to be ignored or included with > other global data? Not sure I follow: what's the second pass? Do you mean for activity-local data? Those shouldn't have field trial data. If they do, it's fine to leave it in, that way we'll know we're doing something wrong.
lgtm https://codereview.chromium.org/2691033002/diff/40001/components/browser_watc... File components/browser_watcher/postmortem_report_collector.cc (right): https://codereview.chromium.org/2691033002/diff/40001/components/browser_watc... components/browser_watcher/postmortem_report_collector.cc:70: if (report && base::StartsWith(key, kFieldTrialKeyPrefix, On 2017/02/14 21:27:57, manzagop wrote: > On 2017/02/14 21:05:16, bcwhite wrote: > > On 2017/02/14 19:21:15, manzagop wrote: > > > On 2017/02/14 15:42:14, bcwhite wrote: > > > > To be clear, you want field-trial strings to go into the collected values > if > > > no > > > > report is requested? > > > > > > Yeah, this is clunky. I'm using this function to collect both the global > data, > > > which may contain finch data, and activity data, which should not. In the > > first > > > case I pass in the report, in the second I don't have one. > > > > On the second pass, do you want the field trials to be ignored or included > with > > other global data? > > Not sure I follow: what's the second pass? Do you mean for activity-local data? > Those shouldn't have field trial data. If they do, it's fine to leave it in, > that way we'll know we're doing something wrong. You said that "in the second I don't have one" so I just wanted to be sure that, during that case, you want all the field trials added as their existing strings into the collected_map.
Thanks for the review! https://codereview.chromium.org/2691033002/diff/40001/components/browser_watc... File components/browser_watcher/postmortem_report_collector.cc (right): https://codereview.chromium.org/2691033002/diff/40001/components/browser_watc... components/browser_watcher/postmortem_report_collector.cc:70: if (report && base::StartsWith(key, kFieldTrialKeyPrefix, On 2017/02/15 16:24:19, bcwhite wrote: > On 2017/02/14 21:27:57, manzagop wrote: > > On 2017/02/14 21:05:16, bcwhite wrote: > > > On 2017/02/14 19:21:15, manzagop wrote: > > > > On 2017/02/14 15:42:14, bcwhite wrote: > > > > > To be clear, you want field-trial strings to go into the collected > values > > if > > > > no > > > > > report is requested? > > > > > > > > Yeah, this is clunky. I'm using this function to collect both the global > > data, > > > > which may contain finch data, and activity data, which should not. In the > > > first > > > > case I pass in the report, in the second I don't have one. > > > > > > On the second pass, do you want the field trials to be ignored or included > > with > > > other global data? > > > > Not sure I follow: what's the second pass? Do you mean for activity-local > data? > > Those shouldn't have field trial data. If they do, it's fine to leave it in, > > that way we'll know we're doing something wrong. > > You said that "in the second I don't have one" so I just wanted to be sure that, > during that case, you want all the field trials added as their existing strings > into the collected_map. Acknowledged.
manzagop@chromium.org changed reviewers: + rkaplow@chromium.org
Hi Rob! Could you give your OWNERS' blessing for the introduced DEPS on components/variations? I make use of variations::MakeActiveGroupId to get ids from field trial name / group. Thanks, Pierre
https://codereview.chromium.org/2691033002/diff/100001/components/browser_wat... File components/browser_watcher/postmortem_report_collector.cc (right): https://codereview.chromium.org/2691033002/diff/100001/components/browser_wat... components/browser_watcher/postmortem_report_collector.cc:75: // This entry represents an active Finch experiment. nit, use Field Trial in chromium-land https://codereview.chromium.org/2691033002/diff/100001/components/browser_wat... File components/browser_watcher/postmortem_report_collector_unittest.cc (right): https://codereview.chromium.org/2691033002/diff/100001/components/browser_wat... components/browser_watcher/postmortem_report_collector_unittest.cc:671: ASSERT_EQ(2, report->field_trials_size()); are you using an assert here because you want the following expect to not ever be out of range (and give an out of bounds error, I guess?) https://codereview.chromium.org/2691033002/diff/100001/components/browser_wat... components/browser_watcher/postmortem_report_collector_unittest.cc:680: ASSERT_EQ(5U, collected_data.size()); can't these safely be expects though? https://codereview.chromium.org/2691033002/diff/100001/components/browser_wat... File components/browser_watcher/stability_report.proto (right): https://codereview.chromium.org/2691033002/diff/100001/components/browser_wat... components/browser_watcher/stability_report.proto:153: // Description of a field trial or experiment that the user is currently can you add pointers to the uma proto which stores these (as an analogue) also you may want a pointer to how these identifiers are generated
Please have another look! https://codereview.chromium.org/2691033002/diff/100001/components/browser_wat... File components/browser_watcher/postmortem_report_collector.cc (right): https://codereview.chromium.org/2691033002/diff/100001/components/browser_wat... components/browser_watcher/postmortem_report_collector.cc:75: // This entry represents an active Finch experiment. On 2017/02/16 21:27:28, rkaplow wrote: > nit, use Field Trial in chromium-land Done. https://codereview.chromium.org/2691033002/diff/100001/components/browser_wat... File components/browser_watcher/postmortem_report_collector_unittest.cc (right): https://codereview.chromium.org/2691033002/diff/100001/components/browser_wat... components/browser_watcher/postmortem_report_collector_unittest.cc:671: ASSERT_EQ(2, report->field_trials_size()); On 2017/02/16 21:27:28, rkaplow wrote: > are you using an assert here because you want the following expect to not ever > be out of range (and give an out of bounds error, I guess?) Yes! https://codereview.chromium.org/2691033002/diff/100001/components/browser_wat... components/browser_watcher/postmortem_report_collector_unittest.cc:680: ASSERT_EQ(5U, collected_data.size()); On 2017/02/16 21:27:28, rkaplow wrote: > can't these safely be expects though? Yes! https://codereview.chromium.org/2691033002/diff/100001/components/browser_wat... File components/browser_watcher/stability_report.proto (right): https://codereview.chromium.org/2691033002/diff/100001/components/browser_wat... components/browser_watcher/stability_report.proto:153: // Description of a field trial or experiment that the user is currently On 2017/02/16 21:27:28, rkaplow wrote: > can you add pointers to the uma proto which stores these (as an analogue) > > also you may want a pointer to how these identifiers are generated > Done.
The CQ bit was checked by manzagop@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...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
Thanks for the review!
The CQ bit was checked by manzagop@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bcwhite@chromium.org Link to the patchset: https://codereview.chromium.org/2691033002/#ps120001 (title: "Address rkaplow's comments")
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": 120001, "attempt_start_ts": 1487598082909870,
"parent_rev": "18c783a47701f4ddc8756f3df1c49ca747f3dc87", "commit_rev":
"4904876279fbe19be093f6a669fd71905a4ac14c"}
Message was sent while issue was closed.
Description was changed from ========== Collect field trial information from the stability file Field trial information is recorded using the global user data mechanism (https://crrev.com/2666653002/). This CL revises global user data collection such that field trial data is processed (from strings to finch identifiers) and collected to a new field trial field in the report. BUG=691595 ========== to ========== Collect field trial information from the stability file Field trial information is recorded using the global user data mechanism (https://crrev.com/2666653002/). This CL revises global user data collection such that field trial data is processed (from strings to finch identifiers) and collected to a new field trial field in the report. BUG=691595 Review-Url: https://codereview.chromium.org/2691033002 Cr-Commit-Position: refs/heads/master@{#451625} Committed: https://chromium.googlesource.com/chromium/src/+/4904876279fbe19be093f6a669fd... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/4904876279fbe19be093f6a669fd... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
