Use reference to Tracer in Span instead of weak_ptr

Signed-off-by: Isaac Hier <ihier@uber.com>

Author: Isaac Hier

src/jaegertracing/Span.cpp

@@ -23,9 +23,7 @@ namespace jaegertracing {
 void Span::SetBaggageItem(opentracing::string_view restrictedKey,
                           opentracing::string_view value) noexcept
 {
-    std::lock_guard<std::mutex> lock(_mutex);
-    std::shared_ptr<const Tracer> tracer(_tracer.lock());
-    const auto& baggageSetter = tracer->baggageSetter();
+    const auto& baggageSetter = _tracer.baggageSetter();
     auto baggage = _context.baggage();
     baggageSetter.setBaggage(*this,
                              baggage,
@@ -41,41 +39,25 @@ void Span::SetBaggageItem(opentracing::string_view restrictedKey,
 void Span::FinishWithOptions(
     const opentracing::FinishSpanOptions& finishSpanOptions) noexcept
 {
-    std::shared_ptr<const Tracer> tracer;
     {
         std::lock_guard<std::mutex> lock(_mutex);
         if (_context.isSampled()) {
             _duration = finishSpanOptions.finish_steady_timestamp - _startTime;
         }
-        tracer = _tracer.lock();
     }
 
     // Call `reportSpan` even for non-sampled traces.
-    if (tracer) {
-        tracer->reportSpan(*this);
-    }
+    _tracer.reportSpan(*this);
 }
 
 const opentracing::Tracer& Span::tracer() const noexcept
 {
-    std::lock_guard<std::mutex> lock(_mutex);
-    std::shared_ptr<const opentracing::Tracer> tracer(_tracer.lock());
-    if (tracer) {
-        return *tracer;
-    }
-    tracer = opentracing::Tracer::Global();
-    assert(tracer);
-    return *tracer;
+    return _tracer;
 }
 
 std::string Span::serviceName() const noexcept
 {
-    std::lock_guard<std::mutex> lock(_mutex);
-    std::shared_ptr<const Tracer> tracer(_tracer.lock());
-    if (!tracer) {
-        return std::string();
-    }
-    return tracer->serviceName();
+    return _tracer.serviceName();
 }
 
 }  // namespace jaegertracing

src/jaegertracing/Span.h

@@ -37,7 +37,7 @@ class Span : public opentracing::Span {
   public:
     using Clock = std::chrono::steady_clock;
 
-    Span(const std::weak_ptr<const Tracer>& tracer,
+    Span(const Tracer& tracer,
          const SpanContext& context,
          const std::string& operationName,
          const Clock::time_point& startTime,
@@ -55,6 +55,7 @@ class Span : public opentracing::Span {
     }
 
     Span(const Span& span)
+        : _tracer(span._tracer)
     {
         std::lock(_mutex, span._mutex);
         std::lock_guard<std::mutex> lock(_mutex, std::adopt_lock);
@@ -234,7 +235,6 @@ class Span : public opentracing::Span {
 
     const SpanContext& context() const noexcept override
     {
-        std::lock_guard<std::mutex> lock(_mutex);
         return _context;
     }
 
@@ -243,7 +243,7 @@ class Span : public opentracing::Span {
     std::string serviceName() const noexcept;
 
   private:
-    bool isFinished() const { return _duration == Clock::duration(); }
+    bool isFinished() const { return _duration != Clock::duration(); }
 
     template <typename FieldIterator>
     void logFieldsNoLocking(FieldIterator first, FieldIterator last) noexcept
@@ -252,7 +252,7 @@ class Span : public opentracing::Span {
         _logs.push_back(log);
     }
 
-    std::weak_ptr<const Tracer> _tracer;
+    const Tracer& _tracer;
     SpanContext _context;
     std::string _operationName;
     Clock::time_point _startTime;

src/jaegertracing/SpanTest.cpp

@@ -17,12 +17,14 @@
 #include <gtest/gtest.h>
 
 #include "jaegertracing/Span.h"
+#include "jaegertracing/Tracer.h"
 
 namespace jaegertracing {
 
 TEST(Span, testThriftConversion)
 {
-    const Span span(std::weak_ptr<Tracer>(),
+    auto tracer = Tracer::make("test-service", Config());
+    const Span span(static_cast<Tracer&>(*tracer),
                     SpanContext(),
                     "",
                     Span::Clock::now(),

src/jaegertracing/Tracer.cpp

@@ -114,7 +114,6 @@ Tracer::startSpanInternal(const SpanContext& context,
                           bool newTrace,
                           const std::vector<Reference>& references) const
 {
-    std::weak_ptr<const Tracer> weakPtr(shared_from_this());
     const auto firstInProcess = (context.parentID() == 0);
 
     std::vector<Tag> spanTags;
@@ -127,7 +126,7 @@ Tracer::startSpanInternal(const SpanContext& context,
     spanTags.insert(
         std::end(spanTags), std::begin(internalTags), std::end(internalTags));
 
-    std::unique_ptr<Span> span(new Span(weakPtr,
+    std::unique_ptr<Span> span(new Span(*this,
                                         context,
                                         operationName,
                                         startTime,

src/jaegertracing/TracerTest.cpp

@@ -44,6 +44,13 @@ TEST(Tracer, testTracer)
                   false,
                   baggage::RestrictionsConfig());
     auto tracer = Tracer::make("test-service", config);
+    auto span = tracer->StartSpanWithOptions("test-operation", {});
+    span->SetOperationName("test-set-operation");
+    span->SetTag("tag-key", "tag-value");
+    span->SetBaggageItem("test-baggage-item-key", "test-baggage-item-value");
+    span->Log({ { "log-bool", true } });
+    std::this_thread::sleep_for(std::chrono::milliseconds(100));
+    span->Finish();
     opentracing::Tracer::InitGlobal(tracer);
 }
 

src/jaegertracing/reporters/ReporterTest.cpp

@@ -54,7 +54,14 @@ class FakeTransport : public Transport {
     std::mutex& _mutex;
 };
 
-const Span span(std::weak_ptr<Tracer>(),
+Tracer& tracer()
+{
+    static const auto tracer =
+        Tracer::make("test-service", jaegertracing::Config());
+    return static_cast<Tracer&>(*tracer);
+}
+
+const Span span(tracer(),
                 SpanContext(),
                 "",
                 Span::Clock::now(),
@@ -78,14 +85,6 @@ TEST(Reporter, testRemoteReporter)
         std::unique_ptr<Transport>(new FakeTransport(spans, mutex)),
         *logger,
         *metrics);
-    const Span span(std::weak_ptr<Tracer>(),
-                    SpanContext(),
-                    "",
-                    Span::Clock::now(),
-                    Span::Clock::duration(),
-                    {},
-                    {},
-                    false);
     constexpr auto kNumReports = 100;
     for (auto i = 0; i < kNumReports; ++i) {
         reporter.report(span);