Set status in start_as_current_span too

[ocelotl]

Fixes #377

[codecov-io]

Codecov Report

Merging #381 into master will increase coverage by 0.24%.
The diff coverage is 75%.

@@            Coverage Diff             @@
##           master     #381      +/-   ##
==========================================
+ Coverage   85.08%   85.32%   +0.24%     
==========================================
  Files          38       38              
  Lines        1897     1929      +32     
  Branches      225      227       +2     
==========================================
+ Hits         1614     1646      +32     
  Misses        218      218              
  Partials       65       65

Impacted Files	Coverage Δ
...emetry-sdk/src/opentelemetry/sdk/trace/__init__.py	90.7% <75%> (-0.17%)	⬇️
...app/src/opentelemetry_example_app/flask_example.py	100% <0%> (ø)	⬆️
...src/opentelemetry/ext/opentracing_shim/__init__.py	95.93% <0%> (+0.17%)	⬆️
...ts/src/opentelemetry/ext/http_requests/__init__.py	89.74% <0%> (+0.26%)	⬆️
opentelemetry-sdk/src/opentelemetry/sdk/util.py	85.88% <0%> (+0.34%)	⬆️
...opentelemetry/sdk/context/propagation/b3_format.py	87.27% <0%> (+0.48%)	⬆️
...ry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py	68.18% <0%> (+0.58%)	⬆️
...xt-jaeger/src/opentelemetry/ext/jaeger/__init__.py	86.54% <0%> (+1.54%)	⬆️
...ry-sdk/src/opentelemetry/sdk/resources/__init__.py	70.83% <0%> (+2.65%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ccb97e5...13a5ef7. Read the comment docs.

[mauriciovasquezbernal]

LGTM, thanks!

[c24t]

Changes LGTM, but note that we'll try to set the status in both places (and end the span twice) if we use the span as a context manager too:

with tracer.start_as_current_span("foo") as span:
    with span:
        raise ValueError()

In practice this doesn't matter since status is non-null at the time of the second call, but it's some evidence that we should rethink this API.

[c24t]

Maybe you covered this in #297, but why not make _set_status_on_exception public? (Also, what's an example of a span where we want this to be false?)

[ocelotl]

It can be made public, that would mean that we accept changing this behavior even after the span was created while specifically telling it to set or not its status on an exception being raised.

Maybe we want to handle the exception in the code that surrounds the span and not bother setting a status if the exception is raised? 🤷‍♂️ This was specifically requested in the issue.

[c24t]

That's something I missed on the last review. We should not be capturing the exception here!

try:
    with tracer.start_as_current_span("foo") as span:
        raise ValueError()
except ValueError as ex:
    print("exception handling in the app")
else:
    print("uh oh")  # our context manager eats the error!

[c24t]

Responding to the other comments:

we accept changing this behavior even after the span was created

Or make it a read-only public property (which is easier in the SDK than the API).

This was specifically requested in the issue.

I wonder if we want this to be a global setting instead of per-span. @Oberon00 what did you have in mind here?

[ocelotl]

That's something I missed on the last review. We should not be capturing the exception here!

try:
    with tracer.start_as_current_span("foo") as span:
        raise ValueError()
except ValueError as ex:
    print("exception handling in the app")
else:
    print("uh oh")  # our context manager eats the error!

Good catch, I have committed a fix.

[c24t]

Thanks, looks good!

[Oberon00]

I did have the per-span setting as it is now in mind. But thinking about it again, explicitly setting the status to OK does the same thing, so this might not be necessary.

@c24t , @ocelotl: I was confused why we shouldn't capture (in my mind = record) the exception here, until I realized that you meant that we swallow the exception. 😄

[c24t]

This should check that the exception escapes with assertRaises.

Also FWIW I think this would be clearer without error_status_test. In general I think it's more important for tests to be readable than DRY.

E.g.

    def test_error_status(self):
        tracer = trace.TracerSource().get_tracer(__name__)

        ex = Exception("unknown")
        with self.assertRaises(Exception) as ec:
            with tracer.start_span("root") as span:
                raise ex

        self.assertIs(ex, ec.exception)
        self.assertIs(
            span.status.canonical_code, StatusCanonicalCode.UNKNOWN
        )
        self.assertEqual(span.status.description, "Exception: unknown")
        self.assertFalse(span.status.is_ok)

        with self.assertRaises(Exception) as ec:
            with tracer.start_as_current_span("root") as span:
                raise ex

        self.assertIs(ex, ec.exception)
        self.assertIs(
            span.status.canonical_code, StatusCanonicalCode.UNKNOWN
        )
        self.assertEqual(span.status.description, "Exception: unknown")
        self.assertFalse(span.status.is_ok)

[ocelotl]

This is not only a matter of being DRY, there is something much more important. What the test does as it is now is that it quickly tells the reader another purpose of this test: we are testing that start_span and start_as_current_span behave in the same way. If we repeat our code we force our reader to carefully look into any possible differences between the two tests to make sure that these two methods are actually behaving in the same way.