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

Add InstrumentationLibrary to describe telemetry from a named Tracer/Meter #84

Merged
merged 7 commits into from
Mar 13, 2020

Conversation

bogdandrutu
Copy link
Member

No description provided.

@carlosalberto
Copy link
Contributor

This looks great to me, specially given we are not modifying the API itself. My opinion on two mentioned issues:

  1. I think at this time, we should only support name/version, as we can always add things in the future.
  2. Is Component's name/version a direct mapping from the named components in the API? Is this expected?

@bogdandrutu
Copy link
Member Author

  1. I think at this time, we should only support name/version, as we can always add things in the future.

I agree.

  1. Is Component's name/version a direct mapping from the named components in the API? Is this expected?

The name and version of the component are the values passed to getTracer and getMeter.

Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

In this proposal, component represents the telemetry library, not the component being instrumented correct?

For instance, if an autoinstrumentation library io.opentelemetry.contrib.auto-mysql version semver:3.0.2, which instruments a library mysql version semver:5.1.2, acquires named tracer with getTracer("io.opentelemetry.contrib.auto-mysql", "semver:3.0.2"), the component is meant to be { name: io.opentelemetry.contrib.auto-mysql, version: semver:3.0.2 }?

What should happen in the, admittedly contrived, case that a library instruments more than one thing? For instance, if an ORM which handles multiple database libraries acquires a tracer, spans for all database queries from the ORM would have the same component regardless of which database library is actually used to serve the request?

Copy link
Member

@Oberon00 Oberon00 left a comment

Choose a reason for hiding this comment

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

Suggesting changes, because it is dangerous to mix up instrumenting and instrumented libraries.

The name used to create a Tracer or Meter must identify the instrumentation libraries (also referred to as integrations) and not the library being instrumented.

-- https://github.com/open-telemetry/oteps/blob/284421863d258b90314a1fec3e84ce5377c585db/text/0016-named-tracers.md#explanation

text/0083-component.md Outdated Show resolved Hide resolved
text/0083-component.md Outdated Show resolved Hide resolved
text/0083-component.md Outdated Show resolved Hide resolved
text/0083-component.md Outdated Show resolved Hide resolved
text/0083-component.md Outdated Show resolved Hide resolved
text/0083-component.md Show resolved Hide resolved
@jmacd
Copy link
Contributor

jmacd commented Feb 4, 2020

In open-telemetry/opentelemetry-specification#354, there's a question about how to talk about the values passed to the named tracer. "Instrumenting library"?

@dyladan
Copy link
Member

dyladan commented Feb 4, 2020

In open-telemetry/opentelemetry-specification#354, there's a question about how to talk about the values passed to the named tracer. "Instrumenting library"?

Do you mean the values passed to the tracer provider? If so, then the OTEP (and incoming spec PR) is using the term "instrumentation library" (but it would not be incorrect to call it the instrumenting library). This is mainly to distinguish the instrumenting library from the instrumented library.

text/0083-component.md Outdated Show resolved Hide resolved
Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

A couple things that are not quite clear to me.

The intent of the “component” concept appears to be “the call-site that emits the telemetry”. The telemetry may be emitted to describe a behaviour of a code that resides elsewhere and may even be written by someone else (e.g. a 3rd party library that is being instrumented). The “component” does not unambiguously map 1:1 to source-code entities such as a source-code file, a source-code directory, a library or a package. A “component” is merely a grouping of telemetry data by an arbitrary criteria as it is seen fit by the developer who decides to emit that telemetry data using the same (or same-named) NamedTracer/NamedMeter object.

This does not limit the “component” to a particular instrumenting library either since NamedTracer/NamedMeter object may be passed around freely and can be used to emit telemetry about virtually anything the developer deems fit.

It appears that we want to make recommendations around how NamedTraced/NamedMeter should be used when one creates an instrumenting code for some other code that is being instrumented. If so then these recommendations should probably be part of OpenTelemetry documentation (if they are not already).

If the above few paragraphs are true then the “component” as I understand it is merely a logical grouping of telemetry data. Using “components” for instrumenting libraries is probably a frequent use-case, but perhaps not the only case (e.g. one can imagine a case when different parts of the Application are considered different “components” even though they are not libraries at all and are parts of the Application codebase).

Given such definition of “component” I do not know whether it is warranted to make them a hard-coded part of the protocol as opposed to simply storing the component name in Span and Metric attributes. The earlier can be seen as an optimization of the later for the case when the same component can emit a large number of Spans or Metrics. Either approach seems reasonable to me and I have no data to have a strong opinion one way or another.

It would be useful to see clarification on these matters in this OTEP or elsewhere.

text/0083-component.md Outdated Show resolved Hide resolved
@Oberon00
Copy link
Member

Honestly, I'm a bit confused how people are so confused about named tracers all the time. It seems people just hear the name and immediately in their mind they have an idea what that is/should be and then nobody reads the actual definitions what that "name" is. At this point maybe it's really better to rename the feature to ORI (Obligatory Reporter Info), then people have to read the docs.

Or am I wrong and is this OTEP not misunderstanding but attempting to completely change the definition of the tracer name?

This does not limit the “component” to a particular instrumenting library either since NamedTracer/NamedMeter object may be passed around freely and can be used to emit telemetry about virtually anything the developer deems fit.

You can technically do that but the definition of the tracer name and what it is supposed to mean is very clear and explicit.

It appears that we want to make recommendations around how NamedTraced/NamedMeter should be used

We already have definitions how named tracers MUST be used (maybe it's not 100% as clear for meters).

@bogdandrutu
Copy link
Member Author

@tigrannajaryan I am not sure I follow the entire comment. The rational to have this as a separate entity is for:

  • Performance on the wire;
  • For metrics, we do guarantee uniqueness of the names at the instrumenting library name, so some backends may use the instrumenting library name as prefix to all the metric names.
  • Some backends may use the instrumenting library name as prefix for Spans.

I am not sure why do we try to flatten everything when we do have this structure in the API, so it will be natural to use the same structure in the data model and not do any flattening. Also if we do use an attribute in the Span: for this exercise let's call it component then the user also records an attribute component what is the expected behavior?

@jmacd
Copy link
Contributor

jmacd commented Mar 9, 2020

What can we do to move this forward?

@bogdandrutu
Copy link
Member Author

bogdandrutu commented Mar 9, 2020

I am re-writing the otep using InstrumentationInfo as a replacement for the Component. Will have something in 1h

@bogdandrutu
Copy link
Member Author

@open-telemetry/specs-approvers please review this proposal again.

…Meter

Signed-off-by: Bogdan Cristian Drutu <bogdandrutu@gmail.com>
@bogdandrutu bogdandrutu changed the title Add component to describe telemetry from a named Tracer/Meter Add InstrumentationLibrary to describe telemetry from a named Tracer/Meter Mar 9, 2020
Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

This looks better to me than the previous version.

@bogdandrutu
Copy link
Member Author

Ping @open-telemetry/specs-approvers :)

Copy link
Member

@Oberon00 Oberon00 left a comment

Choose a reason for hiding this comment

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

Apart from my last comment in the Future possibilities, only copy editing, LGTM!

text/0083-component.md Outdated Show resolved Hide resolved
text/0083-component.md Outdated Show resolved Hide resolved
text/0083-component.md Outdated Show resolved Hide resolved
text/0083-component.md Outdated Show resolved Hide resolved
text/0083-component.md Outdated Show resolved Hide resolved
bogdandrutu and others added 6 commits March 13, 2020 12:34
Co-Authored-By: Christian Neumüller <christian+github@neumueller.me>
Co-Authored-By: Christian Neumüller <christian+github@neumueller.me>
Co-Authored-By: Christian Neumüller <christian+github@neumueller.me>
Co-Authored-By: Christian Neumüller <christian+github@neumueller.me>
Co-Authored-By: Christian Neumüller <christian+github@neumueller.me>
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.

None yet

9 participants