Add macros for ability to log samples that are added to histograms (RTC_LOGGED_*).

Add macros for ability to log samples that are added to histograms (RTC_LOGGED_*).

Adds logging of:
- video stats that are recorded when a stream is removed
- bitrate stats that are recorded at the end of a call
- initial bwe rampup stats

BUG=

Committed: https://crrev.com/58d992e02511219ec59b13589a30e4d74d420da3
Cr-Commit-Position: refs/heads/master@{#12133}

[åsapersson, 2016-03-15 08:10:34]

Description was changed from

==========
Add macros for ability to log samples that are added to histogram stats
(RTC_L*).

BUG=
==========

to

==========
Add macros for ability to log samples that are added to histogram stats
(RTC_L*).

BUG=
==========

[åsapersson, 2016-03-15 08:10:34]

asapersson@webrtc.org changed reviewers:
+ mflodman@webrtc.org, pbos@webrtc.org, stefan@webrtc.org

[åsapersson, 2016-03-15 08:11:29]

Description was changed from

==========
Add macros for ability to log samples that are added to histogram stats
(RTC_L*).

BUG=
==========

to

==========
Add macros for ability to log samples that are added to histograms (RTC_L*).

BUG=
==========

[åsapersson, 2016-03-15 08:11:43]

Patchset #2 (id:20001) has been deleted

[stefan-webrtc, 2016-03-15 10:14:10]

LG, but roughly how much logging does this introduce? Just 20 or so extra logs
at the end of a session?

https://codereview.webrtc.org/1788783002/diff/40001/webrtc/system_wrappers/in...
File webrtc/system_wrappers/include/metrics.h (right):

https://codereview.webrtc.org/1788783002/diff/40001/webrtc/system_wrappers/in...
webrtc/system_wrappers/include/metrics.h:19: #include
"webrtc/system_wrappers/include/logging.h"
Should we use webrtc/base/logging.h?

[åsapersson, 2016-03-15 11:04:23]

Description was changed from

==========
Add macros for ability to log samples that are added to histograms (RTC_L*).

BUG=
==========

to

==========
Add macros for ability to log samples that are added to histograms (RTC_L*).

Adds logging of:
- video stats that are recorded when a stream is removed
- bitrate stats that are recorded at the end of a call
- initial bwe rampup stats

BUG=
==========

[åsapersson, 2016-03-15 12:46:11]

On 2016/03/15 10:14:10, stefan-webrtc (holmer) wrote:
> LG, but roughly how much logging does this introduce? Just 20 or so extra logs
> at the end of a session?
> 
>
https://codereview.webrtc.org/1788783002/diff/40001/webrtc/system_wrappers/in...
> File webrtc/system_wrappers/include/metrics.h (right):
> 
>
https://codereview.webrtc.org/1788783002/diff/40001/webrtc/system_wrappers/in...
> webrtc/system_wrappers/include/metrics.h:19: #include
> "webrtc/system_wrappers/include/logging.h"
> Should we use webrtc/base/logging.h?

Adds around 30 logs (video stats that are recorded when a stream is removed,
bitrate stats that are recorded at the end of a call and initial bwe rampup
stats).

[åsapersson, 2016-03-15 12:46:23]

https://codereview.webrtc.org/1788783002/diff/40001/webrtc/system_wrappers/in...
File webrtc/system_wrappers/include/metrics.h (right):

https://codereview.webrtc.org/1788783002/diff/40001/webrtc/system_wrappers/in...
webrtc/system_wrappers/include/metrics.h:19: #include
"webrtc/system_wrappers/include/logging.h"
On 2016/03/15 10:14:10, stefan-webrtc (holmer) wrote:
> Should we use webrtc/base/logging.h?

Done.

[stefan-webrtc, 2016-03-15 14:44:39]

Seems reasonable then. lgtm

[pbos-webrtc, 2016-03-15 18:12:13]

Should we call these RTC_LOGGED_HISTOGRAM_? I find LHISTOGRAM hard to read.

[åsapersson, 2016-03-16 07:44:45]

Description was changed from

==========
Add macros for ability to log samples that are added to histograms (RTC_L*).

Adds logging of:
- video stats that are recorded when a stream is removed
- bitrate stats that are recorded at the end of a call
- initial bwe rampup stats

BUG=
==========

to

==========
Add macros for ability to log samples that are added to histograms
(RTC_LOGGED_*).

Adds logging of:
- video stats that are recorded when a stream is removed
- bitrate stats that are recorded at the end of a call
- initial bwe rampup stats

BUG=
==========

[åsapersson, 2016-03-16 08:45:25]

On 2016/03/15 18:12:13, pbos-webrtc wrote:
> Should we call these RTC_LOGGED_HISTOGRAM_? I find LHISTOGRAM hard to read.

Ok, I have changed to RTC_LOGGED_*.

[pbos-webrtc, 2016-03-16 11:18:20]

lgtm

[mflodman, 2016-03-29 07:52:20]

lgtm

[åsapersson, 2016-03-29 08:15:45]

The CQ bit was checked by asapersson@webrtc.org

[åsapersson, 2016-03-29 08:15:46]

The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1788783002/#ps100001 (title:
" ")

