Match only prefixes of histogram names.

Match only prefixes of histogram names.

Fix the bug that HistogramTester::GetTotalCountsForPrefix returns histograms
which contain the query string as substrings, not as prefixes.

Additionally, omit explicit base:: namespace here and there, because the files
are already in this namespace.

BUG=659977
Review-Url: https://codereview.chromium.org/2628843002
Cr-Commit-Position: refs/heads/master@{#443620}
Committed: https://chromium.googlesource.com/chromium/src/+/63e22b6fcb629ebccc0f257de04bc8e7f4ef247c

[pkalinnikov, 2017-01-11 12:29:30]

pkalinnikov@chromium.org changed reviewers:
+ phajdan.jr@chromium.org

[pkalinnikov, 2017-01-11 12:29:30]

Hi Pawel,
Please review the change.

[pkalinnikov, 2017-01-11 12:29:41]

The CQ bit was checked by pkalinnikov@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 12:29:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pkalinnikov, 2017-01-11 12:52:39]

Description was changed from

==========
Match only prefixes of histogram names.

BUG=659977
==========

to

==========
Match only prefixes of histogram names.

BUG=659977
==========

[commit-bot: I haz the power, 2017-01-11 13:49:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-11 13:49:10]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Paweł Hajdan Jr., 2017-01-11 20:45:00]

https://codereview.chromium.org/2628843002/diff/20001/base/test/histogram_tes...
File base/test/histogram_tester.cc (right):

https://codereview.chromium.org/2628843002/diff/20001/base/test/histogram_tes...
base/test/histogram_tester.cc:111: // Find matches by using the prefix-matching
logic built into GetSnapshot().
This suggests prefix-matching logic in GetSnapshot was supposed to handle things
correctly.

Could you consult other histogram owners to find the best place for the logic?

I'd be fine with this code, but there may be a better solution.

[pkalinnikov, 2017-01-12 11:16:31]

pkalinnikov@chromium.org changed reviewers:
+ isherman@chromium.org

[pkalinnikov, 2017-01-12 11:16:31]

Hi Ilya and Robert,

Could you please address Pawel's comment in "histogram_tester.cc"? Would it be
better to make StatisticsRecorder::GetSnapshot function check prefixes instead
of substrings, or keeping this logic in HistogramTester::GetTotalCountsForPrefix
seems fine?

[Ilya Sherman, 2017-01-12 20:32:42]

LGTM; this seems like a fine approach.  Some nits inline:

https://codereview.chromium.org/2628843002/diff/20001/base/test/histogram_tes...
File base/test/histogram_tester.cc (right):

