RFC: SpanContext should be immutable

[bhs]

@dawallin made an offhand remark on opentracing/opentracing-java#32 (comment):

I don't like the setBaggageItem method on the span context. It forces span context to be mutable and opened for side effects. I rather like to leave it on the aggregate root (the Span) which can forward the baggage item to the context internally. That would make it possible to have the spanContext immutable.

As I said on that thread:

My best argument for keeping setBaggageItem on SpanContext is just symmetry: having the setter and getter on different objects will confuse people. There's also the question of whether baggage should be settable after Span finish time... if not, that would be yet another argument in favor of what you're proposing.

We should discuss this here... I'm cc'ing a few folks from that PR in case they have thoughts: @adriancole @yurishkuro @michaelsembwever @tinkerware; as well as @dominikh who has been neck-deep in OpenTracing implementation details lately. :)

[codefromthecrypt]

I'm in favor of setting it at the root only (similar to what we do in zipkin for trace identifiers, sampled state etc). This eliminates the interesting, but tedious to support questions on what about branching, after finish etc. This keeps the api surface smaller without precluding it from being added later.

[marcingrzejszczak]

+1 for what @adriancole said. I think misuse, abuse, overuse and all the negative *use can be much smaller if we limit this to the root :)

[dominikh]

I don't feel strongly about this, but I do like the idea of Spans
being the interactive interface one uses for tracing and contexts
being data with no logic of their own. That is, I'm in favour of
having SetBaggageItem on Span.

This separation would especially be useful in implementations where
baggage is handled out of band and requires more logic than just
mutating a map; though I'm not sure how that interacts with the getter
and iterator existing on the context.

On Thu, Jul 14, 2016 at 09:10:53PM -0700, bhs wrote:

@dawallin made an offhand remark on opentracing/opentracing-java#32 (comment):

I don't like the setBaggageItem method on the span context. It forces span context to be mutable and opened for side effects. I rather like to leave it on the aggregate root (the Span) which can forward the baggage item to the context internally. That would make it possible to have the spanContext immutable.

As I said on that thread:

My best argument for keeping setBaggageItem on SpanContext is just symmetry: having the setter and getter on different objects will confuse people. There's also the question of whether baggage should be settable after Span finish time... if not, that would be yet another argument in favor of what you're proposing.

We should discuss this here... I'm cc'ing a few folks from that PR in case they have thoughts: @adriancole @yurishkuro @michaelsembwever @tinkerware; as well as @dominikh who has been neck-deep in OpenTracing implementation details lately. :)

---

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#106

Dominik Honnef

[tinkerware]

How about keeping SpanContext immutable, and merging the two operations of accessing and changing the context into one. In Java, it would look something like this:

interface Span {
  SpanContext contextOf(String baggageKey, String baggageValue);
  SpanContext contextOf(Map<String, String> baggageItems);
  SpanContext context();
}

interface SpanContext extends Iterable<Map.Entry<String, String>> {
  String getBaggageItem(String baggageKey);
}

The contextOf API adds to the set of baggage items in the context, returning a new context that you can use to link to the span it was created from. Making the context immutable and constraining when baggage items can be set eliminates a number of concurrency and ordering issues that, if experience is any indication, implementations are bound to disagree on.

Since adding a baggage item without using the resulting context to link a descendant span or inject to a carrier does not make sense, I don't think this API would burden useful code.

[bhs]

I have a slightly different idea... let's make baggage only settable at Span initialization time, at least until a use case forces some other behavior. I.e.,

type StartSpanOptions struct {
    References []SpanReference
    StartTime  time.Time
    Tags       map[string]interface{}

    // Add a new field here:
    AddedBaggageItems map[string]string
}

...

type addedBaggageItemOption struct {
    key string
    value string
}

func AddBaggageItem(key, value string) addedBaggageItemOption {
    return addedBaggageItemOption{key, value}
}

func (b addedBaggageItemOption) Apply(o *StartSpanOptions) {
    if o.AddedBaggageItems == nil {
        o.AddedBaggageItems = make(map[string]interface{})
    }
    o.AddedBaggageItems[b.key] = b.value
}

And of course similar things in other languages. This would eliminate some uncertainty about when baggage changes are visible. Given that baggage-setting is supposed to be an infrequent activity, I don't mind making the write API cumbersome.

If this LG to people, I am happy to (quickly) make the changes to the various APIs.

[bhs]

Also: @adriancole and @marcingrzejszczak, re

I'm in favor of setting it at the root only

As a general guiding principle, I don't like making tracing APIs that depend on being at the root of the trace. E.g., server-side devs often think they're the "root", then eventually realize they need to incorporate the mobile/web RPC client. And then the mobile/web RPC client realizes that the actual UI button-press is the root. And then the actual UI button-press realizes that the UX task that's being performed ("book a hotel", "follow someone new", "buy that necklace") is the true root.

I.e., one programmer's root is another's child.

[marcingrzejszczak]

Hmm... to make it as simple as possible. If the baggage contains data == it has already been in the root. Wouldn't that be some acceptable solution?

[bhs]

@marcingrzejszczak consider

[ Web Client ]
    |
[ Fronted Server ]
    |
[ Backend Server ]
    |
[ Storage System ]

If the storage system is relying on baggage being set in Frontend Server, what do we do when we hook up the web client? Suddenly that frontend server baggage is no longer available.

This is not an artificial example... The primary motivating usecases for baggage are security and resource accounting things that likely cannot be safely set in an end-user client.

I appreciate that the mental model is simpler if baggage can only be set at the root. Unfortunately I also think that constraint would hobble the feature from a value standpoint.

[dominikh]

On Fri, Jul 15, 2016 at 08:54:34AM -0700, bhs wrote:

I have a slightly different idea... let's make baggage only settable at Span initialization time, at least until a use case forces some other behavior.

I don't feel comfortable about that. I'd expect most baggage to be
dynamically determined after the span has been created, much like
tags.

I do want to pose a more general question, though: why is baggage part
of the OpenTracing spec? What problem does it solve?

[tinkerware]

@bensigelman Wouldn't making baggage settable at span creation time only make it harder to access the relevant detail you want to set as the baggage? For example, if you want to set account_id for a request, you would already want to have a span at the point where you can extract the account id from the request:

SpanContext incomingContext = serverTracer.extract(httpCarrier(incomingReq));
try (Span span = serverTracer.buildSpan("query").asChildOf(incomingContext).start()) {
  QueryDetails query = parse(incomingReq);
  String accountId = query.accountId();  // now what? 
}

I think this may be a common occurrence; the surrounding context to figure out the baggage item likely doesn't exist until you can do some relatively expensive processing. To overcome the problem, you may have to introduce lightweight parsing to extract just enough of the context out of a request (which may require you to rework your app protocol so that anything that may end up as trace baggage is easily accessible from a raw request), or you could make peace with the fact that request parsing will not be attributed to a useful tracing context.

[tinkerware]

The mental image I have of tags and baggage items in OT is that tags belong on the vertices/spans while baggage flows through edges/references. Coupling the addition of new baggage to creating a new reference makes sense to me in that context.

Maybe due to this mental model, I will also admit that SpanContext confuses me as a name; I keep thinking of it more as SpanLink. If SpanContext ends up being immutable, SpanLink may be a better name for it; I tend to associate mutability more with context-like constructs than others.

On the other hand, I wouldn't want to open up another long vocabulary discussion when most of the group is comfortable with the current naming; if that's the case, feel free to ignore the musings in the above paragraph. :)

[bhs]

@tinkerware we discussed a bunch of possible names for SpanContext and none of them is perfect. Certainly including SpanContext.

Re your other comment:

Wouldn't making baggage settable at span creation time only make it harder to access the relevant detail you want to set as the baggage?

It does make it a little harder to set baggage, but it's something one could work around.

• If you don't already have an active Span, we're done: just create one and include the new baggage value(s)
• If you do already have an active Span, you can create a child span and incorporate the new baggage value(s) that way. You could even finish the active Span and initialize the child with a FollowsFrom reference to describe the continuation relationship.

I.e., you can still measure the cost of parsing headers and set the baggage item as long as you don't mind introducing a span that precisely measures just the cost of parsing the headers. Seems like a reasonable thing to me, and perhaps even desirable from a precision standpoint.

[bhs]

@dominikh, re:

I do want to pose a more general question, though: why is baggage part
of the OpenTracing spec? What problem does it solve?

http://opentracing.io/spec/#baggage has a little bit of information... you can read (much) more in the Pivot Tracing paper available via http://pivottracing.io/ ... OT does not yet support most/all of that functionality, but I think the basic storage accounting use case as well as some security auditing can be accomplished with the current baggage mechanism, and I am certain that those use cases are valuable at larger organizations. (Google has been using the dapper code path to do this sort of thing for many years now)

[bhs]

@tinkerware most of the SpanContext naming conversation happened as part of opentracing/opentracing-go#82

[yurishkuro]

What is the need for making SpanContext immutable in the first place? Span is not immutable, and SpanContext is just a mini-me of the Span that can be packaged over the wire.

[bhs]

@yurishkuro two reasons:

• mutability means that SpanContext needs its own memory barrier / mutex, at least for baggage
• there is this odd case of modifying a SpanContext after Span.Finish() has already been called

[yurishkuro]

mutability means that SpanContext needs its own memory barrier / mutex, at least for baggage

That's not the case in several implementations I've seen, where the mutex in SpanContext is the only mutex, Span just shares it.

there is this odd case of modifying a SpanContext after Span.Finish() has already been called

While I agree that it's odd, the behavior of doing so is still clearly defined. For example, we did intend that FollowsFrom spans could be created after the original span was finished. So if baggage is added after finish, they will inherit that baggage.

---

I view SpanContext as a projection of Span state at a given point in time. Baggage is a part of that projection in as much as it needs to be passed to the Inject method(). And if the state is dynamic, the projection can be live/dynamic too. In practice, baggage is not the only dynamic part of the context, most tracers implement sampling, which must be propagated, so if someone does span.setTag(SamplingPriority, 1), the change is reflected in the SpanContext.

And there are certainly use cases for modifying baggage after span creation. For example, vector clocks can be returned by one dowstream call and propagated to future downstream calls, all being children of the same parent span.

I am not in favor of restricting flexibility, without stronger justifications.

There's also this repo http://github.com/openctx, it's trying to solve the problem of DCP (and baggage) outside of tracing (tracing become a plugin). What's interesting about it is that Go version takes the immutable approach (create child context to add baggage), while Node uses mutable context.

[codefromthecrypt]

If the storage system is relying on baggage being set in Frontend Server, what do we do when we hook up the web client? Suddenly that frontend server baggage is no longer available.

In this case, the baggage is propagated to the next hop, which honors it.
I suppose the subtlety is about things that the client typically doesn't
doesn't supply.

Ex in zipkin, there's an edge case (no pun intended) for this which does
muddy the water. For example, sampled (if absent which is the case in
client-originated trace) means defer to a later point.

Ex.

client sends all of the trace baggage except the sampled bit
server adds the sampled bit and continues

There's heavy disclaimers about how messy it would be if the sampled
decision was punted past a point where branching occurs, as you'd end up
with an inconsistent state.

Long story short is that I suppose a tier can add a key not seen yet,
though I'm not sure I'd expose this on Span as it is an internal detail of
inject. IOTW, what baggage can do in general, and what we choose to poke a
hole through for the purposes of the span api could be different.

Generally, this sort of thing is a tough goal in OT as it tries to be
flexible, which makes feature discussions far more broad than if you were
doing the normal "simplest enough to work, then add later" approach typical
apis would do.

[bhs]

A few more thoughts about this:

• Do we want to support baggage additions at span start/init time? If so, we could make that the only baggage write API out of the gate, basically giving us time to live with the limited API and get a better sense of the limitations.
• On that note, should sampling.priority really be a baggage thing rather than a tag thing? seems more natural to me. If Tracer implementations didn't want to pay the cost of sending that string over the wire, they could special-case it, of course.
• Iterating over all baggage is simpler from a concurrency standpoint if the SpanContext is immutable

[dominikh]

On Sat, Jul 16, 2016 at 02:36:17PM -0700, bhs wrote:

• On that note, should sampling.priority really be a baggage thing rather than a tag thing? seems more natural to me. If Tracer implementations didn't want to pay the cost of sending that string over the wire, they could special-case it, of course.

What about the peer tags?

Dominik Honnef

[bhs]

@dominikh peer tags are definitely per-span... but if we're talking about sampling, ideally it's trace-wide (or there's not much point).

[dawallin]

First I think there is a slight ambiguity of the word root here. I proposed the Span to be the root in the aggregate pattern. I read some comments like the trace should be the root which is not what I meant. Every span must have the possibility to add more baggage.

In the aggregate pattern everything must be passed through the root object. It is okey to get immutable objects of inner parts of the aggregate (like the SpanContext out of the Span), but not mutable object, since that opens the possibility to bypass the root and create side effects.

What I talk about here is the public API for the span and spanContext interfaces. For implementations it is still possible that the span can have internal access to a set method on the spanContext, and just pass the setBaggage parameters on to the context. This is actually the more flexible, less restricting solution. If we expose setBaggage on the spanContext, we also force the spanContext to be mutable, otherwise there is no way for the span to know about the changes. By moving the setBaggage method to the span we give the implementors the possibility to choose if the spanContext should be mutable or immutable.

most tracers implement sampling, which must be propagated, so if someone does span.setTag(SamplingPriority, 1), the change is reflected in the SpanContext.

This is just fine. setTag is already a method on the span (the root). The span can choose to propagate the tag into the context if it likes. No evil side effects here.

I view SpanContext as a projection of Span state at a given point in time. Baggage is a part of that projection in as much as it needs to be passed to the Inject method().

Yes! Could not agree more. And this should be the reason why Span exposes the SpanContext, to pass it to the Inject method, not to let the user manipulate it.

I'm not sure if setting baggage only at span creation is a good idea. I think it limits the use cases for baggage. I like to have the possibility to add baggage after creation, just not directly on the context.

[yurishkuro]

What I talk about here is the public API for the span and spanContext interfaces. For implementations it is still possible that the span can have internal access to a set method on the spanContext, and just pass the setBaggage parameters on to the context. This is actually the more flexible, less restricting solution.

I agree, I was also thinking along the same lines. I wouldn't mind moving baggage back to the span, but I do like the ability to read baggage off the SpanContext, especially when that context has just been extracted from the wire. It will be a bit asymmetrical, Span is R/W, SpanContext is R-only, but that's the nature of the projection.

[bhs]

If we think it makes sense to allow for baggage additions at Span start/creation time, I am merely suggesting that we begin with that API and see how things evolve. Baggage is still an experimental feature and we do not yet have meaningful data about how it will be used in the long run.

For that reason, I like the idea of removing the SetBaggageItem method on SpanContext and adding something analogous for span creation (the API details and particular idioms vary by language, of course). If there's a demonstrated need, we can follow up with a baggage-setting API on Span or SpanContext or wherever it seems to make sense at the time.

Thoughts?

[yurishkuro]

I have an open PR to integrate OpenTracing with TChannel, which explicitly relies on the ability to add baggage to a span created elsewhere. It's not the most orthodox use of OpenTracing API, but it will be completely broken (as in severely undercut the functionality in that PR) if baggage cannot be added to existing span.

[bhs]

@yurishkuro interesting, thank you for sending the pointer. I glanced at the PR. In extractInboundSpan you have to create a span with an unknown name... another idea (??) would be to create a Span that represents the early stages of request handling, then create a new span with a FOLLOWS_FROM ref pointing to that early-stage span when the new baggage items are ready (approx here). This would require that Response().span be overwritten, though, and I'm not sure whether other goroutines could have accessed it yet.

Anyway, "good" to know that we already have a strong use case for setting baggage items on already-started Span(Context)s.

Revised strawman: supported baggage methods would be

• Span.SetBaggageItem(key, val)
• SpanContext.BaggageItem(key)
• SpanContext.ForeachBaggageItem(callback)

Nothing more, nothing less. Thoughts?

[yurishkuro]

@bensigelman don't want to muddy this thread w/ tchannel discussion, just want to mention that what you're saying is possible, but makes the prevalent case of thrift/json encoding unnecessarily complex by forcing it through the dual-span approach.

On the proposal, if I put my OOP hat, the following would make more sense to me (NB: dynamic languages say boo-o-o):

type BaggageWriter interface {
     SetBaggageItem(key, val)
}
type BaggageReader interface {
    BaggageItem(key)
    ForeachBaggageItem(callback)
}
type Span interface {
    BaggageReader
    BaggageWriter
    // ...
}
type SpanContext interface {
    BaggageReader
}

This way the user-facing baggage API is primarily on the Span. SpanContext is baggage reader primarily for advanced cases most users don't need to worry about.

[bhs]

The excellent workshop that @adriancole organized has forced me into navel-gazing territory vis-a-vis this issue.

Namely, the https://github.com/openctx/openctx-go work that @kriskowal recently kicked off and the ideas that @rfonseca and @JonathanMace shared made me ponder why Spans are a "thing" at all. (As opposed to just logging structured data and causality / reference information against a context or baggage entity)

The short version of my thinking is that the Span concept exists as an optimization: both for performance and for instrumentation clarity (the "long version of my thinking" includes justification for this assertion :). If we proceed with this change, what we are really saying (in so many words) is that a Span is the only way you mutate and/or record data about a SpanContext.

Two paths forward:

1. Make the PRs across all languages and hope we don't regret it :)
2. Advance some conversations with @kriskowal, @yurishkuro and/or anyone else who wants to get involve and make sure PRs like ^^^ wouldn't be in vain... what if there's some other way to organize baggage (which would obviate any code changes here)?

Thoughts?

[dawallin]

I'm currently on vacation and unfortunately missed the workshop.

@bensigelman I like your proposal above to have get and set baggage on the Span and a read iterator on the SpanContext. It solves the problems I have with the existing mutable SpanContext.

@bensigelman I don't really see what you mean in your last comment, and really like to hear "the long version". Do you suggest we should skip Span and just have a Context? Or is it just a naming problem. To me, a Span is a kind of context, for legacy reasons called a Span. The SpanContext is then just the part of the span/context that is propagated to children spans.

That we have only a single root object (the Span) and everything has to be changed through that root is to me just good API design. Things tend to be messy if objects reference other mutable objects.

I'm opened and interested in further discussions.

[yurishkuro]

my $0.02:

• @rfonseca's "tracing plane" proposal is still conceptually very similar to the DAG algebra provided by the Span API, but it is event based (a la X-Trace), and therefore more expressive than span-based model. But it is also more complex. I don't think at this point we want to consider switching OT API to event based model, so for v. 1.0 I suggest we do not try to adopt that model, leave it for 2.0
• the span context immutability seems to be a desired property by many participants
• the need to change the API one more time in many languages is regrettable, but I think it makes the API less restrictive overall

In summary, I am 👍 for moving get/set baggage back to the Span and only exposing the iterator on the context

[rfonseca]

I just tried to follow this discussion to its start, and since I don't have enough "context", I will not get into details on Span vs SpanContext vis-a-vis baggage.

I'd like to offer a couple of comments related to the tracing plane proposal, which is really an attempt to provide a layered architecture to this space. Please let me know if there is a better place to discuss this than this thread.

(Aside) What is this tracing plane proposal? Essentially a division of the problem into 3 layers:

1. an instrumentation layer, which concerns itself with propagating opaque metadata through the system (really not caring what the data is): the main issue is to make sure stuff follows concurrency structures, messages, queues, etc.
2. a data layer, which is where "baggage" lives. We see baggage as a universal abstraction that can accommodate higher level abstractions, such as Spans. Baggage itself does not understand events, spans, timestamps, etc. It is just a key-value abstraction. The main concerns are what to do when execution splits and joins. There are different options here, and this should be pluggable as the behavior depends on the type of data
3. an "application" layer, where concepts like Span appear.
   (/Aside)

Note that this is an idealized architecture. The last thing we want is to be the what the OSI Layer Model became for networking. (If you know what that is, you get my point. If you don't, well, that is my point! :) -- this was a 7-layer complete model of what layering should be in networking, and the more pragmatic 4-layer TCP/IP model gained all the adoption).

The most important and relevant aspect here, IMO, is the reason for all of this: how you propagate metadata does not vary much among multiple possible uses, what you propagate does. So it's important to not couple the two, for the future. Baggage as an underlying "transport" is flexible enough that you can implement all sorts of apps above, including OT/Zipkin/Dapper/HTrace, but also security, deadlines, Pivot Tracing, Retro, etc. (I also fully appreciate this is the reason baggage is in the OT spec!)

Baggage (as an underlying transport for metadata) does not presuppose an events vs spans model. Baggage does not say anything about data that has to be in-band vs data that has to be out-of-band. For example, the original X-Trace has fixed-size metadata that is just a task-id and an event-id, everything else, including the binding between parents and children, multiple parents for an event, and data about the event, was logged out of band. You would construct a "report" for an event in X-Trace, add all this data to it, and then send it out of band. This is perfectly implementable using baggage. PivotTracing does have query data that is added to the baggage, but this is one use, and is not mandated.

One of the drawbacks of the 3-layer architecture, as @bensigelman pointed out in the workshop, is that it is has a high cognitive load, too many things to specify, code, and deploy, perhaps. One perfectly fine position to take, for example, is to say nothing about the division of layers 1 and 2, maybe implement them together, and this is pretty much what OpenCtx is trying to do. There are some small differences between what we've been thinking at Brown and OpenCtx, mostly having to do with how to express the behavior of joining two pieces of baggage, but this can all be abstracted away and hidden behind transparent APIs and some well defined default behaviors.

A couple of questions as food for thought: 1. Assuming that we have a system that implements OpenCtx and/or the Baggage layer from Brown's proposal, how hard would it be to implement the OT interface on top of it? 2. What would have to change (perhaps simplified) in the OT spec, if these concerns of moving the metadata around, serializing, injecting, extracting, etc, were taken care of by something like OpenCtx/Baggage?

[yurishkuro]

@rfonseca I am very interested in discussing the layered architecture, so forking it as #111.

[kriskowal]

The insight from Brown, that baggage can be lazily accumulated and merged
(and maybe consolidated) only at read sites has me intrigued. This does
solve the problem of having to deploy new libraries for new baggage
properties. I do not know how feasible it is for OpenCtx to pivot in this
direction in time to be useful for our RPC projects.

For OpenCtx, we would have to decide upon a baggage encoding, suitable for
HTTP and TChannel. At a minimum, we would need to support multiple opaque
values.

On Mon, Aug 1, 2016 at 11:13 AM Yuri Shkuro, PhD notifications@github.com
wrote:

@rfonseca https://github.com/rfonseca I am very interested in
discussing the layered architecture, so forking it as #111
#111.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#106 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AADrhgt68do_kH2kBQSGFH9mGFFE-49Vks5qbjdQgaJpZM4JND5y
.

[bhs]

I just rambled a bunch on #111 in response to @rfonseca's message above.

As for this actual issue, I'll prototype something... Assuming we don't do anything drastic (removing Span, etc!), there seems to be convergence around making Span the only mutating interface.

[cwe1ss]

As far as I've seen this has been updated everywhere except Objective-C. However, the public spec still has the methods on SpanContext. Would be great if someone could update this. :)

[bhs]

@cwe1ss good point, I will handle it before closing this issue.

[bhs]

We're all set! Closing.