Add the WebRTC.webkitApiCountPerSession metric.

Add the WebRTC.webkitApiCountPerSession metric.

BUG=348616
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260211

[Jói, 2014-03-27 17:36:21]

tommi: Main reviewer.
asvitkine: histograms.xml

Tommi: One thing I wonder about is whether instead of counting streams, we
should just count RTCPeerConnection objects. What made me wonder about this is
getUserMedia. Take note of the handling that tries to only count it when it
happens "within a call", since in the current implementation if I visit apprtc,
the getUserMedia call does not get counted (since it happens before the first
stream is added to a PeerConnection).

Cheers,
Jói

[tommi (sloooow) - chröme, 2014-03-28 08:37:54]

On 2014/03/27 17:36:21, Jói wrote:
> tommi: Main reviewer.
> asvitkine: histograms.xml
> 
> Tommi: One thing I wonder about is whether instead of counting streams, we
> should just count RTCPeerConnection objects. What made me wonder about this is
> getUserMedia. Take note of the handling that tries to only count it when it
> happens "within a call", since in the current implementation if I visit
apprtc,
> the getUserMedia call does not get counted (since it happens before the first
> stream is added to a PeerConnection).
> 
> Cheers,
> Jói

Yes, that makes sense for peerconnection.  Would it be possible to count gUM
usage separately?  Could be good to know how many apps are using gUM without
PeerConnection and also would be good to know usage of PC with only data
channels (no gUM) etc.  I realize it's the eleventh hour, so this is more a note
to self than anything :)

The current approach lgtm too if you'd like to land that now and we can change
the approach later?

[Jói, 2014-03-28 09:20:15]

Thanks Tommi.

Since this will start reporting metrics right away, it's best not to
change their meaning if we can avoid it. At least not if it hits dev
channel before we change the meaning.

Note that there is already a raw count of the number of times gUM is
called, independent of the "session" concept introduced in this CL.

There are a few alternatives, the way I see it:

a) Leave as-is. This will probably not count "per session" gUM in many
cases, since generally speaking the gUM call will be made before
streams are added to the PC.

b) Remove the special handling for WEBRTC_GET_USER_MEDIA in
PerSessionWebRTCAPIMetrics::LogUsageOnlyOnce. This has the drawback
that the counter for gUM would be incremented once when gUM is called,
even if a PC never gets connected in that render process.

c) Switch to incrementing/decrementing based on the number of
PeerConnection instances, rather than the number of streams added to
PeerConnections. I'm guessing this will in some cases count gUM
correctly "within the session" but not in all cases; if I am thinking
this through correctly it would depend on whether the JavaScript
instantiates the PeerConnection object before or after it gets streams
by calling getUserMedia.

What do you think? Out of the above, I'm honestly not sure what is the
best approach; none of them will correctly count the number of
getUserMedia calls associated with PeerConnections in all cases. I'm
happy to iterate a bit on this change before landing, although ideally
I would like to land it later today. Worst case though, you could
always land it after my departure.

Cheers,
Jói


On Fri, Mar 28, 2014 at 8:37 AM,  <tommi@chromium.org> wrote:
> On 2014/03/27 17:36:21, Jói wrote:
>>
>> tommi: Main reviewer.
>> asvitkine: histograms.xml
>
>
>> Tommi: One thing I wonder about is whether instead of counting streams, we
>> should just count RTCPeerConnection objects. What made me wonder about
>> this is
>> getUserMedia. Take note of the handling that tries to only count it when
>> it
>> happens "within a call", since in the current implementation if I visit
>
> apprtc,
>>
>> the getUserMedia call does not get counted (since it happens before the
>> first
>> stream is added to a PeerConnection).
>
>
>> Cheers,
>> Jói
>
>
> Yes, that makes sense for peerconnection.  Would it be possible to count gUM
> usage separately?  Could be good to know how many apps are using gUM without
> PeerConnection and also would be good to know usage of PC with only data
> channels (no gUM) etc.  I realize it's the eleventh hour, so this is more a
> note
> to self than anything :)
>
> The current approach lgtm too if you'd like to land that now and we can
> change
> the approach later?
>
> https://codereview.chromium.org/214983003/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Jói, 2014-03-28 11:30:37]