[commit-bot: I haz the power, 2016-03-29 08:15:54]

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

[commit-bot: I haz the power, 2016-03-29 09:15:10]

Description was changed from

==========
Add macros for ability to log samples that are added to histograms
(RTC_LOGGED_*).

Adds logging of:
- video stats that are recorded when a stream is removed
- bitrate stats that are recorded at the end of a call
- initial bwe rampup stats

BUG=
==========

to

==========
Add macros for ability to log samples that are added to histograms
(RTC_LOGGED_*).

Adds logging of:
- video stats that are recorded when a stream is removed
- bitrate stats that are recorded at the end of a call
- initial bwe rampup stats

BUG=
==========

[commit-bot: I haz the power, 2016-03-29 09:15:11]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-03-29 09:15:18]

Description was changed from

==========
Add macros for ability to log samples that are added to histograms
(RTC_LOGGED_*).

Adds logging of:
- video stats that are recorded when a stream is removed
- bitrate stats that are recorded at the end of a call
- initial bwe rampup stats

BUG=
==========

to

==========
Add macros for ability to log samples that are added to histograms
(RTC_LOGGED_*).

Adds logging of:
- video stats that are recorded when a stream is removed
- bitrate stats that are recorded at the end of a call
- initial bwe rampup stats

BUG=

