[OBSOLETE] Reporting: Initial implementation.

net/reporting/reporting_service.h

Index: net/reporting/reporting_service.h
diff --git a/net/reporting/reporting_service.h b/net/reporting/reporting_service.h
new file mode 100644
index 0000000000000000000000000000000000000000..0b1991e7334833bc186100a12be489233161070f
--- /dev/null
+++ b/net/reporting/reporting_service.h
@@ -0,0 +1,173 @@
+// Copyright 2016 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 NET_REPORTING_REPORTING_SERVICE_H_
+#define NET_REPORTING_REPORTING_SERVICE_H_
+
+#include <map>
+#include <unordered_set>
+
+#include "base/callback.h"
+#include "base/macros.h"
+#include "base/time/tick_clock.h"
+#include "base/time/time.h"
+#include "base/values.h"
+#include "net/base/backoff_entry.h"
+#include "net/base/net_export.h"
+#include "net/reporting/reporting_metrics.h"
+#include "net/reporting/reporting_report.h"
+#include "net/reporting/reporting_uploader.h"
+#include "net/url_request/url_request_context_getter.h"
+#include "url/gurl.h"
+
+namespace net {
+
+class NET_EXPORT ReportingService {

[Ryan Sleevi, 2017/01/03 21:28:13]

High level Design remarks:
1) This feels like a lot of the implementation details have bled into the public
header - meaning anytime the implementatino changes, that's sort of spread
through to every consumer.

Concrete Suggestions:
a) Forward declare whenever possible
b) Carefully evaluate whether you need a private class member function versus a
function within an unnamed namespace
c) Carefully evaluate whether this class is doing "too much"

2) The testing seems to be litering *ForTesting functions in to the public
interface. While a presubmit helps minimize that, it feels like it might result
in a more carefully balanced operation if you make use of text fixtures, rather
than public interface.
  a) Similarly, if you're needing that much of your "implementation detail" to
be provided via ForTesting() methods, it may be a sign that your layering or
design is trying to do too much, and would benefit from more decoupling.

[Julia Tuttle, 2017/01/25 20:27:46]

My original design had a separate "ReportingCache" class that just did the data
storage, so it was easier for tests to check the effect of parsing headers or
queueing reports. Maybe I should go back to that design.

+ public:
+  struct NET_EXPORT Policy {
+    // Time to keep an unused endpoint around, or zero for no limit.
+    base::TimeDelta endpoint_lifetime;
+    // Exponential backoff policy for uploading to endpoints.
+    BackoffEntry::Policy endpoint_backoff;
+    // Maximum number of failures before discarding an endpoint (once the
+    // BackoffEntry is okay with it), or -1 for no limit.
+    int max_endpoint_failures;
+    // Maximum number of endpoints to keep around, or 0u for no limit.
+    size_t max_endpoint_count;
+
+    // Time to keep a queued report around, or zero for no limit.
+    base::TimeDelta report_lifetime;
+    // Maximum number of failed delivery attempts before discarding a report, or
+    // -1 for no limit.
+    int max_report_failures;
+    // Maximum number of queued reports to keep around, or 0u for no limit.
+    size_t max_report_count;
+
+    // Whether to persist the report queue across network changes or not.
+    bool persist_reports_across_network_changes;
+
+    static Policy GetDefault();

[Ryan Sleevi, 2017/01/03 21:28:13]

DESIGN: Why is an explicit GetDefault required? Why can't default constructing
the object return a sane default?

[Julia Tuttle, 2017/01/25 20:27:46]

Done.

+  };
+
+  ReportingService(const Policy& policy);
+  ~ReportingService();
+
+  void set_uploader(std::unique_ptr<ReportingUploader> uploader);

[Ryan Sleevi, 2017/01/03 21:28:13]

This seems to have more subtlety than just "set_uploader"

[Julia Tuttle, 2017/01/25 20:27:46]

Not really; it's just a separate setter instead of a constructor parameter so it
can be cleared early in profile destruction to avoid circular dependencies like
the mess we had with Domain Reliability.

+  void QueueReport(std::unique_ptr<base::Value> body,
+                   const GURL& url,
+                   const GURL& origin,
+                   const std::string& group,
+                   const std::string& type);
+  void ProcessHeader(const GURL& origin, const std::string& header_value);
+  void SendReports();
+
+  void set_clock_for_testing(std::unique_ptr<base::TickClock> clock);

[Ryan Sleevi, 2017/01/03 21:28:13]

SetClockForTesting

[Julia Tuttle, 2017/01/25 20:27:46]

Why? It is actually just a plain setter under the hood.

+  bool HasEndpointForTesting(const GURL& endpoint_url);
+  bool HasClientForTesting(const GURL& endpoint_url, const GURL& origin_url);
+  int GetEndpointFailuresForTesting(const GURL& endpoint_url);
+  void CollectGarbageForTesting();
+
+ private:
+  // Per-origin configuration and state for an endpoint.
+  struct Client {

[Ryan Sleevi, 2017/01/03 21:28:13]

Why can't you forward declare this?

[Julia Tuttle, 2017/01/25 20:27:46]

Done.

+   public:
+    Client(const GURL& origin,
+           bool subdomains,
+           const std::string& group,
+           base::TimeDelta ttl,
+           base::TimeTicks creation);
+
+    GURL origin;
+    bool subdomains;
+    std::string group;
+    base::TimeDelta ttl;
+    base::TimeTicks creation;
+
+    bool is_expired(base::TimeTicks now) const { return creation + ttl < now; }
+  };
+
+  // An endpoint to which one or more origins (represented by clients) want to
+  // upload reports.
+  struct Endpoint {

[Ryan Sleevi, 2017/01/03 21:28:13]

Why can't you forward declare this?

[Julia Tuttle, 2017/01/25 20:27:46]

Done.

+   public:
+    Endpoint(const GURL& url,
+             const BackoffEntry::Policy& backoff_policy,
+             base::TickClock* clock);
+    ~Endpoint();
+
+    const GURL url;
+
+    BackoffEntry backoff;
+    // For LRU eviction of endpoints.
+    base::TimeTicks last_used;
+    // Whether we currently have an upload in progress to this endpoint.
+    bool pending;
+
+    // Map from client.origin to client.
+    std::map<GURL, Client> clients;
+
+    bool is_expired(base::TimeTicks now) const;
+  };
+
+  // The parsed data from a header representing a single endpoint configuration.
+  struct EndpointTuple {

[Ryan Sleevi, 2017/01/03 21:28:13]

Why can't you forward declare this?

[Julia Tuttle, 2017/01/25 20:27:46]

Done.

+    GURL url;
+    bool subdomains;
+    base::TimeDelta ttl;
+    std::string group;
+
+    static bool FromDictionary(const base::DictionaryValue& dictionary,
+                               EndpointTuple* tuple_out,
+                               std::string* error_out);
+    static bool FromHeader(const std::string& header,
+                           std::vector<EndpointTuple>* tuples_out,
+                           std::vector<std::string>* errors_out);
+
+    std::string ToString() const;
+  };
+
+  struct Delivery {

[Ryan Sleevi, 2017/01/03 21:28:13]

Why can't you forward declare this?

[Julia Tuttle, 2017/01/25 20:27:46]

Done.

+    Delivery(const GURL& endpoint_url,
+             const std::vector<ReportingReport*>& reports);
+    ~Delivery();
+
+    const GURL& endpoint_url;
+    const std::vector<ReportingReport*> reports;
+  };
+
+  using EndpointMap = std::map<GURL, std::unique_ptr<Endpoint>>;
+  using ReportVector = std::vector<std::unique_ptr<ReportingReport>>;
+
+  void ProcessEndpointTuple(const GURL& origin, const EndpointTuple& tuple);
+
+  void OnDeliveryAttemptComplete(const std::unique_ptr<Delivery>& delivery,
+                                 ReportingUploader::Outcome outcome);
+
+  void CollectGarbage();
+
+  Endpoint* FindEndpointForReport(const ReportingReport& report);
+  bool DoesEndpointMatchReport(const Endpoint& endpoint,
+                               const ReportingReport& report);
+  std::string SerializeReports(const std::vector<ReportingReport*>& reports);
+
+  Endpoint* GetEndpointByURL(const GURL& url);
+  void DequeueReport(ReportingReport* report);
+
+  void HistogramHeaderEndpointInternal(bool endpoint_exists,
+                                       bool client_exists,
+                                       base::TimeDelta ttl) const;
+  void HistogramReportInternal(ReportFate fate,
+                               const ReportingReport& report) const;
+
+  Policy policy_;
+  std::unique_ptr<base::TickClock> clock_;
+  std::unique_ptr<ReportingUploader> uploader_;
+
+  ReportVector reports_;
+  EndpointMap endpoints_;
+};
+
+}  // namespace net
+
+#endif  // COMPONENTS_REPORTING_REPORTING_SERVICE_H_