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

Clarify named tracers and meters #76

Merged

Conversation

dyladan
Copy link
Member

@dyladan dyladan commented Jan 8, 2020

Based on conversations with SIGs, especially the specification, the overall motivations and use cases of named tracers seem to be poorly understood.

This change attempts to clarify the named tracer OTEP in order to address some of that confusion. It also adds a glossary of terms at the bottom in order to aid in that disambiguation.

@dyladan dyladan changed the title Clarifiy named tracers and meters Clarify named tracers and meters Jan 8, 2020
text/0016-named-tracers.md Outdated Show resolved Hide resolved
text/0016-named-tracers.md Outdated Show resolved Hide resolved
text/0016-named-tracers.md Outdated Show resolved Hide resolved
@jmacd
Copy link
Contributor

jmacd commented Jan 8, 2020

I would like to propose that we rename this feature something other than "Named" interfaces.

As we have learned--a number of people mis-interpreted this feature to be about providing a namespace for the spans and metrics created through the "Named" interface. The intention of this feature is to supply the "library name" and optional version. While the library does have a name, the library name and version are used here for identification, not the form of naming that many implementors assumed.

I would call this feature Obligatory Reporter Identification.

@danielkhan
Copy link

danielkhan commented Jan 8, 2020

I would call this feature Obligatory Reporter Identification.

I'm also fine with the 'Reporter' wording and we discussed a possible rename but we didn't want to create more confusion by renaming the thing while clarifying its intent.

text/0016-named-tracers.md Show resolved Hide resolved
text/0016-named-tracers.md Outdated Show resolved Hide resolved
@arminru
Copy link
Member

arminru commented Jan 10, 2020

@jmacd To me Obligatory Reporter Identification sounds a bit bulky but I see that it would certainly make the intent clearer than Named Tracer/Meter, even though the latter is already an established name for it.

Would you prefer to rename the provided parameter from instrumentation library to either reporter or reporting library, or would you keep it as it is? I agree with @danielkhan's reasoning above.

text/0016-named-tracers.md Outdated Show resolved Hide resolved
@carlosalberto
Copy link
Contributor

I'd also slightly prefer to have this feature renamed to something other than Named Tracers. Approving, otherwise this looks great.

@dyladan
Copy link
Member Author

dyladan commented Jan 26, 2020

I'd also slightly prefer to have this feature renamed to something other than Named Tracers. Approving, otherwise this looks great.

Some other names were floated but nothing has really stuck.

@jmacd
Copy link
Contributor

jmacd commented Jan 27, 2020

@open-telemetry/specs-approvers Please review and offer feedback or approve.


If no name (null or empty string) is specified, following the suggestions in ["error handling proposal"](https://github.com/open-telemetry/opentelemetry-specification/pull/153), a "smart default" will be applied and a default Tracer / Meter implementation is returned.

### Examples (of Tracer and Meter names)

Since Tracer and Meter names describe the libraries which use those Tracers and Meters, their names should be defined in a way that makes them as unique as possible.
The name of the Tracer / Meter should represent the identity of the library, class or package that provides the instrumentation.
The name of the Tracer / Meter should represent the identity of the library, class or package that provides the instrumentation.
Copy link
Member

@bogdandrutu bogdandrutu Jan 30, 2020

Choose a reason for hiding this comment

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

Just a small clarification, is the name the key that identifies the Tracer/Meter or name + version? What is the expected behavior when (I don't know how is it possible) there are 2 requests to the getTracer with same name and different versions?

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently unspecified. In the javascript version, you get a different instance of tracer per name-version pair, but you could conceivably get a new tracer every time getTracer is called. Do you think that needs to be specified? Since the tracer delegates all its configuration to the TracerProvider, it doesn't really matter if you make a new one per name, new one per name/version pair, new one every time, or just 1 instance of each tracer type (noop or working).

Copy link
Contributor

Choose a reason for hiding this comment

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

My suggestion is to work on this detail in the actual Specification, as this OTEP has been approved and is ready to merge.

Copy link
Member

Choose a reason for hiding this comment

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

I think it is important mainly for the metrics side. One important thing we need to ensure the name of the metrics are unique inside a "component" (instrumentation library), so if the component is identified by the name then I need to keep a list of all the registered instruments at the name level vs at the name+version level.

@bogdandrutu
Copy link
Member

Will close/reopen to re-trigger CLA check.

@bogdandrutu bogdandrutu reopened this Jan 30, 2020
@jmacd
Copy link
Contributor

jmacd commented Feb 3, 2020

This is ready to merge, right?

@carlosalberto carlosalberto merged commit 0c9b077 into open-telemetry:master Feb 3, 2020
@carlosalberto
Copy link
Contributor

Merged, thanks - as a further reminder, please address the last question by Bogdan (about name/version identity) when writing the actual specification side, as it might need further refinement.

@bogdandrutu
Copy link
Member

@jmacd @carlosalberto I was not sure if this is ready since I raised one issue and I didn't hear back any comment. Not a problem that it is merged but was waiting for that response.

@carlosalberto
Copy link
Contributor

@bogdandrutu I see, I thought I was agreed this would be tackled when writing the specification side, my bad. Opened an issue to keep track of this when @dyladan (or anybody else) includes this OTEP into the Specification: open-telemetry/opentelemetry-specification#434

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.