Add translation of InstrumentationLibraryInfo to Zipkin #800

iNikem · 2020-08-13T19:26:50Z

Changes

Add rules for translating InstrumentationLibraryInfo from OTLP to Zipkin

tigrannajaryan · 2020-08-13T20:14:33Z

specification/trace/sdk_exporters/zipkin.md

+### InstrumentationLibraryInfo
+
+OpenTelemetry Span's `InstrumentationLibraryInfo` MUST be reported as `tags` to Zipkin using the following mapping.
+
+| OpenTelemetry | Zipkin
+| ------------- | ------ |
+| `InstrumentationLibraryInfo.name`|`otel.instrumentationLibrary.name`|
+| `InstrumentationLibraryInfo.version`|`otel.instrumentationLibrary.version`|


I don't think this information is exclusive to Zipkin. This is applicable to any format that does not have the built-in notion of InstrumentationLibrary (which is probably every format other than OTLP).

I think it is best to extract this to a separate file that describes common translation rules for all formats. I would aim to have a semantic convention for others fields in OTLP that can be used if there is no equivalent field (or combination of fields) in the target format. We will need such mapping for all OTLP fields which do not have corresponding industry-wide recognition and are not present in all other formats.

https://github.com/open-telemetry/opentelemetry-collector/blob/master/translator/trace/protospan_translation.go is a good starting point of conventions that Collector already uses unofficially.

This is a common issue that every non-OTLP exporter in every SDK has to deal with (and obviously also the Collector when doing the translations).

I believe it will be useful to capture this in a single doc.

I think this document tries to become the official way to do the translation, so collector should follow this as well. This translation happens in multiple places sdks for every exporter and collecor

@bogdandrutu yes, that's what I meant, sorry if it was not clear. I am just saying it is better to put this information in a general translations file, not in the file which describes Zipkin translation, because we have the exact same problem to solve for Jaeger, for OpenCensus and any other format.

specification/trace/sdk_exporters/zipkin.md

Co-authored-by: Tigran Najaryan <[email protected]>

SergeyKanzhelev · 2020-08-14T04:32:49Z

specification/trace/sdk_exporters/zipkin.md

+
+| OpenTelemetry | Zipkin
+| ------------- | ------ |
+| `InstrumentationLibraryInfo.name`|`otel.instrumentation_library.name`|


where the InstrumentationLibraryInfo.name is coming from? Cannot find it: https://github.com/open-telemetry/opentelemetry-specification/search?q=InstrumentationLibraryInfo&unscoped_q=InstrumentationLibraryInfo

From OTLP: https://github.com/open-telemetry/opentelemetry-proto/blob/master/opentelemetry/proto/common/v1/common.proto#L74

Also from the "Name Tracer/Meter" :)

It is not called InstrumentationLibraryInfo in this spec. There is a reference to InstrumentationLibrary that points to OTEP instead of the document in this repo.

opentelemetry-specification/specification/trace/sdk.md

Line 141 in bc272ab

[`InstrumentationLibrary`][otep-83] instance which is stored on the created

Can you please send PR with the move of InstrumentaitonLibrary description from OTEP to specs? and rename InstrumentationLibraryInfo to InstrumentationLibrary

I will certainly rename InstrumentationLibraryInfo to InstrumentationLibrary. I took that name from Java SDK, my mistake.

But I am unsure about your second request. Do you ask me to make a PR to integrate otel-83 into spec? And that PR is pre-requirement for merging this PR?? Isn't that a huge scope creep?

@iNikem it is. I'm hoping you can take this work item. I agree it's not part of this PR, this is why I asked if you can please send PR.

Renaming would be part of this one though.

I can create an issue about that and self-assigned it. I hope this PR (and its twin about Jaeger) can be merged without requirement of that issue to be resolved.

Made #809 . Don't have permissions to assign it to myself.

tigrannajaryan

Approving.
@iNikem please add the issue as discussed here #801 (comment)

iNikem · 2020-08-14T15:53:55Z

Created #806 as a follow-up

anuraaga

Not blocker but find must to be too strict here

anuraaga · 2020-08-14T16:02:42Z

specification/trace/sdk_exporters/zipkin.md

+
+### InstrumentationLibraryInfo
+
+OpenTelemetry Span's `InstrumentationLibraryInfo` MUST be reported as `tags` to Zipkin using the following mapping.


Should this be MUST? Just for reference, for now in the otel x-ray exporter we are ignoring this information when exporting since it doesn't seem very useful for customers. We generally expect the span name and other attributes to provide enough - do we want to mandate this for all backends, including zipkin?

I think "MUST" is the correct one, but we can ask @open-telemetry/specs-trace-approvers @open-telemetry/technical-committee

I think "MUST" fits. If this attribute is not reported then we will lose data during translation. IMO requirements that prevent data losses qualify for a "MUST".

iNikem · 2020-08-14T18:55:14Z

@SergeyKanzhelev do you still want to block this PR?

anuraaga

Not blocking, but still a concern about MUST. Is it possible for a user to delete these attributes? Since this happens at the exporter level, I'm not so sure. e.g., collector with OTLP receiver and zipkin exporter, attribute processor can be used to filter out attributes that a user doesn't need and wants to save space. Is it possible for similar for this protocol level field?

iNikem · 2020-08-17T08:14:37Z

@yurishkuro Can this be merged now?

SergeyKanzhelev · 2020-08-17T16:50:19Z

Not blocking, but still a concern about MUST. Is it possible for a user to delete these attributes? Since this happens at the exporter level, I'm not so sure. e.g., collector with OTLP receiver and zipkin exporter, attribute processor can be used to filter out attributes that a user doesn't need and wants to save space. Is it possible for similar for this protocol level field?

Interesting concern... I can see why it may be needed. I wonder if to implement it we need to allow span processors to set the InstrumentationLibrary to null or empty value to make the solution more generic. I think MUST here reflects the reality. Unless we declare the configuraiton properties for exporters, it is better to have consistent transformation across languages. In any case doens't look to be blocking this PR. Definitely thank you for bringing this concern! If you believe this needs to be addressed before GA - please file an issue. Otherwise I'd suggest we wait for the end user feedback

SergeyKanzhelev · 2020-08-17T18:35:17Z

More than 2 business days since the last update and enough approvals

This addresses spec issues open-telemetry/opentelemetry-specification#800 open-telemetry/opentelemetry-specification#801 and opentelemetry-go issues open-telemetry#1086 open-telemetry#1087

* Add InstrumentationLibrary info to Zipkin/Jaeger exporters This addresses spec issues open-telemetry/opentelemetry-specification#800 open-telemetry/opentelemetry-specification#801 and opentelemetry-go issues #1086 #1087 * Reflect change in CHANGELOG

…metry#1119) * Add InstrumentationLibrary info to Zipkin/Jaeger exporters This addresses spec issues open-telemetry/opentelemetry-specification#800 open-telemetry/opentelemetry-specification#801 and opentelemetry-go issues open-telemetry#1086 open-telemetry#1087 * Reflect change in CHANGELOG

According to spec open-telemetry/opentelemetry-specification#800 and open-telemetry/opentelemetry-specification#801, We MUST export instrumentation library information for Jaeger and Zipkin exporter

…ent version for Jaeger. According to spec open-telemetry/opentelemetry-specification#800 and open-telemetry/opentelemetry-specification#801, We MUST export instrumentation library information for Jaeger and Zipkin exporter. Use crate version as default Jaeger instrumentation library version in consistent with Zipkin.