Committed: https://crrev.com/58d992e02511219ec59b13589a30e4d74d420da3
Cr-Commit-Position: refs/heads/master@{#12133}
==========

[commit-bot: I haz the power, 2016-03-29 09:15:20]

Patchset 5 (id:??) landed as
https://crrev.com/58d992e02511219ec59b13589a30e4d74d420da3
Cr-Commit-Position: refs/heads/master@{#12133}

[tommi, 2016-09-08 14:43:02]

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

[tommi, 2016-09-08 14:43:03]

What do you think about backing out of this change?

The UMA macros are more and more being used for doing both UMA and logging, yet
these things do not have the same design goals or satisfy the same need.
A specific goal of UMA, is to be as efficient as possible and reasonably cheap
as far as code goes. Not get in the way while still allowing us to measure.

With this change, *every* call site has a LOG() statement (which matters
regardless of whether it will be executed or not), which means a sizable chunk
of code + dependency on iostream.

I don't want to encourage mixing these two things (logging and stats), so I
would rather want to put it on the caller to control how logging is done for the
places that need it, rather than make logging a feature of UMA stats.  I also
can't find a precedence (e.g. in Chrome).

[pbos-webrtc, 2016-09-08 14:47:00]

On 2016/09/08 14:43:03, tommi (webrtc) wrote:
> What do you think about backing out of this change?
> 
> The UMA macros are more and more being used for doing both UMA and logging,
yet
> these things do not have the same design goals or satisfy the same need.
> A specific goal of UMA, is to be as efficient as possible and reasonably cheap
> as far as code goes. Not get in the way while still allowing us to measure.
> 
> With this change, *every* call site has a LOG() statement (which matters
> regardless of whether it will be executed or not), which means a sizable chunk
> of code + dependency on iostream.
> 
> I don't want to encourage mixing these two things (logging and stats), so I
> would rather want to put it on the caller to control how logging is done for
the
> places that need it, rather than make logging a feature of UMA stats.  I also
> can't find a precedence (e.g. in Chrome).

Personally I've found it very useful when debugging user logs, when a call ends
UMA logs the stats for all of that call. The same stats have been very useful to
determine overall call quality in terms of packet loss over entire call. I think
we shouldn't necessarily log all UMA stats, but I don't think that we need to
prevent it if it's added consciously.

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

On 2016/09/08 14:47:00, pbos-webrtc wrote:
> On 2016/09/08 14:43:03, tommi (webrtc) wrote:
> > What do you think about backing out of this change?
> > 
> > The UMA macros are more and more being used for doing both UMA and logging,
> yet
> > these things do not have the same design goals or satisfy the same need.
> > A specific goal of UMA, is to be as efficient as possible and reasonably
cheap
> > as far as code goes. Not get in the way while still allowing us to measure.
> > 
> > With this change, *every* call site has a LOG() statement (which matters
> > regardless of whether it will be executed or not), which means a sizable
chunk
> > of code + dependency on iostream.
> > 
> > I don't want to encourage mixing these two things (logging and stats), so I
> > would rather want to put it on the caller to control how logging is done for
> the
> > places that need it, rather than make logging a feature of UMA stats.  I
also
> > can't find a precedence (e.g. in Chrome).
> 
> Personally I've found it very useful when debugging user logs, when a call
ends
> UMA logs the stats for all of that call. The same stats have been very useful
to
> determine overall call quality in terms of packet loss over entire call. I
think
> we shouldn't necessarily log all UMA stats, but I don't think that we need to
> prevent it if it's added consciously.

I can see that that sort of information is useful.  However, do you think that
this is the way to record that information?

[pbos-webrtc, 2016-09-08 15:52:26]

On 2016/09/08 15:08:37, tommi (webrtc) wrote:
> On 2016/09/08 14:47:00, pbos-webrtc wrote:
> > On 2016/09/08 14:43:03, tommi (webrtc) wrote:
> > > What do you think about backing out of this change?
> > > 
> > > The UMA macros are more and more being used for doing both UMA and
logging,
> > yet
> > > these things do not have the same design goals or satisfy the same need.
> > > A specific goal of UMA, is to be as efficient as possible and reasonably
> cheap
> > > as far as code goes. Not get in the way while still allowing us to
measure.
> > > 
> > > With this change, *every* call site has a LOG() statement (which matters
> > > regardless of whether it will be executed or not), which means a sizable
> chunk
> > > of code + dependency on iostream.
> > > 
> > > I don't want to encourage mixing these two things (logging and stats), so
I
> > > would rather want to put it on the caller to control how logging is done
for
> > the
> > > places that need it, rather than make logging a feature of UMA stats.  I
> also
> > > can't find a precedence (e.g. in Chrome).
> > 
> > Personally I've found it very useful when debugging user logs, when a call
> ends
> > UMA logs the stats for all of that call. The same stats have been very
useful
> to
> > determine overall call quality in terms of packet loss over entire call. I
> think
> > we shouldn't necessarily log all UMA stats, but I don't think that we need
to
> > prevent it if it's added consciously.
> 
> I can see that that sort of information is useful.  However, do you think that
> this is the way to record that information?

I think the macros are useful, but we could replace the inline LOG(LS_INFO) <<
constant_name << " " << sample; with a function call. Something like
LogHistogram(__FILE__, __LINE__, constant_name, sample).

Then we'd take the code-size cost for logging only once instead of
per-invocation. Am I correct in assuming that your primary concern here is code
size/bloat or do you think we log too much data?