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 translation of InstrumentationLibraryInfo to Zipkin #800

Merged
merged 4 commits into from
Aug 17, 2020
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion specification/trace/sdk_exporters/zipkin.md
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,16 @@ Zipkin.
| `SpanKind.SERVER`|`SpanKind.SERVER`||
| `SpanKind.CONSUMER`|`SpanKind.CONSUMER`||
| `SpanKind.PRODUCER`|`SpanKind.PRODUCER` ||
|`SpanKind.INTERNAL`|`null` |must be omitted (set to `null`)|
| `SpanKind.INTERNAL`|`null` |must be omitted (set to `null`)|

### InstrumentationLibraryInfo

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

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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 "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".


| OpenTelemetry | Zipkin
| ------------- | ------ |
| `InstrumentationLibraryInfo.name`|`otel.instrumentationLibrary.name`|
iNikem marked this conversation as resolved.
Show resolved Hide resolved
| `InstrumentationLibraryInfo.version`|`otel.instrumentationLibrary.version`|
Copy link
Member

Choose a reason for hiding this comment

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

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.

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 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

Copy link
Member

Choose a reason for hiding this comment

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

@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.


### Attribute

Expand Down