open-telemetry / opentelemetry-cpp

The OpenTelemetry C++ Client
https://opentelemetry.io/
Apache License 2.0
851 stars 409 forks source link

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

Open sirzooro opened 2 years ago

sirzooro commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 https://github.com/open-telemetry/opentelemetry-specification/issues/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[bot] commented 2 years ago

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

lalitb commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.