I'm starting to think we should do (b) and see what the data looks like.
You could then adjust if the data seems bogus. It's probably not as
terrible as I initially was thinking to change the semantics of the metric
even if it has been on the dev channel for a week or two, as long as we
recall that when we look at historical data those first two weeks should be
ignored.

Sound good?

Cheers,
Jói



On Fri, Mar 28, 2014 at 9:20 AM, Jói Sigurðsson <joi@chromium.org> wrote:

> Thanks Tommi.
>
> Since this will start reporting metrics right away, it's best not to
> change their meaning if we can avoid it. At least not if it hits dev
> channel before we change the meaning.
>
> Note that there is already a raw count of the number of times gUM is
> called, independent of the "session" concept introduced in this CL.
>
> There are a few alternatives, the way I see it:
>
> a) Leave as-is. This will probably not count "per session" gUM in many
> cases, since generally speaking the gUM call will be made before
> streams are added to the PC.
>
> b) Remove the special handling for WEBRTC_GET_USER_MEDIA in
> PerSessionWebRTCAPIMetrics::LogUsageOnlyOnce. This has the drawback
> that the counter for gUM would be incremented once when gUM is called,
> even if a PC never gets connected in that render process.
>
> c) Switch to incrementing/decrementing based on the number of
> PeerConnection instances, rather than the number of streams added to
> PeerConnections. I'm guessing this will in some cases count gUM
> correctly "within the session" but not in all cases; if I am thinking
> this through correctly it would depend on whether the JavaScript
> instantiates the PeerConnection object before or after it gets streams
> by calling getUserMedia.
>
> What do you think? Out of the above, I'm honestly not sure what is the
> best approach; none of them will correctly count the number of
> getUserMedia calls associated with PeerConnections in all cases. I'm
> happy to iterate a bit on this change before landing, although ideally
> I would like to land it later today. Worst case though, you could
> always land it after my departure.
>
> Cheers,
> Jói
>
>
> On Fri, Mar 28, 2014 at 8:37 AM,  <tommi@chromium.org> wrote:
> > On 2014/03/27 17:36:21, Jói wrote:
> >>
> >> tommi: Main reviewer.
> >> asvitkine: histograms.xml
> >
> >
> >> Tommi: One thing I wonder about is whether instead of counting streams,
> we
> >> should just count RTCPeerConnection objects. What made me wonder about
> >> this is
> >> getUserMedia. Take note of the handling that tries to only count it when
> >> it
> >> happens "within a call", since in the current implementation if I visit
> >
> > apprtc,
> >>
> >> the getUserMedia call does not get counted (since it happens before the
> >> first
> >> stream is added to a PeerConnection).
> >
> >
> >> Cheers,
> >> Jói
> >
> >
> > Yes, that makes sense for peerconnection.  Would it be possible to count
> gUM
> > usage separately?  Could be good to know how many apps are using gUM
> without
> > PeerConnection and also would be good to know usage of PC with only data
> > channels (no gUM) etc.  I realize it's the eleventh hour, so this is
> more a
> > note
> > to self than anything :)
> >
> > The current approach lgtm too if you'd like to land that now and we can
> > change
> > the approach later?
> >
> > https://codereview.chromium.org/214983003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tommi (sloooow) - chröme, 2014-03-28 13:56:24]

Yup, (b) sounds good to me.  Sorry for the slow response, bit of a crazy
day.


On Fri, Mar 28, 2014 at 12:30 PM, Jói Sigurðsson <joi@chromium.org> wrote:

> I'm starting to think we should do (b) and see what the data looks like.
> You could then adjust if the data seems bogus. It's probably not as
> terrible as I initially was thinking to change the semantics of the metric
> even if it has been on the dev channel for a week or two, as long as we
> recall that when we look at historical data those first two weeks should be
> ignored.
>
> Sound good?
>
> Cheers,
> Jói
>
>
>
> On Fri, Mar 28, 2014 at 9:20 AM, Jói Sigurðsson <joi@chromium.org> wrote:
>
>> Thanks Tommi.
>>
>> Since this will start reporting metrics right away, it's best not to
>> change their meaning if we can avoid it. At least not if it hits dev
>> channel before we change the meaning.
>>
>> Note that there is already a raw count of the number of times gUM is
>> called, independent of the "session" concept introduced in this CL.
>>
>> There are a few alternatives, the way I see it:
>>
>> a) Leave as-is. This will probably not count "per session" gUM in many
>> cases, since generally speaking the gUM call will be made before
>> streams are added to the PC.
>>
>> b) Remove the special handling for WEBRTC_GET_USER_MEDIA in
>> PerSessionWebRTCAPIMetrics::LogUsageOnlyOnce. This has the drawback
>> that the counter for gUM would be incremented once when gUM is called,
>> even if a PC never gets connected in that render process.
>>
>> c) Switch to incrementing/decrementing based on the number of
>> PeerConnection instances, rather than the number of streams added to
>> PeerConnections. I'm guessing this will in some cases count gUM
>> correctly "within the session" but not in all cases; if I am thinking
>> this through correctly it would depend on whether the JavaScript
>> instantiates the PeerConnection object before or after it gets streams
>> by calling getUserMedia.
>>
>> What do you think? Out of the above, I'm honestly not sure what is the
>> best approach; none of them will correctly count the number of
>> getUserMedia calls associated with PeerConnections in all cases. I'm
>> happy to iterate a bit on this change before landing, although ideally
>> I would like to land it later today. Worst case though, you could
>> always land it after my departure.
>>
>> Cheers,
>> Jói
>>
>>
>> On Fri, Mar 28, 2014 at 8:37 AM,  <tommi@chromium.org> wrote:
>> > On 2014/03/27 17:36:21, Jói wrote:
>> >>
>> >> tommi: Main reviewer.
>> >> asvitkine: histograms.xml
>> >
>> >
>> >> Tommi: One thing I wonder about is whether instead of counting
>> streams, we
>> >> should just count RTCPeerConnection objects. What made me wonder about
>> >> this is
>> >> getUserMedia. Take note of the handling that tries to only count it
>> when
>> >> it
>> >> happens "within a call", since in the current implementation if I visit
>> >
>> > apprtc,
>> >>
>> >> the getUserMedia call does not get counted (since it happens before the
>> >> first
>> >> stream is added to a PeerConnection).
>> >
>> >
>> >> Cheers,
>> >> Jói
>> >
>> >
>> > Yes, that makes sense for peerconnection.  Would it be possible to
>> count gUM
>> > usage separately?  Could be good to know how many apps are using gUM
>> without
>> > PeerConnection and also would be good to know usage of PC with only data
>> > channels (no gUM) etc.  I realize it's the eleventh hour, so this is
>> more a
>> > note
>> > to self than anything :)
>> >
>> > The current approach lgtm too if you'd like to land that now and we can
>> > change
>> > the approach later?
>> >
>> > https://codereview.chromium.org/214983003/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Jói, 2014-03-28 15:22:08]

I've uploaded a patch with that change, and it's a trivial diff from what you
reviewed. Pulling the CQ trigger now.

Cheers,
Jói


On 2014/03/28 13:56:24, tommi wrote:
> Yup, (b) sounds good to me.  Sorry for the slow response, bit of a crazy
> day.
> 
> 
> On Fri, Mar 28, 2014 at 12:30 PM, Jói Sigurðsson <mailto:joi@chromium.org>
wrote:
> 
> > I'm starting to think we should do (b) and see what the data looks like.
> > You could then adjust if the data seems bogus. It's probably not as
> > terrible as I initially was thinking to change the semantics of the metric
> > even if it has been on the dev channel for a week or two, as long as we
> > recall that when we look at historical data those first two weeks should be
> > ignored.
> >
> > Sound good?
> >
> > Cheers,
> > Jói
> >
> >
> >
> > On Fri, Mar 28, 2014 at 9:20 AM, Jói Sigurðsson <mailto:joi@chromium.org>
wrote:
> >
> >> Thanks Tommi.
> >>
> >> Since this will start reporting metrics right away, it's best not to
> >> change their meaning if we can avoid it. At least not if it hits dev
> >> channel before we change the meaning.
> >>
> >> Note that there is already a raw count of the number of times gUM is
> >> called, independent of the "session" concept introduced in this CL.
> >>
> >> There are a few alternatives, the way I see it:
> >>
> >> a) Leave as-is. This will probably not count "per session" gUM in many
> >> cases, since generally speaking the gUM call will be made before
> >> streams are added to the PC.
> >>
> >> b) Remove the special handling for WEBRTC_GET_USER_MEDIA in
> >> PerSessionWebRTCAPIMetrics::LogUsageOnlyOnce. This has the drawback
> >> that the counter for gUM would be incremented once when gUM is called,
> >> even if a PC never gets connected in that render process.
> >>
> >> c) Switch to incrementing/decrementing based on the number of
> >> PeerConnection instances, rather than the number of streams added to
> >> PeerConnections. I'm guessing this will in some cases count gUM
> >> correctly "within the session" but not in all cases; if I am thinking
> >> this through correctly it would depend on whether the JavaScript
> >> instantiates the PeerConnection object before or after it gets streams
> >> by calling getUserMedia.
> >>
> >> What do you think? Out of the above, I'm honestly not sure what is the
> >> best approach; none of them will correctly count the number of
> >> getUserMedia calls associated with PeerConnections in all cases. I'm
> >> happy to iterate a bit on this change before landing, although ideally
> >> I would like to land it later today. Worst case though, you could
> >> always land it after my departure.
> >>
> >> Cheers,
> >> Jói
> >>
> >>
> >> On Fri, Mar 28, 2014 at 8:37 AM,  <mailto:tommi@chromium.org> wrote:
> >> > On 2014/03/27 17:36:21, Jói wrote:
> >> >>
> >> >> tommi: Main reviewer.
> >> >> asvitkine: histograms.xml
> >> >
> >> >
> >> >> Tommi: One thing I wonder about is whether instead of counting
> >> streams, we
> >> >> should just count RTCPeerConnection objects. What made me wonder about
> >> >> this is
> >> >> getUserMedia. Take note of the handling that tries to only count it
> >> when
> >> >> it
> >> >> happens "within a call", since in the current implementation if I visit
> >> >
> >> > apprtc,
> >> >>
> >> >> the getUserMedia call does not get counted (since it happens before the
> >> >> first
> >> >> stream is added to a PeerConnection).
> >> >
> >> >
> >> >> Cheers,
> >> >> Jói
> >> >
> >> >
> >> > Yes, that makes sense for peerconnection.  Would it be possible to
> >> count gUM
> >> > usage separately?  Could be good to know how many apps are using gUM
> >> without
> >> > PeerConnection and also would be good to know usage of PC with only data
> >> > channels (no gUM) etc.  I realize it's the eleventh hour, so this is
> >> more a
> >> > note
> >> > to self than anything :)
> >> >
> >> > The current approach lgtm too if you'd like to land that now and we can
> >> > change
> >> > the approach later?
> >> >
> >> > https://codereview.chromium.org/214983003/
> >>
> >
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Jói, 2014-03-28 15:22:14]

The CQ bit was checked by joi@chromium.org

[commit-bot: I haz the power, 2014-03-28 15:24:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/214983003/50001

[Alexei Svitkine (slow), 2014-03-28 15:35:54]

lgtm

https://codereview.chromium.org/214983003/diff/50001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/214983003/diff/50001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:29083: +    Counts number of calls to
WebRTC APIs from JavaScript, once   per
Nit: Too many spaces after once.

[commit-bot: I haz the power, 2014-03-28 18:31:45]

Change committed as 260211