Add InstrumentationLibrary and plugin into the collector #94

bogdandrutu · 2020-01-14T19:23:21Z

Signed-off-by: Bogdan Drutu bogdandrutu@gmail.com

tigrannajaryan

@bogdandrutu can you please provide the context of this change? Is LibraryInfo something that was recently added to the spec? I check the last few commits in spec repo and cannot see anything relevant.

bogdandrutu · 2020-01-14T20:53:17Z

The Library info are the data used for the "Named Tracer/Meter" which are associated with every Span/Instrument created by these instances. I picked the name library info instead of "NamedXYZ".

tigrannajaryan · 2020-01-14T20:57:24Z

Why does it need to be a separate data structure and not simply part of Resource message? We already have semantic conventions that allow to differentiate library attributes from the rest, including library name and version: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/data-resource-semantic-conventions.md#library

bogdandrutu · 2020-01-14T21:01:55Z

Based on my understanding the library name inside the Resource is the "instrumentation" library (99% of the time opentelemetry), this is the "instrumented" library (e.g. mongo, etc.).

tigrannajaryan · 2020-01-14T21:05:49Z

Based on my understanding the library name inside the Resource is the "instrumentation" library (99% of the time opentelemetry), this is the "instrumented" library (e.g. mongo, etc.).

You are, right, my mistake. However, the original question still holds: would it be better if we defined semantic conventions for "instrumented" library or you think this is information is fundamental property of a span, will always be present and is better represented as a hard-coded Protobuf field?

bogdandrutu · 2020-01-14T21:21:54Z

Do you mean semantic conventions inside the Resource? I think this does not belong to the resource (which describes the application) and is a sub-component of the application described by the resource.

tigrannajaryan · 2020-01-15T00:16:31Z

Do you mean semantic conventions inside the Resource? I think this does not belong to the resource (which describes the application) and is a sub-component of the application described by the resource.

Yes, I was thinking library info could be added to Resource and one request could carry multiple ResourceSpans/ResourceMetrics messages, each carrying data from a particular Resource/Library combination.

However, it seems like it logically is separate, so making it a separate message is the right approach (what you did).

opentelemetry/proto/collector/trace/v1/trace_service.proto

Oberon00 · 2020-01-15T11:52:15Z

opentelemetry/proto/common/v1/common.proto

@@ -53,3 +53,10 @@ message StringKeyValue {
  string key = 1;
  string value = 2;
 }
+
+// LibraryInfo is a message representing the instrumented library informations such as the fully


Should this be

Suggested change

// LibraryInfo is a message representing the instrumented library informations such as the fully

// LibraryInfo is a message representing the instrumenting library informations such as the fully

and represent the information that comes from the "named tracer"?

Otherwise, where would this information come from?

It can be both instrumenting and instrumented. Think if a library has a dependency on otel and instruments itself (not via a plugin).

A library can be both. But that is a special case and in all cases where it's different using the "instrumented" library is wrong.

Oberon00 · 2020-01-15T11:57:14Z

Just calling out that we currently do not have any API nor conventions to report the "instrumented" library, just the "instrumenting" (aka reporting) library.

I did not know that "instrumentation" is yet another distinct thing, I thought that to be synonymous to "instrumenting" library.

opentelemetry/proto/collector/metrics/v1/metrics_service.proto

SergeyKanzhelev · 2020-01-15T22:08:44Z

I have the same qq as @tigrannajaryan . We already have this: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/data-resource-semantic-conventions.md#library

BTW, I do believe storing library info in resource has some side effects. Change like this may be needed, I want to make sure this is what you are trying to solve

opentelemetry/proto/common/v1/common.proto

SergeyKanzhelev

I'm blocking PR to better understand intent behind the change and make sure it's aligned with the changes on specs side.

SergeyKanzhelev · 2020-01-15T22:12:12Z

I have the same qq as @tigrannajaryan . We already have this: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/data-resource-semantic-conventions.md#library

BTW, I do believe storing library info in resource has some side effects. Change like this may be needed, I want to make sure this is what you are trying to solve.

bogdandrutu · 2020-01-23T00:30:51Z

@SergeyKanzhelev I think you got some answers, can you clarify what did we miss to explain?

SergeyKanzhelev

qq about language field and general observation that this may be generalized further

SergeyKanzhelev · 2020-01-23T06:34:14Z

opentelemetry/proto/common/v1/common.proto

