Migrate to opentracing-beta5

jcchavezs · Apr 24, 2018 · b09db13 · b09db13
1 parent 8bd1117
commit b09db13
Show file tree

Hide file tree

Showing 6 changed files with 41 additions and 51 deletions.
diff --git a/composer.json b/composer.json
@@ -3,7 +3,7 @@
     "description": "A Zipkin bridge with OpenTracing",
     "type": "library",
     "require": {
-        "opentracing/opentracing": "1.0.0-beta4",
+        "opentracing/opentracing": "1.0.0-beta5",
         "jcchavezs/zipkin": "1.1.1"
     },
     "require-dev": {

diff --git a/src/ZipkinOpenTracing/Scope.php b/src/ZipkinOpenTracing/Scope.php
@@ -17,10 +17,16 @@ final class Scope implements OTScope
      */
     private $wrapped;
 
-    public function __construct(ScopeManager $scopeManager, OTSpan $wrapped)
+    /**
+     * @var bool
+     */
+    private $finishSpanOnClose;
+
+    public function __construct(ScopeManager $scopeManager, OTSpan $wrapped, $finishSpanOnClose)
     {
         $this->scopeManager = $scopeManager;
         $this->wrapped = $wrapped;
+        $this->finishSpanOnClose = $finishSpanOnClose;
         $this->toRestore = $scopeManager->getActive();
     }
 
@@ -37,11 +43,15 @@ public function getSpan()
      */
     public function close()
     {
-        $this->scopeManager->setActive($this->toRestore);
-    }
+        if ($this->scopeManager->getActive() !== $this) {
+            // This shouldn't happen if users call methods in expected order
+            return;
+        }
 
-    public function getToRestore()
-    {
-        return $this->toRestore;
+        if ($this->finishSpanOnClose) {
+            $this->wrapped->finish();
+        }
+
+        $this->scopeManager->setActive($this->toRestore);
     }
 }
diff --git a/src/ZipkinOpenTracing/ScopeManager.php b/src/ZipkinOpenTracing/ScopeManager.php
@@ -15,9 +15,9 @@ final class ScopeManager implements OTScopeManager
     /**
      * {@inheritdoc}
      */
-    public function activate(OTSpan $span)
+    public function activate(OTSpan $span, $finishSpanOnClose)
     {
-        $this->active = new Scope($this, $span);
+        $this->active = new Scope($this, $span, $finishSpanOnClose);
 
         return $this->active;
     }
@@ -30,17 +30,6 @@ public function getActive()
         return $this->active;
     }
 
-    public function getScope(OTSpan $span)
-    {
-        for ($scope = $this->active; $scope !== null; $scope = $scope->getToRestore()) {
-            if ($span === $scope->getSpan()) {
-                return $scope;
-            }
-        }
-
-        return null;
-    }
-
     public function setActive(Scope $scope = null)
     {
         $this->active = $scope;

diff --git a/src/ZipkinOpenTracing/Tracer.php b/src/ZipkinOpenTracing/Tracer.php
@@ -5,7 +5,7 @@
 use OpenTracing\Formats;
 use OpenTracing\Reference;
 use OpenTracing\SpanContext as OTSpanContext;
-use OpenTracing\SpanOptions;
+use OpenTracing\StartSpanOptions;
 use OpenTracing\Tracer as OTTracer;
 use Zipkin\Propagation\Getter;
 use Zipkin\Propagation\Map;
@@ -66,8 +66,8 @@ public function getActiveSpan()
      */
     public function startActiveSpan($operationName, $options = [])
     {
-        if (!$options instanceof SpanOptions) {
-            $options = SpanOptions::create($options);
+        if (!$options instanceof StartSpanOptions) {
+            $options = StartSpanOptions::create($options);
         }
 
         if (!$this->hasParentInOptions($options) && $this->getActiveSpan() !== null) {
@@ -76,14 +76,9 @@ public function startActiveSpan($operationName, $options = [])
         }
 
         $span = $this->startSpan($operationName, $options);
-        $scope = $this->scopeManager->activate($span);
+        $scope = $this->scopeManager->activate($span, $options->shouldFinishSpanOnClose());
 
-        if ($span instanceof ZipkinOpenTracingSpan) {
-            $span->setScope($scope);
-            $span->shouldCloseScopeOnFinish($options->getCloseSpanOnFinish());
-        }
-
-        return $span;
+        return $scope;
     }
 
     /**
@@ -92,8 +87,8 @@ public function startActiveSpan($operationName, $options = [])
      */
     public function startSpan($operationName, $options = [])
     {
-        if (!($options instanceof SpanOptions)) {
-            $options = SpanOptions::create($options);
+        if (!($options instanceof StartSpanOptions)) {
+            $options = StartSpanOptions::create($options);
         }
 
         if (empty($options->getReferences())) {
@@ -207,7 +202,7 @@ private function getGetterByFormat($format)
         throw new \UnexpectedValueException(sprintf('Format %s not implemented', $format));
     }
 
-    private function hasParentInOptions(SpanOptions $options)
+    private function hasParentInOptions(StartSpanOptions $options)
     {
         $references = $options->getReferences();
         foreach ($references as $ref) {

diff --git a/tests/Unit/ScopeManagerTest.php b/tests/Unit/ScopeManagerTest.php
@@ -23,32 +23,18 @@ public function testAbleGetActiveScope()
         $this->assertNull($this->manager->getActive());
 
         $span = $this->prophesize(Span::class)->reveal();
-        $scope = $this->manager->activate($span);
+        $scope = $this->manager->activate($span, false);
 
         $this->assertEquals($scope, $this->manager->getActive());
     }
 
     public function testScopeClosingDeactivates()
     {
         $span = $this->prophesize(Span::class)->reveal();
-        $scope = $this->manager->activate($span);
+        $scope = $this->manager->activate($span, false);
 
         $scope->close();
 
         $this->assertNull($this->manager->getActive());
     }
-
-    public function testAbilityToGetScopeByASpan()
-    {
-        $span1 = $this->prophesize(Span::class)->reveal();
-        $span2 = $this->prophesize(Span::class)->reveal();
-        $span3 = $this->prophesize(Span::class)->reveal();
-
-        $expectedScope = $this->manager->activate($span1);
-        $this->manager->activate($span2);
-        $this->manager->activate($span3);
-
-        $actualScope = $this->manager->getScope($span1);
-        $this->assertEquals($expectedScope, $actualScope);
-    }
 }
diff --git a/tests/Unit/TracerTest.php b/tests/Unit/TracerTest.php
@@ -108,12 +108,22 @@ public function testStartActiveSpanActivatesScope()
         $tracing = TracingBuilder::create()->build();
         $tracer = new Tracer($tracing);
 
-        $expectedSpan = $tracer->startActiveSpan(self::OPERATION_NAME);
+        $expectedScope = $tracer->startActiveSpan(self::OPERATION_NAME);
 
         $scopeManager = $tracer->getScopeManager();
         $activeScope = $scopeManager->getActive();
 
-        $actualSpan = $activeScope->getSpan();
+        $this->assertEquals($expectedScope, $activeScope);
+    }
+
+    public function testGetActiveSpan()
+    {
+        $tracing = TracingBuilder::create()->build();
+        $tracer = new Tracer($tracing);
+
+        $scope = $tracer->startActiveSpan(self::OPERATION_NAME);
+        $expectedSpan = $scope->getSpan();
+        $actualSpan = $tracer->getActiveSpan();
 
         $this->assertEquals($expectedSpan, $actualSpan);
     }