Remove unnecessary call on Span::End()

[ThomsonTan]

End() calls are added to Span so explicit call is no longer necessary, see #287 (comment)

[codecov]

Codecov Report

Merging #449 (5099740) into master (4aa124f) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #449   +/-   ##
=======================================
  Coverage   94.37%   94.37%           
=======================================
  Files         185      185           
  Lines        8014     8014           
=======================================
  Hits         7563     7563           
  Misses        451      451           

[jsuereth]

Is there a test somewhere that verifies the behavior is still correct?

[lalitb]

Looks Good. As @jsuereth mentioned, would be good to have unit test for this which seems missing.

[pyohannes]

An explicit call to End is still necessary, not for the context memory issue, but because spans are not automatically ended anymore when the object goes out of scope (since StartSpan returns a shared_ptr instead of an unique_ptr now).

This should take care of ending the span:

 opentelemetry::trace::Scope scope(span);

[ThomsonTan]

Just for spans in this file, even they are shared_ptr, there should be only one owner which is the declared auto span, so destruction of auto span will destruct the Span object and End() it accordingly. So current code should work.

[pyohannes]

You're right. I still think we should add the Scope, otherwise spans will not be parented correctly (no current span will ever be set, so no parent will be set on any of the spans).

This was not broken by your change, that was already broken before.

[ThomsonTan]

Is there a test somewhere that verifies the behavior is still correct?

@jsuereth @lalitb seems we don't have test to cover these examples while I verified it locally. If this is correct, then we'll need add some test to make sure the examples run correctly.

[lalitb]

@jsuereth @lalitb seems we don't have test to cover these examples while I verified it locally. If this is correct, then we'll need add some test to make sure the examples run correctly.

Correct, and I see you have created a issue for this.

[jsuereth]

Manual testing for now is fine. We should try to avoid adding more examples that require manual tests though :)