Enable FILTERING_MODE for tracing if event filters were given in config

Enable FILTERING_MODE for tracing if event filters were given in config

TracingControllerImpl, TraceMessageFilter and startup tracing code paths
enable FILTERING_MODE if event filters were given by trace_config. This
supports event filters in startup tracing and devtools.

BUG=625170
Review-Url: https://codereview.chromium.org/2466873002
Cr-Commit-Position: refs/heads/master@{#459653}
Committed: https://chromium.googlesource.com/chromium/src/+/2d2a3b2d3247797fd2d2c8f471b18f40b18cfeab

[ssid, 2016-11-01 11:36:30]

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

[commit-bot: I haz the power, 2016-11-01 11:36:43]

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, 2016-11-01 12:21:05]

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

[commit-bot: I haz the power, 2016-11-01 12:21:06]

Dry run: This issue passed the CQ dry run.

[ssid, 2016-11-01 12:38:14]

Description was changed from

==========
Add FILTERING_MODE for tracing if filters were given by clients

TracingControllerImpl, TraceMessageFilter and startup tracing code paths
enable FILTERING_MODE if filters were given by trace_config. This
enables support for filtering on startup tracing and through devtools.

BUG=625170
==========

to

==========
Add FILTERING_MODE for tracing if filters were given by clients

TracingControllerImpl, TraceMessageFilter and startup tracing code paths
enable FILTERING_MODE if filters were given by trace_config. This
supports filters on startup tracing and through devtools.

BUG=625170
==========

[ssid, 2016-11-03 07:23:26]

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

[commit-bot: I haz the power, 2016-11-03 07:23:49]

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

[ssid, 2016-11-03 07:27:42]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2016-11-03 09:24:23]

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

[commit-bot: I haz the power, 2016-11-03 09:24:23]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[ssid, 2016-11-03 09:47:39]

Description was changed from

==========
Add FILTERING_MODE for tracing if filters were given by clients

TracingControllerImpl, TraceMessageFilter and startup tracing code paths
enable FILTERING_MODE if filters were given by trace_config. This
supports filters on startup tracing and through devtools.

BUG=625170
==========

to

==========
Enable FILTERING_MODE for tracing if filters were given in config
TracingControllerImpl, TraceMessageFilter and startup tracing code paths
enable FILTERING_MODE if filters were given by trace_config. This
supports filters on startup tracing and through devtools.

BUG=625170
==========

[ssid, 2016-11-03 09:48:30]

ssid@chromium.org changed reviewers:
+ oysteine@chromium.org

[ssid, 2016-11-03 09:48:31]

+oysteine ptal, thanks.

[ssid, 2016-11-03 09:52:25]

Description was changed from

==========
Enable FILTERING_MODE for tracing if filters were given in config
TracingControllerImpl, TraceMessageFilter and startup tracing code paths
enable FILTERING_MODE if filters were given by trace_config. This
supports filters on startup tracing and through devtools.

BUG=625170
==========

to

==========
Enable FILTERING_MODE for tracing if event filters were given in config

TracingControllerImpl, TraceMessageFilter and startup tracing code paths
enable FILTERING_MODE if event filters were given by trace_config. This
supports event filters in startup tracing and devtools.

BUG=625170
==========

[oystein (OOO til 10th of July), 2016-11-04 19:49:20]

On 2016/11/03 09:48:31, ssid wrote:
> +oysteine ptal, thanks.

Hm, I thought I remembered that what we discussed was that SetEnabled() could be
called with no mode defined, and that would cause the TraceConfig to define
whether filtering was enabled or not, inside TraceLog? It's very possible I'm
misremembering this t hough.

[ssid, 2016-11-07 05:22:10]

On 2016/11/04 19:49:20, oystein wrote:
> On 2016/11/03 09:48:31, ssid wrote:
> > +oysteine ptal, thanks.
> 
> Hm, I thought I remembered that what we discussed was that SetEnabled() could
be
> called with no mode defined, and that would cause the TraceConfig to define
> whether filtering was enabled or not, inside TraceLog? It's very possible I'm
> misremembering this t hough.

I have sent you an email of the discussion. we actually decided SetEnabled and
SetDisabled take modes argument. The clients decide what modes to enable.

[Primiano Tucci (use gerrit), 2016-11-07 16:21:20]

primiano@chromium.org changed reviewers:
+ primiano@chromium.org

[Primiano Tucci (use gerrit), 2016-11-07 16:21:21]

Hmm I'd avoid keeping an extra "enabled" state in the trace controller.
Maybe I am missing something but why can't we have this logic in TraceLog?

[ssid, 2017-03-09 00:52:57]

primiano, ptal thanks!

On 2016/11/07 16:21:21, Primiano Tucci (slow - perf) wrote:
> Hmm I'd avoid keeping an extra "enabled" state in the trace controller.
> Maybe I am missing something but why can't we have this logic in TraceLog?

I rebased the CL.

In case you think we should not have the modes stored in TracingController then
look at the alternate option at https://codereview.chromium.org/2743473002. I
can only think of 2 options: Either TraceLog should magically disable filters
based on how they were enabled or TracingController should remember what it
enabled and disable accordingly.

Reply to comments pasted from discussion thread:

On Tue, Nov 8, 2016 at 4:25 AM Siddhartha S <ssid@google.com> wrote:
I think that was my initial proposal, to decide in TraceLog if we want to enable
filtering based on TraceConfig. The concern was that TraceLog is doing something
magical to decide to disable modes. ie, SetDisabled will disable FILTERING_MODE
if filters were enabled along with RECORDING_MODE, else it would not disabled
filtering. Since we were not okay with this being part of TraceLog api we
decided the embedder should always specify which modes to enable and disable.

Primiano@:
Yeah I also remember that while chatting on the whiteboard we found that just
inferring the mode from the traceconfig was too magical. In general having to
state which mode you want to enable or not (and cross-check that the trace
config is consistent) seems the simplest and safest option.

w.r.t. of changing all the callsites, can we keep the default argument method
around for a bit longer or does it cause problems to what you are trying to do
in filtering mode? Rationale: I'm doing some larger refactorings and want to
avoid asking those people to review our changes twice few weeks apart.

[Primiano Tucci (use gerrit), 2017-03-16 19:59:43]

Okay now I recall the conversation (thanks for the written thread).
Yes, essentially if we do this in TraceLog, the problem is that it becomes super
magical (and it is already magical enough) and specifically SetDisabled()
becomes too dubious. 

LGTM with 1 comment

https://codereview.chromium.org/2466873002/diff/40001/components/tracing/comm...
File components/tracing/common/trace_startup.cc (right):

https://codereview.chromium.org/2466873002/diff/40001/components/tracing/comm...
components/tracing/common/trace_startup.cc:49:
base::trace_event::TraceLog::RECORDING_MODE);
shouldn't this become |modes| ? otherwise you are just setting it and throwing
away :)

[oystein (OOO til 10th of July), 2017-03-20 18:47:18]

On 2016/11/07 at 05:22:10, ssid wrote:
> On 2016/11/04 19:49:20, oystein wrote:
> > On 2016/11/03 09:48:31, ssid wrote:
> > > +oysteine ptal, thanks.
> > 
> > Hm, I thought I remembered that what we discussed was that SetEnabled()
could be
> > called with no mode defined, and that would cause the TraceConfig to define
> > whether filtering was enabled or not, inside TraceLog? It's very possible
I'm
> > misremembering this t hough.
> 
> I have sent you an email of the discussion. we actually decided SetEnabled and
SetDisabled take modes argument. The clients decide what modes to enable.

Yeah I was referring to the discussion we had in person in the Chrome area when
Primiano was down here some time ago. My memory was that we'd have two versions
of SetEnabled and SetDisabled(): ones with no arguments which would pick out the
mode to enable from the TraceConfig (instead of having most of the caller sites
always needing identical boilerplate to pick out the mode from the config, when
they're passing in the TraceConfig in anyway which makes for a weird API to me),
and another set which overrides what to enable/disable for the single
exceptional case (which I don't remember offhand what was, this fell way out of
short-term memory :/).

I think *maybe* the exception was that the Stop Tracing button in
about://tracing should only specifically turn off recording mode and not
filtering mode?

[ssid, 2017-03-22 01:16:53]

On 2017/03/20 18:47:18, oystein wrote:
> On 2016/11/07 at 05:22:10, ssid wrote:
> > On 2016/11/04 19:49:20, oystein wrote:
> > > On 2016/11/03 09:48:31, ssid wrote:
> > > > +oysteine ptal, thanks.
> > > 
> > > Hm, I thought I remembered that what we discussed was that SetEnabled()
> could be
> > > called with no mode defined, and that would cause the TraceConfig to
define
> > > whether filtering was enabled or not, inside TraceLog? It's very possible
> I'm
> > > misremembering this t hough.
> > 
> > I have sent you an email of the discussion. we actually decided SetEnabled
and
> SetDisabled take modes argument. The clients decide what modes to enable.
> 
> Yeah I was referring to the discussion we had in person in the Chrome area
when
> Primiano was down here some time ago. My memory was that we'd have two
versions
> of SetEnabled and SetDisabled(): ones with no arguments which would pick out
the
> mode to enable from the TraceConfig (instead of having most of the caller
sites
> always needing identical boilerplate to pick out the mode from the config,
when
> they're passing in the TraceConfig in anyway which makes for a weird API to
me),
> and another set which overrides what to enable/disable for the single
> exceptional case (which I don't remember offhand what was, this fell way out
of
> short-term memory :/).
> 
> I think *maybe* the exception was that the Stop Tracing button in
> about://tracing should only specifically turn off recording mode and not
> filtering mode?

For all cases which can use event name filtering: startup tracing and slow
reports should disable filtering.
For all cases which can use heap profiler filtering: command line flag should
not disable filtering on any SetDisabled.

Now, considering super-position of these cases with tracing.
				filter set by		
				startup	devtols	cmdline	slow reports
Stop		startup		yes	NA	no	NA
tracing		slow reports	no	no	no	yes
called		devtools	NA	yes	no	no
		tracing UI	no	no	no	no

What the table says is if the filters were set by the same agent that started
recording then filters should be disabled.
But, Primiano feels that doing this in TraceLog is magical.
So, the other option I came up with is this:
 https://codereview.chromium.org/2743473002
Which changes the FILTERING_MODE to persistent. So, now for the case of heap
profiling filtering can be done differently.
But, I still feel that we TraceLog should not be handling this magically and
it's better for the agent to specify explicitly to disable filtering.

There are more cases that are not discussed here which includes tracing being
started by multiple agents at the same time. That causes more complicated issues
with filtering. But, this can be addressed as different issue saying TraceLog
supports only one agent at a time.

[oystein (OOO til 10th of July), 2017-03-24 18:13:35]

On 2017/03/22 at 01:16:53, ssid wrote:
> On 2017/03/20 18:47:18, oystein wrote:
> > On 2016/11/07 at 05:22:10, ssid wrote:
> > > On 2016/11/04 19:49:20, oystein wrote:
> > > > On 2016/11/03 09:48:31, ssid wrote:
> > > > > +oysteine ptal, thanks.
> > > > 
> > > > Hm, I thought I remembered that what we discussed was that SetEnabled()
> > could be
> > > > called with no mode defined, and that would cause the TraceConfig to
define
> > > > whether filtering was enabled or not, inside TraceLog? It's very
possible
> > I'm
> > > > misremembering this t hough.
> > > 
> > > I have sent you an email of the discussion. we actually decided SetEnabled
and
> > SetDisabled take modes argument. The clients decide what modes to enable.
> > 
> > Yeah I was referring to the discussion we had in person in the Chrome area
when
> > Primiano was down here some time ago. My memory was that we'd have two
versions
> > of SetEnabled and SetDisabled(): ones with no arguments which would pick out
the
> > mode to enable from the TraceConfig (instead of having most of the caller
sites
> > always needing identical boilerplate to pick out the mode from the config,
when
> > they're passing in the TraceConfig in anyway which makes for a weird API to
me),
> > and another set which overrides what to enable/disable for the single
> > exceptional case (which I don't remember offhand what was, this fell way out
of
> > short-term memory :/).
> > 
> > I think *maybe* the exception was that the Stop Tracing button in
> > about://tracing should only specifically turn off recording mode and not
> > filtering mode?
> 
> For all cases which can use event name filtering: startup tracing and slow
reports should disable filtering.
> For all cases which can use heap profiler filtering: command line flag should
not disable filtering on any SetDisabled.
> 
> Now, considering super-position of these cases with tracing.
> 				filter set by		
> 				startup	devtols	cmdline	slow reports
> Stop		startup		yes	NA	no	NA
> tracing		slow reports	no	no	no	yes
> called		devtools	NA	yes	no	no
> 		tracing UI	no	no	no	no
> 
> What the table says is if the filters were set by the same agent that started
recording then filters should be disabled.
> But, Primiano feels that doing this in TraceLog is magical.
> So, the other option I came up with is this:
>  https://codereview.chromium.org/2743473002
> Which changes the FILTERING_MODE to persistent. So, now for the case of heap
profiling filtering can be done differently.
> But, I still feel that we TraceLog should not be handling this magically and
it's better for the agent to specify explicitly to disable filtering.
> 
> There are more cases that are not discussed here which includes tracing being
started by multiple agents at the same time. That causes more complicated issues
with filtering. But, this can be addressed as different issue saying TraceLog
supports only one agent at a time.

Ugh. That's fair. We *really* need to come up with a better way of handling
multiple involved tracing agents here :/. But you're right, we shouldn't block
this on that, so lgtm.

Essentially I think we need a priority system for tracing agents. slow reports <
startup tracing < cmdline < devtools < about://tracing. A higher priority
BeginTracing() will first stop the lower priority one. Then the one actual
exception we need, i.e. heap profiler filtering from cmdline/startup tracing
being persistent across about://tracing calls, should be part of the API that
only about://tracing uses and not the default case. Mind adding a TODO/task for
something like this? Though I suspect this isn't something we'll get around to
any time soon...

[ssid, 2017-03-24 21:14:36]

Patchset #4 (id:80001) has been deleted

[ssid, 2017-03-24 21:20:04]

Added a todo and filed a bug. Thanks!

https://codereview.chromium.org/2466873002/diff/40001/components/tracing/comm...
File components/tracing/common/trace_startup.cc (right):

https://codereview.chromium.org/2466873002/diff/40001/components/tracing/comm...
components/tracing/common/trace_startup.cc:49:
base::trace_event::TraceLog::RECORDING_MODE);
On 2017/03/16 19:59:43, Primiano Tucci wrote:
> shouldn't this become |modes| ? otherwise you are just setting it and throwing
> away :)

oops.

[ssid, 2017-03-24 21:20:10]

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

[commit-bot: I haz the power, 2017-03-24 21:20:32]

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-03-24 21:33:50]

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

[commit-bot: I haz the power, 2017-03-24 21:33:51]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ssid, 2017-03-24 21:43:40]

Patchset #3 (id:60001) has been deleted

[ssid, 2017-03-24 21:43:55]

Patchset #3 (id:100001) has been deleted

[ssid, 2017-03-24 21:44:25]

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

[commit-bot: I haz the power, 2017-03-24 21:45:11]

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-03-24 22:46:29]

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

[commit-bot: I haz the power, 2017-03-24 22:46:30]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ssid, 2017-03-25 15:20:51]

The CQ bit was checked by ssid@chromium.org

[ssid, 2017-03-25 15:20:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, oysteine@chromium.org
Link to the patchset: https://codereview.chromium.org/2466873002/#ps120001
(title: "Rebase and add TODO.")

[commit-bot: I haz the power, 2017-03-25 15:21:05]

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-03-25 15:42:40]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1490455251537570,
"parent_rev": "ae019b8737821fa456cccb618a579378c4ca43b3", "commit_rev":
"2d2a3b2d3247797fd2d2c8f471b18f40b18cfeab"}

[commit-bot: I haz the power, 2017-03-25 15:43:22]

Description was changed from

==========
Enable FILTERING_MODE for tracing if event filters were given in config

TracingControllerImpl, TraceMessageFilter and startup tracing code paths
enable FILTERING_MODE if event filters were given by trace_config. This
supports event filters in startup tracing and devtools.

BUG=625170
==========

to

==========
Enable FILTERING_MODE for tracing if event filters were given in config

TracingControllerImpl, TraceMessageFilter and startup tracing code paths
enable FILTERING_MODE if event filters were given by trace_config. This
supports event filters in startup tracing and devtools.

BUG=625170

Review-Url: https://codereview.chromium.org/2466873002
Cr-Commit-Position: refs/heads/master@{#459653}
Committed:
https://chromium.googlesource.com/chromium/src/+/2d2a3b2d3247797fd2d2c8f471b1...
==========

[commit-bot: I haz the power, 2017-03-25 15:43:23]

Committed patchset #3 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/2d2a3b2d3247797fd2d2c8f471b1...