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.

Sign up for GitHub

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

Add translation of InstrumentationLibraryInfo to Zipkin #800

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2654842
Add translation of InstrumentationLibraryInfo to Zipkin
iNikem Aug 13, 2020
f84f1b2
Update specification/trace/sdk_exporters/zipkin.md
iNikem Aug 14, 2020
4e79611
Fix case
iNikem Aug 14, 2020
9d0c71e
Rename InstrumentationLibraryInfo to InstrumentationLibrary
iNikem Aug 14, 2020
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
View file
Open in desktop
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.instrumentation_library.name`|
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Also from the "Name Tracer/Meter" :)

Copy link
Member

Choose a reason for hiding this comment

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

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor Author

@iNikem iNikem Aug 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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

| `InstrumentationLibraryInfo.version`|`otel.instrumentationLibrary.version`|

### Attribute

Expand Down