Make UMA stats creation available in the Java interface.

Make UMA stats creation available in the Java interface.

Only has counts stats right now but enumeration stats can easily be added in the future if needed.

BUG=webrtc:6313
Committed: https://crrev.com/2a5f371df3c5211ea4e1dd7f155e490b2e4bcc7b
Cr-Commit-Position: refs/heads/master@{#14146}

[sakal, 2016-09-07 09:12:07]

Patchset #1 (id:1) has been deleted

[sakal, 2016-09-07 09:13:09]

Description was changed from

==========
Java UMA
==========

to

==========
This CL only implements only one method loggedHistogramCounts10000 but more can
easily be added as needed.
==========

[sakal, 2016-09-07 09:13:32]

Description was changed from

==========
This CL only implements only one method loggedHistogramCounts10000 but more can
easily be added as needed.
==========

to

==========
Make UMA stats creation available in the Java interface.

This CL only implements only one method loggedHistogramCounts10000 but more can
easily be added as needed.
==========

[sakal, 2016-09-07 09:13:49]

Description was changed from

==========
Make UMA stats creation available in the Java interface.

This CL only implements only one method loggedHistogramCounts10000 but more can
easily be added as needed.
==========

to

==========
Make UMA stats creation available in the Java interface.

This CL only implements only one method, loggedHistogramCounts10000, but more
can easily be added as needed.
==========

[sakal, 2016-09-07 09:14:51]

sakal@webrtc.org changed reviewers:
+ magjed@webrtc.org

[sakal, 2016-09-07 09:14:59]

PTAL

[magjed_webrtc, 2016-09-07 09:48:05]

Description was changed from

==========
Make UMA stats creation available in the Java interface.

This CL only implements only one method, loggedHistogramCounts10000, but more
can easily be added as needed.
==========

to

==========
Make UMA stats creation available in the Java interface.

This CL implements only one method, loggedHistogramCounts10000, but more can
easily be added as needed.
==========

[magjed_webrtc, 2016-09-07 11:17:31]

Maybe a different approach would be better. How about wrapping only the base
interface in JNI:
Histogram* webrtc::metrics::HistogramFactoryGetCounts(
    const std::string& name, int sample, int min, int max,
    int bucket_count);
Histogram* webrtc::metrics::HistogramFactoryGetEnumeration(
    const std::string& name, int sample, int boundary);
void webrtc::metrics::HistogramAdd(
    Histogram* histogram_pointer, const std::string& name, int sample);
and then some small helper methods on top of that in Java.

[magjed_webrtc, 2016-09-07 11:18:34]

BTW, do you have a bug number for this? Maybe we should discuss the design
decisions in the bug.

[sakal, 2016-09-07 11:20:01]

Description was changed from

==========
Make UMA stats creation available in the Java interface.

This CL implements only one method, loggedHistogramCounts10000, but more can
easily be added as needed.
==========

to

==========
Make UMA stats creation available in the Java interface.

This CL implements only one method, loggedHistogramCounts10000, but more can
easily be added as needed.

BUG=webrtc:6313
==========

[sakal, 2016-09-07 11:20:38]

I agree with wrapping the three functions and implementing the rest in Java. I
added the bug number in the description.

[sakal, 2016-09-07 11:20:39]

I agree with wrapping the three functions and implementing the rest in Java. I
added the bug number in the description.

[sakal, 2016-09-08 07:51:13]

Description was changed from

==========
Make UMA stats creation available in the Java interface.

This CL implements only one method, loggedHistogramCounts10000, but more can
easily be added as needed.

BUG=webrtc:6313
==========

to

==========
Make UMA stats creation available in the Java interface.

Only has counts stats right now but enumeration stats can easily be added in the
future if needed.

BUG=webrtc:6313
==========

[sakal, 2016-09-08 07:51:57]

PTAL

[magjed_webrtc, 2016-09-08 10:19:54]

https://codereview.webrtc.org/2320473002/diff/40001/webrtc/api/android/jni/an...
File webrtc/api/android/jni/androidmetrics_jni.cc (right):

https://codereview.webrtc.org/2320473002/diff/40001/webrtc/api/android/jni/an...
webrtc/api/android/jni/androidmetrics_jni.cc:65: return
(jlong)webrtc::metrics::HistogramFactoryGetCounts(name, min, max,
Use jlongFromPointer when converting a pointer to a jlong.

https://codereview.webrtc.org/2320473002/diff/40001/webrtc/api/android/jni/an...
webrtc/api/android/jni/androidmetrics_jni.cc:71:
HistogramAdd(reinterpret_cast<webrtc::metrics::Histogram*>(histogram),
This is a new function to the histogram interface. According to the
documentation, WebRTC clients must either:
 - provide implementations of the Histogram
 - or link with the default implementations (i.e.
   system_wrappers/system_wrappers.gyp:metrics_default).

So this feels scary if some client link to their own implementation. I would
sleep a little bit better if we use the existing HistogramAdd function, and
store the name string in Metrics.Histogram. We can push for officially adding a
new HistogramAdd function, and update this code when that function becomes
available.

[sakal, 2016-09-08 11:20:29]

https://codereview.webrtc.org/2320473002/diff/40001/webrtc/api/android/jni/an...
File webrtc/api/android/jni/androidmetrics_jni.cc (right):

https://codereview.webrtc.org/2320473002/diff/40001/webrtc/api/android/jni/an...
webrtc/api/android/jni/androidmetrics_jni.cc:65: return
(jlong)webrtc::metrics::HistogramFactoryGetCounts(name, min, max,
On 2016/09/08 10:19:54, magjed_webrtc wrote:
> Use jlongFromPointer when converting a pointer to a jlong.

Done.

https://codereview.webrtc.org/2320473002/diff/40001/webrtc/api/android/jni/an...
webrtc/api/android/jni/androidmetrics_jni.cc:71:
HistogramAdd(reinterpret_cast<webrtc::metrics::Histogram*>(histogram),
On 2016/09/08 10:19:54, magjed_webrtc wrote:
> This is a new function to the histogram interface. According to the
> documentation, WebRTC clients must either:
>  - provide implementations of the Histogram
>  - or link with the default implementations (i.e.
>    system_wrappers/system_wrappers.gyp:metrics_default).
> 
> So this feels scary if some client link to their own implementation. I would
> sleep a little bit better if we use the existing HistogramAdd function, and
> store the name string in Metrics.Histogram. We can push for officially adding
a
> new HistogramAdd function, and update this code when that function becomes
> available.
The reason I added this function is that I didn't want to pass the name from the
Java side on every addSample call. That's something we wanted to avoid because
JavaToStdString is probably expensive.

This file is already broken if client decides to use any other implementation
that metrics_default. It relies on for example webrtc::metrics::GetAndReset that
is declared in metrics_default.h. I can declare this new method in
metrics_default.h as well if that is better in your opinion.

Code not using this class wouldn't be broken by adding the method to metrics.h
because it is valid to declare a method and never define it as long as you don't
use it.

[magjed_webrtc, 2016-09-08 11:53:29]

https://codereview.webrtc.org/2320473002/diff/40001/webrtc/api/android/jni/an...
File webrtc/api/android/jni/androidmetrics_jni.cc (right):

https://codereview.webrtc.org/2320473002/diff/40001/webrtc/api/android/jni/an...
webrtc/api/android/jni/androidmetrics_jni.cc:71:
HistogramAdd(reinterpret_cast<webrtc::metrics::Histogram*>(histogram),
On 2016/09/08 11:20:29, sakal wrote:
> On 2016/09/08 10:19:54, magjed_webrtc wrote:
> > This is a new function to the histogram interface. According to the
> > documentation, WebRTC clients must either:
> >  - provide implementations of the Histogram
> >  - or link with the default implementations (i.e.
> >    system_wrappers/system_wrappers.gyp:metrics_default).
> > 
> > So this feels scary if some client link to their own implementation. I would
> > sleep a little bit better if we use the existing HistogramAdd function, and
> > store the name string in Metrics.Histogram. We can push for officially
adding
> a
> > new HistogramAdd function, and update this code when that function becomes
> > available.
> The reason I added this function is that I didn't want to pass the name from
the
> Java side on every addSample call. That's something we wanted to avoid because
> JavaToStdString is probably expensive.
> 
> This file is already broken if client decides to use any other implementation
> that metrics_default. It relies on for example webrtc::metrics::GetAndReset
that
> is declared in metrics_default.h. I can declare this new method in
> metrics_default.h as well if that is better in your opinion.
> 
> Code not using this class wouldn't be broken by adding the method to metrics.h
> because it is valid to declare a method and never define it as long as you
don't
> use it.

I see. I didn't realize we had already committed to using metrics_default for
the Java interface.

Yeah, move the new function declaration to metrics_default.h and leave metrics.h
untouched.

[sakal, 2016-09-08 12:29:26]

On 2016/09/08 11:53:29, magjed_webrtc wrote:
>
https://codereview.webrtc.org/2320473002/diff/40001/webrtc/api/android/jni/an...
> File webrtc/api/android/jni/androidmetrics_jni.cc (right):
> 
>
https://codereview.webrtc.org/2320473002/diff/40001/webrtc/api/android/jni/an...
> webrtc/api/android/jni/androidmetrics_jni.cc:71:
> HistogramAdd(reinterpret_cast<webrtc::metrics::Histogram*>(histogram),
> On 2016/09/08 11:20:29, sakal wrote:
> > On 2016/09/08 10:19:54, magjed_webrtc wrote:
> > > This is a new function to the histogram interface. According to the
> > > documentation, WebRTC clients must either:
> > >  - provide implementations of the Histogram
> > >  - or link with the default implementations (i.e.
> > >    system_wrappers/system_wrappers.gyp:metrics_default).
> > > 
> > > So this feels scary if some client link to their own implementation. I
would
> > > sleep a little bit better if we use the existing HistogramAdd function,
and
> > > store the name string in Metrics.Histogram. We can push for officially
> adding
> > a
> > > new HistogramAdd function, and update this code when that function becomes
> > > available.
> > The reason I added this function is that I didn't want to pass the name from
> the
> > Java side on every addSample call. That's something we wanted to avoid
because
> > JavaToStdString is probably expensive.
> > 
> > This file is already broken if client decides to use any other
implementation
> > that metrics_default. It relies on for example webrtc::metrics::GetAndReset
> that
> > is declared in metrics_default.h. I can declare this new method in
> > metrics_default.h as well if that is better in your opinion.
> > 
> > Code not using this class wouldn't be broken by adding the method to
metrics.h
> > because it is valid to declare a method and never define it as long as you
> don't
> > use it.
> 
> I see. I didn't realize we had already committed to using metrics_default for
> the Java interface.
> 
> Yeah, move the new function declaration to metrics_default.h and leave
metrics.h
> untouched.

Done.

[sakal, 2016-09-08 13:50:22]

The CQ bit was checked by sakal@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-08 13:50:29]

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

[magjed_webrtc, 2016-09-08 13:51:15]

lgtm

[sakal, 2016-09-08 14:01:32]

The CQ bit was unchecked by sakal@webrtc.org

[sakal, 2016-09-08 14:01:38]

The CQ bit was checked by sakal@webrtc.org

[commit-bot: I haz the power, 2016-09-08 14:01:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-09-08 14:11:35]

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

[commit-bot: I haz the power, 2016-09-08 14:11:37]

Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/8188)

[sakal, 2016-09-08 14:12:43]

sakal@webrtc.org changed reviewers:
+ tommi@webrtc.org

[sakal, 2016-09-08 14:12:54]

PTAL

[tommi, 2016-09-08 14:45:26]

https://codereview.webrtc.org/2320473002/diff/80001/webrtc/api/android/java/s...
File webrtc/api/android/java/src/org/webrtc/Metrics.java (right):

https://codereview.webrtc.org/2320473002/diff/80001/webrtc/api/android/java/s...
webrtc/api/android/java/src/org/webrtc/Metrics.java:88: public void
addSampleLogged(int sample) {
my gut feeling is that we don't need this.  I'd like to remove the C++ version
of the same, so prefer not to add it in java.

https://codereview.webrtc.org/2320473002/diff/80001/webrtc/system_wrappers/so...
File webrtc/system_wrappers/source/metrics_default.cc (right):

https://codereview.webrtc.org/2320473002/diff/80001/webrtc/system_wrappers/so...
webrtc/system_wrappers/source/metrics_default.cc:253: ptr->Add(sample);
nit: could also do:

void HistogramAdd(Histogram* histogram_pointer, int sample) {
  RtcHistogram* ptr = reinterpret_cast<RtcHistogram*>(histogram_pointer);
  if (ptr)
    ptr->Add(sample);
}

[tommi, 2016-09-08 15:01:37]

lgtm.  I like the ideas we talked about on improving the C++ implementation we
have for UMA.  Seems we're overpromising on features there and possibly not
being as efficient as we should be wrt string copying.

[sakal, 2016-09-08 15:02:44]

The CQ bit was checked by sakal@webrtc.org

[sakal, 2016-09-08 15:02:44]

The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2320473002/#ps120001 (title:
"Also update the example.")

[commit-bot: I haz the power, 2016-09-08 15:02:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-09-08 17:03:24]

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

[commit-bot: I haz the power, 2016-09-08 17:03:25]

Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_clang_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_clang_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_drmemory_light on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_x64_clang_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_x64_clang_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_x64_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_x64_gyp_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_x64_gyp_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_x64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[sakal, 2016-09-09 07:03:25]

The CQ bit was checked by sakal@webrtc.org

[commit-bot: I haz the power, 2016-09-09 07:03:34]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-09-09 07:11:50]

Description was changed from

==========
Make UMA stats creation available in the Java interface.

Only has counts stats right now but enumeration stats can easily be added in the
future if needed.

BUG=webrtc:6313
==========

to

==========
Make UMA stats creation available in the Java interface.

Only has counts stats right now but enumeration stats can easily be added in the
future if needed.

BUG=webrtc:6313
==========

[commit-bot: I haz the power, 2016-09-09 07:11:53]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2016-09-09 07:11:57]

Description was changed from

==========
Make UMA stats creation available in the Java interface.

Only has counts stats right now but enumeration stats can easily be added in the
future if needed.

BUG=webrtc:6313
==========

to

==========
Make UMA stats creation available in the Java interface.

Only has counts stats right now but enumeration stats can easily be added in the
future if needed.

BUG=webrtc:6313

Committed: https://crrev.com/2a5f371df3c5211ea4e1dd7f155e490b2e4bcc7b
Cr-Commit-Position: refs/heads/master@{#14146}
==========

[commit-bot: I haz the power, 2016-09-09 07:11:58]

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