Skip to content
This repository has been archived by the owner on Oct 3, 2023. It is now read-only.

Only set status of the root span on exit if an HTTP response code has been set #194

Merged
merged 2 commits into from
Jul 19, 2018
Merged

Only set status of the root span on exit if an HTTP response code has been set #194

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
297a86e
Only set status of the root span on exit if an HTTP response code has…
kyleleiby Jul 11, 2018
034b00b
Account for the fact that there is no analogue for the span status fi…
kyleleiby Jul 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/Trace/RequestHandler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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, [
Expand Down
11 changes: 9 additions & 2 deletions tests/unit/Trace/RequestHandlerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}

Expand All @@ -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')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a bug in the extension that causes it to ignore status? If so, let's open another bug - the behavior when the extension is installed should be the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There appears to be no analogous concept of a Status object (or the status field, for that matter) on the span data maintained by the extension. Unlike other SpanData fields, status is set directly on a Span instead of via methods on Tracer. So when Span::setStatus is invoked on the root span on shutdown, there is no mechanism for propagating that to the state being tracked by the extension.

I'll submit a bug describing this, and I can outline some options for fixing the behavior, but I'm still relatively new to OpenCensus so I'm not that familiar with how opinionated it is on these features or whether it should match how the other language libraries do it, etc.

In the meantime, is this PR acceptable for addressing the issue with HTTP codes in a CLI context? The extension-dependent behavior of status being asserted in the unit test didn't actually change as part of this PR -- it's just being exposed in a test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Submitted Issue #196 to describe this

$this->assertNull($spanData->status());
} else {
$this->assertInstanceOf(Status::class, $spanData->status());
$this->assertEquals(200, $spanData->status()->code());
}
}
}