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

Proposal: Separate Layer for Context Propagation #66

Merged
merged 79 commits into from
Jan 18, 2020

Conversation

tedsuo
Copy link
Contributor

@tedsuo tedsuo commented Nov 15, 2019

This proposal provides a high level description of OpenTelemetry, which shows how context propagation, observability, and baggage could be cleanly decoupled.

This is a second draft of the OTEP originally submitted in #42. It is resubmitted as a new PR, as much has changed. Link to a readable version of the latest draft can be found here.

Changes from the first draft:

  • Use language from aspect-oriented programming to describe cross-cutting concerns (the term "distributed application" was too confusing).
  • Removed detailed descriptions of the observability APIs, they were too distracting.
  • Focused less on correlations, as this OTEP does not intent to define the details of that API.
  • Added links to prototypes in four languages: Go, Java, Python, and Ruby.
  • Added examples in pseudocode.
  • General cleanup in response to feedback.

A couple of notes on terminology:

In this doc, I refer to TagMap as Correlations, and Tags as Labels. There can be a separate discussion for what to name this, but since Tag has repeatedly come up as a contentious name for this feature, I tried to use something "similar but different" to avoid this debate. I actually feel that the term correlation contains a more accurate meaning than tag, so it may be an improvement.

The goal with this spec proposal is to find a way to describe these public APIs as simple as possible, but no simpler. This description is meant to be high level, but not inaccurate. If a detail appears to be missing, it is intentional - presumably handled at the SDK layer. Possibly, it should be present. Please review this document with this in mind.

tedsuo and others added 30 commits September 9, 2019 18:04
Co-Authored-By: Christian Neumüller <christian+github@neumueller.me>
Co-Authored-By: Christian Neumüller <christian+github@neumueller.me>
Co-Authored-By: Reiley Yang <reyang@microsoft.com>
Co-Authored-By: Reiley Yang <reyang@microsoft.com>
Co-Authored-By: Reiley Yang <reyang@microsoft.com>
Co-Authored-By: Reiley Yang <reyang@microsoft.com>
* Remove registry concept
* Add explicit chaining
tedsuo and others added 3 commits January 15, 2020 10:28
Co-Authored-By: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-Authored-By: Tyler Yahn <MrAlias@users.noreply.github.com>
@tedsuo
Copy link
Contributor Author

tedsuo commented Jan 16, 2020

@tedsuo what I was looking from this OTEP is how to detach Span, SpanContext, Resource propagation from the Tracer and Resource API. We already have DistributedContext fully detached from those in the spec.

This OTEP splits the packages a bit further than we already have. But honestly, it is not a huge change. The package layout would look like the following, with the lower-level Context and Propagation packages separated from observability:

Context
Observability
  Trace
  Metrics
  Correlations
Propagation

Here are what I believe to be the important changes:

  • DistributedContext is split in two pieces, the Correlations API and a local Context API.
  • Tracing and Correlations to be handled as independent concerns. They each have their own APIs, and their own propagators.
  • Inject and Extract call sites can handle multiple propagators, so these separate concerns can all be injected and extracted without knowing about each other.
  • While the default implementation for Observability packages is a No-op, the Context and Propagation packages always have a canonical implementation.

Some questions that are unclear from the spec - how the API that stores and restore Span and SpanContext from context looks like? Does it return typed objects? How propagators should be implemented? They must have a reference to the SpanContext type, but we also wish to have them built-in into the "context" package.

I believe that many of these API details about types and objects would be language-specific. But, in general, each observability API should wrap the context object, implement its own set of propagators, and be independent from the other APIs . All of the observability APIs make use of the same local context object to store their data, but that could be an internal detail most of the time, in languages which allow for implicit context propagation. For Inject/Extract, all of the separate propagators are injected and extracted at the same call site. the Propagation package can have a registry, or another mechanism which iterates over all of the registered propagators.

Since each concern knows only about its own details - the Tracing SDK only knows how to handle span data locally in the context, and what headers should be used for injecting and extracting of the span context, and how to respond to sampling. Likewise, the Correlations SDK only knows the details about how to manage the correlations data locally, and how to inject and extract a filtered subset of correlations.

So, while not a huge change, this does further separate concerns.

There are API details to discuss, but I would like to avoid getting into them in this OTEP. They are addressed in the prototypes, and can be clarified in the Spec. The purpose of the OTEP is just to clarify the new package layout and their relationships. The sample APIs and examples were added by request from other community members, so that they could better understad how the packages relate to each other, and help get the prototypes written.

I'll take a look into PRs mentioned now to try to get those answers. I wish though it will be clearer from text.
Sorry; it has been difficult to make this OTEP short enough to be understood, yet long enough to be clear.

Another minor questions - does this OTEP expresses an opinion that filter must be removed: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/api-distributedcontext.md#entrypropagationfilter or just don't mention it? It alludes to the manual removal of Correlations from context if needed. But there is already a filter in spec that makes it easier to do globally.

That is not mentioned; though I agree we need a way to filter for Correlations.

@kbrockhoff
Copy link
Member

I think the overall concept of this proposal is great. Providing a universal, reliable mechanism for transmitting context throughout a system could make OpenTelemetry attractive to development teams currently not using an observability framework.

@tedsuo
Copy link
Contributor Author

tedsuo commented Jan 17, 2020

@bogdandrutu @SergeyKanzhelev since this has approval, and is primarily meant to describe the high-level goals, I would like to go ahead and merge it. We can continue discussion about the API details in spec PRs, based on the prototypes.

Copy link
Member

@c24t c24t left a comment

Choose a reason for hiding this comment

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

LGTM to move this forward to specs.

@tedsuo tedsuo merged commit 322da45 into open-telemetry:master Jan 18, 2020
@tedsuo tedsuo deleted the context-prop-2 branch January 18, 2020 00:10
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.

None yet