Background tracing: Added config option for repeated trigger behavior

Background tracing: Add config option to control whether or not to stop reactive tracing when a repeated trigger occurs

R=simonhatch@chromium.org
BUG=637129
Committed: https://crrev.com/fba230c24383ae08ea6d924401bdff81b46c7108
Cr-Commit-Position: refs/heads/master@{#412577}

[oystein (OOO til 10th of July), 2016-08-16 21:47:39]

Description was changed from

==========
Background tracing: Add config option to control whether or not to stop reactive
tracing when a repeated trigger occurs

R=simonhatch@chromium.org
BUG=637129
==========

to

==========
Background tracing: Add config option to control whether or not to stop reactive
tracing when a repeated trigger occurs

R=simonhatch@chromium.org
BUG=637129
==========

[oystein (OOO til 10th of July), 2016-08-16 21:47:49]

ptal!

[oystein (OOO til 10th of July), 2016-08-16 21:48:02]

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

[commit-bot: I haz the power, 2016-08-16 21:48: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, 2016-08-16 23:12:26]

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

[commit-bot: I haz the power, 2016-08-16 23:12:27]

Dry run: This issue passed the CQ dry run.

[ssid, 2016-08-17 04:11:35]

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

[ssid, 2016-08-17 04:11:36]

https://codereview.chromium.org/2247033005/diff/1/content/browser/tracing/bac...
File content/browser/tracing/background_tracing_rule.cc (right):

https://codereview.chromium.org/2247033005/diff/1/content/browser/tracing/bac...
content/browser/tracing/background_tracing_rule.cc:63: trigger_delay_(-1),
From what I understand:
Probably unrelated to this CL, but this value is set to -1 by default.
Only the navigation config sets this value as 30 seconds. The HistogramRule
constructor never sets this value.
I thought this value was 30 by deafult. The memory triggers do not set this
delay value and we should be starting and stopping tracing immediately?
Or am I missing something? I think I should fix this in the config?

[petrcermak, 2016-08-17 09:28:02]

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

[petrcermak, 2016-08-17 09:28:03]

Drive by: The patch title is too long (ellipsis). Please shorten it.

[shatch, 2016-08-17 14:53:58]

lgtm % petrcermak/ssid comments/questions

https://codereview.chromium.org/2247033005/diff/1/content/browser/tracing/bac...
File content/browser/tracing/background_tracing_rule.cc (right):

https://codereview.chromium.org/2247033005/diff/1/content/browser/tracing/bac...
content/browser/tracing/background_tracing_rule.cc:63: trigger_delay_(-1),
On 2016/08/17 04:11:36, ssid wrote:
> From what I understand:
> Probably unrelated to this CL, but this value is set to -1 by default.
> Only the navigation config sets this value as 30 seconds. The HistogramRule
> constructor never sets this value.
> I thought this value was 30 by deafult. The memory triggers do not set this
> delay value and we should be starting and stopping tracing immediately?
> Or am I missing something? I think I should fix this in the config?

Yeah you should probably add a delay in your config, and maybe we want to set a
default in HistogramRule.

[oystein (OOO til 10th of July), 2016-08-17 17:38:51]

On 2016/08/17 04:11:36, ssid wrote:
>
https://codereview.chromium.org/2247033005/diff/1/content/browser/tracing/bac...
> File content/browser/tracing/background_tracing_rule.cc (right):
> 
>
https://codereview.chromium.org/2247033005/diff/1/content/browser/tracing/bac...
> content/browser/tracing/background_tracing_rule.cc:63: trigger_delay_(-1),
> From what I understand:
> Probably unrelated to this CL, but this value is set to -1 by default.
> Only the navigation config sets this value as 30 seconds. The HistogramRule
> constructor never sets this value.
> I thought this value was 30 by deafult. The memory triggers do not set this
> delay value and we should be starting and stopping tracing immediately?
> Or am I missing something? I think I should fix this in the config?

Huh, interesting; I would've thought the memory-infra traces would be empty
then?

[oystein (OOO til 10th of July), 2016-08-17 17:38:59]

The CQ bit was checked by oysteine@chromium.org

[commit-bot: I haz the power, 2016-08-17 17:39:20]

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-08-17 17:43:53]

Description was changed from

==========
Background tracing: Add config option to control whether or not to stop reactive
tracing when a repeated trigger occurs

R=simonhatch@chromium.org
BUG=637129
==========

to

==========
Background tracing: Add config option to control whether or not to stop reactive
tracing when a repeated trigger occurs

R=simonhatch@chromium.org
BUG=637129
==========

[commit-bot: I haz the power, 2016-08-17 17:43:56]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-08-17 17:46:57]

Description was changed from

==========
Background tracing: Add config option to control whether or not to stop reactive
tracing when a repeated trigger occurs

R=simonhatch@chromium.org
BUG=637129
==========

to

==========
Background tracing: Add config option to control whether or not to stop reactive
tracing when a repeated trigger occurs

R=simonhatch@chromium.org
BUG=637129

Committed: https://crrev.com/fba230c24383ae08ea6d924401bdff81b46c7108
Cr-Commit-Position: refs/heads/master@{#412577}
==========

[commit-bot: I haz the power, 2016-08-17 17:46:58]

Patchset 1 (id:??) landed as
https://crrev.com/fba230c24383ae08ea6d924401bdff81b46c7108
Cr-Commit-Position: refs/heads/master@{#412577}

[ssid, 2016-08-17 17:49:26]

On 2016/08/17 17:38:51, oystein wrote:
> On 2016/08/17 04:11:36, ssid wrote:
> >
>
https://codereview.chromium.org/2247033005/diff/1/content/browser/tracing/bac...
> > File content/browser/tracing/background_tracing_rule.cc (right):
> > 
> >
>
https://codereview.chromium.org/2247033005/diff/1/content/browser/tracing/bac...
> > content/browser/tracing/background_tracing_rule.cc:63: trigger_delay_(-1),
> > From what I understand:
> > Probably unrelated to this CL, but this value is set to -1 by default.
> > Only the navigation config sets this value as 30 seconds. The HistogramRule
> > constructor never sets this value.
> > I thought this value was 30 by deafult. The memory triggers do not set this
> > delay value and we should be starting and stopping tracing immediately?
> > Or am I missing something? I think I should fix this in the config?
> 
> Huh, interesting; I would've thought the memory-infra traces would be empty
> then?

My guess would be that the memory dumps are quick enough to be get done in
renderer process before the stop tracing IPC reaches. Browser I am not sure how
does it finish dumps. I sent a cl to fix the config.

[megjablon, 2016-08-23 00:09:04]

A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/2265423002/ by megjablon@chromium.org.

The reason for reverting is: Test is flaky on builder "Android Tests"

Builder: https://build.chromium.org/p/chromium.linux/builders/Android%20Tests

Failure:
https://build.chromium.org/p/chromium.linux/builders/Android%20Tests/builds/3...

Output:
BackgroundTracingManagerBrowserTest.ReactiveSecondTriggerIgnored (run #1):
[ RUN      ] BackgroundTracingManagerBrowserTest.ReactiveSecondTriggerIgnored
[WARNING:dns_config_service_posix.cc(316)] Failed to read DnsConfig.
[ERROR:devtools_http_handler.cc(228)] Cannot start http server for devtools.
Stop devtools.
../../content/browser/tracing/background_tracing_manager_browsertest.cc:1210:
Failure
Value of: BackgroundTracingManagerImpl::GetInstance()->IsTracingForTesting()
  Actual: false
Expected: true
../../content/browser/tracing/background_tracing_manager_browsertest.cc:95:
Failure
Value of: value
  Actual: true
Expected: expected
Which is: false
[  FAILED  ] BackgroundTracingManagerBrowserTest.ReactiveSecondTriggerIgnored,
where TypeParam =  and GetParam() =  (188 ms)
[----------] 1 test from BackgroundTracingManagerBrowserTest (188 ms total).

[oystein (OOO til 10th of July), 2016-08-26 19:35:16]

A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/2281073003/ by oysteine@chromium.org.

The reason for reverting is: Re-landing with fix.