+
+// LibraryInfo is a message representing the instrumented library informations such as the fully
+// qualified name and version. 
+message LibraryInfo {


Spec https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/data-resource-semantic-conventions.md#library ? also defines a language. Should it be defined here as well?

Does the discussion from today sig meeting + open-telemetry/oteps#84 + open-telemetry/opentelemetry-specification#494 clarifies.

@SergeyKanzhelev As discussed today, the linked resource convention is unrelated, so "language" does not apply here (yet).

opentelemetry/proto/collector/trace/v1/trace_service.proto

This adds the maturity levels for individual parts of the protocol according to recently agreed project-wide definition of maturity levels. After merging this change we will declare Trace (Span), Resource and Common parts of the protocol as Beta, which means no more than once in 3 months changes are allowed after that. We need this declaration of maturity so that we can begin implementation of OTLP in the Collector. One known pending change that prevents us from declaring the components as Stable is open-telemetry#94 We assume that if this change is accepted there will be no other changes within the next 3-months timeframe. The assumption seems reasonable since there are no other known shortcomings in this part of the protocol that may require more modifications.

This adds the maturity levels for individual parts of the protocol according to recently agreed project-wide definition of maturity levels. After merging this change we will declare Trace (Span), Resource and Common parts of the protocol as Beta, which means no more than once in 3 months changes are allowed after that. We need this declaration of maturity so that we can begin implementation of OTLP in the Collector. One known pending change that prevents us from declaring the components as Stable is open-telemetry#94 We assume that there will be no other changes within the next 3-months timeframe. The assumption seems reasonable since there are no other known shortcomings in this part of the protocol that may require more modifications. Note that metrics part is considered Alpha since it is still under active development.

This adds the maturity levels for individual parts of the protocol according to recently agreed project-wide definition of maturity levels. After merging this change we will declare Trace (Span), Resource and Common parts of the protocol as Beta, which means no more than once in 3 months changes are allowed after that. We need this declaration of maturity so that we can begin implementation of OTLP in the Collector. One known pending change that prevents us from declaring the components as Stable is #94 We assume that there will be no other changes within the next 3-months timeframe. The assumption seems reasonable since there are no other known shortcomings in this part of the protocol that may require more modifications. Note that metrics part is considered Alpha since it is still under active development. Co-authored-by: Sergey Kanzhelev <S.Kanzhelev@live.com> Co-authored-by: Bogdan Drutu <lazy@splunk.com>

bogdandrutu · 2020-03-09T22:31:17Z

Updated to match the current names proposed in the open-telemetry/oteps#84

opentelemetry/proto/metrics/v1/metrics.proto

opentelemetry/proto/common/v1/common.proto

arminru

Please also change the PR title to match the updated content before merging.

opentelemetry/proto/common/v1/common.proto

arminru · 2020-03-12T16:23:23Z

opentelemetry/proto/common/v1/common.proto

+// such as the fully qualified name and version. 
+message InstrumentationLibrary {
+  string name = 1;
+  string version = 2;


Please specify the format or add a link to the spec.
https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/api-tracing.md#obtaining-a-tracer is a bit vague on the version format, though.

opentelemetry/proto/metrics/v1/metrics.proto

opentelemetry/proto/trace/v1/trace.proto

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>

bogdandrutu requested review from AloisReitbauer, c24t, carlosalberto, iredelmeier, jmacd, reyang, SergeyKanzhelev, songy23, tedsuo, tigrannajaryan and yurishkuro as code owners January 14, 2020 19:23

bogdandrutu force-pushed the libinfo branch from 1a1ae82 to ca47835 Compare January 14, 2020 19:25

bogdandrutu mentioned this pull request Jan 14, 2020

Move Resource[Spans|Metrics] to the main proto files #95

Closed

tigrannajaryan reviewed Jan 14, 2020

View reviewed changes

Oberon00 reviewed Jan 15, 2020

View reviewed changes

tigrannajaryan reviewed Jan 15, 2020

View reviewed changes

opentelemetry/proto/collector/metrics/v1/metrics_service.proto Outdated Show resolved Hide resolved

bogdandrutu force-pushed the libinfo branch from 3c90304 to 85e4db4 Compare January 15, 2020 18:45

tigrannajaryan approved these changes Jan 15, 2020

View reviewed changes

SergeyKanzhelev reviewed Jan 15, 2020

View reviewed changes

opentelemetry/proto/common/v1/common.proto Outdated Show resolved Hide resolved

SergeyKanzhelev suggested changes Jan 15, 2020

View reviewed changes

Oberon00 mentioned this pull request Jan 21, 2020

Clarify named tracers and meters open-telemetry/oteps#76

Merged

SergeyKanzhelev suggested changes Jan 23, 2020

View reviewed changes

bogdandrutu force-pushed the libinfo branch from 85e4db4 to 60b204a Compare January 31, 2020 21:35

tigrannajaryan mentioned this pull request Feb 12, 2020

Declare Trace, Resource and Common as Beta #103

Merged

SergeyKanzhelev requested a review from arminru as a code owner February 17, 2020 19:57

mwear mentioned this pull request Feb 26, 2020

Associating Spans with a Resource open-telemetry/opentelemetry-specification#491

Closed

bogdandrutu force-pushed the libinfo branch 2 times, most recently from 766486e to add2ece Compare March 9, 2020 22:30

tigrannajaryan reviewed Mar 10, 2020

View reviewed changes

opentelemetry/proto/metrics/v1/metrics.proto Show resolved Hide resolved

opentelemetry/proto/metrics/v1/metrics.proto Show resolved Hide resolved

Oberon00 reviewed Mar 10, 2020

View reviewed changes

opentelemetry/proto/common/v1/common.proto Show resolved Hide resolved

Oberon00 approved these changes Mar 10, 2020

View reviewed changes

arminru approved these changes Mar 12, 2020

View reviewed changes

SergeyKanzhelev approved these changes Mar 13, 2020

View reviewed changes

bogdandrutu changed the title ~~Add LibraryInfo and plugin into the collector~~ Add InstrumentationLibrary and plugin into the collector Mar 13, 2020

bogdandrutu force-pushed the libinfo branch 2 times, most recently from 86a37b1 to 05b057d Compare March 14, 2020 00:03

Add InstrumentationLibrary and plugin into the collector

9f0bf32

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>

bogdandrutu force-pushed the libinfo branch from 05b057d to 9f0bf32 Compare March 14, 2020 00:15

bogdandrutu merged commit 425bcf2 into open-telemetry:master Mar 14, 2020

bogdandrutu deleted the libinfo branch March 14, 2020 00:17

SergeyKanzhelev mentioned this pull request May 7, 2020

Clarify and potentially remove InstrumentationLibrarySpans #149

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add InstrumentationLibrary and plugin into the collector #94

Add InstrumentationLibrary and plugin into the collector #94

bogdandrutu commented Jan 14, 2020

tigrannajaryan left a comment

bogdandrutu commented Jan 14, 2020

tigrannajaryan commented Jan 14, 2020

bogdandrutu commented Jan 14, 2020

tigrannajaryan commented Jan 14, 2020

bogdandrutu commented Jan 14, 2020

tigrannajaryan commented Jan 15, 2020

Oberon00 Jan 15, 2020

bogdandrutu Jan 15, 2020

Oberon00 Jan 21, 2020

Oberon00 commented Jan 15, 2020

SergeyKanzhelev commented Jan 15, 2020

SergeyKanzhelev left a comment

SergeyKanzhelev commented Jan 15, 2020

bogdandrutu commented Jan 23, 2020

SergeyKanzhelev left a comment

SergeyKanzhelev Jan 23, 2020

bogdandrutu Mar 10, 2020

Oberon00 Mar 10, 2020

bogdandrutu commented Mar 9, 2020

arminru left a comment

arminru Mar 12, 2020

	// LibraryInfo is a message representing the instrumented library informations such as the fully
	// LibraryInfo is a message representing the instrumenting library informations such as the fully

Add InstrumentationLibrary and plugin into the collector #94

Add InstrumentationLibrary and plugin into the collector #94

Conversation

bogdandrutu commented Jan 14, 2020

tigrannajaryan left a comment

Choose a reason for hiding this comment

bogdandrutu commented Jan 14, 2020

tigrannajaryan commented Jan 14, 2020

bogdandrutu commented Jan 14, 2020

tigrannajaryan commented Jan 14, 2020

bogdandrutu commented Jan 14, 2020

tigrannajaryan commented Jan 15, 2020

Oberon00 Jan 15, 2020

Choose a reason for hiding this comment

bogdandrutu Jan 15, 2020

Choose a reason for hiding this comment

Oberon00 Jan 21, 2020

Choose a reason for hiding this comment

Oberon00 commented Jan 15, 2020

SergeyKanzhelev commented Jan 15, 2020

SergeyKanzhelev left a comment

Choose a reason for hiding this comment

SergeyKanzhelev commented Jan 15, 2020

bogdandrutu commented Jan 23, 2020

SergeyKanzhelev left a comment

Choose a reason for hiding this comment

SergeyKanzhelev Jan 23, 2020

Choose a reason for hiding this comment

bogdandrutu Mar 10, 2020

Choose a reason for hiding this comment

Oberon00 Mar 10, 2020

Choose a reason for hiding this comment

bogdandrutu commented Mar 9, 2020

arminru left a comment

Choose a reason for hiding this comment

arminru Mar 12, 2020

Choose a reason for hiding this comment