-
Notifications
You must be signed in to change notification settings - Fork 894
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
Conversation
30f836e
to
bddea67
Compare
bddea67
to
6358cf3
Compare
Please also resolve the conflicts :) |
868de24
to
05b0626
Compare
05b0626
to
d2544de
Compare
08efc33
to
eb0ac73
Compare
0ef4cbc
to
06a3e1a
Compare
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 |
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.
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?
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.
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?
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 |
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.
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.
A `Tracer` is more than just the code though. Each `Tracer` has a name and | |
Each `Tracer` has a name and |
specification/api-tracing.md
Outdated
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 | ||
optionally a version. The API must provide [library developers][] a way to |
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.
should this be normative?
optionally a version. The API must provide [library developers][] a way to | |
optionally a version. The API MUST provide [library developers][] a way to |
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 |
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 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.
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.
The otep says it is optional:
The version (following the convention proposed in open-telemetry/oteps#38) is basically optional but should be supplied
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.
Gotcha, thanks for the ref 😄
responsible for code that becomes a deployable artifact to run with some | ||
configuration by an operator. The application developer's project may depend | ||
on third party libraries that have been instrumented and may include its own | ||
libraries, making the application developer potentially a library developer as |
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.
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.
libraries, making the application developer potentially a library developer as | |
OpenTelemetry instrumentation, making the application developer potentially a library developer as |
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.
No, I don't think that was the intent here. It is definitely a confusing sentence now and maybe should just be removed.
on third party libraries that have been instrumented and may include its own | ||
libraries, making the application developer potentially a library developer as | ||
well. But only the end user, or operator, should include an OpenTelemetry SDK | ||
implementation as a dependency and configure, either through code or |
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.
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?
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 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.
Co-Authored-By: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-Authored-By: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-Authored-By: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-Authored-By: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-Authored-By: Tyler Yahn <MrAlias@users.noreply.github.com>
Getting the ball rolling. This is only a reworking of the initial Tracer definition and I think it still needs some work.
The "user definitions" shouldn't go in this document but I threw them in at the bottom just to get them somewhere for now.
I will be working on the "Tracer operations" section next and, assuming this PR isn't quickly finished up and merged (which I doubt), I'll be adding that commit to this PR.