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 @ OTEP 66 #381

Closed
wants to merge 25 commits into from

Conversation

jmacd
Copy link
Contributor

@jmacd jmacd commented Dec 11, 2019

This implements a proof of concept for the proposal in OTEP 66.

The new API Propagators API is found in:
api/context/propagation

The new global Propagators is added to api/global.

These new packages are refactored from existing propagators:
api/trace/propagation
api/context/baggage/propagation

This has been prepared to support OTEP 66, as such it is not in ready-to-merge form. Several new tests should be introduced for the new functionality introduced.

@jmacd
Copy link
Contributor Author

jmacd commented Dec 11, 2019

There are still some correctness issues here and the confusion discussed in open-telemetry/oteps#68 is real. We have both a SpanContext and a Span in the context.Context and it's not clear this is what we want. Presently the "current" SpanContext and Span are lacking coordination, so the Tracer.StartSpan implementation is using the current Span's SpanContext which is not always correct. (There are no tests that uncover this bug though.) This remains a work in progress.

@jmacd jmacd changed the title Context Propagation @ OTEP 66 WIP: Context Propagation @ OTEP 66 Dec 11, 2019
@jmacd
Copy link
Contributor Author

jmacd commented Dec 12, 2019

This PR removes the non-compliant ChildOf and FollowsFrom interfaces and the Relation type, which were inherited from OpenTracing via the initial prototype. This exposed the need to tighten up OTEP 66 with regards to clarifying when to use the current Span and when to use an extracted SpanContext as the parent when specifying the explicit and/or implicit parent context. The logic I propose is here, which says to use the remote span context if its TraceID differs from the current Span.

https://github.com/open-telemetry/opentelemetry-go/pull/381/files#diff-0f0866c95e7bc01ff2da6b0ccf5272e2

@mwear and @tedsuo will follow up in the spec.

@jmacd
Copy link
Contributor Author

jmacd commented Dec 12, 2019

FYI this PR is a bit difficult to review because it mixes refactoring with several API changes. To really merge this properly it should be split into several steps.

@jmacd
Copy link
Contributor Author

jmacd commented Dec 12, 2019

FYI the OpenTracing bridge is broken by this PR. It will take more work.

@lizthegrey lizthegrey added this to the Alpha v0.3 milestone Jan 6, 2020
@jmacd jmacd closed this Jan 27, 2020
@jmacd jmacd deleted the jmacd/ctxprop branch March 6, 2020 20:02
hstan referenced this pull request in hstan/opentelemetry-go Oct 15, 2020
…#381)

* Examples into separate modules

- Create separate go.mod for intrumentation examples
- Update and clean-up go.mod files for instrumentation
- Use replace directive to on-disk location where appropriate

* Remove SDK from metric tests

- replace SDK with internal trace test helper package

* Update dependabot config

* Update gitignore to exclude example binaries

* Update CHANGELOG.md

* PR feedback - include additional replace directives

- Include missing replace directives in example go.mod files
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants