MockSpanContext should expose its properties via BaggageItems

[ndrwrbgs]

To provide interop with other tracers and properly implement the spec, the baggage (SpanId/TraceId) should be accessible via the BaggageItems accessor (at least until/unless those two properties are eventually promoted in the spec to first class)

[cwe1ss]

What kind of interop do you need? MockTracer is meant to be used for unit testing - it is not a production tracer.

TraceID and SpanID will become official properties with the upcoming „Trace Identifiers RFC“.

[ndrwrbgs]

I’m not sure that it being just for testing should be reason to deny this request - so I’ll focus on the why in theory this would be useful (for testing).

I have a tracer impl I’m working on that will make “pseudo tracer” implementations easy - an event hook tracer that exposes an event on span start and Finish. In unit tests, I’d like to compare the spans that triggered events to ensure they were the proper ones. The underlying tracer, for test, is the MockTracer since it does in process propagation and maintains the baggage (should maybe be InMemory tracer rather than Mock? but that’s another question).

However, without exposing its properties, it’s hard to assert that I have the proper object. Ideally I could compare the baggage items since those are supposed to represent the state needed by the tracer for the span across boundaries (in this case just traceid and spanid), but they’re not exposed by the interface the type is implementing so, as far as “program to the interface” would be concerned, they don’t exist.

I also can’t simply cast the span either because the span itself could feasibly be a MockSpan or an EventHookSpan that contains a MockSpan (or a ConsoleLoggingSpan that contains a MockSpan etc etc).

[cwe1ss]

Couldn't you just check if it's the same object by doing e.g. Assert.Same(expectedSpan, actualSpan) (xunit).

// Pseudo code - don't know if it compiles.
ISpan actualSpan = null;
EventHookTracer tracer = new EventHookTracer(originalTracer);
tracer.OnSpanFinished += (span) => actualSpan = span;
EventHookSpan span = (EventHookSpan)tracer.BuildSpan("foo").Start();
Assert.Same(span.InnerSpan, actualSpan);

[ndrwrbgs]

I can do this if I expect the items to be the exact same. This is, however, an artificial constraint that is not made by the MockTracer today - though if you're willing to update the documentation and contract I can do this :)

Problem: MockTracer makes no guarantee that, for instance, successive calls to IScopeManager.Active.Span will return the same object. It's a constraint per OT that they represent the same span, but implementation wise you could easily in the future be newing up a new object for each request - in which case since ISpan does not implement IEquatable I cannot even expect object.Equals to work, much less ReferenceEquals as I imagine Same would do. Perhaps you can see how the interface contract/implementation as it is today leaves us prone to future changes - and such brittleness usually hints at an issue with the existing contracts being made.

TLDR - the only thing that OT asserts about a Span is that it has a Baggage. From that I'm actually taking a leap to even assume conceptually equal Spans will have the same Baggage (I'm not sure we can prove this assumption based on the existing axioms of OT) but it feels safer to future changes to assume that (since Baggage is supposed to even cross the wire) than to presume something like ReferenceEquals requirement for ~= spans (which would definitely not be true if there were any cross-wire [e.g. cross AppDomain and back] operations going on).

[cwe1ss]

Problem: MockTracer makes no guarantee that, for instance, successive calls to IScopeManager.Active.Span will return the same object. It's a constraint per OT that they represent the same span, but implementation wise you could easily in the future be newing up a new object for each request

What would be a use case for that? The whole purpose of IScopeManager.Active.Span is to return the one active span at the current code execution moment.

Usefulness aside, newing up a new (wrapper) span on each property access would be a considerable performance/memory hit. If you create a wrapper system, you should only wrap the span once - when it is created.

[ndrwrbgs]

Haha, obviously I agree, but as I've had experience in CS, I know full well better than to take a dependency on "no, that's silly, that'll never be done" and instead take only dependencies on things that are documented/contractual/impossible-to-violate-per-the-code.

But if you want to talk about Memory hit, actually keeping one in memory at all would be less efficient than storing it off to disk, and storing it off to disk would mean you'd have to make a new object when it's requested. I'm not saying do this, but the fact that I can even imagine such a case means we as developers should design for it.

Let me pose the question to you - as the context is supposed to represent the data needed to transmit a span across the wire, why would you NOT expose span/traceID in the baggage (which is the only thing that contractually the context has)?

[cwe1ss]

@ndrwrbgs Let me pose the question to you - as the context is supposed to represent the data needed to transmit a span across the wire, why would you NOT expose span/traceID in the baggage (which is the only thing that contractually the context has)?

As also said by @yurishkuro, tracers implement the OpenTracing API interfaces and add whatever functionality they want/need to their concrete types. You can store as much/little stuff as you want in your span context implementation. It's important to understand that OpenTracing is not trying to abstract everything that a tracer can/should do. It's just the minimum contract that's required to instrument code and to make this data consumable to tracers.

And as I've said before, TraceID & SpanID will become part of the interface. Will that resolve this issue?

[ndrwrbgs]

Yeah I thought that I already said that would work fine, looks like I missed that - that's what I get for replying on vacation!

[cwe1ss]

OK great! Feel free to create a new issue if that won't cover your scenario (once its merged/released).