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

Extend TracerBuilder to facilitate ITracer interception #308

Conversation

pcwiese
Copy link
Contributor

@pcwiese pcwiese commented Oct 23, 2019

Background:
When the active Span changes, I want to be notified and do something
with the current SpanContext. One way I can see to do that is to intercept
calls to ITracer::WithSpan (or whatever ITracer method(s) are used to activate a Span).

However, the only way to get an instance of ITracer is through the TracerFactory.
I am proposing that we add a new option to TracerBuilder where people can specify
a Func<ITracer, ITracer> that if specified is invoked for all returned ITracers.

This will allow me to create a proxy ITracer implementation and intercept every call.

usage is:

services.AddOpenTelemetry(builder =>
{
    builder
        .SetTracerConstructionInterceptor(t => new TracerProxy(t));
});

Absolutely open to other suggestions to get what I need here.

Background:
When the active Span changes, I want to be notified and do something
with the current SpanContext. One way I can see to do that is to intercept
calls to ITracer::WithSpan.

However, the only way to get an instance of ITracer is through the TracerFactory.
I am proposing that we add a new option to TracerBuilder where people can specify
a Func<ITracer, ITracer> that if specified is invoked for all returned ITracers.

This will allow me to create a proxy ITracer implementation and intercept every call.

usage is:
```csharp

services.AddOpenTelemetry(builder =>
{
    builder
        .SetTracerConstructionInterceptor(t => new TracerProxy(t))

```
@SergeyKanzhelev
Copy link
Member

Wouldn't SpanProcessor achieve this? It has OnBegin callback. What am I missing?

@pcwiese
Copy link
Contributor Author

pcwiese commented Oct 23, 2019

Wouldn't SpanProcessor achieve this? It has OnBegin callback. What am I missing?

A couple of things from looking at the Span implementation:

  • The SpanContext is currently set AFTER the SpanProcessor runs. I want/need access to the SpanContext that belongs to the Span
  • The SpanProcessor will run even if the Span isn't active/activated yet.

Also, I do think there is some value in giving people some way to intercept all calls to ITracer regardless.

@SergeyKanzhelev
Copy link
Member

Another question - is it different from overriding the entire TracerFactory and just proxy back to the original?

I'm trying to find a way to minimize the number of extensibility points needed. I see how this scenario of updating SpanContext is valuable

@pcwiese
Copy link
Contributor Author

pcwiese commented Oct 23, 2019

TracerFactory isn't overridable as is. There is no protected ctor.
And even if it were, the library guidelines say to get an instance of ITracer via TracerFactory, which means I we have to keep the TracerFactory as the public point of entry (currently). This is particularly true for non DI scenarios where people are calling TracerFactory.Create directly. For DI scenarios, I could potentially override the registration with my own implementation, but then mixed use of DI and non DI TracerFactory stuff would collide. This could possibly be solved by disallowing the use of TracerFactory directly and instead get an instance of that from some other global registry/singleton.

My particular scenario is to take that active SpanContext and serialize it / send it out of band to an eventing pipeline where I can create child Spans using that SpanContext at some point in the future.

@SergeyKanzhelev
Copy link
Member

My particular scenario is to take that active SpanContext and serialize it / send it out of band to an eventing pipeline where I can create child Spans using that SpanContext at some point in the future.

So you want to record an active SpanContext on every span creation independently from the sampling? And you still want a regular pipeline of sampling, processors and exporter work?

BTW, similar thing proposed here: open-telemetry/oteps#58 but with the different approach.

@pcwiese
Copy link
Contributor Author

pcwiese commented Oct 24, 2019

I definitely want to keep a regular pipeline of sampling, processing, and exporting.

If a Span is sampled out, I don’t want the SpanContext associated with it. Thinking through it a bit more I actually need to know when the active Span changes, not just when a new one is activated. When it does change I would send the SpanContext associated with the current Span through to our application event processing pipeline via an EventSource based event where all of our existing telemetry is generated. From there I would create appropriate child Spans as necessary. Our event processing was originally designed to be 100% out of proc.

So I think I might need to do more than simply intercept new Span activations. I would also need to wrap Disposables returned from the ITracer interface to capture the deactivations (and subsequent reactivation of the previous Span up the stack).

I was thinking something like this on my ITracer proxy (based on the current ITracer interface):

/// <inheritdoc/>
public IDisposable WithSpan(ISpan span)
{
    var disposable = this.provider.WithSpan(span);

    void EmitActiveSpanContextIfNecessary()
    {
        if (this.CurrentSpan.IsRecording)
        {
            // The Span is sampled.
            Shared.Hosting.Abstractions.Diagnostics.Events.Instance.ActiveSpanContext(() => this.BinaryFormat.ToByteArray(this.CurrentSpan.Context));
        }
    }

    EmitActiveSpanContextIfNecessary();

    // On dispose of this, emit newly restored SpanContext.
    return Disposable.Create(EmitActiveSpanContextIfNecessary);
}

still noodling over this approach though.

@pcwiese
Copy link
Contributor Author

pcwiese commented Oct 24, 2019

I'm going to close this review for now. It feels like a hack, even though it isn't a big change

@pcwiese pcwiese closed this Oct 24, 2019
@SergeyKanzhelev
Copy link
Member

@pcwiese please keep posted on your scenario and how you implemented it. I'm very curious on how do you use this data.

@pcwiese
Copy link
Contributor Author

pcwiese commented Oct 24, 2019

@pcwiese please keep posted on your scenario and how you implemented it. I'm very curious on how do you use this data.

For now I'm going to try to use a customized SpanProcessor to send the SpanContext out of band. See #312.

Yun-Ting pushed a commit to Yun-Ting/opentelemetry-dotnet that referenced this pull request Oct 13, 2022
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

Successfully merging this pull request may close these issues.

2 participants