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

[language-agnostic-wg] first attempt at a language neutral explanation of the Tracer API #354

Closed
wants to merge 25 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
79f45e6
first attempt at a language neutral explanation of the Tracer API
tsloughter Nov 14, 2019
a763ef9
add separate glossary file
tsloughter Nov 14, 2019
3f475fb
remove "span ending" from what the Tracer is responsible for
tsloughter Nov 14, 2019
9b208db
explain need for passing name/version at tracer retrieval time
tsloughter Nov 14, 2019
d891d8f
add user definitions for api and sdk developer
tsloughter Nov 15, 2019
b1c3e16
use markdown shortcut for common link target
tsloughter Jan 3, 2020
be6bf16
add note about operator role
tsloughter Jan 3, 2020
5a9977e
replace 'end user' with 'application developer'
tsloughter Jan 3, 2020
347f847
cleanup based on PR reviews
tsloughter Jan 3, 2020
8b92bb5
remove required/optional from name for named tracer
tsloughter Jan 3, 2020
8ff3174
add TracerProvider to Tracer section of api-tracing
tsloughter Jan 3, 2020
b7cd992
change method to function
tsloughter Jan 3, 2020
c3339a9
merge of updates from Yuri and Daniel
tsloughter Jan 3, 2020
5c6707f
remove mention of tracer being used for context and propagation
tsloughter Jan 18, 2020
1bfbb2b
reorder glossary
tsloughter Jan 18, 2020
313c3dd
remove mention of Factory
tsloughter Jan 19, 2020
da41503
remove confusing sentences about not how tracers are named
tsloughter Jan 19, 2020
fa2873c
remove operator restriction from glossary
tsloughter Jan 19, 2020
82e6078
fix table of contents
tsloughter Jan 19, 2020
06a3e1a
remove mention of Resource from named tracers
tsloughter Feb 5, 2020
6d32d43
Update specification/api-tracing.md
tsloughter Feb 7, 2020
7e8fa47
Update specification/glossary.md
tsloughter Feb 7, 2020
d4d1214
Update specification/glossary.md
tsloughter Feb 7, 2020
2a4b7ef
Update specification/glossary.md
tsloughter Feb 7, 2020
efd8003
Update specification/api-tracing.md
tsloughter Feb 8, 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
98 changes: 39 additions & 59 deletions specification/api-tracing.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,6 @@ Table of Contents
* [Tracer](#tracer)
* [Obtaining a tracer](#obtaining-a-tracer)
* [Tracer operations](#tracer-operations)
* [GetCurrentSpan](#getcurrentspan)
* [WithSpan](#withspan)
* [SpanBuilder](#spanbuilder)
* [GetBinaryFormat](#getbinaryformat)
* [GetHttpTextFormat](#gethttptextformat)
* [SpanContext](#spancontext)
* [Span](#span)
* [Span creation](#span-creation)
Expand Down Expand Up @@ -73,69 +68,51 @@ A duration is the elapsed time between two events.

## Tracer

The OpenTelemetry library achieves in-process context propagation of `Span`s by
way of the `Tracer`.
A `Tracer` is the code responsible for starting `Spans`s and it exposes the API
which [library developers][] use when instrumenting their code. The API MUST
allow the [application developers][] to configure or specify the implementation
Copy link
Contributor

Choose a reason for hiding this comment

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

These last two sentences seem out of place. I interpret them as describing the overall project design rather than descriptions of the Tracer. Have I missed your intent?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at another PR I noticed a similar idea in an introduction: https://github.com/open-telemetry/opentelemetry-specification/pull/430/files#diff-9c6d57b34defe784fc21653cedacae6dR56-R58

I'm guessing this was the intent here, right?

at runtime. The implementation is referred to as the SDK in this spec, which is
used by all instrumented code within the program.

The `Tracer` is responsible for tracking the currently active `Span`, and
exposes methods for creating and activating new `Span`s. The `Tracer` is
configured with `Propagator`s which support transferring span context across
process boundaries.
A `Tracer` is more than just the code though. Each `Tracer` has a name and
Copy link
Contributor

Choose a reason for hiding this comment

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

This first sentence doesn't seem to add to the idea this paragraph is about. Instead it seems to add conflict with the first sentence of the previous paragraph.

Suggested change
A `Tracer` is more than just the code though. Each `Tracer` has a name and
Each `Tracer` has a name and

Copy link
Contributor

Choose a reason for hiding this comment

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

It might be useful to say:

A Tracer is identified by a name and optionally a version.

This would help convey the significance of the name/version pair.

Also, is the version optional? I thought based on OTEP 16 this was a needed part of the tracer identity.

Copy link
Member Author

Choose a reason for hiding this comment

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

The otep says it is optional:

The version (following the convention proposed in open-telemetry/oteps#38) is basically optional but should be supplied

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha, thanks for the ref 😄

optionally a version. The API must provide [library developers][] a component to
access a `Tracer` with a specific name and version. We'll call that
component the `TracerProvider`.

### Obtaining a Tracer
If [application developers][] do not specify a library which implements
the `TracerProvider`, like the OpenTelemetry SDK, the API MUST include a default
minimal implementation of a `TracerProvider` which returns a no-op `Tracer`. The
[library developers][] MUST be able to depend on the API and instrument their
Copy link
Contributor

Choose a reason for hiding this comment

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

This sentence is phrase in a way that describes the general guarantees the project has set forth about the API/SDK separation. Can we change it to be more specific about the TraceProvider? Maybe something like this:

The requirement for a default TraceProvider is motivated by the fact that [library developers][] depend solely on the API not the implementation of the API. The code they instrument needs to function regardless of an SDK being configured or not.

This way it still helps lead into the next topic of the library developer in the next paragraph, but is framed around the TraceProvider still.

(Also, I don't think this needed to be normative, but the previous sentence should be. Please correct me if I missed the intent)

code without thought to whether or not the final deployable application includes
the SDK or any other implementation.

New `Tracer` instances can be created via a `TracerFactory` and its `getTracer`
method. This method expects two string arguments:
To facilitate this, the [library developers][] MUST NOT specify a
`TracerProvider` implementation to use. The API MUST provide a way for the
developer to access a `Tracer` from a `TracerProvider`, which at runtime may be
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a redundant requirement already introduced above. We should remove it.

the default minimal implementation from the API, the default full implementation
known as the SDK, or a third party implementation.

`TracerFactory`s are generally expected to be used as singletons. Implementations
SHOULD provide a single global default `TracerFactory`.
### Named Tracers
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be a separate section? I don't think there are anything but Named Tracers at this point. It seems like it can be merged into the Tracer section.


Some applications may use multiple `TracerFactory` instances, e.g. to provide
different settings (e.g. `SpanProcessor`s) to each of those instances and -
in further consequence - to the `Tracer` instances created by them.
[library developers][] can, and should, give the `Tracer` a name and version:
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be normative?

Suggested change
[library developers][] can, and should, give the `Tracer` a name and version:
[library developers][] SHOULD give the `Tracer` a name and version:


- `name` (required): This name must identify the instrumentation library (also
referred to as integration, e.g. `io.opentelemetry.contrib.mongodb`) and *not*
- `name`: This name must identify the instrumentation library (also
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- `name`: This name must identify the instrumentation library (also
- `name`: This name MUST identify the instrumentation library (also

referred to as integration, e.g. `io.opentelemetry.contrib.mongodb`) and NOT
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
referred to as integration, e.g. `io.opentelemetry.contrib.mongodb`) and NOT
referred to as integration, e.g. `io.opentelemetry.contrib.mongodb`) and MUST NOT refer to

the instrumented library.
In case an invalid name (null or empty string) is specified, a working
default Tracer implementation as a fallback is returned rather than returning
null or throwing an exception.
A library, implementing the OpenTelemetry API *may* also ignore this name and
return a default instance for all calls, if it does not support "named"
functionality (e.g. an implementation which is not even observability-related).
A TracerFactory could also return a no-op Tracer here if application owners configure
the SDK to suppress telemetry produced by this library.
- `version` (optional): Specifies the version of the instrumentation library
(e.g. `semver:1.0.0`).

Implementations might require the user to specify configuration properties at
`TracerFactory` creation time, or rely on external configuration, e.g. when using the
provider pattern.

#### Runtimes with multiple deployments/applications

Runtimes that support multiple deployments or applications might need to
provide a different `TracerFactory` instance to each deployment. To support this,
the global `TracerFactory` registry may delegate calls to create new instances of
`TracerFactory` to a separate `Provider` component, and the runtime may include
its own `Provider` implementation which returns a different `TracerFactory` for
each deployment.

`Provider` instances are registered with the API via some language-specific
mechanism, for instance the `ServiceLoader` class in Java.
- `version` (optional): Specifies the
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this needs to be normative, but I'm not sure (based on OTEP 16) that is is optional. Maybe the optional/required question is out of scope of this PR though 🤷‍♀.

Suggested change
- `version` (optional): Specifies the
- `version` (OPTIONAL): Specifies the

version of the instrumentation library (e.g. `semver:1.0.0`).

The API MUST offer functionality for the library developer to pass the `name`
and `version` to `TracerProvider` when retrieving the `Tracer`.

### Tracer operations

The `Tracer` MUST provide methods to:
There MUST be functions associated with a `Tracer` to:

- Get the currently active `Span`
- Create a new `Span`
- Make a given `Span` as active

The `Tracer` SHOULD allow end users to configure other tracing components that
control how `Span`s are passed across process boundaries, including the binary
and text format `Propagator`s used to serialize `Span`s created by the
`Tracer`.

When getting the current span, the `Tracer` MUST return a placeholder `Span`
with an invalid `SpanContext` if there is no currently active `Span`.

Expand All @@ -146,7 +123,7 @@ explicit parent is provided or the option to create a span without a parent is
selected, or the current active `Span` is invalid.

The `Tracer` MUST provide a way to update its active `Span`, and MAY provide
convenience methods to manage a `Span`'s lifetime and the scope in which a
convenience functions to manage a `Span`'s lifetime and the scope in which a
`Span` is active. When an active `Span` is made inactive, the previously-active
`Span` SHOULD be made active. A `Span` maybe finished (i.e. have a non-null end
time) but stil active. A `Span` may be active on one thread after it has been
Expand Down Expand Up @@ -337,7 +314,7 @@ Links SHOULD preserve the order in which they're set.

### Span operations

With the exception of the method to retrieve the `Span`'s `SpanContext` and
With the exception of the function to retrieve the `Span`'s `SpanContext` and
recording status, none of the below may be called after the `Span` is finished.

#### Get Context
Expand Down Expand Up @@ -452,10 +429,10 @@ It is highly discouraged to update the name of a `Span` after its creation.
spans. And often, filtering logic will be implemented before the `Span` creation
for performance reasons. Thus the name update may interfere with this logic.

The method name is called `UpdateName` to differentiate this method from the
regular property setter. It emphasizes that this operation signifies a
major change for a `Span` and may lead to re-calculation of sampling or
filtering decisions made previously depending on the implementation.
The name `UpdateName`, as opposed to `SetName`, is to emphasize that this
Copy link
Contributor

Choose a reason for hiding this comment

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

With the goal of making this language agnostic we should probably rephrase this paragraph to be centered around the functional importance of this function rather than the name (which is stylistically language dependent).

Possibly:

This span operation affect a major change for a Span rather than simply replacing the Span. Due to the chance for major changes of the Span, this operation may lead to re-calculation of sampling or filtering decisions made previously depending on the implementation.

operation signifies a major change for a `Span` and may lead to re-calculation
of sampling or filtering decisions made previously depending on the
implementation.

Alternatives for the name update may be late `Span` creation, when Span is
started with the explicit timestamp from the past at the moment where the final
Expand Down Expand Up @@ -633,3 +610,6 @@ To summarize the interpretation of these kinds:
| `PRODUCER` | | yes | | maybe |
| `CONSUMER` | | yes | maybe | |
| `INTERNAL` | | | | |

[library developers]: glossary.md#library-developer
[application developers]: glossary.md#app-developer
28 changes: 28 additions & 0 deletions specification/glossary.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
# Glossary

## User Definitions

- <a name="app-developer"></a>Application Developer: An application developer is
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these list items be subsections instead? That way standard markdown section header linking can be used instead of having this HTML?

responsible for code that becomes a deployable artifact to run with some
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
responsible for code that becomes a deployable artifact to run with some
responsible for code that becomes a runnable artifact to run with some

configuration by an operator. The application developer's code may depend
on third party libraries that have been instrumented with OpenTelemetry and may include its own
libraries, making the application developer potentially a library developer as
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we replace "libraries" with "OpenTelemetry instrumentation"? I think we are trying to say that by performing their own instrumentation they become library developers as well, not that by including their own libraries makes them library developers.

Suggested change
libraries, making the application developer potentially a library developer as
OpenTelemetry instrumentation, making the application developer potentially a library developer as

Copy link
Member Author

Choose a reason for hiding this comment

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

No, I don't think that was the intent here. It is definitely a confusing sentence now and maybe should just be removed.

well. But only the end user, or operator, should include an OpenTelemetry SDK
implementation as a dependency and configure, either through code or
Copy link
Contributor

Choose a reason for hiding this comment

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

This sentence seems out of place for a user glossary definition for an application developer. It seems prescriptive of the operation of OpenTelemetry instrumented software rather than defining a user category.

Does it belong somewhere in the specification or maybe a Roles section?

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 could see all of this instead moved to a roles section of the spec and removing the glossary. I dont't think these roles being in a glossary can say much if they don't describe these points, so if a Roles section is to be done instead I'd say it should all be moved.

configuration files loaded by the program, the `TracerProvider` used by all libraries
within the final program.
- <a name="library-developer"></a>Library Developer: This is a developer working
on code that will be integrated into other code. They are not creating a final deployable
artifact and must only rely on the OpenTelemetry API as a dependency of their
library. The library may have the express purpose of making another library
observable (such libraries are called "integrations") or they may develop a
library with any other purpose that has observability built in (e.g., a
database client library that creates Spans itself when making database calls).
- <a name="api-developer"></a>API Developer: A developer working on the
OpenTelemetry API library for a particular language.
- <a name="sdk-developer"></a>SDK Developer: A developer working on implementing
the logic either defined in the official SDK specification or a third party
implementation. The implementation must be used only through the API by a
library developer or application developer.
- <a name="operator"></a>Operator: The person deploying and maintaining the
deployable artifact.