[EXAMPLE] Use of WithActiveSpan() is confusing

[nleclercq]

In the provided http example, the server does the following upon reception of a client request:


{
  StartSpanOptions options;
  options.kind  = SpanKind::kServer; 
  ...
  auto new_context = prop->Extract(carrier, current_context);
  options.parent   = GetSpan(new_context)->GetContext();
  ...
  // start span with parent context extracted from http header
  auto span = get_tracer("http-server")->StartSpan(span_name, {}, options);
  // mark the span as active
  auto scope = get_tracer(‘’)->WithActiveSpan(span);
  ...
  span->End();
  ...
}

I can hardly figure out why the span is created, then explicitly activated. I would expect something like:

{
  ...
  // start span with parent context extracted from http header **and make it the active one**
  auto scope = Scope(get_tracer("http-server")->StartSpan(span_name, {}, options));
  ...
}

Ok, one could argue that there's now no way to explicitly End the span. Ok, but why not simply add an accessor to the underlying Span to openTelemetry::trace::Scope? Something like:

{
  ...
  // start span with parent context extracted from http header **and make it the active one**
  auto scope = Scope(get_tracer("http-server")->StartSpan(span_name, {}, options));
  ...
  scope.Span()->AddEvent(...);
  ...
  // if really required...
  scope.Span()->End();
}

Would that violate a design choice or the Scope paradigm?

[owent]

I think if you want to explicitly End the span, you can just use a variable to copy the shared_ptr of span and use it to call End().

Scope is a component to help users to maintain call stack,and it's optional.Actually, sometimes we can't use it(e.g. in coroutine may break the call stack management).

{
  ...
  // start span with parent context extracted from http header **and make it the active one**
  auto span = get_tracer("http-server")->StartSpan(span_name, {}, options);
  auto scope = Scope(span );
  ...
  span ->AddEvent(...);
  ...
  // if really required...
  span->End();
}

[nleclercq]

Well, what I would like is to avoid to have to explicitly activate the Span by calling WithActiveSpan. The Scope already activate/deactivate the Span using the RAII paradigm. Moreover, I was convinced that the Scope call End on the associated span when released but that's not case. So, finally I wrote I'm own flavor of Scope to fulfill my need:

using SpanPtr =  opentelemetry::nostd::shared_ptr<opentelemetry::trace::Span>;

class Scope final
{
  public:

    Scope(const SpanPtr &span) noexcept :
        span_(span),
        token_(opentelemetry::context::RuntimeContext::Attach(
            opentelemetry::context::RuntimeContext::GetCurrent().SetValue(opentelemetry::trace::kSpanKey, span)))
    {
    }

    ~Scope()
    {
        span_->End();
    }

    SpanPtr &Span() noexcept
    {
        return span_;
    }

  private:
    SpanPtr span_;
    opentelemetry::nostd::unique_ptr<opentelemetry::context::Token> token_;
};

It does the job. Unless I missed something...

[marcalff]

Bonjour @nleclercq

The span End() method can take a EndSpanOptions parameter.

Keeping a pointer on the span to call End() allows to pass EndSpanOptions.

Most of the time, end span options are not needed, but when passing an end timestamp is required, the call to End(option) must be explicit.

Having the Scope class calling span->End() prevents this.

The Scope class is designed to maintain the current active span in thread local storage (the runtime context), but does not interfere with the span given.

Note that when nothing calls End(), when the shared pointer on the span is destroyed, the span destructor is called, which will End() the span if not already done.

[marcalff]

Instead of a Scope::Span() method, the following is preferred:

{
  ...
  // start span with parent context extracted from http header
  auto span = get_tracer("http-server")->StartSpan(span_name, {}, options);
  // make it the active one
  auto scope = Scope(span);
  ...
  span->AddEvent(...);
  ...
  // if really required...
  span->End();
}

Now, this issue also exposed that the following code is confusing:

  // mark the span as active
  auto scope = get_tracer("")->WithActiveSpan(span);

WithActiveSpan is a static method, calling it using an object is misleading.

Considering that WithActiveSpan only creates a Scope, this code in the examples can be rewritten as:

  // mark the span as active
  auto scope = Scope(span);

Keeping this issue opened to address this part.

[nleclercq]

Bonjour @nleclercq

The span End() method can take a EndSpanOptions parameter.

Keeping a pointer on the span to call End() allows to pass EndSpanOptions.

Most of the time, end span options are not needed, but when passing an end timestamp is required, the call to End(option) must be explicit.

Having the Scope class calling span->End() prevents this.

The Scope class is designed to maintain the current active span in thread local storage (the runtime context), but does not interfere with the span given.

Note that when nothing calls End(), when the shared pointer on the span is destroyed, the span destructor is called, which will End() the span if not already done.

Ok, I see. I'm going to adopt the proposed impl. Thanks.

[github-actions]

This issue was marked as stale due to lack of activity.