Implement W3C Correlation Context propagator #179

sjkaris · 2019-10-10T01:30:36Z

A couple open items on this PR.

I feel that the interface shouldn't take/return []core.KeyValue, but was unsure what structure should be used in its place. tag.Map (soon to be renamed to dctx.Map/distrubtedcontext.Map) is a candidate, but doesn't allow for multiple same-valued keys as https://w3c.github.io/correlation-context/ states should be allowed.
Properties defined in https://w3c.github.io/correlation-context/ are not handled very well, and its unclear to me what should be done with them.
Do we want a separate interface for Correlation Context propagation vs SpanContext propagation

freeformz · 2019-10-10T03:15:22Z

What decides and when what keys/values need to be propagated ? Reading the linked doc I don't get a great feel beyond "user-defined baggage associated with the trace". Is that type of data likely to be different for each trace? Or is it something meant to be setup once or modified only irregularly?

freeformz · 2019-10-10T03:52:23Z

You could do something like this: https://github.com/sjkaris/opentelemetry-go/compare/feature/sk/w3s-correlation-ctx-prop...heroku:feature/sk/w3s-correlation-ctx-prop?expand=1

^ ends up conforming the new propagator to the propagation.TextFormatPropagator interface. To do so, I needed to expand core.SpanContext to also contain a []core.KeyValue. This has a side effect of making core.SpanContext not comparable, which can be dealt with in most cases, but not all. One example of a place that change affects is that the experimental streamer uses span contexts as a map key, which the new core.SpanContext can't be used as (adding a Hash() string function to spanContext or equivalent can get around that.

Thoughts?

jmacd · 2019-10-11T16:45:50Z

@sjkaris @freeformz
I've read and thought about this a bit -- I'll admit I've been less focused on propagation issues than I should be. My understanding is that the grand "Context" consists of distributed context + span context. Whether they actually are stored in the same object or not seems to be the question here. Is the expectation here that Extract will convert the SpanContext into standards-conventional key-values? I believe until now, we've said that Inject should return the explicit key-values and SpanContext, not assemble a Golang Context directly--that should be up to the following span Start.

Am I understanding the issue, at least?

Also, @sjkaris I couldn't find what you mean by "doesn't allow for multiple same-valued keys"?

@freeformz About the experimental SDK. Distributed context can't be "forgotten" by the streaming SDK the way all other state can. I believe we should maintain a distributedcontext.Map in the context for all SDKs, i.e., this is part of the API. The experimental streaming back-end maintains the current state of the span using the SpanContext as the index key, which is independent from the current distributedcontext.Map values, and whenever the Span changes it uses a new EventID to convey that the span is in a new state. So if you added a new M distributedcontext.Map field in SpanContext to store distributedcontext.Map, that would be fine, but it wouldn't be used to index current state in the streaming SDK.

sjkaris · 2019-10-11T18:37:52Z

@jmacd, yep I think you understand the issue. Just to re-state, from what I gathered at the SIG meeting, this PR should be changed such that we return both the SpanContext and the dctx.Map in Extract and additionally take a dctx.Map as an argument to Inject to pass arbitrary keys along.

Its poorly worded, but what I meant by "doesn't allow for multiple same-valued keys", is basically that the dctx.Map doesn't let you have multiple entries for the same key, like a slice or list would allow. I'm not sure that this is actually problematic.

sjkaris · 2019-10-11T18:38:36Z

I would be interested to hear @freeformz thoughts on this as well though

freeformz · 2019-10-11T18:57:29Z

I don't know what happened at the SIG meeting unfortunately. I'd prefer to not to add another interface like the original PR did, but make an interface that works for all of these types of headers, possibly with optional secondary interfaces as needed.

I am a bit worried about the question of needing to support multiple values for the same key. If that is a requirement then dctx.Map will need to be changed or can't be used for this.

rghetia · 2019-10-14T23:33:39Z

Separation of correlation context from observability is discussed here. As part of this discussion, the interface for propagators will change as well. May I suggest that we finish this PR with a single propagator that handles both, trace-context and distributed-context with following signature?

inject(ctx context.Context, supplier Supplier)
extract(ctx, contex.Context, supplier Supplier) (SpanContext, dctx.Map)

Regarding multiple values correlation context - As per the DistributedContext spec, a key can only have one value. So for now, if there are duplicate keys during extraction we either take the last or the first entry. Open an issue to resolve the multiple value support in the spec.

freeformz · 2019-10-15T01:20:54Z

Separation discussion URL: open-telemetry/oteps#42

rghetia · 2019-10-15T05:06:27Z

Separation discussion URL: open-telemetry/oteps#42

thanks for posting the correct link.

sjkaris · 2019-10-15T16:53:25Z

Sounds good, ill update this today.

api/propagation/propagator.go

rghetia · 2019-10-16T18:28:19Z

plugin/httptrace/httptrace.go

+	correlationCtx.Foreach(func(kv core.KeyValue) bool {
+		attrs = append(attrs, kv)
+		return true
+	})


should this be appended to attributes or returned second argument. I believe 'Context Entries' refer to DistributedContext here. @jmacd , can you please confirm?

Ah, I suspect you are right, this should be returned as the second arg.

(I'm open to improvements on this API.)

jmacd · 2019-10-16T22:35:32Z

plugin/httptrace/httptrace.go

+	correlationCtx.Foreach(func(kv core.KeyValue) bool {
+		attrs = append(attrs, kv)
+		return true
+	})


(I'm open to improvements on this API.)

rghetia

@sjkaris, LGTM. Can you please rebase?

rghetia · 2019-10-17T18:21:41Z

@sjkaris, LGTM. Can you please rebase?

one more conflict due to #218

…/github.com/Shopify/sarama (#179) * Bump google.golang.org/grpc Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.30.0 to 1.31.0. - [Release notes](https://github.com/grpc/grpc-go/releases) - [Commits](grpc/grpc-go@v1.30.0...v1.31.0) Signed-off-by: dependabot[bot] <[email protected]> * Auto-fix go.sum changes in dependent modules Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: dependabot[bot] <dependabot[bot]@users.noreply.github.com> Co-authored-by: Tyler Yahn <[email protected]>

sjkaris requested a review from rghetia October 10, 2019 01:30

sjkaris requested review from bogdandrutu, jmacd, pjanotti and tedsuo as code owners October 10, 2019 01:30

sjkaris force-pushed the feature/sk/w3s-correlation-ctx-prop branch from eadbb28 to b99e571 Compare October 16, 2019 18:02

sjkaris changed the title ~~WIP Implement W3C Correlation Context propagator~~ Implement W3C Correlation Context propagator Oct 16, 2019

rghetia reviewed Oct 16, 2019

View reviewed changes

jmacd approved these changes Oct 16, 2019

View reviewed changes

rghetia approved these changes Oct 16, 2019

View reviewed changes

sjkaris force-pushed the feature/sk/w3s-correlation-ctx-prop branch from f3728c5 to bc37f3d Compare October 16, 2019 23:33

Steven Karis added 5 commits October 17, 2019 11:29

Implement W3C Correlation Context propagator

fb8bdc8

PR comments

856c789

PR comments

788fe97

Update test to inject context properly

baaa8ca

Fix merge

c138077

sjkaris force-pushed the feature/sk/w3s-correlation-ctx-prop branch from 1eaf812 to c138077 Compare October 17, 2019 18:32

Merge branch 'master' into feature/sk/w3s-correlation-ctx-prop

f9591ac

rghetia merged commit e11b90c into open-telemetry:master Oct 17, 2019

rghetia mentioned this pull request Oct 24, 2019

W3C Correlation Context Propagator #124

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement W3C Correlation Context propagator #179

Implement W3C Correlation Context propagator #179

sjkaris commented Oct 10, 2019

freeformz commented Oct 10, 2019

freeformz commented Oct 10, 2019

jmacd commented Oct 11, 2019

sjkaris commented Oct 11, 2019

sjkaris commented Oct 11, 2019

freeformz commented Oct 11, 2019

rghetia commented Oct 14, 2019 •

edited

Loading

freeformz commented Oct 15, 2019

rghetia commented Oct 15, 2019

sjkaris commented Oct 15, 2019

rghetia Oct 16, 2019

sjkaris Oct 16, 2019

jmacd Oct 16, 2019

jmacd Oct 16, 2019

rghetia left a comment

rghetia commented Oct 17, 2019

Implement W3C Correlation Context propagator #179

Implement W3C Correlation Context propagator #179

Conversation

sjkaris commented Oct 10, 2019

freeformz commented Oct 10, 2019

freeformz commented Oct 10, 2019

jmacd commented Oct 11, 2019

sjkaris commented Oct 11, 2019

sjkaris commented Oct 11, 2019

freeformz commented Oct 11, 2019

rghetia commented Oct 14, 2019 • edited Loading

freeformz commented Oct 15, 2019

rghetia commented Oct 15, 2019

sjkaris commented Oct 15, 2019

rghetia Oct 16, 2019

Choose a reason for hiding this comment

sjkaris Oct 16, 2019

Choose a reason for hiding this comment

jmacd Oct 16, 2019

Choose a reason for hiding this comment

jmacd Oct 16, 2019

Choose a reason for hiding this comment

rghetia left a comment

Choose a reason for hiding this comment

rghetia commented Oct 17, 2019

rghetia commented Oct 14, 2019 •

edited

Loading