Report trace buffer usage as number of events, not only percentage

Report trace buffer usage as number of events, not only percentage

The total event count will be used later to show progress indicator when retrieving recorded events. The progress will be estimated as total events received divided by total buffer size.

BUG=426117
Committed: https://crrev.com/d57ba6fe68ea4b8bfbdc403ea7b3a3e74e38b03f
Cr-Commit-Position: refs/heads/master@{#304404}

[yurys, 2014-11-12 15:30:28]

yurys@chromium.org changed reviewers:
+ caseq@chromium.org, pfeldman@chromium.org

[yurys, 2014-11-12 15:33:48]

Blink change that relies on this CL: https://codereview.chromium.org/722693002/

[yurys, 2014-11-12 15:34:04]

yurys@chromium.org changed reviewers:
+ alph@chromium.org, loislo@chromium.org

[yurys, 2014-11-12 19:04:28]

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

[caseq, 2014-11-12 20:52:49]

https://codereview.chromium.org/717083003/diff/1/base/debug/trace_event_impl.h
File base/debug/trace_event_impl.h (right):

https://codereview.chromium.org/717083003/diff/1/base/debug/trace_event_impl....
base/debug/trace_event_impl.h:498: size_t GetBufferUsage() const;
Can we get more descriptive -- e.g. GetApproximateEventCount()? Also, perhaps
remove GetEventsSize() in favor of this?

[yurys, 2014-11-13 15:54:33]

PTAl

https://codereview.chromium.org/717083003/diff/1/base/debug/trace_event_impl.h
File base/debug/trace_event_impl.h (right):

https://codereview.chromium.org/717083003/diff/1/base/debug/trace_event_impl....
base/debug/trace_event_impl.h:498: size_t GetBufferUsage() const;
On 2014/11/12 20:52:49, caseq wrote:
> Can we get more descriptive -- e.g. GetApproximateEventCount()? Also, perhaps
> remove GetEventsSize() in favor of this?

Done.

[caseq, 2014-11-13 17:16:21]

lgtm

https://codereview.chromium.org/717083003/diff/20001/base/debug/trace_event_i...
File base/debug/trace_event_impl.h (right):

https://codereview.chromium.org/717083003/diff/20001/base/debug/trace_event_i...
base/debug/trace_event_impl.h:498: void GetBufferUsage(float* percent_full,
nit: we could probably keep several callsites less verbose if this were to
return struct { float percent full; size_t approximate_event_count; }.

[yurys, 2014-11-14 10:07:00]

https://codereview.chromium.org/717083003/diff/20001/base/debug/trace_event_i...
File base/debug/trace_event_impl.h (right):

https://codereview.chromium.org/717083003/diff/20001/base/debug/trace_event_i...
base/debug/trace_event_impl.h:498: void GetBufferUsage(float* percent_full,
On 2014/11/13 17:16:21, caseq wrote:
> nit: we could probably keep several callsites less verbose if this were to
> return struct { float percent full; size_t approximate_event_count; }.

Done.

[yurys, 2014-11-14 10:10:54]

yurys@chromium.org changed reviewers:
+ jochen@chromium.org

[yurys, 2014-11-14 10:10:55]

@nduca: please review

@jochen: please do OWNERS review for
content/browser/browser_shutdown_profile_dumper.cc
content/public/browser/tracing_controller.h

[alph, 2014-11-14 10:59:31]

lgtm

https://codereview.chromium.org/717083003/diff/40001/components/tracing/traci...
File components/tracing/tracing_messages.h (right):

https://codereview.chromium.org/717083003/diff/40001/components/tracing/traci...
components/tracing/tracing_messages.h:75: size_t /*trace buffer usage in number
of events*/)
nit: Having it contain size and capacity instead would seem to be more uniform.

https://codereview.chromium.org/717083003/diff/40001/content/browser/browser_...
File content/browser/browser_shutdown_profile_dumper.cc (right):

https://codereview.chromium.org/717083003/diff/40001/content/browser/browser_...
content/browser/browser_shutdown_profile_dumper.cc:37: DVLOG(1) << "Flushing
shutdown traces to disc. The buffer is %"
out of curiosity what does % mean here?

https://codereview.chromium.org/717083003/diff/40001/content/browser/browser_...
content/browser/browser_shutdown_profile_dumper.cc:40: .approximate_event_count
<< " full.";
is the change from percent to count intentional?

https://codereview.chromium.org/717083003/diff/40001/content/browser/tracing/...
File content/browser/tracing/tracing_controller_impl.h (right):

https://codereview.chromium.org/717083003/diff/40001/content/browser/tracing/...
content/browser/tracing/tracing_controller_impl.h:152: int
pending_trace_buffer_percent_full_ack_count_;
s/percent_full/buffer_usage/

https://codereview.chromium.org/717083003/diff/40001/content/browser/tracing/...
content/browser/tracing/tracing_controller_impl.h:153: TraceMessageFilterSet
pending_trace_buffer_percent_full_filters_;
ditto

https://codereview.chromium.org/717083003/diff/40001/content/browser/tracing/...
content/browser/tracing/tracing_controller_impl.h:165:
GetTraceBufferUsageCallback pending_trace_buffer_percent_full_callback_;
ditto

[yurys, 2014-11-14 13:03:47]

https://codereview.chromium.org/717083003/diff/40001/components/tracing/traci...
File components/tracing/tracing_messages.h (right):

https://codereview.chromium.org/717083003/diff/40001/components/tracing/traci...
components/tracing/tracing_messages.h:75: size_t /*trace buffer usage in number
of events*/)
On 2014/11/14 10:59:30, alph wrote:
> nit: Having it contain size and capacity instead would seem to be more
uniform.

Done.

https://codereview.chromium.org/717083003/diff/40001/content/browser/browser_...
File content/browser/browser_shutdown_profile_dumper.cc (right):

https://codereview.chromium.org/717083003/diff/40001/content/browser/browser_...
content/browser/browser_shutdown_profile_dumper.cc:37: DVLOG(1) << "Flushing
shutdown traces to disc. The buffer is %"
On 2014/11/14 10:59:30, alph wrote:
> out of curiosity what does % mean here?

No idea, it was here before. I believe it is just a typo. Fixed.

https://codereview.chromium.org/717083003/diff/40001/content/browser/browser_...
content/browser/browser_shutdown_profile_dumper.cc:40: .approximate_event_count
<< " full.";
On 2014/11/14 10:59:30, alph wrote:
> is the change from percent to count intentional?

Good catch. Done.

https://codereview.chromium.org/717083003/diff/40001/content/browser/tracing/...
File content/browser/tracing/tracing_controller_impl.h (right):

https://codereview.chromium.org/717083003/diff/40001/content/browser/tracing/...
content/browser/tracing/tracing_controller_impl.h:152: int
pending_trace_buffer_percent_full_ack_count_;
On 2014/11/14 10:59:31, alph wrote:
> s/percent_full/buffer_usage/

Done.

https://codereview.chromium.org/717083003/diff/40001/content/browser/tracing/...
content/browser/tracing/tracing_controller_impl.h:153: TraceMessageFilterSet
pending_trace_buffer_percent_full_filters_;
On 2014/11/14 10:59:30, alph wrote:
> ditto

Done.

https://codereview.chromium.org/717083003/diff/40001/content/browser/tracing/...
content/browser/tracing/tracing_controller_impl.h:165:
GetTraceBufferUsageCallback pending_trace_buffer_percent_full_callback_;
On 2014/11/14 10:59:31, alph wrote:
> ditto

Done.

[jochen (gone - plz use gerrit), 2014-11-14 13:04:44]

my bits lgtm

[yurys, 2014-11-14 13:07:00]

yurys@chromium.org changed reviewers:
+ dcheng@chromium.org

[yurys, 2014-11-14 13:07:01]

@dcheng: please do security review for components/tracing/tracing_messages.h

[nduca, 2014-11-14 18:11:17]

lgtm if you're in a rush.

though: I notice how you had to touch the UNIVERSE in order to add more fields
to the status response. If you think about some of the realtime monitoring cases
we've talked about, we might end up wanting more realtime fields in the
response, and you'll have to do another one of these sprawling changes again,
which is not only not fun but it limits our ability to hack in new ideas in the
future for status-tracking.

Therefore, would it make more sense to introduce
base::debug::TraceEventStatusResult struct that you pass around and then convert
to a response json at the last moment in tracing_ui or in the devtools handler?
then the middle plumbing wouldn't have to change

https://codereview.chromium.org/717083003/diff/60001/content/browser/devtools...
File content/browser/devtools/protocol/tracing_handler.cc (right):

https://codereview.chromium.org/717083003/diff/60001/content/browser/devtools...
content/browser/devtools/protocol/tracing_handler.cc:147:
client_->BufferUsage(BufferUsageParams::Create()->set_value(percent_full));
i think you're changing the protocol response to be a dict instead of an objct
here? or are you adding a field? if a field name changed, we need to update
tracing ui and telemetry, I think...

https://codereview.chromium.org/717083003/diff/60001/content/browser/tracing/...
File content/browser/tracing/tracing_ui.cc (right):

https://codereview.chromium.org/717083003/diff/60001/content/browser/tracing/...
content/browser/tracing/tracing_ui.cc:135: std::string str =
base::DoubleToString(percent_full);
here i notice you're not updating the tracing ui. I think it'd be polite to
update the tracing ui as well.... or at least add a new xhr endpoint
json/get_buffer_status that returns an object and then filling a trace-viwer bug
to switch to that endpoint.

[dcheng, 2014-11-15 18:45:01]

lgtm for IPC changes.

[yurys, 2014-11-17 09:10:01]

On 2014/11/14 18:11:17, nduca wrote:
> lgtm if you're in a rush.
> 
> though: I notice how you had to touch the UNIVERSE in order to add more fields
> to the status response. If you think about some of the realtime monitoring
cases
> we've talked about, we might end up wanting more realtime fields in the
> response, and you'll have to do another one of these sprawling changes again,
> which is not only not fun but it limits our ability to hack in new ideas in
the
> future for status-tracking.
> 
> Therefore, would it make more sense to introduce
> base::debug::TraceEventStatusResult struct that you pass around and then
convert
> to a response json at the last moment in tracing_ui or in the devtools
handler?
> then the middle plumbing wouldn't have to change
> 
Done. Introduced base::debug::TraceLogStatus which contains only
event_count/event_capacity at the moment. TracingController::GetTraceBufferUsage
still uses two separate parameters though as percent_full is calculated inside
TracingControllerImpl based on TraceLogStatus received from all child processes.

https://codereview.chromium.org/717083003/diff/60001/content/browser/devtools...
File content/browser/devtools/protocol/tracing_handler.cc (right):

https://codereview.chromium.org/717083003/diff/60001/content/browser/devtools...
content/browser/devtools/protocol/tracing_handler.cc:147:
client_->BufferUsage(BufferUsageParams::Create()->set_value(percent_full));
On 2014/11/14 18:11:16, nduca wrote:
> i think you're changing the protocol response to be a dict instead of an objct
> here? or are you adding a field? if a field name changed, we need to update
> tracing ui and telemetry, I think...

I'm not changing the event here. My plan was to add a new field for the event
count and rename value field to percentFull. This should be done in several
steps to keep exiting code working. caseq told me that Telemetry doesn't depend
on this event and I thought that tracing uses tracing_ui.cc as backend. I will
double check but it seems that Telemetry in fact doesn't depend on this event.
It is used by chromedriver though and I see about_tracing.js references it. Is
the later needed when running tracing via remote debugger?

https://codereview.chromium.org/717083003/diff/60001/content/browser/tracing/...
File content/browser/tracing/tracing_ui.cc (right):

https://codereview.chromium.org/717083003/diff/60001/content/browser/tracing/...
content/browser/tracing/tracing_ui.cc:135: std::string str =
base::DoubleToString(percent_full);
On 2014/11/14 18:11:17, nduca wrote:
> here i notice you're not updating the tracing ui. I think it'd be polite to
> update the tracing ui as well....
I'm not updating devtools handler in this patch either as it requires changing
the protocol and will be done in a few steps. 

> or at least add a new xhr endpoint
> json/get_buffer_status that returns an object and then filling a trace-viwer
bug
> to switch to that endpoint.
Done. I've added json/get_buffer_status which can be used in trace-viewer. Will
file a bug and try to update it to use this new endpoint.

[yurys, 2014-11-17 09:10:08]

The CQ bit was checked by yurys@chromium.org

[commit-bot: I haz the power, 2014-11-17 09:10:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/717083003/80001

[commit-bot: I haz the power, 2014-11-17 09:28:12]

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

[commit-bot: I haz the power, 2014-11-17 09:28:13]

Try jobs failed on following builders:
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yurys, 2014-11-17 09:50:26]

The CQ bit was checked by yurys@chromium.org

[commit-bot: I haz the power, 2014-11-17 09:51:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/717083003/80001

[yurys, 2014-11-17 09:53:34]

The CQ bit was checked by yurys@chromium.org

[commit-bot: I haz the power, 2014-11-17 09:53:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/717083003/100001

[commit-bot: I haz the power, 2014-11-17 10:58:05]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-11-17 10:59:00]

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

[yurys, 2014-11-17 16:09:17]

On 2014/11/14 18:11:17, nduca wrote:
>
https://codereview.chromium.org/717083003/diff/60001/content/browser/tracing/...
> File content/browser/tracing/tracing_ui.cc (right):
> 
>
https://codereview.chromium.org/717083003/diff/60001/content/browser/tracing/...
> content/browser/tracing/tracing_ui.cc:135: std::string str =
> base::DoubleToString(percent_full);
> here i notice you're not updating the tracing ui. I think it'd be polite to
> update the tracing ui as well.... or at least add a new xhr endpoint
> json/get_buffer_status that returns an object and then filling a trace-viwer
bug
> to switch to that endpoint.

@nduca: It's clear how I can update built-in trace-viewer to use
json/get_buffer_status instead of json/get_trace_buffer_usage but what is the
process for doing this for trace-viewer that connects to
devtools_tracing_handler and uses remote debugging protocol to control tracing
on Android? Do we require running same Chrome version on the desktop as on the
Android device or do we try to support backward compatibility with the older
backend? Tracing domain is marked as hidden in the protocol which means that we
can change it any time and that may require client code to be updated.

[yurys, 2014-11-17 16:09:20]

On 2014/11/14 18:11:17, nduca wrote:
>
https://codereview.chromium.org/717083003/diff/60001/content/browser/tracing/...
> File content/browser/tracing/tracing_ui.cc (right):
> 
>
https://codereview.chromium.org/717083003/diff/60001/content/browser/tracing/...
> content/browser/tracing/tracing_ui.cc:135: std::string str =
> base::DoubleToString(percent_full);
> here i notice you're not updating the tracing ui. I think it'd be polite to
> update the tracing ui as well.... or at least add a new xhr endpoint
> json/get_buffer_status that returns an object and then filling a trace-viwer
bug
> to switch to that endpoint.

@nduca: It's clear how I can update built-in trace-viewer to use
json/get_buffer_status instead of json/get_trace_buffer_usage but what is the
process for doing this for trace-viewer that connects to
devtools_tracing_handler and uses remote debugging protocol to control tracing
on Android? Do we require running same Chrome version on the desktop as on the
Android device or do we try to support backward compatibility with the older
backend? Tracing domain is marked as hidden in the protocol which means that we
can change it any time and that may require client code to be updated.

[nduca, 2014-11-18 22:23:51]

tracing has to be backcompat to stable channel...

[yurys, 2014-11-19 10:24:00]

On 2014/11/18 22:23:51, nduca wrote:
> tracing has to be backcompat to stable channel...

Then someone has to take care of making "Tracing" domain in the protocol public.
We normally don't commit to backward compatibility of the hidden
domains/commands/events in the remote debugging protocol.