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.

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

Scope should be default constructable #1889

Closed
VivekSubr opened this issue Dec 27, 2022 · 11 comments
Closed

Scope should be default constructable #1889

VivekSubr opened this issue Dec 27, 2022 · 11 comments
Assignees

Comments

@VivekSubr
Copy link

Is your feature request related to a problem?
Wrapping a span in a class so that'll be a global object necessitates keeping track of scopes as members... but making a scope as a member variable is difficult as it is not default constructible.

Describe the solution you'd like
Make Scope default constructible, ie add 'Scope() = default' to the class.

@lalitb
Copy link
Member

lalitb commented Dec 27, 2022

Scope need to be associated with a Span during it's construction. It doesn't make sense to have the default constructor for it. Can you share an example what you are trying to achieve?

@sirzooro
Copy link

I logged similar issue in the past, with possible workaround: #1298

@VivekSubr
Copy link
Author

@sirzooro - thank you, I just added the default constructor and recompiled.

@lalitb - Let's say we are wrapping an global 'active span' for a application within a class, and provide a startSpan api, which sets the ActiveSpan in trace::tracer as well,

 m_currentSpan  = m_tracer->StartSpan(spanName);
 m_tracer->WithActiveSpan(m_currentSpan);

We need to keep track of the scopes as well, but scope can't be made a member variable since it's not default constructible.

@VivekSubr
Copy link
Author

@lalitb - unfortunately, default constructed Scopes crash!

(gdb) bt
#0 0x00000000004961e9 in opentelemetry::v1::context::RuntimeContext::Detach (token=...)
at /usr/include/opentelemetry/context/runtime_context.h:99
#1 0x0000000000496189 in opentelemetry::v1::context::Token::~Token (this=0x9018c0)
at /usr/include/opentelemetry/context/runtime_context.h:183
#2 0x000000000049615a in opentelemetry::v1::nostd::unique_ptropentelemetry::v1::context::Token::delete_ptr (this=0x9004a0)
at /usr/include/opentelemetry/nostd/unique_ptr.h:133
#3 0x000000000049611b in opentelemetry::v1::nostd::unique_ptropentelemetry::v1::context::Token::reset (this=0x9004a0, ptr=0x0)
at /usr/include/opentelemetry/nostd/unique_ptr.h:108
#4 0x00000000004960e9 in opentelemetry::v1::nostd::unique_ptropentelemetry::v1::context::Token::~unique_ptr (this=0x9004a0)
at /usr/include/opentelemetry/nostd/unique_ptr.h:64
#5 0x00000000004960c5 in opentelemetry::v1::trace::Scope::~Scope (this=0x9004a0) at /usr/include/opentelemetry/trace/scope.h:20

Any idea how I can fix this??

@lalitb lalitb self-assigned this Jan 8, 2023
@sirzooro
Copy link

sirzooro commented Jan 8, 2023

@sirzooro - thank you, I just added the default constructor and recompiled.

I mentioned different solution in that issue - use unique_ptr to store Scope object. It looks that implicit move constructor works fine. Here is code snipped copied from that issue

std::unique_ptr<opentelemetry::trace::Scope> scope;
if (...)
    scope = std::unique_ptr<opentelemetry::trace::Scope>(new opentelemetry::trace::Scope(tracer->WithActiveSpan(span)));

@VivekSubr
Copy link
Author

@sirzooro - that doesn't help... still get the destructor crash as above.

@sirzooro
Copy link

sirzooro commented Jan 9, 2023

@VivekSubr maybe that default constructor which you added breaks something? I use Scope move constructor when I put it into unique_ptr like in code above, and this works fine for me.

@VivekSubr
Copy link
Author

@sirzooro - Crash happened without the default constructor... I have a deque as a member variable of a wrapper class, and it crashes with the above trace on destruction of the wrapper class.

Commenting out the 'Detach' in Token destructor makes this crash go away.

@VivekSubr
Copy link
Author

@lalitb / @sirzooro - additional info, it only happens if the wrapper class is static (in my case, singleton).

So, default contructed scope (either by adding the constructor or by sirzooro's move constructor way), and the context being static triggers this crash.

@lalitb
Copy link
Member

lalitb commented Feb 2, 2023

@VivekSubr - Sorry, I haven't been following up with this issue. Is this issue still relevant ?

@lalitb
Copy link
Member

lalitb commented Feb 18, 2023

Closing. please reopen if the issue is still relevant.

@lalitb lalitb closed this as completed Feb 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants