Add OnBeforeTraceLogDisabled notification to EnabledStateObserver

Add OnBeforeTraceLogDisabled notification to EnabledStateObserver

The method can be used to perform last call actions that require
tracing still being enabled.

BUG=406277

[alph, 2016-09-10 00:38:40]

alph@chromium.org changed reviewers:
+ nduca@chromium.org

[alph, 2016-09-10 00:38:41]

This is a resurrection of ancient CL https://codereview.chromium.org/492823002/

Nat, could you please do the review. Thanks!

[alph, 2016-09-10 00:39:12]

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

[commit-bot: I haz the power, 2016-09-10 00:39:44]

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-09-10 03:57:45]

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

[commit-bot: I haz the power, 2016-09-10 03:57:45]

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

[alph, 2016-09-10 19:13:33]

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

[commit-bot: I haz the power, 2016-09-10 19:13:42]

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-09-10 20:31:10]

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

[commit-bot: I haz the power, 2016-09-10 20:31:12]

Dry run: This issue passed the CQ dry run.

[Primiano Tucci (use gerrit), 2016-09-12 08:08:49]

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

[Primiano Tucci (use gerrit), 2016-09-12 08:08:50]

Uhm what is the context and the use case for this?
Is it because you want some events to "definitely" be included in the trace and
not dropped? If so we have better ways to deal with it, see [1]
The thing that makes me worry here is that this feature just reduces a racing
window: the fact that you add an event in onbeforedisable doesn't really
guarantee that the event will be there (will make it very likely though).
You could have a storm of events coming in parallel to the onbeforedisable. Or
you could just have too many clients using onbeforedisable.


[1]
https://cs.chromium.org/chromium/src/base/trace_event/memory_dump_manager.cc?...

[alph, 2016-09-12 22:18:51]

On 2016/09/12 08:08:50, Primiano Tucci wrote:
> Uhm what is the context and the use case for this?

I'm going to record the CpuProfile event at the end of the tracing session. 

> Is it because you want some events to "definitely" be included in the trace
and

That's because I don't have the CPU profile ready until the recording is done.

> not dropped? If so we have better ways to deal with it, see [1]

This one emits events in OnTraceLogEnabled, which is different.

> The thing that makes me worry here is that this feature just reduces a racing
> window: the fact that you add an event in onbeforedisable doesn't really
> guarantee that the event will be there (will make it very likely though).

Why's that? I'd assume that all events are there unless you're in a cyclic
buffer mode, when it's fine if some events are preempted.

> You could have a storm of events coming in parallel to the onbeforedisable. Or
> you could just have too many clients using onbeforedisable.

That's fine.

> 
> 
> [1]
>
https://cs.chromium.org/chromium/src/base/trace_event/memory_dump_manager.cc?...

[Primiano Tucci (use gerrit), 2016-09-12 23:02:57]

On 2016/09/12 22:18:51, alph wrote:
> I'm going to record the CpuProfile event at the end of the tracing session. 
> 
> > Is it because you want some events to "definitely" be included in the trace
> and
> 
> That's because I don't have the CPU profile ready until the recording is done.

We have the same problem with the heap profiler, where we want to emit one
object that has all the stack traces dictionary, but that is known only at the
end.

The trick we use ([1] above) is the following:
you emit an event when you know that you will need it (very likely
ontracelogenabled or something similar *) even if you don't have the final data
yet (because it's too early when you emit the event).
You pass a ConvertableToTraceFormat object as argument to this event. Think to
this object as a promise (in the JS sense) that will be resolved only at the end
of the trace.
CTTF::AppendAsTraceFormat is called at the end of the trace. You know that this
is the right time to dump all the data you need and that no further event can
possibly be appended after this.
Essentially you append beforehand an object to the trace that you know will be
expanded at the very end. The code I linked above does precisely this
Can this work for you?
 
* The only extra feature of a "metadata" event (poor name choice) is that it
gives the further guarantee that the event will be there, even in cycling mode.
But sounds like you don't care about this case, so a plain event should just
work.

>Why's that? I'd assume that all events are there unless you're in a cyclic
buffer mode, when it's fine if some events are preempted.
Ah fine, I was thinking to the cycling mode ("record continuously")

[alph, 2016-09-12 23:08:22]

On 2016/09/12 23:02:57, Primiano Tucci wrote:
> On 2016/09/12 22:18:51, alph wrote:
> > I'm going to record the CpuProfile event at the end of the tracing session. 
> > 
> > > Is it because you want some events to "definitely" be included in the
trace
> > and
> > 
> > That's because I don't have the CPU profile ready until the recording is
done.
> 
> We have the same problem with the heap profiler, where we want to emit one
> object that has all the stack traces dictionary, but that is known only at the
> end.
> 
> The trick we use ([1] above) is the following:
> you emit an event when you know that you will need it (very likely
> ontracelogenabled or something similar *) even if you don't have the final
data
> yet (because it's too early when you emit the event).
> You pass a ConvertableToTraceFormat object as argument to this event. Think to
> this object as a promise (in the JS sense) that will be resolved only at the
end
> of the trace.
> CTTF::AppendAsTraceFormat is called at the end of the trace. You know that
this
> is the right time to dump all the data you need and that no further event can
> possibly be appended after this.
> Essentially you append beforehand an object to the trace that you know will be
> expanded at the very end. The code I linked above does precisely this
> Can this work for you?

Sounds like it can. I'll give it a try. Thanks.

>  
> * The only extra feature of a "metadata" event (poor name choice) is that it
> gives the further guarantee that the event will be there, even in cycling
mode.
> But sounds like you don't care about this case, so a plain event should just
> work.

This perhaps may make sense, as if the event is preempted I'll lose the entire
profile. 
 
> >Why's that? I'd assume that all events are there unless you're in a cyclic
> buffer mode, when it's fine if some events are preempted.
> Ah fine, I was thinking to the cycling mode ("record continuously")