https://codereview.chromium.org/2628843002/diff/20001/base/test/histogram_tes...
base/test/histogram_tester.cc:106: const std::string& query) const {
nit: While you're fixing up this method, please update this parameter name to be
"prefix", both here and in the header file.  ("query" makes a bit more sense for
the StatisticsRecorder code, which is primarily dealing with HTML query
strings.)

https://codereview.chromium.org/2628843002/diff/20001/base/test/histogram_tes...
base/test/histogram_tester.cc:111: // Find matches by using the prefix-matching
logic built into GetSnapshot().
On 2017/01/11 20:45:00, Paweł Hajdan Jr. wrote:
> This suggests prefix-matching logic in GetSnapshot was supposed to handle
things
> correctly.
> 
> Could you consult other histogram owners to find the best place for the logic?
> 
> I'd be fine with this code, but there may be a better solution.

This comment is not quite correct.  StatisticsRecorder::GetSnapshot()
intentionally filters by substring rather than by prefix.  So, let's update or
omit this comment.

https://codereview.chromium.org/2628843002/diff/20001/base/test/histogram_tes...
base/test/histogram_tester.cc:116: for (base::HistogramBase* histogram :
query_matches) {
Optional nit: While you're here, it would be nice to drop this unnecessary use
of the "base::" namespace.

https://codereview.chromium.org/2628843002/diff/20001/base/test/histogram_tes...
File base/test/histogram_tester_unittest.cc (right):

https://codereview.chromium.org/2628843002/diff/20001/base/test/histogram_tes...
base/test/histogram_tester_unittest.cc:123:
UMA_HISTOGRAM_ENUMERATION(kHistogramSuffix, 2, 5);
Optional nit: I think it would be clearer to inline the constant value here,
since anyone reading this test will need to know what the constant value is to
understand the checks below.

[pkalinnikov, 2017-01-13 10:28:48]

Description was changed from

==========
Match only prefixes of histogram names.

BUG=659977
==========

to

==========
Match only prefixes of histogram names.

Fix the bug that HistogramTester::GetTotalCountsForPrefix returns histograms
which contain the query string as substrings, not as prefixes.

Additionally, omit explicit base:: namespace here and there, because the files
are already in this namespace.

BUG=659977
==========

[pkalinnikov, 2017-01-13 10:28:59]

PTAL again.

https://codereview.chromium.org/2628843002/diff/20001/base/test/histogram_tes...
File base/test/histogram_tester.cc (right):

https://codereview.chromium.org/2628843002/diff/20001/base/test/histogram_tes...
base/test/histogram_tester.cc:106: const std::string& query) const {
On 2017/01/12 20:32:41, Ilya Sherman wrote:
> nit: While you're fixing up this method, please update this parameter name to
be
> "prefix", both here and in the header file.  ("query" makes a bit more sense
for
> the StatisticsRecorder code, which is primarily dealing with HTML query
> strings.)

Done.

https://codereview.chromium.org/2628843002/diff/20001/base/test/histogram_tes...
base/test/histogram_tester.cc:111: // Find matches by using the prefix-matching
logic built into GetSnapshot().
On 2017/01/12 20:32:41, Ilya Sherman wrote:
> On 2017/01/11 20:45:00, Paweł Hajdan Jr. wrote:
> > This suggests prefix-matching logic in GetSnapshot was supposed to handle
> things
> > correctly.
> > 
> > Could you consult other histogram owners to find the best place for the
logic?
> > 
> > I'd be fine with this code, but there may be a better solution.
> 
> This comment is not quite correct.  StatisticsRecorder::GetSnapshot()
> intentionally filters by substring rather than by prefix.  So, let's update or
> omit this comment.

Updated the comment.

https://codereview.chromium.org/2628843002/diff/20001/base/test/histogram_tes...
base/test/histogram_tester.cc:116: for (base::HistogramBase* histogram :
query_matches) {
On 2017/01/12 20:32:41, Ilya Sherman wrote:
> Optional nit: While you're here, it would be nice to drop this unnecessary use
> of the "base::" namespace.

Done here and throughout this file and its header.

https://codereview.chromium.org/2628843002/diff/20001/base/test/histogram_tes...
File base/test/histogram_tester_unittest.cc (right):

https://codereview.chromium.org/2628843002/diff/20001/base/test/histogram_tes...
base/test/histogram_tester_unittest.cc:123:
UMA_HISTOGRAM_ENUMERATION(kHistogramSuffix, 2, 5);
On 2017/01/12 20:32:41, Ilya Sherman wrote:
> Optional nit: I think it would be clearer to inline the constant value here,
> since anyone reading this test will need to know what the constant value is to
> understand the checks below.

Done.

[pkalinnikov, 2017-01-13 10:29:26]

Description was changed from

==========
Match only prefixes of histogram names.

Fix the bug that HistogramTester::GetTotalCountsForPrefix returns histograms
which contain the query string as substrings, not as prefixes.

Additionally, omit explicit base:: namespace here and there, because the files
are already in this namespace.

BUG=659977
==========

to

==========
Match only prefixes of histogram names.

Fix the bug that HistogramTester::GetTotalCountsForPrefix returns histograms
which contain the query string as substrings, not as prefixes.

Additionally, omit explicit base:: namespace here and there, because the files
are already in this namespace.

BUG=659977
==========

[Paweł Hajdan Jr., 2017-01-13 16:43:30]

LGTM

Thanks for review, Ilya. Nice cleanup. :)

[pkalinnikov, 2017-01-13 16:53:29]

The CQ bit was checked by pkalinnikov@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-13 16:53:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-13 18:23:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-13 18:23:06]

Dry run: This issue passed the CQ dry run.

[pkalinnikov, 2017-01-13 18:24:35]

The CQ bit was checked by pkalinnikov@chromium.org

[pkalinnikov, 2017-01-13 18:24:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2628843002/#ps40001
(title: "Address comments of isherman@; omit explicit namespace.")

[commit-bot: I haz the power, 2017-01-13 18:25:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-13 18:30:44]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484331875565780,
"parent_rev": "843960f748b26fbd707e1bf6e3a3f51bc201db6a", "commit_rev":
"63e22b6fcb629ebccc0f257de04bc8e7f4ef247c"}

[commit-bot: I haz the power, 2017-01-13 18:31:18]

Description was changed from

==========
Match only prefixes of histogram names.

Fix the bug that HistogramTester::GetTotalCountsForPrefix returns histograms
which contain the query string as substrings, not as prefixes.

Additionally, omit explicit base:: namespace here and there, because the files
are already in this namespace.

BUG=659977
==========

to

==========
Match only prefixes of histogram names.

Fix the bug that HistogramTester::GetTotalCountsForPrefix returns histograms
which contain the query string as substrings, not as prefixes.

Additionally, omit explicit base:: namespace here and there, because the files
are already in this namespace.

BUG=659977

Review-Url: https://codereview.chromium.org/2628843002
Cr-Commit-Position: refs/heads/master@{#443620}
Committed:
https://chromium.googlesource.com/chromium/src/+/63e22b6fcb629ebccc0f257de04b...
==========

[commit-bot: I haz the power, 2017-01-13 18:31:19]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/63e22b6fcb629ebccc0f257de04b...