Add UMA histogram for SDP bandwidth values. This change adds a new UMA histogram WebRTC.PeerConnection.SdpBandwidth to track the values provided in the b= SDP lines. This helps in understanding the distribution of bandwidth values and identifying potential issues with extremely large values or parse failures. The histogram uses the following categories: - parse failure - -1 - 0 - between 1 and INT_MAX/1000 - between INT_MAX/1000 and INT_MAX Bug: chromium:501883592 Change-Id: If8cca989a2fb1ab714f840fecc4be83932898836 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/463782 Commit-Queue: Harald Alvestrand <hta@webrtc.org> Reviewed-by: Henrik Boström <hbos@webrtc.org> Cr-Commit-Position: refs/heads/main@{#47414}
diff --git a/api/BUILD.gn b/api/BUILD.gn index ac6bbc6..b6692cd 100644 --- a/api/BUILD.gn +++ b/api/BUILD.gn
@@ -347,6 +347,7 @@ ":rtp_transceiver_direction", ":sctp_transport_interface", ":sequence_checker", + ":uma_metrics", "../media:codec", "../media:media_constants", "../media:rid_description", @@ -372,6 +373,7 @@ "../rtc_base:stringutils", "../rtc_base/system:no_unique_address", "../rtc_base/system:rtc_export", + "../system_wrappers:metrics", "audio:audio_frame_api", "//third_party/abseil-cpp/absl/algorithm:container", "//third_party/abseil-cpp/absl/base:nullability", @@ -1856,6 +1858,7 @@ ":rtp_transceiver_direction", ":scoped_refptr", ":sequence_checker", + ":uma_metrics", "../media:codec", "../media:media_constants", "../media:rid_description", @@ -1878,10 +1881,12 @@ "../rtc_base:rtc_event", "../rtc_base:socket_address", "../rtc_base:ssl", + "../rtc_base:stringutils", "../rtc_base:task_queue_for_test", "../rtc_base/containers:flat_set", "../rtc_base/synchronization:sequence_checker_internal", "../rtc_base/system:unused", + "../system_wrappers:metrics", "../test:fileutils", "../test:test_support", "audio_codecs/opus:unittests",
diff --git a/api/uma_metrics.h b/api/uma_metrics.h index daecfe8..8f86a32 100644 --- a/api/uma_metrics.h +++ b/api/uma_metrics.h
@@ -250,6 +250,17 @@ kMaxValue = kRejected, }; +// These values are persisted to logs. Entries should not be renumbered and +// numeric values should never be reused. +enum SdpBandwidthCategory { + kSdpBandwidthParseFailure = 0, + kSdpBandwidthNegativeOne = 1, + kSdpBandwidthZero = 2, + kSdpBandwidthSmall = 3, // 1 to INT_MAX/1000 + kSdpBandwidthLarge = 4, // INT_MAX/1000 + 1 to INT_MAX + kSdpBandwidthMax +}; + // When adding new metrics please consider using the style described in // https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histograms/README.md#usage // instead of the legacy enums used above.
diff --git a/api/webrtc_sdp.cc b/api/webrtc_sdp.cc index eb49785..736c207 100644 --- a/api/webrtc_sdp.cc +++ b/api/webrtc_sdp.cc
@@ -38,6 +38,7 @@ #include "api/rtp_parameters.h" #include "api/rtp_transceiver_direction.h" #include "api/sctp_transport_interface.h" +#include "api/uma_metrics.h" #include "media/base/codec.h" #include "media/base/media_constants.h" #include "media/base/rid_description.h" @@ -61,6 +62,7 @@ #include "rtc_base/ssl_fingerprint.h" #include "rtc_base/string_encode.h" #include "rtc_base/strings/string_builder.h" +#include "system_wrappers/include/metrics.h" namespace webrtc { @@ -2605,8 +2607,23 @@ } int b = 0; if (!GetValueFromString(*line, bandwidth, &b, error)) { + RTC_HISTOGRAM_ENUMERATION("WebRTC.PeerConnection.SdpBandwidth", + kSdpBandwidthParseFailure, kSdpBandwidthMax); return false; } + if (b == -1) { + RTC_HISTOGRAM_ENUMERATION("WebRTC.PeerConnection.SdpBandwidth", + kSdpBandwidthNegativeOne, kSdpBandwidthMax); + } else if (b == 0) { + RTC_HISTOGRAM_ENUMERATION("WebRTC.PeerConnection.SdpBandwidth", + kSdpBandwidthZero, kSdpBandwidthMax); + } else if (b > 0 && b <= INT_MAX / 1000) { + RTC_HISTOGRAM_ENUMERATION("WebRTC.PeerConnection.SdpBandwidth", + kSdpBandwidthSmall, kSdpBandwidthMax); + } else if (b > INT_MAX / 1000) { + RTC_HISTOGRAM_ENUMERATION("WebRTC.PeerConnection.SdpBandwidth", + kSdpBandwidthLarge, kSdpBandwidthMax); + } // TODO(deadbeef): Historically, applications may be setting a value // of -1 to mean "unset any previously set bandwidth limit", even // though ommitting the "b=AS" entirely will do just that. Once we've
diff --git a/api/webrtc_sdp_unittest.cc b/api/webrtc_sdp_unittest.cc index 91b74ca..c30435c 100644 --- a/api/webrtc_sdp_unittest.cc +++ b/api/webrtc_sdp_unittest.cc
@@ -30,6 +30,7 @@ #include "api/media_types.h" #include "api/rtp_parameters.h" #include "api/rtp_transceiver_direction.h" +#include "api/uma_metrics.h" #include "media/base/codec.h" #include "media/base/media_constants.h" #include "media/base/rid_description.h" @@ -45,6 +46,8 @@ #include "rtc_base/message_digest.h" #include "rtc_base/socket_address.h" #include "rtc_base/ssl_fingerprint.h" +#include "rtc_base/strings/string_builder.h" +#include "system_wrappers/include/metrics.h" #include "test/gmock.h" #include "test/gtest.h" @@ -3946,6 +3949,48 @@ EXPECT_EQ(kAutoBandwidth, vcd->bandwidth()); } +TEST_F(WebRtcSdpTest, SdpBandwidthMetrics) { + metrics::Reset(); + auto get_sdp = [](absl::string_view value) { + StringBuilder sb; + sb << "v=0\r\n" + << "o=- 18446744069414584320 18446462598732840960 IN IP4 127.0.0.1\r\n" + << "s=-\r\n" + << "t=0 0\r\n" + << "m=video 3457 RTP/SAVPF 120\r\n" + << "b=AS:" << value << "\r\n"; + return sb.Release(); + }; + + // kSdpBandwidthNegativeOne + SdpDeserialize(get_sdp("-1")); + EXPECT_METRIC_EQ(1, metrics::NumEvents("WebRTC.PeerConnection.SdpBandwidth", + kSdpBandwidthNegativeOne)); + + // kSdpBandwidthZero + SdpDeserialize(get_sdp("0")); + EXPECT_METRIC_EQ(1, metrics::NumEvents("WebRTC.PeerConnection.SdpBandwidth", + kSdpBandwidthZero)); + + // kSdpBandwidthSmall + SdpDeserialize(get_sdp("1000")); + EXPECT_METRIC_EQ(1, metrics::NumEvents("WebRTC.PeerConnection.SdpBandwidth", + kSdpBandwidthSmall)); + + // kSdpBandwidthLarge + SdpDeserialize(get_sdp("3000000")); + EXPECT_METRIC_EQ(1, metrics::NumEvents("WebRTC.PeerConnection.SdpBandwidth", + kSdpBandwidthLarge)); + + // kSdpBandwidthParseFailure + SdpDeserialize(get_sdp("999999999999")); + EXPECT_METRIC_EQ(1, metrics::NumEvents("WebRTC.PeerConnection.SdpBandwidth", + kSdpBandwidthParseFailure)); + + EXPECT_METRIC_EQ(5, + metrics::NumSamples("WebRTC.PeerConnection.SdpBandwidth")); +} + // Test that "ufrag"/"pwd" in the candidate line itself are ignored, and only // the "a=ice-ufrag"/"a=ice-pwd" attributes are used. // Regression test for: