Make trace/ Exporter API consistent with the stats/ one. (#9)

Also:
- Add sampler_test.
- Replace Span's default constructor with BlankSpan().
- Add BlankSpan test.
- Add FullSpanTest that verifies SpanData.
- Initialize threads after the rest of the exporter, to avoid racing.

opencensus/stats/internal/stats_exporter.cc

@@ -77,13 +77,13 @@ class StatsExporterImpl {
   }
 
   const absl::Duration export_interval_ = absl::Seconds(10);
-  std::thread t_;
 
   mutable absl::Mutex mu_;
 
   std::vector<std::unique_ptr<StatsExporter::Handler>> handlers_
       GUARDED_BY(mu_);
   std::unordered_map<std::string, std::unique_ptr<View>> views_ GUARDED_BY(mu_);
+  std::thread t_;
 };
 
 void StatsExporter::AddView(const ViewDescriptor& view) {

opencensus/stats/stats_exporter.h

@@ -43,8 +43,7 @@ class StatsExporter final {
   // export to) and calls StatsExporter::RegisterHandler itself.
   class Handler {
    public:
-    virtual ~Handler() {}
-
+    virtual ~Handler() = default;
     virtual void ExportViewData(const ViewDescriptor& descriptor,
                                 const ViewData& data) = 0;
   };

opencensus/trace/BUILD

@@ -152,6 +152,18 @@ cc_test(
     ],
 )
 
+cc_test(
+    name = "sampler_test",
+    srcs = ["internal/sampler_test.cc"],
+    deps = [
+        ":trace",
+        "@com_google_absl//absl/strings",
+        "@com_google_absl//absl/synchronization",
+        "@com_google_absl//absl/time",
+        "@com_google_googletest//:gtest_main",
+    ],
+)
+
 cc_test(
     name = "span_test",
     srcs = ["internal/span_test.cc"],
@@ -201,6 +213,7 @@ cc_test(
         "@com_google_absl//absl/memory",
         "@com_google_absl//absl/strings",
         "@com_google_absl//absl/synchronization",
+        "@com_google_absl//absl/time",
         "@com_google_googletest//:gtest_main",
     ],
 )

opencensus/trace/exporter/span_exporter.h

@@ -30,28 +30,17 @@ class SpanExporterImpl;
 class SpanExporter final {
  public:
   // Handlers allow different tracing services to export recorded data for
-  // sampled spans in their own format.
-  //
-  // To export data, the Handler MUST be registered to the global SpanExporter
-  // using RegisterHandler().
+  // sampled spans in their own format. The exporter should provide a static
+  // Register() method that takes any arguments needed by the exporter (e.g. a
+  // URL to export to) and calls SpanExporter::RegisterHandler itself.
   class Handler {
    public:
     virtual ~Handler() = default;
-
-   private:
-    friend class SpanExporterImpl;
-
-    // Exports sampled (see TraceOptions::IsSampled()) Spans using the immutable
-    // SpanData representation.
-    //
-    // The implementation SHOULD NOT block the calling thread. It should execute
-    // the export on a different thread if possible. This function must be
-    // thread-safe.
     virtual void Export(const std::vector<SpanData>& spans) = 0;
   };
 
-  // Register a handler that's used to export SpanData for sampled Spans.
-  static void Register(std::unique_ptr<Handler> handler);
+  // This should only be called by Handler's Register() method.
+  static void RegisterHandler(std::unique_ptr<Handler> handler);
 };
 
 }  // namespace exporter

opencensus/trace/internal/message_event.cc

@@ -27,8 +27,8 @@ std::string MessageEvent::DebugString() const {
       "Type: ",
       (type_ == Type::RECEIVED) ? "RECEIVED"
                                 : ((type_ == Type::SENT) ? "SENT" : "UNKNOWN"),
-      "\nMessage Id: ", id_, "\ncompressed message size: ", compressed_size_,
-      "\nuncompressed message size: ", uncompressed_size_);
+      "  Message Id: ", id_, "  compressed message size: ", compressed_size_,
+      "  uncompressed message size: ", uncompressed_size_);
 }
 
 }  // namespace exporter

opencensus/trace/internal/sampler_test.cc

