Skip to content
This repository was archived by the owner on Sep 17, 2025. It is now read-only.

only attempt to set the span status when a current span is available#464

Merged
c24t merged 6 commits intocensus-instrumentation:masterfrom
rsalmond:err-without-context
Jan 24, 2019
Merged

only attempt to set the span status when a current span is available#464
c24t merged 6 commits intocensus-instrumentation:masterfrom
rsalmond:err-without-context

Conversation

@rsalmond
Copy link
Copy Markdown
Contributor

Hey folks, not sure if this is a great approach but I'll explain how I got here and let you decide.

I've been working with Google support trying to figure out why we're seeing hundreds of thousands of exceptions in Stackdriver Errors that look like this.

exc_info:  "Traceback (most recent call last):
  File "/<our-app-path>/image.binary.runfiles/pypi__opencensus_0_1_8/opencensus/trace/ext/flask/flask_middleware.py", line 255, in _teardown_request
    message=str(exception)
AttributeError: 'NoneType' object has no attribute 'status'"   

Turns out Stackdriver Errors simply parses app logs for anything that looks like a stack trace and reports it as an error. Because of this, the log line in the exception handler causes what at first glance should have been a handled exception, to appear as an unhandled one in Stackdriver.

In general I think that behaviour is desirable, in this specific case I am not certain if it is expected that a call to get_current_span() might return None but my guess is that this is what happens when we're not tracing. If that's the case then I think this change should eliminate the issue while maintaining desired behaviour.

@c24t
Copy link
Copy Markdown
Member

c24t commented Jan 23, 2019

The change looks good, and we should expect get_current_span to return None.

To merge this we just need a test that fails on the old behavior. If you add it to this PR I can review and merge.

@rsalmond
Copy link
Copy Markdown
Contributor Author

Hey good call @c24t, writing this test took me on a tour through the instrumentation, feel much better using it now. Had to do some whacky stuff to replicate that no current span scenario though.

Under what circumstances do you expect to see this happening?

@c24t
Copy link
Copy Markdown
Member

c24t commented Jan 24, 2019

Had to do some whacky stuff to replicate that no current span scenario though

I was surprised to see you monkey patch the propagator instead of sending the trace options in the headers like:

trace_options = TraceOptions()
trace_options.set_enabled(False)
headers = middleware.propagator.to_headers(
    SpanContext(trace_options=trace_options)
)
with self.assertRaises(Exception):
    app.test_client().get('/error', headers=headers)

but it looks like the propagator parses this wrong and re-enables tracing:

>>> middleware.propagator.to_headers(SpanContext(trace_options=TraceOptions(0)))
{'X-Cloud-Trace-Context': 'd5e525e45bbe4d25b571fa0988749117/None;o=0'}

>>> middleware.propagator.from_headers({'X-Cloud-Trace-Context': 'd5e525e45bbe4d25b571fa0988749117/None;o=0'})
SpanContext(trace_id=d5e525e45bbe4d25b571fa0988749117, span_id=None, trace_options=TraceOptions(enabled=True), tracestate=None)

This looks like another bug to me, but I don't know if it's related to your problem.

@c24t
Copy link
Copy Markdown
Member

c24t commented Jan 24, 2019

Under what circumstances do you expect to see this happening?

It at least seems to happen when we re-enable tracing when there's no span in the received context, but this may not be related. I don't know when else we would expect not to have a current span.

@c24t c24t merged commit b144219 into census-instrumentation:master Jan 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants