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

Clean up Tracing API spec, clarify Tracer vs TracerProvider. #619

Merged

Conversation

Oberon00
Copy link
Member

@Oberon00 Oberon00 commented May 25, 2020

This cleans up the tracing API spec, and clarifies what TracerProviders/Tracers are (not) allowed to do for different Tracers.

It also removes some wording regarding propagators that seems obsolete by now (OTEP66).

specification/trace/api.md Outdated Show resolved Hide resolved
specification/trace/api.md Outdated Show resolved Hide resolved
specification/trace/api.md Outdated Show resolved Hide resolved
specification/trace/api.md Outdated Show resolved Hide resolved
specification/trace/api.md Outdated Show resolved Hide resolved
specification/trace/api.md Outdated Show resolved Hide resolved
specification/trace/api.md Outdated Show resolved Hide resolved
specification/trace/api.md Outdated Show resolved Hide resolved
specification/trace/api.md Outdated Show resolved Hide resolved
Copy link
Contributor

@z1c0 z1c0 left a comment

Choose a reason for hiding this comment

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

Looks great!

specification/trace/api.md Outdated Show resolved Hide resolved
specification/trace/api.md Outdated Show resolved Hide resolved
specification/trace/api.md Outdated Show resolved Hide resolved
specification/trace/api.md Outdated Show resolved Hide resolved
specification/trace/api.md Outdated Show resolved Hide resolved
specification/trace/api.md Outdated Show resolved Hide resolved
specification/trace/api.md Outdated Show resolved Hide resolved
specification/trace/api.md Outdated Show resolved Hide resolved
specification/trace/api.md Outdated Show resolved Hide resolved
specification/trace/sdk.md Outdated Show resolved Hide resolved
specification/trace/api.md Outdated Show resolved Hide resolved

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`
Copy link
Member

Choose a reason for hiding this comment

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

This can be achieved either by allowing to work with an outdated configuration or
by ensuring that new configuration applies also to previously returned Tracers.

From the API, I would expect a SHOULD and not a MUST here

Copy link
Member Author

@Oberon00 Oberon00 Jun 10, 2020

Choose a reason for hiding this comment

The 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.

@jmacd
Copy link
Contributor

jmacd commented Jun 10, 2020

Just about everything being said about Providers and globals in this PR applies to Meters and MeterProviders. I filed #587 asking that we separate the specification for Providers so that we are not duplicating this kind of text between the two.

The only trace-specific aspects that I see here are in the examples: "for example, in Tracing, the active span is per-provider". Could we split this document out of the tracing API/SDK and create a dedicated place for this content?

@Oberon00
Copy link
Member Author

I think having a general definition for Foo vs FooProvider could very well make sense, but this is out of Scope for this PR (see issue #587).

@carlosalberto carlosalberto added spec:trace Related to the specification/trace directory area:sdk Related to the SDK labels Jun 12, 2020
@Oberon00
Copy link
Member Author

@open-telemetry/technical-committee This is ready to merge (addressing TC because I think you are the only ones with merge access).

@Oberon00
Copy link
Member Author

@carlosalberto I think this is mainly area:api.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:sdk Related to the SDK spec:trace Related to the specification/trace directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants