Add some histograms to the D-Bus library:

Add some histograms to the D-Bus library:

- DBus.SyncMethodCallTime: time spent to perform synchronos method calls
- DBus.AsyncMethodCallTime: time spent to perform asynchronos method calls
- DBus.SyncMethodCallSuccess: success ratio of synchronous method calls
- DBus.AsyncMethodCallSuccess: success ratio of asynchronous method calls
- DBus.ExportedMethodHandleTime: time spent to handle calls to export methods
- DBus.ExportedMethodHandleSuccess: success ratio of the exported method calls
- DBus.SignalHandleTime: time spent to handle signals
- DBus.SignalSendTime: time spent to send signals

BUG=chromium:90036
TEST=dbus_unittests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=99811

[stevenjb, 2011-09-02 20:43:26]

http://codereview.chromium.org/7824054/diff/1/dbus/exported_object.cc
File dbus/exported_object.cc (right):

http://codereview.chromium.org/7824054/diff/1/dbus/exported_object.cc#newcode254
dbus/exported_object.cc:254: response ? 1 : 0, 2);
nit: What does the '2' refer to?

http://codereview.chromium.org/7824054/diff/1/dbus/object_proxy.cc
File dbus/object_proxy.cc (right):

http://codereview.chromium.org/7824054/diff/1/dbus/object_proxy.cc#newcode67
dbus/object_proxy.cc:67: response_message ? 1 : 0, 2);
nit: 2?

http://codereview.chromium.org/7824054/diff/1/dbus/object_proxy.cc#newcode164
dbus/object_proxy.cc:164: start_time,
We seem to be doing a lot of passing along start_time. Would it make sense to
store it in response_callback?

[satorux1, 2011-09-02 21:57:11]

http://codereview.chromium.org/7824054/diff/1/dbus/exported_object.cc
File dbus/exported_object.cc (right):

http://codereview.chromium.org/7824054/diff/1/dbus/exported_object.cc#newcode254
dbus/exported_object.cc:254: response ? 1 : 0, 2);
On 2011/09/02 20:43:26, Steven Bennetts wrote:
> nit: What does the '2' refer to?

2 means this enumeration histogram takes values less than 2: 0 or 1. Any ideas
on how to make it clearer?

http://codereview.chromium.org/7824054/diff/1/dbus/object_proxy.cc
File dbus/object_proxy.cc (right):

http://codereview.chromium.org/7824054/diff/1/dbus/object_proxy.cc#newcode67
dbus/object_proxy.cc:67: response_message ? 1 : 0, 2);
On 2011/09/02 20:43:26, Steven Bennetts wrote:
> nit: 2?

commented n the other file.

http://codereview.chromium.org/7824054/diff/1/dbus/object_proxy.cc#newcode164
dbus/object_proxy.cc:164: start_time,
On 2011/09/02 20:43:26, Steven Bennetts wrote:
> We seem to be doing a lot of passing along start_time. Would it make sense to
> store it in response_callback?

You are right that passing along start_time a lot is rather annoying, but
response_callback is supplied from the caller, so we cannot add start_time to
it. Instead, we could introduce a struct to group response_callback and
start_time, but it wouldn't make the code clearer.

[stevenjb, 2011-09-06 16:51:04]

LGTM

http://codereview.chromium.org/7824054/diff/1/dbus/exported_object.cc
File dbus/exported_object.cc (right):

http://codereview.chromium.org/7824054/diff/1/dbus/exported_object.cc#newcode254
dbus/exported_object.cc:254: response ? 1 : 0, 2);
On 2011/09/02 21:57:11, satorux1 wrote:
> On 2011/09/02 20:43:26, Steven Bennetts wrote:
> > nit: What does the '2' refer to?
> 
> 2 means this enumeration histogram takes values less than 2: 0 or 1. Any ideas
> on how to make it clearer?
const int histogram_max_value = 2;
On the other hand, it's just a histogram, so '2' is probably fine.

http://codereview.chromium.org/7824054/diff/1/dbus/object_proxy.cc
File dbus/object_proxy.cc (right):

http://codereview.chromium.org/7824054/diff/1/dbus/object_proxy.cc#newcode164
dbus/object_proxy.cc:164: start_time,
On 2011/09/02 21:57:11, satorux1 wrote:
> On 2011/09/02 20:43:26, Steven Bennetts wrote:
> > We seem to be doing a lot of passing along start_time. Would it make sense
to
> > store it in response_callback?
> 
> You are right that passing along start_time a lot is rather annoying, but
> response_callback is supplied from the caller, so we cannot add start_time to
> it. Instead, we could introduce a struct to group response_callback and
> start_time, but it wouldn't make the code clearer.

RIght, right, I was somehow thinking ResponseCallback was a struct already. I
agree that making it a struct isn't worth it. Passing along the start time is
awkward, but it's fine.

[satorux1, 2011-09-06 17:32:04]

http://codereview.chromium.org/7824054/diff/1/dbus/exported_object.cc
File dbus/exported_object.cc (right):

http://codereview.chromium.org/7824054/diff/1/dbus/exported_object.cc#newcode254
dbus/exported_object.cc:254: response ? 1 : 0, 2);
On 2011/09/06 16:51:04, Steven Bennetts wrote:
> On 2011/09/02 21:57:11, satorux1 wrote:
> > On 2011/09/02 20:43:26, Steven Bennetts wrote:
> > > nit: What does the '2' refer to?
> > 
> > 2 means this enumeration histogram takes values less than 2: 0 or 1. Any
ideas
> > on how to make it clearer?
> const int histogram_max_value = 2;
> On the other hand, it's just a histogram, so '2' is probably fine.

You were right that 2 was cryptic. Introduced kSuccessRateHistogramMaxValue = 2
with some comment.