Should IScope expose FinishSpanOnDispose property?

[ndrwrbgs]

Per the API exposed, there is no way to produce an IScope that does not involve setting FinishSpanOnDispose (explicitly or implicitly). This makes me feel like this is a genuine first-class property of an IScope.

The problem that I hit with this property not being exposed is demonstrated here. To explain it briefly, IScopeManager.Active allows a user to retrieve an IScope, but the IScope interface itself does not provide enough information to properly write a wrapper/decorator around an underlying IScopeManager and maintain the exact calls that are being passed in (e.g. actually calling underlyingScopeManager.Active, because to intercept all calls as a wrapper a new type must be returned, and it is unknown at IScopeManager.Active call time whether the returned value should interpret a call to IScope.Dispose as an implicit call to ISpan.Finish or not).

Perhaps I am vastly misunderstanding the flow here, but personally I feel if it's impossible to create something with a specific property, then that property should be exposed from the thing for wrappers to take advantage of.

[cwe1ss]

As you wrap an inner type, I think it shouldn't be necessary for you to know about this. You would just call _innerScope.Dispose() and that would call _span.Finish() if finishSpanOnDispose is true. That span will also be a wrapper type of yours so you would call your tracer.OnSpanFinished() there after you've called _innerSpan.Finish().

If you call tracer.OnSpanFinished() in your EventHookScope it would get called twice, wouldn't it?

Another thing I've seen is that you might currently wrap too often. You're already creating an EventHookSpan in your EventHookSpanBuilder and then you're doing it again in the EventHookScope.Span accessor.

[ndrwrbgs]

As you wrap an inner type, I think it shouldn't be necessary for you to know about this

Unfortunately it is necessary since in CS theory, I should implement IScopeManager.Active by calling the underlying IScopeManager.Active. Now I have an instance of IScope but no information about it. I need to give something back to the user that, when they close it, I can call an event. I cannot rely on that IScope calling Finish on it's ISpan because, obviously, I don't control what is inside that IScope (it's a black box to me, as an interface should be :) ). Therefore if I don't intercept the IScope itself I have no availability to hook into the Finish event. TLDR "That span will also be a wrapper type of yours" isn't true since the interfaces expose IScope IScopeManager.Active, so really all I have available is an IScope not an ISpan I can wrap (without losing information since I don't know whether the IScope would have finished or not).

^ Obviously my concerns make sense to me, so I may not be explaining them well enough for another to understand. If you could help me to explain better by asking any questions about the above that don't make sense it'd help me to understand where you are so I can better chart a path to where I am mentally.

If you call tracer.OnSpanFinished() in your EventHookScope it would get called twice, wouldn't it?

This was a bug in the version at the time of writing, so yes :) But not a mortal flaw with the approach. The idea here is to wrap everything as it goes out to the caller, and underlying call the exact same method that was called on the underlying implementation.

Another thing I've seen is that you might currently wrap too often.

Again, totally true, v0 code that hasn't been cleaned up yet because I'm physically not able to implement it correctly with the aforementioned missing property (without imposing some artificial concept on top of it as I do in the link like saying the Active scope will be propagated by Async, which I should not need to do to intercept requests and add event hooks).

[cwe1ss]

A general question regarding your project: I understand the value of having hooks for "OnSpanStarted" & "OnSpanFinished" but what's the value of having a "OnSpanActivated" hook? As that's an internal "feature"/necessity ("internal" as in useful probably only for instrumentation code), it seems unnecessary to have your own ScopeManager.

[ndrwrbgs]

If any tracer ever will need the concept of activated, then the hook needs to exist as in theory you should be able to implement any tracer via these event hooks and not have to deal with the overhead complexity of the OT APIs.

[cwe1ss]

If any tracer ever will need the concept of activated, then the hook needs to exist as in theory you should be able to implement any tracer via these event hooks and not have to deal with the overhead complexity of the OT APIs.

You're talking about "passive" tracers like metrics systems (e.g. prometheus) etc, right? As your project is a wrapper tracer, a regular tracer would still have to target the regular OpenTracing API and be passed to your EventHookTracer.

Could you provide an actual use case where subscribing to "OnSpanActivated" makes sense? It doesn't make much sense to discuss something if there's no practical use for it.

[ndrwrbgs]

Honestly, I'm just trying to make the hooks - how they're used shouldn't in theory be of any concern of mine :) Granted, in practice that's naive.

I'm trying to do something similar to prometheus. And a simple cause for activated would be to tell how much time the span is active for. TLDR, if the interface designers though active had a valid purpose at all, then I can't imagine validity to any argument that folks using said interfaces should have no interest in it. Basically I'm fully basing my need for it to be exposed on the fact that the interfaces expose it - and if they shouldn't that's another discussion but until they don't....

Side question - why is it of such interest for this property the OnActivated? Even if I ignore that event, the thing I cannot catch here is the OnFinished, which obviously would be of interest right? I cannot properly intercept Finished while this property isn't exposed.

[cwe1ss]

@ndrwrbgs Side question - why is it of such interest for this property the OnActivated? Even if I ignore that event, the thing I cannot catch here is the OnFinished, which obviously would be of interest right? I cannot properly intercept Finished while this property isn't exposed.

Why not? You've wrapped the span when it has been created so any call to ISpan.Finish will call your Finish method.

[ndrwrbgs]

I wrapped the span when the consumer asked for it, when it was created. Internally, that span is implemented by the underlying tracer. Take for instance this:

ITracer tracer = ...;
var wrapped = tracer.BuildSpan().StartActive();
tracer.ScopeManager.Active.Dispose();

This is exactly the case I'm trying to make sure to handle correctly. My wrapper shouldn't be responsible for knowing which span is active, that's up to the implementation ITracer. The thing I return from Active can be wrapped, but when the user calls Dispose on that I do not know if that should be interpreted as a Finish. Nor can that call be intercepted below there since Active has to return a scope that was wrapped from the underlying IScopeManager.Active (meaning the ISpan on it is the ISpan exposed by the underlying tracer, not your ISpan, and you can't during Active convert it to your ISpan for the aforementioned reason - you don't know if that IScope is just calling dispose on it's close or other logic (this would break scenarios like Yuri was suggesting of expecting the type passed to me to always be my own, if I start reaching into the objects and having to modify or extract things from them).

Can you think of any way in which I can properly handle the call to IScopeManager.Active.Dispose without this information?

[ndrwrbgs]

Oh it's also important to note that if you mean when the span was created like I wrapped the span before sending it to IScopeManager.Activate, I cannot do that either because many of the tracers (my internal one as well) do what Yuri suggested of force casting the object back to a concrete type. THis means I must give that Activate method it's own span that it created, meaning I can only use the one 'I created' "above" (e.g. in user code) and have to always pass the 'real version' "below" (e.g. to the underlying tracer)

[cwe1ss]

I think this should work. It obviously only implements some interface methods though.

public class EventHookSpan : ISpan
{
    private readonly ISpan span;
    private readonly EventHookTracer tracer;

    internal EventHookSpan(ISpan span, EventHookTracer tracer)
    {
        this.span = span;
        this.tracer = tracer;
    }

    public ISpan SetTag(string key, string value)
    {
        span.SetTag(key, value);
        return this;
        //That's unnecessary: return new EventHookSpan(span, this.tracer);
    }

    public void Finish()
    {
        span.Finish();
        // Should be after the call to finish... otherwise it should probably be called "OnSpanFinishing" or "OnSpanFinish" or so
        tracer.OnSpanFinished(span);
    }

    internal ISpan WrappedSpan() => span;
}
public class EventHookSpanBuilder : ISpanBuilder
{
    private readonly ISpanBuilder impl;
    private readonly EventHookTracer tracer;

    internal EventHookSpanBuilder(ISpanBuilder impl, EventHookTracer tracer)
    {
        this.impl = impl;
        this.tracer = tracer;
    }

    public ISpanBuilder WithTag(string key, string value)
    {
        impl.WithTag(key, value);
        return this;
        // That's unnecessary: return new EventHookSpanBuilder(builder, this.tracer);
    }

    public ISpan Start()
    {
        ISpan span = impl.Start();
        tracer.OnSpanStarted(span); // Another possible hook
        return new EventHookSpan(span, tracer);
    }

    public IScope StartActive(bool finishSpanOnDispose)
    {
        // Don't call impl.Start() here!
        ISpan span = Start();

        // We now pass our EventHookSpan to the ScopeManager!
        return tracer.ScopeManager.Activate(span, finishSpanOnDispose);
    }
}
public class EventHookScopeManager : IScopeManager
{
    // This whole type won't be necessary if you don't need the OnSpanActivated hook.

    private readonly IScopeManager impl;
    private readonly EventHookTracer tracer;

    public EventHookScopeManager(IScopeManager impl, EventHookTracer tracer)
    {
        this.impl = impl;
        this.tracer = tracer;
    }

    // We controlled "Activate" so this will always return "our" scope.
    public IScope Active => impl.Active;

    public IScope Activate(ISpan span, bool finishSpanOnDispose)
    {
        EventHookSpan eventHookSpan = span as EventHookSpan;
        if (eventHookSpan == null)
        {
            // This shouldn't happen as spans can only be created via our methods
            // so this might as well just log an error.
            eventHookSpan = new EventHookSpan(span, tracer);
        }

        // Now we activate our span and not the wrapped one
        // We don't have to pass it the wrapped span as IScopeManager is a completely separate
        // concept that just happens to sit on ITracer (for easier accessibility).
        IScope scope = impl.Activate(span, finishSpanOnDispose);

        // Call OnSpanActivated afterwards and pass the wrapped span.
        tracer.OnSpanActivated(eventHookSpan.WrappedSpan());

        // The scope contains our span so we don't even need our own EventHookScope type.
        return scope;
    }
}

public class EventHookTracer : ITracer
{
    private readonly ITracer impl;

    public EventHookTracer(ITracer impl)
    {
        this.impl = impl;
        ScopeManager = new EventHookScopeManager(impl.ScopeManager, this);
    }

    // Don't wrap on each call to the property.
    public IScopeManager ScopeManager { get; }

    // No wrapping here... that's just a convenience accessor.
    public ISpan ActiveSpan => ScopeManager.Active?.Span;

    public ISpanBuilder BuildSpan(string operationName)
    {
        ISpanBuilder spanBuilder = impl.BuildSpan(operationName);
        return new EventHookSpanBuilder(spanBuilder, this);
    }

    public void OnSpanStarted(ISpan span)
    {
    }

    public void OnSpanActivated(ISpan span)
    {
    }

    public void OnSpanFinished(ISpan span)
    {
    }
}