-
Notifications
You must be signed in to change notification settings - Fork 83
Only set status of the root span on exit if an HTTP response code has been set #194
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
|
Need to reproduce test failures locally and link CLA-authorized email to GitHub. |
Once you sign the CLA please reply "I signed it!" |
@klibbbs The tests are failing when then c-extension is installed. |
@bogdandrutu I'm operating under a corporate CLA that has already been signed, but I hadn't attached my organization email address to my public GitHub account when I first submitted this (it's attached now). Does it need to be visible? |
CLAs look good, thanks! |
Thanks @chingor13, able to reproduce. Will update PR when I have a fix. |
$this->assertEquals(200, $spanData->status()->code()); | ||
$this->assertEquals([Span::ATTRIBUTE_STATUS_CODE => 200], $spanData->attributes()); | ||
|
||
if (extension_loaded('opencensus')) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Hi!
I'd like to contribute a fix to prevent an invalid Status from being set on the root span when there is no available HTTP response code. When OpenCensus is used in a CLI context with the StackdriverExporter, the Stackdriver Trace API rejects the
false
status code (returned byhttp_response_code
) as invalid.