Add metric for broken DNS configuration.
Attempt a DNS test using Google's public DNS server if portal detection
failed due to DNS failure, and report the result of the DNS test
to UMA metric.
BUG=chromium:366301
TEST=unit tests, manual
Manual Test:
1. Connect a chrome device to an AP with no internet access,
then browse to "chrome://histograms" and verify there is a
histogram for "Network.Shill.DNSTestResult" with a hit for
value 1 (Failure).
2. Manually update the code to perform DNS test when portal detection
succeed. Connect a chrome device to "GoogleGuest", and verify
there is a hit for value 0 (Success) for the histogram
"Network.Shill.DNSTestResult".
Change-Id: I8cb22f7664fcfa7fd08d3d3dd24902f7896a4e3e
Reviewed-on: https://chromium-review.googlesource.com/199174
Reviewed-by: Peter Qiu <zqiu@chromium.org>
Commit-Queue: Peter Qiu <zqiu@chromium.org>
Tested-by: Peter Qiu <zqiu@chromium.org>
diff --git a/device.cc b/device.cc
index bab0076..3482747 100644
--- a/device.cc
+++ b/device.cc
@@ -25,6 +25,7 @@
#include "shill/dhcp_config.h"
#include "shill/dhcp_provider.h"
#include "shill/dns_client.h"
+#include "shill/dns_client_factory.h"
#include "shill/error.h"
#include "shill/event_dispatcher.h"
#include "shill/geolocation_info.h"
@@ -75,6 +76,16 @@
const char Device::kStorageReceiveByteCount[] = "ReceiveByteCount";
// static
const char Device::kStorageTransmitByteCount[] = "TransmitByteCount";
+// static
+const char Device::kFallbackDnsTestHostname[] = "www.gstatic.com";
+// static
+const char* Device::kFallbackDnsServers[] = {
+ "8.8.8.8",
+ "8.8.8.4"
+};
+
+// static
+const int Device::kDNSTimeoutMilliseconds = 5000;
Device::Device(ControlInterface *control_interface,
EventDispatcher *dispatcher,
@@ -99,8 +110,11 @@
manager_(manager),
weak_ptr_factory_(this),
adaptor_(control_interface->CreateDeviceAdaptor(this)),
+ dns_client_factory_(DNSClientFactory::GetInstance()),
portal_detector_callback_(Bind(&Device::PortalDetectorCallback,
weak_ptr_factory_.GetWeakPtr())),
+ dns_client_callback_(Bind(&Device::DNSClientCallback,
+ weak_ptr_factory_.GetWeakPtr())),
technology_(technology),
portal_attempts_to_online_(0),
receive_byte_offset_(0),
@@ -867,6 +881,39 @@
manager_->ReportServicesOnSameNetwork(connection_id);
}
+void Device::PerformFallbackDNSTest() {
+ // Return if DNS client already running.
+ if(fallback_dns_test_client_.get()) {
+ return;
+ }
+
+ // Send DNS request to Google's DNS server.
+ vector<string> dns_servers(std::begin(kFallbackDnsServers),
+ std::end(kFallbackDnsServers));
+ fallback_dns_test_client_.reset(
+ dns_client_factory_->CreateDNSClient(IPAddress::kFamilyIPv4,
+ connection_->interface_name(),
+ dns_servers,
+ kDNSTimeoutMilliseconds,
+ dispatcher_,
+ dns_client_callback_));
+ Error error;
+ if (!fallback_dns_test_client_->Start(kFallbackDnsTestHostname, &error)) {
+ LOG(ERROR) << __func__ << ": Failed to start DNS client "
+ << error.message();
+ fallback_dns_test_client_.reset();
+ }
+}
+
+void Device::DNSClientCallback(const Error &error, const IPAddress& ip) {
+ int result = Metrics::kDNSTestResultFailure;
+ if (error.IsSuccess()) {
+ result = Metrics::kDNSTestResultSuccess;
+ }
+ metrics()->NotifyFallbackDNSTestResult(result);
+ fallback_dns_test_client_.reset();
+}
+
void Device::SetServiceConnectedState(Service::ConnectState state) {
DCHECK(selected_service_.get());
@@ -922,10 +969,11 @@
portal_attempts_to_online_ += result.num_attempts;
+ int portal_status = Metrics::PortalDetectionResultToEnum(result);
metrics()->SendEnumToUMA(
metrics()->GetFullMetricName(Metrics::kMetricPortalResultSuffix,
technology()),
- Metrics::PortalDetectionResultToEnum(result),
+ portal_status,
Metrics::kPortalResultMax);
if (result.status == PortalDetector::kStatusSuccess) {
@@ -948,6 +996,14 @@
Metrics::kMetricPortalAttemptsMin,
Metrics::kMetricPortalAttemptsMax,
Metrics::kMetricPortalAttemptsNumBuckets);
+
+ // Perform fallback DNS test if the portal failure is DNS related.
+ // The test will send a DNS request to Google's DNS server to determine
+ // if the DNS failure is due to bad DNS server settings.
+ if ((portal_status == Metrics::kPortalResultDNSFailure) ||
+ (portal_status == Metrics::kPortalResultDNSTimeout)) {
+ PerformFallbackDNSTest();
+ }
}
}
diff --git a/device.h b/device.h
index 95dec9f..34500aa 100644
--- a/device.h
+++ b/device.h
@@ -30,6 +30,8 @@
class ControlInterface;
class DHCPProvider;
class DeviceAdaptorInterface;
+class DNSClient;
+class DNSClientFactory;
class Endpoint;
class Error;
class EventDispatcher;
@@ -493,6 +495,9 @@
static const char kStoragePowered[];
static const char kStorageReceiveByteCount[];
static const char kStorageTransmitByteCount[];
+ static const char kFallbackDnsTestHostname[];
+ static const char* kFallbackDnsServers[];
+ static const int kDNSTimeoutMilliseconds;
// Configure static IP address parameters if the service provides them.
void ConfigureStaticIPTask();
@@ -526,6 +531,12 @@
// Emit a property change signal for the "IPConfigs" property of this device.
void UpdateIPConfigsProperty();
+ // Perform fallback DNS test by sending DNS request to Google's DNS server.
+ void PerformFallbackDNSTest();
+
+ // Callback for DNS Client.
+ void DNSClientCallback(const Error &error, const IPAddress &ip);
+
// |enabled_persistent_| is the value of the Powered property, as
// read from the profile. If it is not found in the profile, it
// defaults to true. |enabled_| reflects the real-time state of
@@ -574,8 +585,14 @@
scoped_ptr<DeviceAdaptorInterface> adaptor_;
scoped_ptr<PortalDetector> portal_detector_;
scoped_ptr<LinkMonitor> link_monitor_;
+ // DNS Client for performing fallback DNS test when portal detector failed
+ // due to DNS failure.
+ scoped_ptr<DNSClient> fallback_dns_test_client_;
+ DNSClientFactory *dns_client_factory_;
base::Callback<void(const PortalDetector::Result &)>
portal_detector_callback_;
+ base::Callback<void(const Error &, const IPAddress &)>
+ dns_client_callback_;
Technology::Identifier technology_;
// The number of portal detection attempts from Connected to Online state.
// This includes all failure/timeout attempts and the final successful
diff --git a/device_unittest.cc b/device_unittest.cc
index 37605cf..468408b 100644
--- a/device_unittest.cc
+++ b/device_unittest.cc
@@ -32,6 +32,8 @@
#include "shill/mock_device_info.h"
#include "shill/mock_dhcp_config.h"
#include "shill/mock_dhcp_provider.h"
+#include "shill/mock_dns_client.h"
+#include "shill/mock_dns_client_factory.h"
#include "shill/mock_glib.h"
#include "shill/mock_ip_address_store.h"
#include "shill/mock_ipconfig.h"
@@ -866,6 +868,7 @@
SelectService(service_);
SetConnection(connection_.get());
device_->portal_detector_.reset(portal_detector_); // Passes ownership.
+ device_->dns_client_factory_ = MockDNSClientFactory::GetInstance();
SetManager(&manager_);
}
@@ -893,6 +896,23 @@
void ExpectPortalDetectorIsMock() {
EXPECT_EQ(portal_detector_, device_->portal_detector_.get());
}
+ void ExpectDNSClientReset() {
+ EXPECT_FALSE(device_->fallback_dns_test_client_.get());
+ }
+ void ExpectDNSClientSet() {
+ EXPECT_TRUE(device_->fallback_dns_test_client_.get());
+ }
+ void ClearDNSClient() {
+ device_->fallback_dns_test_client_.reset();
+ }
+ void InvokeDNSClientCallback(bool result) {
+ Error error;
+ if (!result) {
+ error.CopyFrom(Error(Error::kOperationFailed, ""));
+ }
+ IPAddress address(IPAddress::kFamilyUnknown);
+ device_->DNSClientCallback(error, address);
+ }
scoped_refptr<MockConnection> connection_;
StrictMock<MockManager> manager_;
scoped_refptr<MockService> service_;
@@ -1206,6 +1226,106 @@
ExpectPortalDetectorReset();
}
+TEST_F(DevicePortalDetectionTest, PortalDetectionDNSFailure) {
+ MockDNSClientFactory *dns_client_factory
+ = MockDNSClientFactory::GetInstance();
+ MockDNSClient * dns_client = new MockDNSClient();
+
+ const string kInterfaceName("int0");
+ EXPECT_CALL(*connection_.get(), interface_name())
+ .WillRepeatedly(ReturnRef(kInterfaceName));
+ const string portal_url;
+ EXPECT_CALL(manager_, GetPortalCheckURL())
+ .WillRepeatedly(ReturnRef(portal_url));
+
+ // DNS Failure, DNS Client started successfully.
+ EXPECT_CALL(*service_.get(), IsConnected())
+ .WillOnce(Return(true));
+ EXPECT_CALL(*service_.get(), SetState(Service::kStatePortal));
+ EXPECT_CALL(*connection_.get(), is_default())
+ .WillOnce(Return(false));
+ EXPECT_CALL(*dns_client_factory, CreateDNSClient(_,_,_,_,_,_))
+ .WillOnce(Return(dns_client));
+ EXPECT_CALL(*dns_client, Start(_, _))
+ .WillOnce(Return(true));
+ PortalDetectorCallback(PortalDetector::Result(
+ PortalDetector::kPhaseDNS,
+ PortalDetector::kStatusFailure,
+ kPortalAttempts,
+ true));
+ ExpectDNSClientSet();
+ ClearDNSClient();
+ Mock::VerifyAndClearExpectations(dns_client_factory);
+ Mock::VerifyAndClearExpectations(dns_client);
+
+ // DNS Timeout, DNS Client started successfully.
+ dns_client = new MockDNSClient();
+ EXPECT_CALL(*service_.get(), IsConnected())
+ .WillOnce(Return(true));
+ EXPECT_CALL(*service_.get(), SetState(Service::kStatePortal));
+ EXPECT_CALL(*connection_.get(), is_default())
+ .WillOnce(Return(false));
+ EXPECT_CALL(*dns_client_factory, CreateDNSClient(_,_,_,_,_,_))
+ .WillOnce(Return(dns_client));
+ EXPECT_CALL(*dns_client, Start(_, _))
+ .WillOnce(Return(true));
+ PortalDetectorCallback(PortalDetector::Result(
+ PortalDetector::kPhaseDNS,
+ PortalDetector::kStatusTimeout,
+ kPortalAttempts,
+ true));
+ ExpectDNSClientSet();
+ ClearDNSClient();
+ Mock::VerifyAndClearExpectations(dns_client_factory);
+ Mock::VerifyAndClearExpectations(dns_client);
+
+ // DNS Failure, DNS Client failed to start.
+ dns_client = new MockDNSClient();
+ EXPECT_CALL(*service_.get(), IsConnected())
+ .WillOnce(Return(true));
+ EXPECT_CALL(*service_.get(), SetState(Service::kStatePortal));
+ EXPECT_CALL(*connection_.get(), is_default())
+ .WillOnce(Return(false));
+ EXPECT_CALL(*dns_client_factory, CreateDNSClient(_,_,_,_,_,_))
+ .WillOnce(Return(dns_client));
+ EXPECT_CALL(*dns_client, Start(_, _))
+ .WillOnce(Return(false));
+ PortalDetectorCallback(PortalDetector::Result(
+ PortalDetector::kPhaseDNS,
+ PortalDetector::kStatusFailure,
+ kPortalAttempts,
+ true));
+ ExpectDNSClientReset();
+ ClearDNSClient();
+ Mock::VerifyAndClearExpectations(dns_client_factory);
+ Mock::VerifyAndClearExpectations(dns_client);
+
+ // Other Failure, DNS test not triggered.
+ EXPECT_CALL(*service_.get(), IsConnected())
+ .WillOnce(Return(true));
+ EXPECT_CALL(*service_.get(), SetState(Service::kStatePortal));
+ EXPECT_CALL(*connection_.get(), is_default())
+ .WillOnce(Return(false));
+ EXPECT_CALL(*dns_client_factory, CreateDNSClient(_,_,_,_,_,_)).Times(0);
+ PortalDetectorCallback(PortalDetector::Result(
+ PortalDetector::kPhaseConnection,
+ PortalDetector::kStatusFailure,
+ kPortalAttempts,
+ true));
+ Mock::VerifyAndClearExpectations(dns_client_factory);
+}
+
+TEST_F(DevicePortalDetectionTest, DNSClientCallback) {
+ EXPECT_CALL(metrics_,
+ NotifyFallbackDNSTestResult(Metrics::kDNSTestResultFailure)).Times(1);
+ InvokeDNSClientCallback(false);
+ Mock::VerifyAndClearExpectations(&metrics_);
+
+ EXPECT_CALL(metrics_,
+ NotifyFallbackDNSTestResult(Metrics::kDNSTestResultSuccess)).Times(1);
+ InvokeDNSClientCallback(true);
+}
+
class DeviceByteCountTest : public DeviceTest {
public:
DeviceByteCountTest()
diff --git a/metrics.cc b/metrics.cc
index 23c0eb6..a6c91c9 100644
--- a/metrics.cc
+++ b/metrics.cc
@@ -289,15 +289,21 @@
const char Metrics::kMetricUserInitiatedEvents[] =
"Network.Shill.UserInitiatedEvents";
+// static
const char Metrics::kMetricWifiTxBitrate[] =
"Network.Shill.WiFi.TransmitBitrateMbps";
const int Metrics::kMetricWifiTxBitrateMax = 7000;
const int Metrics::kMetricWifiTxBitrateMin = 1;
const int Metrics::kMetricWifiTxBitrateNumBuckets = 100;
+// static
const char Metrics::kMetricWifiUserInitiatedConnectionResult[] =
"Network.Shill.WiFi.UserInitiatedConnectionResult";
+// static
+const char Metrics::kMetricFallbackDNSTestResult[] =
+ "Network.Shill.FallbackDNSTestResult";
+
Metrics::Metrics(EventDispatcher *dispatcher)
: dispatcher_(dispatcher),
library_(&metrics_library_),
@@ -1122,6 +1128,12 @@
kUserInitiatedConnectionResultMax);
}
+void Metrics::NotifyFallbackDNSTestResult(int result) {
+ SendEnumToUMA(kMetricFallbackDNSTestResult,
+ result,
+ kDNSTestResultMax);
+}
+
bool Metrics::SendEnumToUMA(const string &name, int sample, int max) {
SLOG(Metrics, 5)
<< "Sending enum " << name << " with value " << sample << ".";
diff --git a/metrics.h b/metrics.h
index 4306388..58ea4fd 100644
--- a/metrics.h
+++ b/metrics.h
@@ -285,6 +285,12 @@
kUserInitiatedConnectionResultMax
};
+ enum DNSTestResult {
+ kDNSTestResultSuccess = 0,
+ kDNSTestResultFailure,
+ kDNSTestResultMax
+ };
+
static const char kMetricDisconnectSuffix[];
static const int kMetricDisconnectMax;
static const int kMetricDisconnectMin;
@@ -485,6 +491,9 @@
// User-initiated wifi connection attempt result.
static const char kMetricWifiUserInitiatedConnectionResult[];
+ // DNS test result.
+ static const char kMetricFallbackDNSTestResult[];
+
explicit Metrics(EventDispatcher *dispatcher);
virtual ~Metrics();
@@ -671,6 +680,9 @@
// Notifies this object about user-initiated event.
virtual void NotifyUserInitiatedEvent(int event);
+ // Notifies this object about the result of the fallback DNS test.
+ virtual void NotifyFallbackDNSTestResult(int result);
+
// Sends linear histogram data to UMA.
virtual bool SendEnumToUMA(const std::string &name, int sample, int max);
diff --git a/metrics_unittest.cc b/metrics_unittest.cc
index ae9329c..a32b2fc 100644
--- a/metrics_unittest.cc
+++ b/metrics_unittest.cc
@@ -791,6 +791,14 @@
Metrics::kUserInitiatedConnectionResultSuccess);
}
+TEST_F(MetricsTest, NotifyFallbackDNSTestResult) {
+ EXPECT_CALL(library_,
+ SendEnumToUMA(Metrics::kMetricFallbackDNSTestResult,
+ Metrics::kDNSTestResultSuccess,
+ Metrics::kDNSTestResultMax));
+ metrics_.NotifyFallbackDNSTestResult(Metrics::kDNSTestResultSuccess);
+}
+
#ifndef NDEBUG
typedef MetricsTest MetricsDeathTest;
diff --git a/mock_metrics.h b/mock_metrics.h
index 88d166b..e050a59 100644
--- a/mock_metrics.h
+++ b/mock_metrics.h
@@ -49,6 +49,7 @@
MOCK_METHOD1(NotifyWifiTxBitrate, void(int bitrate));
MOCK_METHOD2(NotifyUserInitiatedConnectionResult,
void(const std::string &name, int result));
+ MOCK_METHOD1(NotifyFallbackDNSTestResult, void(int result));
private:
DISALLOW_COPY_AND_ASSIGN(MockMetrics);