telemetry: Fix TracingCategoryFilter

telemetry: Fix TracingCategoryFilter

R=nednguyen@google.com, tonyg@chromium.org
BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289487

[ernstm, 2014-08-13 20:39:31]

PTAL

[ernstm, 2014-08-13 20:41:43]

On 2014/08/13 20:39:31, ernstm wrote:
> PTAL

+tonyg

[nednguyen, 2014-08-13 21:05:09]

What would happen if we add a include a category and then exclude it?

https://codereview.chromium.org/474613002/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/platform/tracing_category_filter.py (right):

https://codereview.chromium.org/474613002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/tracing_category_filter.py:125: if
category_glob not in self._included_categories:
Since _included_categories is already a set, I don't think this check is needed.
Similar to _excluded_categories

https://codereview.chromium.org/474613002/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/platform/tracing_category_filter_unittest.py
(right):

https://codereview.chromium.org/474613002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/tracing_category_filter_unittest.py:30:
self.assertEquals(a.stable_filter_string, 'foo')
Can you improve the test a little bit more. e.g: Adding 'foo' twice?

[ernstm, 2014-08-13 21:39:31]

If a category is both included and excluded, the inclusion will win. See
https://code.google.com/p/chromium/codesearch#chromium/src/base/debug/trace_e...

https://codereview.chromium.org/474613002/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/platform/tracing_category_filter.py (right):

https://codereview.chromium.org/474613002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/tracing_category_filter.py:125: if
category_glob not in self._included_categories:
On 2014/08/13 21:05:09, nednguyen wrote:
> Since _included_categories is already a set, I don't think this check is
needed.
> Similar to _excluded_categories

Done.

https://codereview.chromium.org/474613002/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/platform/tracing_category_filter_unittest.py
(right):

https://codereview.chromium.org/474613002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/tracing_category_filter_unittest.py:30:
self.assertEquals(a.stable_filter_string, 'foo')
On 2014/08/13 21:05:09, nednguyen wrote:
> Can you improve the test a little bit more. e.g: Adding 'foo' twice?

Done.

[nednguyen, 2014-08-13 21:43:55]

On 2014/08/13 21:39:31, ernstm wrote:
> If a category is both included and excluded, the inclusion will win. See
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/debug/trace_e...

Thanks for pointing that out. On telemetry side, I think we may want to assert
that no category is both included and excluded.

> 
>
https://codereview.chromium.org/474613002/diff/1/tools/telemetry/telemetry/co...
> File tools/telemetry/telemetry/core/platform/tracing_category_filter.py
(right):
> 
>
https://codereview.chromium.org/474613002/diff/1/tools/telemetry/telemetry/co...
> tools/telemetry/telemetry/core/platform/tracing_category_filter.py:125: if
> category_glob not in self._included_categories:
> On 2014/08/13 21:05:09, nednguyen wrote:
> > Since _included_categories is already a set, I don't think this check is
> needed.
> > Similar to _excluded_categories
> 
> Done.
> 
>
https://codereview.chromium.org/474613002/diff/1/tools/telemetry/telemetry/co...
> File
tools/telemetry/telemetry/core/platform/tracing_category_filter_unittest.py
> (right):
> 
>
https://codereview.chromium.org/474613002/diff/1/tools/telemetry/telemetry/co...
>
tools/telemetry/telemetry/core/platform/tracing_category_filter_unittest.py:30:
> self.assertEquals(a.stable_filter_string, 'foo')
> On 2014/08/13 21:05:09, nednguyen wrote:
> > Can you improve the test a little bit more. e.g: Adding 'foo' twice?
> 
> Done.

LGTM

[ernstm, 2014-08-13 22:53:27]

On 2014/08/13 21:43:55, nednguyen wrote:
> On 2014/08/13 21:39:31, ernstm wrote:
> > If a category is both included and excluded, the inclusion will win. See
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/debug/trace_e...
> 
> Thanks for pointing that out. On telemetry side, I think we may want to assert
> that no category is both included and excluded.

Done

[ernstm, 2014-08-13 22:54:41]

The CQ bit was checked by ernstm@chromium.org

[commit-bot: I haz the power, 2014-08-13 22:57:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ernstm@chromium.org/474613002/40001

[commit-bot: I haz the power, 2014-08-14 08:02:07]

Committed patchset #3 (40001) as 289487