…ent version for Jaeger. (#243) According to spec open-telemetry/opentelemetry-specification#800 and open-telemetry/opentelemetry-specification#801, We MUST export instrumentation library information for Jaeger and Zipkin exporter.

* Add InstrumentationLibrary info to Zipkin/Jaeger exporters This addresses spec issues open-telemetry/opentelemetry-specification#800 open-telemetry/opentelemetry-specification#801 and opentelemetry-go issues open-telemetry/opentelemetry-go#1086 open-telemetry/opentelemetry-go#1087 * Reflect change in CHANGELOG

…ent version for Jaeger. (#243) According to spec open-telemetry/opentelemetry-specification#800 and open-telemetry/opentelemetry-specification#801, We MUST export instrumentation library information for Jaeger and Zipkin exporter.

…ry#800) * Add translation of InstrumentationLibraryInfo to Zipkin * Update specification/trace/sdk_exporters/zipkin.md Co-authored-by: Tigran Najaryan <[email protected]> * Fix case * Rename InstrumentationLibraryInfo to InstrumentationLibrary Co-authored-by: Tigran Najaryan <[email protected]>

Add translation of InstrumentationLibraryInfo to Zipkin

2654842

iNikem requested review from a team and yurishkuro August 13, 2020 19:26

tigrannajaryan reviewed Aug 13, 2020

View reviewed changes

tigrannajaryan mentioned this pull request Aug 13, 2020

Add translation of InstrumentationLibraryInfo to Jaeger #801

Merged

tigrannajaryan reviewed Aug 13, 2020

View reviewed changes

specification/trace/sdk_exporters/zipkin.md Outdated Show resolved Hide resolved

Update specification/trace/sdk_exporters/zipkin.md

f84f1b2

Co-authored-by: Tigran Najaryan <[email protected]>

SergeyKanzhelev suggested changes Aug 14, 2020

View reviewed changes

Fix case

4e79611

iNikem mentioned this pull request Aug 14, 2020

Add instrumentation library info as semantic convention attributes #746

Closed

tigrannajaryan approved these changes Aug 14, 2020

View reviewed changes

iNikem mentioned this pull request Aug 14, 2020

Extract common protocol translation rules into single document #806

Open

anuraaga approved these changes Aug 14, 2020

View reviewed changes

bogdandrutu requested a review from SergeyKanzhelev August 14, 2020 17:27

bogdandrutu assigned yurishkuro Aug 14, 2020

Rename InstrumentationLibraryInfo to InstrumentationLibrary

9d0c71e

SergeyKanzhelev approved these changes Aug 14, 2020

View reviewed changes

anuraaga approved these changes Aug 15, 2020

View reviewed changes

SergeyKanzhelev assigned SergeyKanzhelev and unassigned yurishkuro Aug 17, 2020

SergeyKanzhelev merged commit 1af9464 into open-telemetry:master Aug 17, 2020

iNikem deleted the library-info-zipkin branch August 17, 2020 18:36

anuraaga mentioned this pull request Aug 18, 2020

Consideration when using OTel SDK (and maybe collector) with non-OTLP backend #823

Open

andrewhsu mentioned this pull request Aug 21, 2020

Integrate OTEP-83 (Introducing InstrumentationLibrary) into spec #809

Open

MrAlias mentioned this pull request Aug 24, 2020

Add translation of InstrumentationLibraryInfo to Zipkin open-telemetry/opentelemetry-go#1087

Closed

evantorrie mentioned this pull request Sep 2, 2020

Add InstrumentationLibrary info to Zipkin/Jaeger exporters open-telemetry/opentelemetry-go#1119

Merged

TommyCpp mentioned this pull request Oct 4, 2020

[exporter] Export instrument library information in Jaeger and Zipkin. open-telemetry/opentelemetry-rust#243

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add translation of InstrumentationLibraryInfo to Zipkin #800

Add translation of InstrumentationLibraryInfo to Zipkin #800

iNikem commented Aug 13, 2020

tigrannajaryan Aug 13, 2020

bogdandrutu Aug 13, 2020

tigrannajaryan Aug 13, 2020

SergeyKanzhelev Aug 14, 2020

iNikem Aug 14, 2020

bogdandrutu Aug 14, 2020

SergeyKanzhelev Aug 14, 2020

iNikem Aug 14, 2020

SergeyKanzhelev Aug 14, 2020

iNikem Aug 14, 2020

iNikem Aug 14, 2020 •

edited

Loading

tigrannajaryan left a comment

iNikem commented Aug 14, 2020

anuraaga left a comment

anuraaga Aug 14, 2020

iNikem Aug 14, 2020

tigrannajaryan Aug 14, 2020

iNikem commented Aug 14, 2020 •

edited

Loading

anuraaga left a comment

iNikem commented Aug 17, 2020

SergeyKanzhelev commented Aug 17, 2020

SergeyKanzhelev commented Aug 17, 2020


		### InstrumentationLibraryInfo

		OpenTelemetry Span's `InstrumentationLibraryInfo` MUST be reported as `tags` to Zipkin using the following mapping.

Add translation of InstrumentationLibraryInfo to Zipkin #800

Add translation of InstrumentationLibraryInfo to Zipkin #800

Conversation

iNikem commented Aug 13, 2020

Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iNikem Aug 14, 2020 • edited Loading

Choose a reason for hiding this comment

tigrannajaryan left a comment

Choose a reason for hiding this comment

iNikem commented Aug 14, 2020

anuraaga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iNikem commented Aug 14, 2020 • edited Loading

anuraaga left a comment

Choose a reason for hiding this comment

iNikem commented Aug 17, 2020

SergeyKanzhelev commented Aug 17, 2020

SergeyKanzhelev commented Aug 17, 2020

iNikem Aug 14, 2020 •

edited

Loading

iNikem commented Aug 14, 2020 •

edited

Loading