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.

Sign up for GitHub

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

Jump to bottom

Initial composite/global Propagators update for OTEP 66. #440

Merged
merged 15 commits into from
Feb 10, 2020
Merged

Initial composite/global Propagators update for OTEP 66. #440

Changes from 6 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
f9112aa
Initial Propagators update for OTEP 66.
carlosalberto Feb 5, 2020
4dc7c6c
Fix style.
carlosalberto Feb 5, 2020
07d1f44
Initial feedback pass.
carlosalberto Feb 6, 2020
c75e1e1
Update specification/api-propagators.md
carlosalberto Feb 6, 2020
7297fe7
Update specification/api-propagators.md
carlosalberto Feb 6, 2020
ecec495
Update specification/api-propagators.md
carlosalberto Feb 6, 2020
616d307
Update specification/api-propagators.md
carlosalberto Feb 7, 2020
ab26041
Merge branch 'master' into propagators_context_update
carlosalberto Feb 7, 2020
3460697
Update specification/api-propagators.md
carlosalberto Feb 7, 2020
c433d24
Fix the mixed up getter/setter.
carlosalberto Feb 7, 2020
caca9a6
Update specification/api-propagators.md
carlosalberto Feb 7, 2020
b06e03c
Update specification/api-propagators.md
carlosalberto Feb 7, 2020
ba0b300
Do not use RPC in api-propagators
carlosalberto Feb 10, 2020
c37c597
Use Format instead of format.
carlosalberto Feb 10, 2020
57a76fc
Clarification on format in overview.md
carlosalberto Feb 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 86 additions & 5 deletions specification/api-propagators.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
Table of Contents
</summary>

- [Overview](#overview)
- [Binary Format](#binary-format)
- [ToBytes](#tobytes)
- [FromBytes](#frombytes)
Expand All @@ -16,10 +17,22 @@ Table of Contents
- [Extract](#extract)
- [Getter argument](#getter)
- [Get](#get)
- [Composite Propagator](#composite-propagator)
- [Global Propagators](#global-propagators)

</details>

Propagators API consists of two main formats:
## Overview
carlosalberto marked this conversation as resolved.
Show resolved Hide resolved

Cross-cutting concerns send their state to the next process using
`Propagator`s, which are defined as objects used to read and write
context data to and from RPC requests. Each concern creates a set
carlosalberto marked this conversation as resolved.
Show resolved Hide resolved
of `Propagator`s for every supported format.
carlosalberto marked this conversation as resolved.
Show resolved Hide resolved

The Propagators API is expected to be leveraged by users writing
instrumentation libraries.

The Propagators API defines two main formats:

- `BinaryFormat` is used to serialize and deserialize a value into a binary representation.
- `HTTPTextFormat` is used to inject and extract a value as text into carriers that travel
Expand Down Expand Up @@ -59,8 +72,8 @@ Returns a value deserialized from bytes.

## HTTP Text Format

`HTTPTextFormat` is a formatter that injects and extracts a value as text into carriers that
travel in-band across process boundaries.
`HTTPTextFormat` is a formatter that injects and extracts a cross-cutting concern
value as text into carriers that travel in-band across process boundaries.
carlosalberto marked this conversation as resolved.
Show resolved Hide resolved

Encoding is expected to conform to the HTTP Header Field semantics. Values are often encoded as
RPC/HTTP request headers.
Expand Down Expand Up @@ -94,7 +107,7 @@ Injects the value downstream. For example, as http headers.

Copy link
Member

Choose a reason for hiding this comment

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

I do not follow anything in the preceding Fields section. It seems very vague and random. And certainly does not belong specifically under the HTTP format.

Required arguments:

- the value to be injected, can be `SpanContext` or `DistributedContext`.
- the cross-cutting concern value to be injected, such as `SpanContext` or `DistributedContext`.
- the carrier that holds propagation fields. For example, an outgoing message or http request.
- the `Setter` invoked for each propagation key to add or remove.

Expand Down Expand Up @@ -131,7 +144,7 @@ Required arguments:
- the carrier holds propagation fields. For example, an outgoing message or http request.
- the instance of `Getter` invoked for each propagation key to get.

Returns the non-null extracted value.
Returns the non-null cross-cutting concern extracted value.

#### Getter argument

Expand All @@ -151,3 +164,71 @@ Required arguments:
- the key of the field.

The Get function is responsible for handling case sensitivity. If the getter is intended to work with a HTTP request object, the getter MUST be case insensitive. To improve compatibility with other text-based protocols, text format implemenations MUST ensure to always use the canonical casing for their attributes. NOTE: Cannonical casing for HTTP headers is usually title case (e.g. `Content-Type` instead of `content-type`).

## Composite Propagator

Implementations MUST offer a facility to group multiple `Propagator`s
from different cross-cutting concerns in order to leverage them as a
single entity.

The resulting composite `Propagator` will invoke the `Propagators`
in the order they were specified.

Each composite `Propagator` will be bound to a specific format, such
as `HttpTextFormat`, as different formats will likely work on a different
carlosalberto marked this conversation as resolved.
Show resolved Hide resolved
data types.
There MUST be functions to accomplish the following operations.

- Create a composite propagator
- Extract from a composite propagator
- Inject into a composite propagator

carlosalberto marked this conversation as resolved.
Show resolved Hide resolved
### Create a Composite Propagator

Required arguments:

- A list of `Propagator`s.

Returns a new composite `Propagator` with the specified `Propagator`s.

### Extract
carlosalberto marked this conversation as resolved.
Show resolved Hide resolved

Required arguments:

- A `Context`.
carlosalberto marked this conversation as resolved.
Show resolved Hide resolved
- The carrier that holds propagation fields.
- the `Setter` invoked for each propagation key to add or remove.
carlosalberto marked this conversation as resolved.
Show resolved Hide resolved
carlosalberto marked this conversation as resolved.
Show resolved Hide resolved

carlosalberto marked this conversation as resolved.
Show resolved Hide resolved
### Inject
Copy link
Member

Choose a reason for hiding this comment

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

Are we going to specify whether inject mutates the carrier or returns a new one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So currently the carrier that is passed is in turn passed to the Setter instance, which decides what to do with it along with the keys/values. In practice (99% of the time?) the carrier is indeed mutated, but not 'enforced'.

Copy link
Member

Choose a reason for hiding this comment

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

I was going to mark this as resolved, but for some reason don't have the button available like in the spec repo. Are permissions different here?

Copy link
Member

Choose a reason for hiding this comment

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

The Carrier APIs should be defined earlier in the doc. They are generally mutable by definition, although I could see how this could irk some FP-oriented people. It is rather difficult to make individual pieces immutable and composable, because the Carrier API fundamentally implies some transformation that is unknown to the propagator, like writing every key-value pair as HTTP header is a specific implementation of HTTPRequest.

Copy link
Member

Choose a reason for hiding this comment

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

(well, it's actually not THAT difficult, using some pipeline like propagators.map(_.stateAsMap(context)).flatMap(carrier.accumulate),
but it's rather inefficient).

Copy link
Member

Choose a reason for hiding this comment

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

@dyladan You need to be the author who opened the PR or have write permissions to the repo (i.e., be in spec-approvers) to resolve conversations.


Required arguments:

- A `Context`.
carlosalberto marked this conversation as resolved.
Show resolved Hide resolved
- The carrier that holds propagation fields.
- The instance of `Getter` invoked for each propagation key to get.

## Global Propagators

Implementations can optionally provide global `Propagator`s for
carlosalberto marked this conversation as resolved.
Show resolved Hide resolved
each supported format.

If offered, the global `Propagator`s should default to a composite `Propagator`
containing W3C TraceContext and CorrelationContext `Propagator`s,
carlosalberto marked this conversation as resolved.
Show resolved Hide resolved
in order to provide propagation even in the presence of no-op
OpenTelemetry implementations.
carlosalberto marked this conversation as resolved.
Show resolved Hide resolved
carlosalberto marked this conversation as resolved.
Show resolved Hide resolved

### Get Global Propagator
Copy link
Member

Choose a reason for hiding this comment

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

Do we want one global propagator (which may be composite), or do we want to add the propagator to the TracerProvider? Reason being that if you have multiple apps in a single runtime, each of which has its own tracer provider, you may want to use different propagation formats between apps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To me doing that sounds like an overkill for now - in practice, it looks like most people want a single place/component to do the entire injection/extraction.

We could add in the future an additional Propagator component to items such as TracerProvider and similar components (defaulting to null) in case users want to (optionally) override the global one, though 😃

Copy link
Member

Choose a reason for hiding this comment

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

How should we be calling the propagator?

  1. api.propagator.extract(...)
  2. tracer.extract(...)
  3. tracer_provider.extract(...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You should be using 1) as this will be the 'default' thing - we might (but we might not) add 3) as a custom, override thing, in which case we could call that one as well.

The second option is due to be removed ;)


This method MUST exist for each supported format.

Returns a global `Propagator`. This usually will be composite instance.

### Set Global Propagator

This method MUST exist for each supported format.

Sets the global `Propagator` instance.

Required parameters:

- A `Propagator`. This usually will be a composite instance.