OpenTelemetry to Zipkin Mapping #221

hekike · 2019-08-18T16:16:02Z

Define mapping between OpenTelemetry and Zipkin Span(s).
This is my very first spec here, probably I'd need some guidance to carry this to the finish line.

Some of the things in this spec requires further discussion, this initial version is mainly based on the current opencensus-node behavior.

https://github.com/openzipkin/zipkin-api/blob/master/zipkin2-api.yaml
Related issue: #220

bogdandrutu · 2019-08-20T01:06:21Z

work_in_progress/zipkin/zipkin.md

+|`span.attributes` | `span.tags`
+|`span.status.code` | tag as `ot.status_code: OK`
+|`span.status.message` | optional tag as `ot.status_description: {msg}`
+|`span.events` | `span.annotations`, skip on event attributes


Maybe encode the attributes as well: $name + "{" + $attr.key + " : " + $attr.value + ", " + ...}

Event with the same name can happen multiple times. Maybe we could add a counter like:
$name + ($index) + "{" + $attr.key + " : " + $attr.value + ", " + ...} for example:

{"my-event-name(1): { "key1": "value1", "key2": "value2" }, {"my-event-name(2): { "key1": "value1", "key2": "value2" }

bogdandrutu · 2019-08-20T01:07:16Z

work_in_progress/zipkin/zipkin.md

+### SpanKind
+
+Zipkin doesn't support OpenTelemetry's `SpanKind.INTERNAL`.
+`SpanKind.INTERNAL` type MUST be reported to Zipkin as `SERVER`.


SpanKind.INTERNAL means no present for Zipkin, that's how I read their description.

Do you mean that we shouldn't even send it?
I don't see mention of INTERNAL here: https://github.com/openzipkin/zipkin-api/blob/master/zipkin2-api.yaml

I'd prefer to report them because SpanKind.INTERNAL is our default SpanKind type in OpenTelemetry. We can easily confuse people at integration time when they create spans but don't see them in the backend. Or force them to change their code in the case of switching exporters.

I'd vote for mapping INTERNAL to a different SpanKind or make mapping configurable with an option of skipping it with. The default could be SERVER. What do you think?

I am suggesting to not set the type in Zipkin for these spans.

As a member of the Zipkin team +1 to what @bogdandrutu has said, default value, which SpanData.INTERNAL signifies, is null in Zipkin

bogdandrutu · 2019-08-20T01:08:01Z

work_in_progress/zipkin/zipkin.md

+
+Status|Tag Key| Tag Value
+|--|--|--|
+|Code | `ot.status_code` | Name of the code, for example: `OK`


This is opentracing standardization, does Zipkin follow that?

In my understanding, it doesn't. Status is a nonexisting concept in Zipkin. No mention of Status here https://github.com/openzipkin/zipkin-api/blob/master/zipkin2-api.yaml

Then I would suggest probably to put a TODO and change this when opentelemetry defines this.

Writing "status" as some specific tag is what was requested in OC Zipkin Java: census-instrumentation/opencensus-java#1931 (comment)

The only thing is that the prefix ot may get people thinking that this is OpenTracing...

and Zipkin pre-dates OpenTracing and has its own conventions

In OpenCensus service we tried to capture the handling of status and conversions at this doc https://github.com/Omnition/internal-opencensus-service/blob/internal-production/translator/trace/README.md

@pjanotti his file must be in private repo. Link shows 404 for me

@tigrannajaryan do you think you can share this document somehow?

We have the exact same code and README in public OpenTelemetry Collector: https://github.com/open-telemetry/opentelemetry-collector/tree/master/translator/trace

@hekike I'd suggest to follow the OC Agent conversion logic whenever possible

devinsba · 2019-10-01T23:12:53Z

work_in_progress/zipkin/zipkin.md

+### SpanKind
+
+Zipkin doesn't support OpenTelemetry's `SpanKind.INTERNAL`.
+`SpanKind.INTERNAL` type MUST be reported to Zipkin as `SERVER`.


As a member of the Zipkin team +1 to what @bogdandrutu has said, default value, which SpanData.INTERNAL signifies, is null in Zipkin

devinsba · 2019-10-01T23:37:18Z

work_in_progress/zipkin/zipkin.md

+
+## Unit of Time
+
+Zipkin times like `timestamp`, `duration` and `event.timestamp` MUST be reported


Suggested change

Zipkin times like `timestamp`, `duration` and `event.timestamp` MUST be reported

Zipkin times like `timestamp`, `duration` and `annotation.timestamp` MUST be reported

tigrannajaryan · 2019-10-22T17:50:47Z

@hekike we have a working translation (in Collector code) between Zipkin and OpenCensus (which is close to OpenTelemetry). Did you have a chance to see if your proposal matches the existing translation or it is different: https://github.com/open-telemetry/opentelemetry-collector/tree/master/translator/trace#zipkin-to-oc

SergeyKanzhelev · 2019-10-22T20:25:53Z

@open-telemetry/specs-approvers this PR was stale for a while. Any takers to move it forward? It seems that there is enough reference materials to get going

tsloughter · 2019-11-28T15:39:08Z

work_in_progress/zipkin/zipkin.md

+
+OpenTelemetry `Event` has an optional `Attribute`(s) which is not supported by
+Zipkin.
+These `Event` attributes are may be left out from the final Zipkin payload.


Zipkin doesn't have an Event field does it? In the OpenCensus Erlang reporter I turned them into annotations.

If that is the case, why not define a way to include them. In the opencensus reporter for Erlang I include them when buliding annotations by making a string https://github.com/opencensus-beam/opencensus_zipkin/blob/master/src/oc_reporter_zipkin.erl#L95 and https://github.com/opencensus-beam/opencensus_zipkin/blob/master/src/oc_reporter_zipkin.erl#L101

If there is going to be a mapping we may as well go all the way and not leave out data :)

