Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unnecessary call on Span::End() #449

Merged
merged 5 commits into from
Dec 18, 2020
Merged

Remove unnecessary call on Span::End() #449

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
974ff34
Remove unncessary call on Span::End()
ThomsonTan Dec 14, 2020
f5c2c94
Wrap span into trace::Scope
ThomsonTan Dec 17, 2020
bfa5539
Merge branch 'master' into RemoveEnd
lalitb Dec 18, 2020
57be845
Merge branch 'master' into RemoveEnd
lalitb Dec 18, 2020
5099740
Merge branch 'master' into RemoveEnd
lalitb Dec 18, 2020
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: 0 additions & 6 deletions examples/otlp/foo_library/foo_library.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,9 @@ nostd::shared_ptr<trace::Tracer> get_tracer()
return provider->GetTracer("foo_library");
}

// TODO: Remove all calls to span->End() once context memory issue is fixed
// (https://github.com/open-telemetry/opentelemetry-cpp/issues/287)

void f1()
{
auto span = get_tracer()->StartSpan("f1");
span->End();
Copy link
Contributor

Choose a reason for hiding this comment

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

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);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}

void f2()
Expand All @@ -26,7 +22,6 @@ void f2()

f1();
f1();
span->End();
}
} // namespace

Expand All @@ -35,5 +30,4 @@ void foo_library()
auto span = get_tracer()->StartSpan("library");

f2();
span->End();
}