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
Clean up Tracing API spec, clarify Tracer vs TracerProvider. #619
Changes from 23 commits
7cdc7d3
89816ca
156afbc
438d0ec
e0a1401
f7a6135
977abff
394adaf
1c50341
385ef1e
ecec9bf
915d6af
815ee15
b474d2c
b12f036
0cf54f9
6da477a
c31b87c
547d4ee
80877cb
9392125
b8c510c
95eb589
1afc409
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -142,15 +142,23 @@ TODO: Split out the parent handling. | |
## Tracer Creation | ||
|
||
New `Tracer` instances are always created through a `TracerProvider` (see | ||
[API](api.md#obtaining-a-tracer)). The `name` and `version` arguments | ||
[API](api.md#tracerprovicer)). The `name` and `version` arguments | ||
supplied to the `TracerProvider` must be used to create an | ||
[`InstrumentationLibrary`][otep-83] instance which is stored on the created | ||
`Tracer`. | ||
|
||
All configuration objects (SDK specific) and extension points (span processors, | ||
propagators) must be provided to the `TracerProvider`. `Tracer` instances must | ||
not duplicate this data (unless for read-only access) to avoid that different | ||
`Tracer` instances of a `TracerProvider` have different versions of these data. | ||
Configuration (i.e., [Span processors](#span-processor) and [`Sampler`](#sampling)) | ||
MUST be managed solely by the `TracerProvider` and it MUST provide some way to | ||
configure them, at least when creating or initializing it. | ||
|
||
The TracerProvider MAY provide methods to update the configuration. If | ||
configuration is updated (e.g., adding a `SpanProcessor`), | ||
the updated configuration MUST also apply to all already returned `Tracers` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
From the API, I would expect a SHOULD and not a MUST here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the SDK spec. The API spec allows an arbitrary implementation to "to work with an outdated configuration", but the spec for the official OpenTelemetry SDK can be stricter. |
||
(i.e. it MUST NOT matter whether a `Tracer` was obtained from the | ||
`TracerProvider` before or after the configuration change). | ||
Note: Implementation-wise, this could mean that `Tracer` instances have a | ||
reference to their `TracerProvider` and access configuration only via this | ||
reference. | ||
|
||
The readable representations of all `Span` instances created by a `Tracer` must | ||
provide a `getInstrumentationLibrary` method that returns the | ||
|
@@ -168,16 +176,6 @@ exportable representation and passing batches to exporters. | |
Span processors can be registered directly on SDK `TracerProvider` and they are | ||
invoked in the same order as they were registered. | ||
|
||
All `Tracer` instances created by a `TracerProvider` share the same span processors. | ||
Changes to this collection reflect in all `Tracer` instances. | ||
Implementation-wise, this could mean that `Tracer` instances have a reference to | ||
their `TracerProvider` and can access span processor objects only via this | ||
reference. | ||
|
||
Manipulation of the span processors collection must only happen on `TracerProvider` | ||
instances. This means methods like `addSpanProcessor` must be implemented on | ||
`TracerProvider`. | ||
|
||
Each processor registered on `TracerProvider` is a start of pipeline that consist | ||
of span processor and optional exporter. SDK MUST allow to end each pipeline with | ||
individual exporter. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we say the exact opposite? I am not winning the battle against globals, but I am going to continue fighting it. It's not "normal" to access a global, it's a provably bad design pattern. I would like OpenTelemetry specs to take a stance that "fall back to globals only when nothing else works", not as the first option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel you, but I don't think this PR is the right place for such design decisions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the same logic, this PR is not a place to introduce this kind of language if it wasn't in the spec previously.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It isn't the right place for that and it was not my intention to strengthen globals in the spec. The current wording even talks about "singletons", which are a design (anti-)pattern to prevent multiple instance of the same object, so I'd say I even moved the wording a bit away from "single global instance" here. See https://github.com/open-telemetry/opentelemetry-specification/blob/v0.5.0/specification/trace/api.md#obtaining-a-tracer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't consider singleton an anti-pattern - it simply means that there should be only one (*) instance of provider, which is true in most cases. Singleton != global, singletons can be managed properly as dependencies.
(*) as we discussed, the wording about situations where one needs multiple providers is pretty odd. E.g. in a JVM-based app server that runs multiple web apps (like Tomcat), absolutely nothing changes about having a single provider per web app, because class loaders perform the required isolation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is called singleton does imply global access. Sure there are other ways to ensure only one instance is created without also providing global access, but that's not the singleton design pattern anymore then.