shill: fix Network.Shill.Wifi.TimeToJoin metric
The Network.Shill.Wifi.TimeToJoin metric was not being collected, due
to subtleties of how it was supposed to be hooked in to Metrics. So
fix it.
More specifically, What's supposed to happen is this:
Metrics::RegisterService
-> WiFiService::InitializeCustomMetrics // overrides base class impl
-> Metrics::AddServiceStateTransitionTimer(...)
The problem was that Metrics::RegisterService is called from
Service::Service. Thus, at the time that Metrics::RegisterService is called,
we only have a Service object, not a WiFiService object. Consequently, the
virtual method call from Manager::RegisterService resolves to the base class
implementation of InitializeCustomMetrics, which has an empty body.
To fix this, we make InitializeCustomMetrics a non-virtual method (it
only had a meaningful implementation in WiFiService, anyway), and call
it explicitly from the WiFiService ctor, rather than implicitly via
Metrics::RegisterService.
While there:
- Remove RegisterService calls in MetricsTests. These are both
unnecessary, and actively harmful. The harm comes from the fact
that the explicit call to RegisterService in the unit tests overrides
the effect of the RegisterService in the Service.
In the case of WiFiService, this meant calling the derived-class
implementation of InitializeCustomMetrics. That meant the test passed,
but only because it did not reflect actual program behavior.
- Log a WARNING if a Service is re-registered with Metrics (without an
intervening call to DeregisterService).
- Remove unused |service| field of ServiceMetrics.
- Add parenthesis to ctor invocation in Metrics::RegisterService. I
believe the fields are non-POD, and initialized either way (with or
without parentheses). But it seems clearer to use the parentheses,
since we want the fields initialized.
- Fix whitespace issue in WiFiService::InitializeCustomMetrics.
- Tweak log message in service.cc.
BUG=chromium:268058
TEST=new unit test, manual
Manual testing
--------------
1. boot system
2. connect to wifi
3. open chrome://histograms
4. find that Network.Shill.Wifi.TimeToJoin exists, and has at least one sample
Change-Id: Icd925148415ea30fb63709bf034e63b8930c179c
Reviewed-on: https://gerrit.chromium.org/gerrit/65739
Tested-by: mukesh agrawal <quiche@chromium.org>
Reviewed-by: Paul Stewart <pstew@chromium.org>
Commit-Queue: mukesh agrawal <quiche@chromium.org>
diff --git a/metrics.h b/metrics.h
index 91db769..18471a4 100644
--- a/metrics.h
+++ b/metrics.h
@@ -454,10 +454,9 @@
// Tracks the time it takes |service| to go from |start_state| to
// |stop_state|. When |stop_state| is reached, the time is sent to UMA.
- void AddServiceStateTransitionTimer(const Service *service,
- const std::string &histogram_name,
- Service::ConnectState start_state,
- Service::ConnectState stop_state);
+ virtual void AddServiceStateTransitionTimer(
+ const Service *service, const std::string &histogram_name,
+ Service::ConnectState start_state, Service::ConnectState stop_state);
// Specializes |metric_name| for the specified |technology_id|.
std::string GetFullMetricName(const char *metric_name,
@@ -599,10 +598,6 @@
typedef std::map<Service::ConnectState, TimerReportersList>
TimerReportersByState;
struct ServiceMetrics {
- ServiceMetrics() : service(NULL) {}
- // The service is registered/deregistered in the Service
- // constructor/destructor, therefore there is no need to keep a ref count.
- const Service *service;
// All TimerReporter objects are stored in |timers| which owns the objects.
// |start_on_state| and |stop_on_state| contain pointers to the
// TimerReporter objects and control when to start and stop the timers.