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

[WIP] Context propagation #297

Closed
wants to merge 7 commits into from

Conversation

krnowak
Copy link
Member

@krnowak krnowak commented Nov 7, 2019

This is my initial proposal for context propagation, which more or less follows OTEP 42:

  • changes in the api/distributedcontext package:
    • replace Map with Correlations and Baggage
      • Correlations only allows to add more stuff to it
      • Baggage gives the user more control of the stuff there.
      • both are kept in the context
  • changes in the api/propagation package:
    • have two subpackages, binary and http
    • each subpackage have separate interfaces for span context propagator, correlations propagator and baggage propagator, as well as an interface for propagator that contains all the three (for convenience)
    • for now I didn't delete the old code, just told go to ignore it
    • http subpackage also has the chained propagator
  • changes in the propagation package:
    • not much, basically port the existing propagators to use the new interfaces
    • for now I didn't delete the old code, but told go to ignore it
  • the rest of the code is adapting to the API changes
  • questions:
    • should the types in distributedcontext be plain structs or rather interfaces, which SDK can implement (to optimize the way it stores correlations for example)
    • OTEP mentions functions like getHTTPInjector or getHTTPExtractor for baggage or correlations. I didn't implement those - I think that basically picking an implementation of the propagator can be left to the library the wants to propagate stuff.
  • TODO:
    • documentation
    • tests
    • use this API in opentracing bridge
      • this is going to be tricky:
        • OT stores baggage in span context, otel it go context
          • I'm still not sure how to store all the baggage coming from either OT or Otel API in a single place, so both APIs can see the baggage set by the other one
        • OT doesn't seem to have any notion of correlations, otel does have it
          • we probably still want to propagate correlations from the bridge anyway

@krnowak krnowak changed the title Context propagation [WIP] Context propagation Nov 7, 2019
@ghost
Copy link

ghost commented Nov 7, 2019

I'm -1 for sub packages for binary and http. I'd much prefer different interfaces, appropriately named, and put them into a propagation package.

api/distributedcontext/baggage.go Outdated Show resolved Hide resolved
api/distributedcontext/baggage.go Show resolved Hide resolved
api/distributedcontext/baggage.go Outdated Show resolved Hide resolved
api/distributedcontext/baggage.go Outdated Show resolved Hide resolved
api/distributedcontext/context.go Outdated Show resolved Hide resolved
@@ -0,0 +1,68 @@
// Copyright 2019, OpenTelemetry Authors
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO propagation.BinarySpanContext reads better than binary.SpanContextPropagator

Think <topic>.<thing> vs <namespace>.<what+topic>. Example: http.Server not server.HTTP

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

propagation.BinarySpanContext is a misleading name - it's a propagator, not a span context. The package names are kinda following the scheme from standard library as in encoding/json or encoding/hex, so we have propagation/binary and propagation/http.

// convert SpanContext to/from byte array.
type SpanContextPropagator interface {
Inject(core.SpanContext, Supplier)
Extract(Supplier) core.SpanContext
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The above signatures of Inject and Extract do not allow to have generic chainInjectors and chainExtractor described in the otep.
However, if these were to change, for example:

Extract(Supplier) context.Context

then the propagator has to extract the spanContext and save it in context.Context. Now we have two keys, one for spanContext and the other for Span. OR propagators have to start a span using the extracted spanContext which has its own issues.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have only written chained propagators for specific stuff and they don't exactly match the behaviour from otep, since chained extractors are called until we get the valid span context, correlation or baggage. I'm not sure what's the use of the generic chainExtractor that calls all the extractors…

And about spancontext in the context - should the users of propagators get the span context from the extractor and pass it themselves to Tracer.Start through some of the relationship-establishing option?

@jmacd jmacd added pkg:API Related to an API package pkg:bridges Related to a bridge package area:propagators Part of OpenTelemetry context propagation help wanted Extra attention is needed question Further information is requested labels Nov 25, 2019
@iredelmeier iredelmeier self-assigned this Nov 27, 2019
@lizthegrey
Copy link
Member

What needs to happen to move this forward?

@rghetia
Copy link
Contributor

rghetia commented Dec 4, 2019

What needs to happen to move this forward?

I have not been following context-propagation discussion on gitter or otep#66. It seems like otep#66 is ready for approval. If so, next step would be to align this PR to what is agreed in the otep (or maybe wait until the spec is finalized).

@jmacd
Copy link
Contributor

jmacd commented Dec 9, 2019

This branch is sadly full of conflicts, particularly stemming from when we moved propagation into the api/propagators. I've begun looking into this and will see if I can rescue this PR.

@jmacd
Copy link
Contributor

jmacd commented Dec 10, 2019

I plan to close this PR in a day or two after building a replacement that's closer to OTEP 66. Thanks @krnowak for the prototype.

@jmacd
Copy link
Contributor

jmacd commented Dec 11, 2019

Closing this in favor of #381 which tracks the current state of OTEP66.

@jmacd jmacd closed this Dec 11, 2019
@krnowak krnowak deleted the krnowak/ctxprop branch March 5, 2020 20:32
hstan referenced this pull request in hstan/opentelemetry-go Oct 15, 2020
* Add integration testing for unary client

* Update to test both client and server unary interceptors

* Add testing for streaming gRPC

* lint

* Update CHANGELOG

* Update Changelog with PR number

* Remove duplicate testing const
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:propagators Part of OpenTelemetry context propagation help wanted Extra attention is needed pkg:API Related to an API package pkg:bridges Related to a bridge package question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants