Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
5 years, 6 months ago by joachim Modified:
5 years, 3 months ago Reviewers:
tommi, guoweis_webrtc, juberti1, tommi (sloooow) - chröme, juberti CC:
Andrew MacDonald, interface-changes_webrtc.org, niklas.enbom, qiang.lu, rwolff_gocast.it, tterriberry_mozilla.com, webrtc-reviews_webrtc.org, yujie_mao (webrtc) Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionReport 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}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Rebased and updated calling of GetStats #Patch Set 3 : Overload "AddHistogramSample" to store metrics. #
Total comments: 2
Patch Set 4 : Removed unnecessary semicolon. #Patch Set 5 : Fix path to new file in GYP script. #
Total comments: 2
Patch Set 6 : Renamed properties and provide accessors. #
Total comments: 10
Patch Set 7 : Feedback from tommi #
Created: 5 years, 5 months ago
Messages
Total messages: 42 (13 generated)
jbauch@webrtc.org changed reviewers: + juberti@google.com, tommi@chromium.org
PTAL, this can be used from Chromium for UMA metrics later.
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?
juberti@webrtc.org changed reviewers: + guoweis@webrtc.org, juberti@webrtc.org
+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?
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.
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.
Sorry for the delay, I now also updated the CL to use a more general API to store the metrics.
ping?
tommi@webrtc.org changed reviewers: + tommi@webrtc.org
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
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.
The CQ bit was checked by jbauch@webrtc.org to run a CQ dry run
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.")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1156143005/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
Tommi, anything I need to take care of to get the Windows trybots pick up the new file?
Oh stupid me, the path in the GYP file was wrong. Interesting that this does not fail on Linux...
The CQ bit was checked by jbauch@webrtc.org to run a CQ dry run
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.")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1156143005/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
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.
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.
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)
@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.
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).
lgtm
The CQ bit was checked by jbauch@webrtc.org to run a CQ dry run
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")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1156143005/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by tommi@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1156143005/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/ac8869ec5a606e0a0ab71e70937c8fbf403630ce Cr-Commit-Position: refs/heads/master@{#9537}
Message was sent while issue was closed.
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...
Message was sent while issue was closed.
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. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
