From 297a86e467ce70f0a1ae418693a4eb357e805a44 Mon Sep 17 00:00:00 2001 From: kleiby Date: Wed, 11 Jul 2018 21:57:46 +0000 Subject: [PATCH 1/2] Only set status of the root span on exit if an HTTP response code has been set --- src/Trace/RequestHandler.php | 5 +- tests/unit/Trace/RequestHandlerTest.php | 49 +++++++++++++++++++ tests/unit/Trace/mock_http_response_code.php | 50 ++++++++++++++++++++ 3 files changed, 102 insertions(+), 2 deletions(-) create mode 100644 tests/unit/Trace/mock_http_response_code.php diff --git a/src/Trace/RequestHandler.php b/src/Trace/RequestHandler.php index e590c4cd8..444ef2617 100644 --- a/src/Trace/RequestHandler.php +++ b/src/Trace/RequestHandler.php @@ -261,8 +261,9 @@ public function addMessageEvent($type, $id, $options) public function addCommonRequestAttributes(array $headers) { - $responseCode = http_response_code(); - $this->rootSpan->setStatus($responseCode, "HTTP status code: $responseCode"); + if ($responseCode = http_response_code()) { + $this->rootSpan->setStatus($responseCode, "HTTP status code: $responseCode"); + } $this->tracer->addAttribute(Span::ATTRIBUTE_STATUS_CODE, $responseCode, [ 'spanId' => $this->rootSpan->spanId() ]); diff --git a/tests/unit/Trace/RequestHandlerTest.php b/tests/unit/Trace/RequestHandlerTest.php index 988afaadf..5b0c1c60d 100644 --- a/tests/unit/Trace/RequestHandlerTest.php +++ b/tests/unit/Trace/RequestHandlerTest.php @@ -17,17 +17,21 @@ namespace OpenCensus\Tests\Unit\Trace; +require_once __DIR__ . '/mock_http_response_code.php'; + use OpenCensus\Trace\Annotation; use OpenCensus\Trace\Link; use OpenCensus\Trace\MessageEvent; use OpenCensus\Trace\Span; use OpenCensus\Trace\SpanContext; use OpenCensus\Trace\SpanData; +use OpenCensus\Trace\Status; use OpenCensus\Trace\RequestHandler; use OpenCensus\Trace\Exporter\ExporterInterface; use OpenCensus\Trace\Sampler\SamplerInterface; use OpenCensus\Trace\Tracer\NullTracer; use OpenCensus\Trace\Propagator\HttpHeaderPropagator; +use OpenCensus\Trace\MockHttpResponseCode; use PHPUnit\Framework\TestCase; /** @@ -663,4 +667,49 @@ public function testAddsMessageEventToSpecificDetachedSpan() $this->assertEquals(123, $messageEvent->compressedSize()); $this->assertEquals(234, $messageEvent->uncompressedSize()); } + + public function testNoStatusOfRootSpanOnExitWithoutHttpResponse() + { + $this->sampler->shouldSample()->willReturn(true); + $rt = new RequestHandler( + $this->exporter->reveal(), + $this->sampler->reveal(), + new HttpHeaderPropagator(), + [ + 'skipReporting' => true + ] + ); + MockHttpResponseCode::$status = false; + $rt->onExit(); + $spans = $rt->tracer()->spans(); + $this->assertCount(1, $spans); + $spanData = $spans[0]; + $this->assertInstanceOf(SpanData::class, $spanData); + $this->assertNotEmpty($spanData->endTime()); + $this->assertEquals('main', $spanData->name()); + $this->assertNull($spanData->status()); + } + + public function testSetsStatusOfRootSpanOnExitWithHttpResponse() + { + $this->sampler->shouldSample()->willReturn(true); + $rt = new RequestHandler( + $this->exporter->reveal(), + $this->sampler->reveal(), + new HttpHeaderPropagator(), + [ + 'skipReporting' => true + ] + ); + MockHttpResponseCode::$status = 200; + $rt->onExit(); + $spans = $rt->tracer()->spans(); + $this->assertCount(1, $spans); + $spanData = $spans[0]; + $this->assertInstanceOf(SpanData::class, $spanData); + $this->assertNotEmpty($spanData->endTime()); + $this->assertEquals('main', $spanData->name()); + $this->assertInstanceOf(Status::class, $spanData->status()); + $this->assertEquals(200, $spanData->status()->code()); + } } diff --git a/tests/unit/Trace/mock_http_response_code.php b/tests/unit/Trace/mock_http_response_code.php new file mode 100644 index 000000000..75ab5f5c9 --- /dev/null +++ b/tests/unit/Trace/mock_http_response_code.php @@ -0,0 +1,50 @@ + Date: Wed, 18 Jul 2018 15:26:07 +0000 Subject: [PATCH 2/2] Account for the fact that there is no analogue for the span status field in the PECL --- src/Trace/RequestHandler.php | 6 +++--- tests/unit/Trace/RequestHandlerTest.php | 11 +++++++++-- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/src/Trace/RequestHandler.php b/src/Trace/RequestHandler.php index 444ef2617..bb1bc00c1 100644 --- a/src/Trace/RequestHandler.php +++ b/src/Trace/RequestHandler.php @@ -263,10 +263,10 @@ public function addCommonRequestAttributes(array $headers) { if ($responseCode = http_response_code()) { $this->rootSpan->setStatus($responseCode, "HTTP status code: $responseCode"); + $this->tracer->addAttribute(Span::ATTRIBUTE_STATUS_CODE, $responseCode, [ + 'spanId' => $this->rootSpan->spanId() + ]); } - $this->tracer->addAttribute(Span::ATTRIBUTE_STATUS_CODE, $responseCode, [ - 'spanId' => $this->rootSpan->spanId() - ]); foreach (self::ATTRIBUTE_MAP as $attributeKey => $headerKeys) { if ($val = $this->detectKey($headerKeys, $headers)) { $this->tracer->addAttribute($attributeKey, $val, [ diff --git a/tests/unit/Trace/RequestHandlerTest.php b/tests/unit/Trace/RequestHandlerTest.php index 5b0c1c60d..9865e4d17 100644 --- a/tests/unit/Trace/RequestHandlerTest.php +++ b/tests/unit/Trace/RequestHandlerTest.php @@ -687,6 +687,7 @@ public function testNoStatusOfRootSpanOnExitWithoutHttpResponse() $this->assertInstanceOf(SpanData::class, $spanData); $this->assertNotEmpty($spanData->endTime()); $this->assertEquals('main', $spanData->name()); + $this->assertEquals([], $spanData->attributes()); $this->assertNull($spanData->status()); } @@ -709,7 +710,13 @@ public function testSetsStatusOfRootSpanOnExitWithHttpResponse() $this->assertInstanceOf(SpanData::class, $spanData); $this->assertNotEmpty($spanData->endTime()); $this->assertEquals('main', $spanData->name()); - $this->assertInstanceOf(Status::class, $spanData->status()); - $this->assertEquals(200, $spanData->status()->code()); + $this->assertEquals([Span::ATTRIBUTE_STATUS_CODE => 200], $spanData->attributes()); + + if (extension_loaded('opencensus')) { + $this->assertNull($spanData->status()); + } else { + $this->assertInstanceOf(Status::class, $spanData->status()); + $this->assertEquals(200, $spanData->status()->code()); + } } }