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

Please add default constructor and move constructor/assignment op to Scope class #1298

Open
sirzooro opened this issue Mar 30, 2022 · 7 comments
Assignees

Comments

@sirzooro
Copy link

Is your feature request related to a problem?
I have event loop which gets objects from a queue and processes them. I want to create Span in this loop and set is as active span. Some of these objects are for health checks, and I want to exclude them from telemetry. Span objects are stored in nostd::shared_ptr, so I can declare variable first and then conditionally assign value to it. However I cannot do the same with Scope object, declaration and initialization must happen at the same time.

Describe the solution you'd like
Please add default constructor and move constructor/assignment operator to Scope class. This would allow to create empty Scope object first, and then conditionally move-assign value later.

Describe alternatives you've considered
Now I need to create two code branches - one for tracing enabled where code creates Scope object and perform processing, and another with processing only.

Additional context
This sample code presents what I would like to do:

void EventLoop()
{
    while (1)
    {
        auto obj = queue->Dequeue();
        
        opentelemetry::nostd::shared_ptr<opentelemetry::trace::Span> span;
        opentelemetry::trace::Scope scope;
        if (obj->TraceEnabled())
        {
            span = tracer->StartSpan(obj->GetName());
            scope = tracer->WithActiveSpan(span);
        }
        
        obj->Run();
        
        if (nullptr != span)
            span->End();
    }
}
@lalitb
Copy link
Member

lalitb commented Apr 7, 2022

Scope object is meant to control the Span reference life cycle inside Context - It attaches Span to Context during creation and detaches during destruction. Creating Scope objects without Span may have an undesired behavior if not used properly.

Also, going through your scenario above, I don't think you need a Scope object if you are explicitly ending the Span using Span::End().

@sirzooro
Copy link
Author

sirzooro commented Apr 7, 2022

Scope object is meant to control the Span reference life cycle inside Context - It attaches Span to Context during creation and detaches during destruction. Creating Scope objects without Span may have an undesired behavior if not used properly.

This needs to be analyzed and documented. Maybe it is would be enough to track if Scope object is empty, and disallow assigning value to non-empty one (e.g. by throwing std::logic_error). Plus of course extra check in destructor to properly destroy empty Scope.

An alternative is to have some way to disable tracing, e.g. like discussed in open-telemetry/opentelemetry-specification#530 .

BTW, in the meantime I found that Scope class has implicit move constructor, so I am able to dynamically move-create Scope like below. This workaround is good enough for me:

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

Also, going through your scenario above, I don't think you need a Scope object if you are explicitly ending the Span using Span::End().

Deep inside obj->Run() other child Spans are created and I do not want to modify half of my project files to explicitly pass Spans everywhere. Scope objects helps a lot here.

@github-actions
Copy link

This issue was marked as stale due to lack of activity. It will be closed in 7 days if no furthur activity occurs.

@lalitb
Copy link
Member

lalitb commented Jul 31, 2023

@sirzooro, Do you think adding a new api (say) Tracer::UseSpan() api to make the span current be a better approach here:

void EventLoop()
{
    while (1)
    {
        auto obj = queue->Dequeue();
        
        if (obj->TraceEnabled())
        {
            tracer->UseSpan(tracer->StartSpan(obj->GetName()));
        }
        
        obj->Run();
        
        if (tracer->GetCurrentSpan()->GetContext()->IsValid()) {
             tracer->GetCurrentSpan()->End();
        }
    }
}

@sirzooro
Copy link
Author

What Tracer::UseSpan() will do? Will it replace active span or add new one to the stack? For latter case, how it will be removed from the stack?

@lalitb
Copy link
Member

lalitb commented Aug 11, 2023

My initial thought was not to use this in combination with the Scope object. And this should be automatically removed once Span goes out of scope. Let me think more.

@sirzooro
Copy link
Author

My initial thought was not to use this in combination with the Scope object. And this should be automatically removed once Span goes out of scope. Let me think more.

Some interaction is needed - obj->Run() may create new spans which should have active span as a parent. Beside this your proposition seems fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants