Report metrics about negotiated ciphers.

Report metrics about negotiated ciphers.

This CL adds an API to the metrics observer interface to report negotiated
ciphers for WebRTC sessions. This can be used from Chromium for UMA metrics
later to get an idea which cipher suites are used by clients (e.g. compare
the use of DTLS 1.0 / 1.2).

BUG=428343
Committed: https://crrev.com/ac8869ec5a606e0a0ab71e70937c8fbf403630ce
Cr-Commit-Position: refs/heads/master@{#9537}

[joachim, 2015-06-05 15:05:04]

jbauch@webrtc.org changed reviewers:
+ juberti@google.com, tommi@chromium.org

[joachim, 2015-06-05 15:05:05]

PTAL, this can be used from Chromium for UMA metrics later.

[juberti1, 2015-06-06 00:16:00]

On 2015/06/05 15:05:05, joachim wrote:
> PTAL, this can be used from Chromium for UMA metrics later.

Can you add some info to the description on what we will use this information
for?

[juberti1, 2015-06-06 00:18:39]

juberti@webrtc.org changed reviewers:
+ guoweis@webrtc.org, juberti@webrtc.org

[juberti1, 2015-06-06 00:18:41]

+guowei who has done similar metrics in the past

https://codereview.webrtc.org/1156143005/diff/1/talk/app/webrtc/peerconnectio...
File talk/app/webrtc/peerconnectioninterface.h (right):

https://codereview.webrtc.org/1156143005/diff/1/talk/app/webrtc/peerconnectio...
talk/app/webrtc/peerconnectioninterface.h:131: virtual void
CiphersNegotiated(const std::string& content_name,
This seems very ad-hoc. Can we abstract this into a general API where we can
specify the frequency of string values?

[guoweis_webrtc, 2015-06-08 17:09:12]

https://codereview.webrtc.org/1156143005/diff/1/talk/app/webrtc/webrtcsession.cc
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1156143005/diff/1/talk/app/webrtc/webrtcsession...
talk/app/webrtc/webrtcsession.cc:1396: ReportNegotiatedCiphers(transport);
Both ReportBestConnectionState and ReportNegotiatedCiphers call GetStats
internally. I could imagine there might be other reporting in the future.
Suggest to have the GetStats outside and pass the state into each function.

[joachim, 2015-06-08 20:58:40]

Thanks for your feedback, I also updated the description of the CL.

https://codereview.webrtc.org/1156143005/diff/1/talk/app/webrtc/peerconnectio...
File talk/app/webrtc/peerconnectioninterface.h (right):

https://codereview.webrtc.org/1156143005/diff/1/talk/app/webrtc/peerconnectio...
talk/app/webrtc/peerconnectioninterface.h:131: virtual void
CiphersNegotiated(const std::string& content_name,
On 2015/06/06 00:18:40, juberti1 wrote:
> This seems very ad-hoc. Can we abstract this into a general API where we can
> specify the frequency of string values?

Are you thinking about something like
IncrementCounter(PeerConnectionMetricsKey type, const std::string& value);

where 'key' would be 'kPeerConnectionAudioSrtpCipher' or
'kPeerConnectionVideoSslCipher', etc. and 'value' the actual cipher suite?

https://codereview.webrtc.org/1156143005/diff/1/talk/app/webrtc/webrtcsession.cc
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1156143005/diff/1/talk/app/webrtc/webrtcsession...
talk/app/webrtc/webrtcsession.cc:1396: ReportNegotiatedCiphers(transport);
On 2015/06/08 17:09:11, guoweis wrote:
> Both ReportBestConnectionState and ReportNegotiatedCiphers call GetStats
> internally. I could imagine there might be other reporting in the future.
> Suggest to have the GetStats outside and pass the state into each function.

Done.

[joachim, 2015-06-22 21:13:18]

Sorry for the delay, I now also updated the CL to use a more general API to
store the metrics.

[joachim, 2015-06-29 21:07:54]

ping?

[tommi, 2015-06-30 12:42:50]

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

[tommi, 2015-06-30 12:42:51]

lgtm

https://codereview.webrtc.org/1156143005/diff/40001/talk/app/webrtc/peerconne...
File talk/app/webrtc/peerconnectioninterface.h (right):

https://codereview.webrtc.org/1156143005/diff/40001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnectioninterface.h:132: const std::string& value) {};
remove semicolon

[joachim, 2015-06-30 16:06:38]

Thanks for your review.

https://codereview.chromium.org/1156143005/diff/40001/talk/app/webrtc/peercon...
File talk/app/webrtc/peerconnectioninterface.h (right):

https://codereview.chromium.org/1156143005/diff/40001/talk/app/webrtc/peercon...
talk/app/webrtc/peerconnectioninterface.h:132: const std::string& value) {};
On 2015/06/30 12:42:51, tommi (webrtc) wrote:
> remove semicolon

Done.

[joachim, 2015-06-30 16:20:48]

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

[joachim, 2015-06-30 16:20:49]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1156143005/#ps60001 (title:
"Removed unnecessary semicolon.")

[commit-bot: I haz the power, 2015-06-30 16:21:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1156143005/60001

[commit-bot: I haz the power, 2015-06-30 16:23:10]

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

[commit-bot: I haz the power, 2015-06-30 16:23:11]

Dry run: Exceeded global retry quota

[joachim, 2015-06-30 16:34:40]

Tommi, anything I need to take care of to get the Windows trybots pick up the
new file?

[joachim, 2015-06-30 19:04:24]

Oh stupid me, the path in the GYP file was wrong. Interesting that this does not
fail on Linux...

[joachim, 2015-06-30 19:38:09]

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

[joachim, 2015-06-30 19:38:10]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1156143005/#ps80001 (title:
"Fix path to new file in GYP script.")

[commit-bot: I haz the power, 2015-06-30 19:38:21]

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

[commit-bot: I haz the power, 2015-06-30 20:41:34]

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

[commit-bot: I haz the power, 2015-06-30 20:41:36]

Dry run: Exceeded global retry quota

[juberti1, 2015-07-01 02:06:15]

One moderate change requested.

https://codereview.webrtc.org/1156143005/diff/80001/talk/app/webrtc/fakemetri...
File talk/app/webrtc/fakemetricsobserver.h (right):

https://codereview.webrtc.org/1156143005/diff/80001/talk/app/webrtc/fakemetri...
talk/app/webrtc/fakemetricsobserver.h:66: int
peer_connection_metrics_name_[kPeerConnectionMetricsCounter_Max];
Realize that you didn't write most of this code, but these names are pretty
inscrutable to me. Suggest renaming them to

int counters_[blah]
int int_histogram_samples_[blah];
std::string string_histogram_samples_[blah];

You could also go farther and do something like
std::vector<std::pair<PeerConnectionMetricsName, int>> int_histogram_samples

so that you could actually store more than one sample for a given type.

Lastly, these should not be publicly accessible now. Please make them private
and provide accessors (e.g. GetHistogramSamples(type)) to avoid clients making
assumptions about the implementation.

[joachim, 2015-07-01 14:18:42]

https://codereview.webrtc.org/1156143005/diff/80001/talk/app/webrtc/fakemetri...
File talk/app/webrtc/fakemetricsobserver.h (right):

https://codereview.webrtc.org/1156143005/diff/80001/talk/app/webrtc/fakemetri...
talk/app/webrtc/fakemetricsobserver.h:66: int
peer_connection_metrics_name_[kPeerConnectionMetricsCounter_Max];
On 2015/07/01 02:06:15, juberti1 wrote:
> Realize that you didn't write most of this code, but these names are pretty
> inscrutable to me. Suggest renaming them to
> 
> int counters_[blah]
> int int_histogram_samples_[blah];
> std::string string_histogram_samples_[blah];
> 
> You could also go farther and do something like
> std::vector<std::pair<PeerConnectionMetricsName, int>> int_histogram_samples
> 
> so that you could actually store more than one sample for a given type.
> 
> Lastly, these should not be publicly accessible now. Please make them private
> and provide accessors (e.g. GetHistogramSamples(type)) to avoid clients making
> assumptions about the implementation.

Renamed, made private and provided accessors. Changing to store a vector of
values should be done in a separate CL if necessary.

[tommi (sloooow) - chröme, 2015-07-02 11:32:29]

https://codereview.chromium.org/1156143005/diff/100001/talk/app/webrtc/fakeme...
File talk/app/webrtc/fakemetricsobserver.h (right):

https://codereview.chromium.org/1156143005/diff/100001/talk/app/webrtc/fakeme...
talk/app/webrtc/fakemetricsobserver.h:41: void Reset() {
Is this method necessary? (can we just do this in the ctor and not expose a
method?)

https://codereview.chromium.org/1156143005/diff/100001/talk/app/webrtc/fakeme...
talk/app/webrtc/fakemetricsobserver.h:50: counters_[type]++;
nit: ++counters_[type];

It looks like this class is single thread.  Can you add a ThreadChecker to the
class and DCHECKs to ensure all methods are called on the right thread?

Also, it would be good to split the implementation out into a separate .cc file.

https://codereview.chromium.org/1156143005/diff/100001/talk/app/webrtc/fakeme...
talk/app/webrtc/fakemetricsobserver.h:54: ASSERT(int_histogram_samples_[type] ==
0);
can we use DCHECK instead of ASSERT (see checks.h)

https://codereview.chromium.org/1156143005/diff/100001/talk/app/webrtc/fakeme...
talk/app/webrtc/fakemetricsobserver.h:74: int AddRef() override { return 1; }
why not implement proper reference counting?  See e.g. RefCountedObject.

https://codereview.chromium.org/1156143005/diff/100001/talk/app/webrtc/webrtc...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.chromium.org/1156143005/diff/100001/talk/app/webrtc/webrtc...
talk/app/webrtc/webrtcsession.cc:1881: ASSERT(metrics_observer_ != NULL);
DCHECK? (same below if we can switch)

[joachim, 2015-07-02 11:51:59]

@tommi: Thanks for your feedback. Almost all of that code already existed before
in "webrtcsession_unittest.cc", I just moved it to a separate file as I needed
it from the other tests, but I didn't really look into changing functionality. I
will do so sometime later and will update the CL based on your suggestions.

[joachim, 2015-07-02 22:06:06]

All done.

https://codereview.chromium.org/1156143005/diff/100001/talk/app/webrtc/fakeme...
File talk/app/webrtc/fakemetricsobserver.h (right):

https://codereview.chromium.org/1156143005/diff/100001/talk/app/webrtc/fakeme...
talk/app/webrtc/fakemetricsobserver.h:41: void Reset() {
On 2015/07/02 11:32:29, tommi wrote:
> Is this method necessary? (can we just do this in the ctor and not expose a
> method?)

Reset is used by the unittests, so this needs to stay.

https://codereview.chromium.org/1156143005/diff/100001/talk/app/webrtc/fakeme...
talk/app/webrtc/fakemetricsobserver.h:50: counters_[type]++;
On 2015/07/02 11:32:29, tommi wrote:
> nit: ++counters_[type];

Done.

> It looks like this class is single thread.  Can you add a ThreadChecker to the
> class and DCHECKs to ensure all methods are called on the right thread?

Done.

> Also, it would be good to split the implementation out into a separate .cc
file.

Done.

https://codereview.chromium.org/1156143005/diff/100001/talk/app/webrtc/fakeme...
talk/app/webrtc/fakemetricsobserver.h:54: ASSERT(int_histogram_samples_[type] ==
0);
On 2015/07/02 11:32:29, tommi wrote:
> can we use DCHECK instead of ASSERT (see checks.h)

Done.

https://codereview.chromium.org/1156143005/diff/100001/talk/app/webrtc/fakeme...
talk/app/webrtc/fakemetricsobserver.h:74: int AddRef() override { return 1; }
On 2015/07/02 11:32:29, tommi wrote:
> why not implement proper reference counting?  See e.g. RefCountedObject.

I assume it was done like this because the class was only used for testing.
Anyway, changed it.

https://codereview.chromium.org/1156143005/diff/100001/talk/app/webrtc/webrtc...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.chromium.org/1156143005/diff/100001/talk/app/webrtc/webrtc...
talk/app/webrtc/webrtcsession.cc:1881: ASSERT(metrics_observer_ != NULL);
On 2015/07/02 11:32:29, tommi wrote:
> DCHECK? (same below if we can switch)

Done and also changed the "ASSERT(false)" to RTC_NOTREACHED (here and below).

[juberti1, 2015-07-03 02:31:07]

lgtm

[joachim, 2015-07-03 07:24:23]

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

[joachim, 2015-07-03 07:24:23]

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

[commit-bot: I haz the power, 2015-07-03 07:24:30]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1156143005/120001

[commit-bot: I haz the power, 2015-07-03 08:14:02]

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

[commit-bot: I haz the power, 2015-07-03 08:14:04]

Dry run: This issue passed the CQ dry run.

[tommi, 2015-07-03 08:35:00]

lgtm

[tommi, 2015-07-03 08:35:09]

The CQ bit was checked by tommi@webrtc.org

[commit-bot: I haz the power, 2015-07-03 08:35:18]

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

[commit-bot: I haz the power, 2015-07-03 08:36:19]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-07-03 08:36:29]

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

[andresp, 2015-07-04 09:55:42]

On 2015/07/03 08:36:29, commit-bot: I haz the power wrote:
> Patchset 7 (id:??) landed as
> https://crrev.com/ac8869ec5a606e0a0ab71e70937c8fbf403630ce
> Cr-Commit-Position: refs/heads/master@{#9537}

Just came across this.... are you familiar with
webrtc/system_wrappers/interface/metrics.h ?
It allows one to directly export uma histograms from webrtc without having to
wire things such as metric observers or so...

[guoweis_webrtc, 2015-09-10 22:07:19]

On 2015/07/04 09:55:42, andresp wrote:
> On 2015/07/03 08:36:29, commit-bot: I haz the power wrote:
> > Patchset 7 (id:??) landed as
> > https://crrev.com/ac8869ec5a606e0a0ab71e70937c8fbf403630ce
> > Cr-Commit-Position: refs/heads/master@{#9537}
> 
> Just came across this.... are you familiar with
> webrtc/system_wrappers/interface/metrics.h ?
> It allows one to directly export uma histograms from webrtc without having to
> wire things such as metric observers or so...

I realized that what's been done in this CL for metric reporting is not
supported in chrome. The histogram has to be an integer, can't be a string. 

For all types of cipher that we could run into, is there is a list for that?
AES_CM_128_HMAC_SHA1_80
AES_CM_128_HMAC_SHA1_32

Anything else? these 2 are from NSSS and OpenSSL.