@@ -0,0 +1,75 @@
+// Copyright 2017, OpenCensus Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "opencensus/trace/sampler.h"
+
+#include <atomic>
+
+#include "absl/strings/str_cat.h"
+#include "absl/time/clock.h"
+#include "gtest/gtest.h"
+#include "opencensus/trace/span.h"
+#include "opencensus/trace/trace_params.h"
+
+namespace opencensus {
+namespace trace {
+namespace {
+
+// Example of a stateful sampler class.
+class SampleEveryNth : public Sampler {
+ public:
+  explicit SampleEveryNth(int nth) : state_(new State(nth)) {}
+
+  bool ShouldSample(const SpanContext* parent_context, bool has_remote_parent,
+                    const TraceId& trace_id, const SpanId& span_id,
+                    absl::string_view name,
+                    const std::vector<Span*>& parent_links) const override {
+    // The shared_ptr is const, but the underlying State it points to isn't.
+    return state_->Increment();
+  }
+
+ private:
+  class State {
+   public:
+    explicit State(int nth) : nth_(nth), current_(0) {}
+    bool Increment() {
+      int prev = current_.fetch_add(1, std::memory_order_acq_rel);
+      return (prev + 1) % nth_ == 0;
+    }
+
+   private:
+    const int nth_;
+    std::atomic<int> current_;
+  };
+
+  std::shared_ptr<State> state_;
+};
+
+TEST(SamplerTest, SampleNth) {
+  static constexpr int kSampleRate = 4;
+  SampleEveryNth sampler(kSampleRate);
+
+  for (int i = 1; i <= 100; ++i) {
+    auto span = Span::StartSpan(absl::StrCat("MySpan", i), nullptr, {&sampler});
+    if (i % kSampleRate == 0) {
+      EXPECT_TRUE(span.IsSampled());
+    } else {
+      EXPECT_FALSE(span.IsSampled());
+    }
+  }
+}
+
+}  // namespace
+}  // namespace trace
+}  // namespace opencensus

opencensus/trace/internal/span.cc

@@ -111,6 +111,8 @@ struct SpanGenerator {
   }
 };
 
+Span Span::BlankSpan() { return Span(); }
+
 Span Span::StartSpan(absl::string_view name, const Span* parent,
                      const StartSpanOptions& options) {
   SpanContext parent_ctx;

opencensus/trace/internal/span_exporter.cc

@@ -23,8 +23,8 @@ namespace opencensus {
 namespace trace {
 namespace exporter {
 
-void SpanExporter::Register(std::unique_ptr<Handler> handler) {
-  SpanExporterImpl::Get()->Register(std::move(handler));
+void SpanExporter::RegisterHandler(std::unique_ptr<Handler> handler) {
+  SpanExporterImpl::Get()->RegisterHandler(std::move(handler));
 }
 
 }  // namespace exporter

opencensus/trace/internal/span_exporter_impl.cc

@@ -27,9 +27,9 @@ namespace exporter {
 SpanExporterImpl* SpanExporterImpl::span_exporter_ = nullptr;
 
 SpanExporterImpl* SpanExporterImpl::Get() {
-  static SpanExporterImpl* span_exporter_impl = new SpanExporterImpl(
+  static SpanExporterImpl* global_span_exporter_impl = new SpanExporterImpl(
       kDefaultBufferSize, absl::Milliseconds(kIntervalWaitTimeInMillis));
-  return span_exporter_impl;
+  return global_span_exporter_impl;
 }
 
 // Create detached worker thread
@@ -40,9 +40,9 @@ SpanExporterImpl::SpanExporterImpl(uint32_t buffer_size,
       size_(0),
       t_(&SpanExporterImpl::RunWorkerLoop, this) {}
 
-void SpanExporterImpl::Register(
+void SpanExporterImpl::RegisterHandler(
     std::unique_ptr<SpanExporter::Handler> handler) {
-  absl::MutexLock lock(&handler_mu_);
+  absl::MutexLock l(&handler_mu_);
   handlers_.emplace_back(std::move(handler));
 }
 

opencensus/trace/internal/span_exporter_impl.h

@@ -52,7 +52,7 @@ class SpanExporterImpl {
   void AddSpan(const std::shared_ptr<opencensus::trace::SpanImpl>& span_impl);
   // Registers a handler with the exporter. This is intended to be done at
   // initialization.
-  void Register(std::unique_ptr<SpanExporter::Handler> handler);
+  void RegisterHandler(std::unique_ptr<SpanExporter::Handler> handler);
 
   static constexpr uint32_t kDefaultBufferSize = 64;
   static constexpr uint32_t kIntervalWaitTimeInMillis = 5000;
@@ -77,14 +77,13 @@ class SpanExporterImpl {
   // mutex within an AwaitWithTimeout, so we need to store the size in another
   // variable.
   std::atomic<size_t> size_;
-  std::thread t_;
   mutable absl::Mutex span_mu_;
   mutable absl::Mutex handler_mu_;
-
   std::vector<std::shared_ptr<opencensus::trace::SpanImpl>> spans_
       GUARDED_BY(span_mu_);
   std::vector<std::unique_ptr<SpanExporter::Handler>> handlers_
       GUARDED_BY(handler_mu_);
+  std::thread t_;
 };
 
 }  // namespace exporter

opencensus/trace/internal/span_exporter_test.cc

@@ -16,6 +16,7 @@
 
 #include "absl/memory/memory.h"
 #include "absl/synchronization/mutex.h"
+#include "absl/time/clock.h"
 #include "gtest/gtest.h"
 #include "opencensus/trace/exporter/span_data.h"
 #include "opencensus/trace/sampler.h"
@@ -25,53 +26,66 @@ namespace opencensus {
 namespace trace {
 namespace {
 
-class MyExporter : public exporter::SpanExporter::Handler {
+class Counter {
  public:
-  void Export(const std::vector<exporter::SpanData>& spans) override {
+  static Counter* Get() {
+    static Counter* global_counter = new Counter;
+    return global_counter;
+  }
+
+  void Increment(int n) {
     absl::MutexLock l(&mu_);
-    num_exported_ += spans.size();
+    value_ += n;
   }
 
-  int num_exported() const {
+  int value() const {
     absl::MutexLock l(&mu_);
-    return num_exported_;
+    return value_;
   }
 
  private:
+  Counter() = default;
   mutable absl::Mutex mu_;
-  int num_exported_ GUARDED_BY(mu_) = 0;
+  int value_ GUARDED_BY(mu_) = 0;
 };
 
-class SpanExporterTest : public ::testing::Test {
+class MyExporter : public exporter::SpanExporter::Handler {
  public:
-  SpanExporterTest() : exporter_(new MyExporter) {
-    exporter::SpanExporter::Register(
-        absl::WrapUnique<exporter::SpanExporter::Handler>(exporter_));
+  static void Register() {
+    exporter::SpanExporter::RegisterHandler(absl::make_unique<MyExporter>());
   }
 
+  void Export(const std::vector<exporter::SpanData>& spans) override {
+    Counter::Get()->Increment(spans.size());
+  }
+};
+
+class SpanExporterTest : public ::testing::Test {
  protected:
-  MyExporter* exporter_;
+  static void SetUpTestCase() {
+    // Only register once.
+    MyExporter::Register();
+  }
 };
 
 TEST_F(SpanExporterTest, BasicExportTest) {
   ::opencensus::trace::AlwaysSampler sampler;
   ::opencensus::trace::StartSpanOptions opts = {&sampler};
-
   auto span1 = ::opencensus::trace::Span::StartSpan("Span1", nullptr, opts);
-  absl::SleepFor(absl::Milliseconds(100));
+  absl::SleepFor(absl::Milliseconds(10));
   auto span2 = ::opencensus::trace::Span::StartSpan("Span2", &span1, opts);
-  absl::SleepFor(absl::Milliseconds(200));
+  absl::SleepFor(absl::Milliseconds(20));
   auto span3 = ::opencensus::trace::Span::StartSpan("Span3", &span2);
-  absl::SleepFor(absl::Milliseconds(300));
+  absl::SleepFor(absl::Milliseconds(30));
   span3.End();
   span2.End();
   span1.End();
 
   for (int i = 0; i < 10; ++i) {
-    if (exporter_->num_exported() >= 3) break;
-    sleep(1);
+    if (Counter::Get()->value() >= 3) break;
+    absl::SleepFor(absl::Seconds(1));
   }
-  EXPECT_EQ(3, exporter_->num_exported());
+  EXPECT_EQ(3, Counter::Get()->value());
 }
 
 }  // namespace

opencensus/trace/internal/span_options_test.cc

@@ -60,36 +60,6 @@ TEST(SpanTest, StartSpanOptions) {
   EXPECT_EQ(SpanId(), SpanTestPeer::GetParentSpanId(&span));
 }
 
-#if 0
-// No BlankSpan functionality yet.
-TEST(SpanTest, BlankSpan) {
-  constexpr uint8_t trace_id[] = {1, 2, 3, 4, 5, 6, 7, 8,
-                                  9, 0, 1, 2, 3, 4, 5, 6};
-  constexpr uint8_t span_id[] = {00, 11, 22, 33, 44, 55, 66, 77};
-  Span span;
-  MessageEvent event;
-
-  EXPECT_EQ("", span.name());
-  EXPECT_EQ(SpanContext(), span.context());
-  EXPECT_FALSE(span.context().IsValid());
-
-  // Check that it does not crash with operations on a blank span.
-  span.AddMessageEvent(event);
-  span.AddLink(
-      {::opencensus::trace::Link::Type::kParentLinkedSpan,
-       ::opencensus::trace::TraceId(trace_id),
-       ::opencensus::trace::SpanId(span_id),
-       {{"test", ::opencensus::trace::AttributeValue::String("attribute")}}});
-  span.AddAnnotation(Annotation::FromDescriptionAndAttributes(
-      "This is an annotation.", {{"hello", AttributeValue::String("world")},
-                                 {"latency", AttributeValue::Int(1234)}}));
-  span.AddAttribute("bool attribute", AttributeValue::Bool(true));
-  span.SetStatus(StatusCode::CANCELLED, "error text");
-  span.End();
-  span.DebugString();
-}
-#endif
-
 TEST(SpanTest, EndAndStatus) {
   AlwaysSampler sampler;
   auto span = Span::StartSpan("test_span", nullptr, {&sampler});