feat(zipkin): add mapping

b89037b

hekike requested review from AloisReitbauer, bogdandrutu, c24t, carlosalberto, iredelmeier, reyang, SergeyKanzhelev, songy23, tedsuo and yurishkuro as code owners August 18, 2019 16:16

hekike mentioned this pull request Aug 18, 2019

Zipkin exporter mappings #220

Closed

hekike changed the title ~~feat(zipkin): add mapping~~ OpenTelemetry to Zipkin Mapping Aug 18, 2019

hekike mentioned this pull request Aug 18, 2019

Zipkin Exporter open-telemetry/opentelemetry-js#192

Merged

bogdandrutu reviewed Aug 20, 2019

View reviewed changes

bogdandrutu and others added 2 commits August 26, 2019 12:16

Merge branch 'master' into feat/zipkin-mapping

3039b7b

Merge branch 'master' into feat/zipkin-mapping

c496b60

SergeyKanzhelev requested a review from tigrannajaryan as a code owner September 27, 2019 20:59

devinsba reviewed Oct 1, 2019

View reviewed changes

SergeyKanzhelev added this to the Alpha v0.3 milestone Oct 16, 2019

tsloughter reviewed Nov 28, 2019

View reviewed changes

SergeyKanzhelev mentioned this pull request Dec 10, 2019

Transformation to Zipkin #380

Merged

SergeyKanzhelev closed this Dec 10, 2019

TuckTuckFloof pushed a commit to TuckTuckFloof/opentelemetry-specification that referenced this pull request Oct 15, 2020

Add shasum for grpc and format file. (open-telemetry#221)

9195cf7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OpenTelemetry to Zipkin Mapping #221

OpenTelemetry to Zipkin Mapping #221

hekike commented Aug 18, 2019 •

edited

Loading

bogdandrutu Aug 20, 2019

hekike Aug 20, 2019

bogdandrutu Aug 20, 2019

hekike Aug 20, 2019 •

edited

Loading

bogdandrutu Aug 20, 2019

devinsba Oct 1, 2019

bogdandrutu Aug 20, 2019

hekike Aug 20, 2019

bogdandrutu Aug 20, 2019

pjanotti Aug 20, 2019

pjanotti Aug 20, 2019

pjanotti Aug 20, 2019

SergeyKanzhelev Sep 23, 2019

SergeyKanzhelev Oct 1, 2019

tigrannajaryan Oct 1, 2019 •

edited

Loading

SergeyKanzhelev Oct 1, 2019

devinsba Oct 1, 2019

devinsba Oct 1, 2019

tigrannajaryan commented Oct 22, 2019

SergeyKanzhelev commented Oct 22, 2019

tsloughter Nov 28, 2019


		## Unit of Time

		Zipkin times like `timestamp`, `duration` and `event.timestamp` MUST be reported

OpenTelemetry to Zipkin Mapping #221

OpenTelemetry to Zipkin Mapping #221

Conversation

hekike commented Aug 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hekike Aug 20, 2019 • edited Loading

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

Choose a reason for hiding this comment

tigrannajaryan Oct 1, 2019 • edited Loading

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

tigrannajaryan commented Oct 22, 2019

SergeyKanzhelev commented Oct 22, 2019

Choose a reason for hiding this comment

hekike commented Aug 18, 2019 •

edited

Loading

hekike Aug 20, 2019 •

edited

Loading

tigrannajaryan Oct 1, 2019 •

edited

Loading