PerfProvider: Get collection parameters from Finch

PerfProvider: Get collection parameters from variation parameters

Add a field trial for perf collection that allows overriding the
collection parameters and perf command lines. This will allow fast
reconfiguration of these parameters instead of waiting for the full
ChromeOS release cycle.

BUG=538759
Committed: https://crrev.com/788b3017b63b5c13ffc10a6f8c7b595db18ec7a0
Cr-Commit-Position: refs/heads/master@{#355359}

[dhsharp, 2015-10-14 18:10:05]

Patchset #1 (id:1) has been deleted

[dhsharp, 2015-10-15 19:36:29]

dhsharp@chromium.org changed reviewers:
+ asvitkine@chromium.org

[dhsharp, 2015-10-15 19:36:29]

Last one! Unfortunately, also the biggest.

[Alexei Svitkine (slow), 2015-10-16 15:37:50]

https://codereview.chromium.org/1392153003/diff/20001/chrome/browser/metrics/...
File chrome/browser/metrics/perf/perf_provider_chromeos.cc (right):

https://codereview.chromium.org/1392153003/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/perf/perf_provider_chromeos.cc:224: namespace {
Put these in the existing anon namespaces.

https://codereview.chromium.org/1392153003/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/perf/perf_provider_chromeos.cc:261: // The CPU specified
in the Finch params could be "default", a system
Nit: Finch is an internal codename. Use "Variations" or "FieldTrial" in open
source code. Also, update CL description.

https://codereview.chromium.org/1392153003/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/perf/perf_provider_chromeos.cc:311: if
(GetInt64Param(params, "ProfileCollectionDurationSec", &value))
Nit: {}'s for multi-line bodies.

https://codereview.chromium.org/1392153003/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/perf/perf_provider_chromeos.cc:325: if
(GetInt64Param(params, "RestoreSession::MaxCollectionDelaySec", &value))
Nit: Is it possible to use shorter names for these params? They are part of the
payload and every byte adds up.

https://codereview.chromium.org/1392153003/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/perf/perf_provider_chromeos.cc:352: if
(!(base::StringToDouble(weight_str, &weight) && weight > 0.0))
Nit: !(a && b) -> !a || !b

https://codereview.chromium.org/1392153003/diff/20001/chrome/browser/metrics/...
File chrome/browser/metrics/perf/perf_provider_chromeos.h (right):

https://codereview.chromium.org/1392153003/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/perf/perf_provider_chromeos.h:135: const
CollectionParams& collection_params() { return collection_params_; }
Nit: Mark the method const as well. Same below.

https://codereview.chromium.org/1392153003/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/perf/perf_provider_chromeos.h:160: const
std::map<std::string, std::string> &params);
Nit: indent 2 more - or maybe just run "git cl format".

[dhsharp, 2015-10-19 22:44:51]

Description was changed from

==========
PerfProvider: Get collection parameters from Finch

Add a field trial for perf collection that allows overriding the
collection parameters and perf command lines. This will allow fast
reconfiguration of these parameters instead of waiting for the full
ChromeOS release cycle.

BUG=538759
==========

to

==========
PerfProvider: Get collection parameters from variation parameters

Add a field trial for perf collection that allows overriding the
collection parameters and perf command lines. This will allow fast
reconfiguration of these parameters instead of waiting for the full
ChromeOS release cycle.

BUG=538759
==========

[dhsharp, 2015-10-19 22:45:28]

https://codereview.chromium.org/1392153003/diff/20001/chrome/browser/metrics/...
File chrome/browser/metrics/perf/perf_provider_chromeos.cc (right):

https://codereview.chromium.org/1392153003/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/perf/perf_provider_chromeos.cc:224: namespace {
On 2015/10/16 15:37:50, Alexei Svitkine wrote:
> Put these in the existing anon namespaces.

I'd prefer to keep these near where they are used.

