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

Merge SpanContext into DistributedContext #211

Closed
toumorokoshi opened this issue Aug 10, 2019 · 3 comments
Closed

Merge SpanContext into DistributedContext #211

toumorokoshi opened this issue Aug 10, 2019 · 3 comments
Labels
spec:context Related to the specification/context directory

Comments

@toumorokoshi
Copy link
Member

After spending some time working on implementing propagators, there's a pattern that feels a little weird to me: the fact that propagators can return back both SpanContext and DistributedContext. I think this is because having to have two context propagation objects is more difficult to understand: there may be a chance to simplify here.

Specifically, I don't think there's anything in SpanContext that precludes it from using the DistributedContext. This could look like:

  • context_id: a unique identifier for this specific context (span_id could use this)
  • entries: key/value pairs describing this context
    • trace_id could fit into this
    • feature / consuming service
    • tracestate

There are a few major benefits here:

  1. language libraries would only have to worry about attaching a single object to a given request / context, rather than requiring some way to attach both to a thread / greenlet local.
  2. The propagators API can be simplified: no need to support multiple types.

If we go through with this, there may be value in passing in an existing DistributedContext object into extract. This enables multiple HTTPTextFormat or BinaryFormat propagators to add to the same object, enabling easy composition of multiple propagators.

@c24t
Copy link
Member

c24t commented Aug 14, 2019

After talking about this offline with @reyang and @toumorokoshi, the tl;dr as I understand it is this:

The W3C trace context is unlikely to change much before being adopted as a W3C recommendation, and it's generally safe to build against the current spec. The trace context includes the tracestate field, which is meant to carry vendor-specific info that generally shouldn't be exposed to application code. The trace context authors were concerned that end users (not vendors!) would stuff application context into tracestate, and proposed the correlation context specifically to carry user-defined data.

We shouldn't put trace data in the correlation context because we don't expect correlation context to be adopted as widely as trace context, and applications may choose to drop data from the correlation context or replace it with their own data.

@c24t
Copy link
Member

c24t commented Aug 14, 2019

@toumorokoshi IIRC you suggested making propagators de/serialize the trace and correlation context in the same call instead of having separate types for each type of context. Would that solve this issue? What new problems would that introduce?

@toumorokoshi
Copy link
Member Author

Yep, that's what I'm planning on doing. I think the guidance from Gitter was to code a proposal to share.

I'm fine closing this issue for now, since I no longer believe that this is the right approach, and no one else is championing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec:context Related to the specification/context directory
Projects
None yet
Development

No branches or pull requests

3 participants