RAPPOR implementation

components/rappor/rappor_service.h

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_RAPPOR_RAPPOR_SERVICE_H_
+#define COMPONENTS_RAPPOR_RAPPOR_SERVICE_H_
+
+#include <string>
+
+#include "base/basictypes.h"
+#include "base/lazy_instance.h"

[Ilya Sherman, 2014/02/13 01:39:03]

nit: This doesn't seem like it's used.

[Steven Holte, 2014/02/13 05:11:12]

Done.

+#include "base/memory/weak_ptr.h"

[Ilya Sherman, 2014/02/13 01:39:03]

nit: This doesn't seem like it's used.

[Steven Holte, 2014/02/13 05:11:12]

Done.

+#include "base/prefs/pref_service.h"
+#include "base/time/time.h"
+#include "base/timer/timer.h"
+#include "components/rappor/log_uploader.h"
+#include "components/rappor/proto/rappor_metric.pb.h"
+#include "components/rappor/rappor_metric.h"
+
+class PrefRegistrySimple;
+
+namespace rappor {
+
+// The type of data stored in a metric.
+enum RapporType {
+  ETLD_PLUS_ONE_RAPPOR_TYPE = 0,
+  NUM_RAPPOR_TYPES
+};
+
+// This class provides an interface for recording samples for rappor metrics,
+// and periodically generates and uploads reports based on the collected data.
+class RapporService {
+ public:
+  RapporService();
+  virtual ~RapporService();
+
+  // Starts the periodic generation of reports and upload attempts.
+  void Start(PrefService* pref_service, net::URLRequestContextGetter* context);
+
+  // Records a sample of the rappor metric specified by |metric_name|.
+  // Creates and initializes the metric, if it doesn't yet exist.
+  void RecordSample(const std::string& metric_name,
+                    RapporType type,
+                    const std::string& sample);
+
+  // Sets the cohort value.  For use by tests only.
+  void SetCohortForTesting(uint32_t cohort) { cohort_ = cohort; }
+
+  // Sets the secret value.  For use by tests only.
+  void SetSecretForTesting(std::string secret) { secret_ = secret; }

[Ilya Sherman, 2014/02/13 01:39:03]

nit: Pass by const-reference.

[Ilya Sherman, 2014/02/13 01:39:03]

Optional nit: I prefer to restrict visibility of for-testing methods to
protected, rather than private.  However, the "ForTesting" suffix is sufficient,
so if you prefer the public visibility, I'm ok with it.

[Steven Holte, 2014/02/13 05:11:12]

Leaving it public.

[Steven Holte, 2014/02/13 05:11:12]

Done.

+
+  // Registers the names of all of the preferences used by RapporService in the
+  // provided PrefRegistry. This should be called before calling Start().
+  static void RegisterPrefs(PrefRegistrySimple* registry);
+
+ protected:
+  // Logs all of the collected metrics to the reports proto message.  Exposed
+  // for tests.  Returns true if any metrics were recorded.
+  bool ExportMetrics(RapporReports* reports);

[Ilya Sherman, 2014/02/13 01:39:03]

nit: Where is the "RapporReports" type declared?  I'm not sure what datatype
this is.

[Steven Holte, 2014/02/13 05:11:12]

It's the proto/rappor_metric.pb.h type.

+
+  // Records a sample of the rappor metric specified by |parameters|.
+  // Creates and initializes the metric, if it doesn't yet exist.

[Ilya Sherman, 2014/02/13 01:39:03]

nit: Exposed for tests?

[Steven Holte, 2014/02/13 05:11:12]

Done.

+  void RecordSampleInternal(const std::string& metric_name,
+                            const RapporParameters& parameters,
+                            const std::string& sample);
+
+ private:
+  // Check if the service has been started successfully.
+  bool IsInitialized() const;
+
+  // Retrieves the cohort number this client was assigned to, generating it if
+  // doesn't already exist.  The cohort should be persistent.
+  void LoadCohort(PrefService* pref_service);
+
+  // Retrieves the value for secret_ from preferences, generating it if doesn't
+  // already exist.  The secret should be persistent, so that additional bits
+  // from the client do not get exposed over time.
+  void LoadSecret(PrefService* pref_service);
+
+  // Called whenever the logging interval elapses to generate a new log of
+  // reports and pass it to the uploader.
+  void OnLogInterval();
+
+  // Finds a metric in the metrics_map_, creating it if it doesn't already
+  // exist.
+  RapporMetric* LookupMetric(const std::string& metric_name,

[Ilya Sherman, 2014/02/13 01:39:03]

nit: Lookup -> LookUp (noun vs. verb)

[Steven Holte, 2014/02/13 05:11:12]

Done.

+                             const RapporParameters& parameters);
+
+  // Client-side secret used to generate fake bits.
+  std::string secret_;
+
+  // The cohort this client is assigned to. -1 is uninitialized.
+  int32_t cohort_;
+
+  // Timer which schedules calls to OnLogInterval()
+  base::OneShotTimer<RapporService> log_rotation_timer_;
+
+  // A private LogUploader instance for sending reports to the server.
+  scoped_ptr<LogUploader> uploader_;

[Ilya Sherman, 2014/02/13 01:39:03]

nit: Can this be a direct class member, rather than a scoped_ptr?

[Steven Holte, 2014/02/13 05:11:12]

No, it's constructor requires the request_context passed to Start.

+
+  // We keep all registered histograms in a map, from name to histogram.

[Ilya Sherman, 2014/02/13 01:39:03]

nit: "histogram" -> "metric"?

[Steven Holte, 2014/02/13 05:11:12]

Done.

+  std::map<std::string, RapporMetric*> metrics_map_;

[Ilya Sherman, 2014/02/13 01:39:03]

Please document that the map owns the metrics it contains.

[Steven Holte, 2014/02/13 05:11:12]

Done.

+
+  // Lock protects access to above map.
+  base::Lock lock_;

[Ilya Sherman, 2014/02/13 01:39:03]

IMPORTANT: Hmm, why does the map need a lock?  This class is unlikely to be
thread-safe.

[Steven Holte, 2014/02/13 05:11:12]

I'm not sure what thread the Timer executes it's callback on, but if it
shouldn't actually matter for measurements we are taking now, since
PrefMetricsService records it's sample shortly after Start().  I'll remove the
lock for now, since it's causing problems in the try jobs anyway.

[Ilya Sherman, 2014/02/13 23:23:08]

The Timer runs on whatever thread it's instantiated on.  It's important to keep
in mind that most Chromium code is intentionally not thread-safe, to reduce
complexity.  Except in cases where performance really matters, we prefer message
passing, which copies data around between threads.  (No action to take on this
comment, just useful info for hacking on Chromium.)

+
+  DISALLOW_COPY_AND_ASSIGN(RapporService);
+};
+
+}  // namespace rappor
+
+#endif  // COMPONENTS_RAPPOR_RAPPOR_SERVICE_H_