https://codereview.chromium.org/1392153003/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/perf/perf_provider_chromeos.cc:261: // The CPU specified
in the Finch params could be "default", a system
On 2015/10/16 15:37:50, Alexei Svitkine wrote:
> Nit: Finch is an internal codename. Use "Variations" or "FieldTrial" in open
> source code. Also, update CL description.

Done.

https://codereview.chromium.org/1392153003/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/perf/perf_provider_chromeos.cc:311: if
(GetInt64Param(params, "ProfileCollectionDurationSec", &value))
On 2015/10/16 15:37:50, Alexei Svitkine wrote:
> Nit: {}'s for multi-line bodies.

Done.

https://codereview.chromium.org/1392153003/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/perf/perf_provider_chromeos.cc:325: if
(GetInt64Param(params, "RestoreSession::MaxCollectionDelaySec", &value))
On 2015/10/16 15:37:50, Alexei Svitkine wrote:
> Nit: Is it possible to use shorter names for these params? They are part of
the
> payload and every byte adds up.

It's certainly possible, heh. I guess it's a tradeoff between length and
readability. Do you have suggestions? I'm not exactly thrilled with the name
"MaxCollectionDelaySec" either. The parameter it controls is the upper bound of
the the randomly selected delay between the trigger and collection. I could omit
"Collection", but "MaxDelaySec" leaves me wondering "delay of what?".

I've done that for now. Let me know if you have better ideas.

https://codereview.chromium.org/1392153003/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/perf/perf_provider_chromeos.cc:352: if
(!(base::StringToDouble(weight_str, &weight) && weight > 0.0))
On 2015/10/16 15:37:50, Alexei Svitkine wrote:
> Nit: !(a && b) -> !a || !b

I could apply DeMorgan's laws. However:
- I prefer to make positive assertions about required conditions.
- I have to use "weight > 0.0", because the inverse condition, "weight <= 0.0",
would not reject NaN since it would evaluate false (I have test coverage for
this). I think "!(weight > 0.0)" is uglier.

https://codereview.chromium.org/1392153003/diff/20001/chrome/browser/metrics/...
File chrome/browser/metrics/perf/perf_provider_chromeos.h (right):

https://codereview.chromium.org/1392153003/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/perf/perf_provider_chromeos.h:135: const
CollectionParams& collection_params() { return collection_params_; }
On 2015/10/16 15:37:50, Alexei Svitkine wrote:
> Nit: Mark the method const as well. Same below.

Done.

https://codereview.chromium.org/1392153003/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/perf/perf_provider_chromeos.h:160: const
std::map<std::string, std::string> &params);
On 2015/10/16 15:37:50, Alexei Svitkine wrote:
> Nit: indent 2 more - or maybe just run "git cl format".

Done.

[Alexei Svitkine (slow), 2015-10-20 20:24:59]

https://codereview.chromium.org/1392153003/diff/20001/chrome/browser/metrics/...
File chrome/browser/metrics/perf/perf_provider_chromeos.cc (right):

https://codereview.chromium.org/1392153003/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/perf/perf_provider_chromeos.cc:224: namespace {
On 2015/10/19 22:45:28, dhsharp wrote:
> On 2015/10/16 15:37:50, Alexei Svitkine wrote:
> > Put these in the existing anon namespaces.
> 
> I'd prefer to keep these near where they are used.

In Chromium, the convention is to have a single anon namespace at the top of the
file, however.

https://codereview.chromium.org/1392153003/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/perf/perf_provider_chromeos.cc:325: if
(GetInt64Param(params, "RestoreSession::MaxCollectionDelaySec", &value))
On 2015/10/19 22:45:28, dhsharp wrote:
> On 2015/10/16 15:37:50, Alexei Svitkine wrote:
> > Nit: Is it possible to use shorter names for these params? They are part of
> the
> > payload and every byte adds up.
> 
> It's certainly possible, heh. I guess it's a tradeoff between length and
> readability. Do you have suggestions? I'm not exactly thrilled with the name
> "MaxCollectionDelaySec" either. The parameter it controls is the upper bound
of
> the the randomly selected delay between the trigger and collection. I could
omit
> "Collection", but "MaxDelaySec" leaves me wondering "delay of what?".
> 
> I've done that for now. Let me know if you have better ideas.
> 

Another option is to have the string names themselves be more cryptic but to
have string constants at the top of the file that spell them out more. Though,
I'm ok with current names too, I guess.

https://codereview.chromium.org/1392153003/diff/40001/chrome/browser/metrics/...
File chrome/browser/metrics/perf/perf_provider_chromeos.cc (right):

https://codereview.chromium.org/1392153003/diff/40001/chrome/browser/metrics/...
chrome/browser/metrics/perf/perf_provider_chromeos.cc:226: bool
GetInt64Param(const std::map<std::string, std::string>& params,
Nit: Add a short comment.

https://codereview.chromium.org/1392153003/diff/40001/chrome/browser/metrics/...
chrome/browser/metrics/perf/perf_provider_chromeos.cc:227: const std::string&
key, int64* out) {
Nit: I'd prefer the last param be on a separate line, as it's an out param and
would be easier to spot glancing at the code.

https://codereview.chromium.org/1392153003/diff/40001/chrome/browser/metrics/...
chrome/browser/metrics/perf/perf_provider_chromeos.cc:233: if
(!base::StringToInt64(it->second, &value))
Nit: How about this more concise form:

if (it == params.end() || !base::StringToInt64())

https://codereview.chromium.org/1392153003/diff/40001/chrome/browser/metrics/...
chrome/browser/metrics/perf/perf_provider_chromeos.cc:246: if (tokens[0] !=
"PerfCommand")
Nit: Ditto, combine the ifs.

https://codereview.chromium.org/1392153003/diff/40001/chrome/browser/metrics/...
chrome/browser/metrics/perf/perf_provider_chromeos.cc:293: intel_uarch != "" &&
cpu_specifier == intel_uarch) {
Nit: !intel_uarch.empty()

https://codereview.chromium.org/1392153003/diff/40001/chrome/browser/metrics/...
chrome/browser/metrics/perf/perf_provider_chromeos.cc:338: if
(best_cpu_specifier.empty())  // no matching cpu specifier. Keep defaults.
Nit: Capitalize No

https://codereview.chromium.org/1392153003/diff/40001/chrome/browser/metrics/...
File chrome/browser/metrics/perf/perf_provider_chromeos_unittest.cc (right):

https://codereview.chromium.org/1392153003/diff/40001/chrome/browser/metrics/...
chrome/browser/metrics/perf/perf_provider_chromeos_unittest.cc:833:
params.insert(param_t("PeriodicProfilingIntervalMs","3600000"));
Nit: Space after ,

Fix throughout.

[dhsharp, 2015-10-21 02:17:03]

https://codereview.chromium.org/1392153003/diff/20001/chrome/browser/metrics/...
File chrome/browser/metrics/perf/perf_provider_chromeos.cc (right):

https://codereview.chromium.org/1392153003/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/perf/perf_provider_chromeos.cc:224: namespace {
On 2015/10/20 20:24:58, Alexei Svitkine (slow) wrote:
> On 2015/10/19 22:45:28, dhsharp wrote:
> > On 2015/10/16 15:37:50, Alexei Svitkine wrote:
> > > Put these in the existing anon namespaces.
> > 
> > I'd prefer to keep these near where they are used.
> 
> In Chromium, the convention is to have a single anon namespace at the top of
the
> file, however.

Well, okay. Done.

But you might want to tell these 414 files that (I realize it's a small fraction
of Chromium files):
https://code.google.com/p/chromium/codesearch#search/&q=pcre:yes%20(?m:namesp...

https://codereview.chromium.org/1392153003/diff/40001/chrome/browser/metrics/...
File chrome/browser/metrics/perf/perf_provider_chromeos.cc (right):

https://codereview.chromium.org/1392153003/diff/40001/chrome/browser/metrics/...
chrome/browser/metrics/perf/perf_provider_chromeos.cc:226: bool
GetInt64Param(const std::map<std::string, std::string>& params,
On 2015/10/20 20:24:58, Alexei Svitkine (slow) wrote:
> Nit: Add a short comment.

Done.

https://codereview.chromium.org/1392153003/diff/40001/chrome/browser/metrics/...
chrome/browser/metrics/perf/perf_provider_chromeos.cc:227: const std::string&
key, int64* out) {
On 2015/10/20 20:24:58, Alexei Svitkine (slow) wrote:
> Nit: I'd prefer the last param be on a separate line, as it's an out param and
> would be easier to spot glancing at the code.

Done.

https://codereview.chromium.org/1392153003/diff/40001/chrome/browser/metrics/...
chrome/browser/metrics/perf/perf_provider_chromeos.cc:233: if
(!base::StringToInt64(it->second, &value))
On 2015/10/20 20:24:58, Alexei Svitkine (slow) wrote:
> Nit: How about this more concise form:
> 
> if (it == params.end() || !base::StringToInt64())

I don't like to combine unrelated conditions. In this case, I think it also
makes the placement of the definition of value and the comment regarding
StringToInt64 awkward.

https://codereview.chromium.org/1392153003/diff/40001/chrome/browser/metrics/...
chrome/browser/metrics/perf/perf_provider_chromeos.cc:246: if (tokens[0] !=
"PerfCommand")
On 2015/10/20 20:24:58, Alexei Svitkine (slow) wrote:
> Nit: Ditto, combine the ifs.

I don't like how the code reads. I think it minimizes the importance of
requiring "PerfCommand" as the first token. I see this code as processing each
of 3 tokens. So: first, split and check that there are 3 tokens. Then check that
the first is "PerfCommand". Then store the second. Finally, ignore the third.

https://codereview.chromium.org/1392153003/diff/40001/chrome/browser/metrics/...
chrome/browser/metrics/perf/perf_provider_chromeos.cc:293: intel_uarch != "" &&
cpu_specifier == intel_uarch) {
On 2015/10/20 20:24:58, Alexei Svitkine (slow) wrote:
> Nit: !intel_uarch.empty()

Done.

https://codereview.chromium.org/1392153003/diff/40001/chrome/browser/metrics/...
chrome/browser/metrics/perf/perf_provider_chromeos.cc:338: if
(best_cpu_specifier.empty())  // no matching cpu specifier. Keep defaults.
On 2015/10/20 20:24:58, Alexei Svitkine (slow) wrote:
> Nit: Capitalize No

Done.

https://codereview.chromium.org/1392153003/diff/40001/chrome/browser/metrics/...
File chrome/browser/metrics/perf/perf_provider_chromeos_unittest.cc (right):

https://codereview.chromium.org/1392153003/diff/40001/chrome/browser/metrics/...
chrome/browser/metrics/perf/perf_provider_chromeos_unittest.cc:833:
params.insert(param_t("PeriodicProfilingIntervalMs","3600000"));
On 2015/10/20 20:24:58, Alexei Svitkine (slow) wrote:
> Nit: Space after ,
> 
> Fix throughout.

Woops, Done.

[Alexei Svitkine (slow), 2015-10-21 16:52:56]

https://codereview.chromium.org/1392153003/diff/60001/chrome/browser/metrics/...
File chrome/browser/metrics/perf/perf_provider_chromeos_unittest.cc (right):

https://codereview.chromium.org/1392153003/diff/60001/chrome/browser/metrics/...
chrome/browser/metrics/perf/perf_provider_chromeos_unittest.cc:758:
scoped_ptr<TestPerfProvider> perf_provider_;
I don't think this needs to be a member variable.

In all your tests, you just create this near the end and call a method on it.

I suggest changing the tests to just declare this locally in the tests. It
doesn't even need to be a scoped_ptr, just stack allocated.

https://codereview.chromium.org/1392153003/diff/60001/chrome/browser/metrics/...
chrome/browser/metrics/perf/perf_provider_chromeos_unittest.cc:764:
DISALLOW_COPY_AND_ASSIGN(PerfProviderCollectionParamsTest);
Nit: This should be in private: section. Suggest putting a private: section
above any members that aren't needed to be used from the tests.

https://codereview.chromium.org/1392153003/diff/60001/chrome/browser/metrics/...
chrome/browser/metrics/perf/perf_provider_chromeos_unittest.cc:787:
params.insert(param_t("PerfCommand::default::0", ""));
Nit: Can you avoid the "using param_t" above if you just do
params.insert(make_pair("foo", "bar"))? If that works, suggest using that
instead.

https://codereview.chromium.org/1392153003/diff/60001/chrome/browser/metrics/...
File chrome/browser/metrics/perf/random_selector.h (right):

https://codereview.chromium.org/1392153003/diff/60001/chrome/browser/metrics/...
chrome/browser/metrics/perf/random_selector.h:88: ::std::ostream& operator<<(
Can you add a comment for why this is needed? If it's just to satisfy tests for
the macros, suggest moving this to the unittest file.

[dhsharp, 2015-10-21 18:19:05]

https://codereview.chromium.org/1392153003/diff/60001/chrome/browser/metrics/...
File chrome/browser/metrics/perf/perf_provider_chromeos_unittest.cc (right):

https://codereview.chromium.org/1392153003/diff/60001/chrome/browser/metrics/...
chrome/browser/metrics/perf/perf_provider_chromeos_unittest.cc:758:
scoped_ptr<TestPerfProvider> perf_provider_;
On 2015/10/21 16:52:56, Alexei Svitkine (slow) wrote:
> I don't think this needs to be a member variable.
> 
> In all your tests, you just create this near the end and call a method on it.
> 
> I suggest changing the tests to just declare this locally in the tests. It
> doesn't even need to be a scoped_ptr, just stack allocated.

Done.

https://codereview.chromium.org/1392153003/diff/60001/chrome/browser/metrics/...
chrome/browser/metrics/perf/perf_provider_chromeos_unittest.cc:764:
DISALLOW_COPY_AND_ASSIGN(PerfProviderCollectionParamsTest);
On 2015/10/21 16:52:56, Alexei Svitkine (slow) wrote:
> Nit: This should be in private: section. Suggest putting a private: section
> above any members that aren't needed to be used from the tests.

Done.

https://codereview.chromium.org/1392153003/diff/60001/chrome/browser/metrics/...
chrome/browser/metrics/perf/perf_provider_chromeos_unittest.cc:787:
params.insert(param_t("PerfCommand::default::0", ""));
On 2015/10/21 16:52:56, Alexei Svitkine (slow) wrote:
> Nit: Can you avoid the "using param_t" above if you just do
> params.insert(make_pair("foo", "bar"))? If that works, suggest using that
> instead.

Yeah, that's better. I didn't think the type deduction would work, but I guess
pair<const char *, const char *> is convertible to pair<std::string,
std::string>. Done.

https://codereview.chromium.org/1392153003/diff/60001/chrome/browser/metrics/...
File chrome/browser/metrics/perf/random_selector.h (right):

https://codereview.chromium.org/1392153003/diff/60001/chrome/browser/metrics/...
chrome/browser/metrics/perf/random_selector.h:88: ::std::ostream& operator<<(
On 2015/10/21 16:52:56, Alexei Svitkine (slow) wrote:
> Can you add a comment for why this is needed? If it's just to satisfy tests
for
> the macros, suggest moving this to the unittest file.

Added comment. It's used in two unit test files, and it needs to be global to be
used by gtest.

[Alexei Svitkine (slow), 2015-10-21 18:31:53]

lgtm

[dhsharp, 2015-10-21 18:32:51]

The CQ bit was checked by dhsharp@chromium.org

[commit-bot: I haz the power, 2015-10-21 18:35:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1392153003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1392153003/80001

[commit-bot: I haz the power, 2015-10-21 19:00:53]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2015-10-21 19:01:51]

Patchset 4 (id:??) landed as
https://crrev.com/788b3017b63b5c13ffc10a6f8c7b595db18ec7a0
Cr-Commit-Position: refs/heads/master@{#355359}