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

OpenTelemetry to Zipkin Mapping #221

Closed
wants to merge 3 commits into from
Closed

OpenTelemetry to Zipkin Mapping #221

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b89037b
feat(zipkin): add mapping
Aug 18, 2019
3039b7b
Merge branch 'master' into feat/zipkin-mapping
bogdandrutu Aug 26, 2019
c496b60
Merge branch 'master' into feat/zipkin-mapping
SergeyKanzhelev Sep 27, 2019
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
75 changes: 75 additions & 0 deletions work_in_progress/zipkin/zipkin.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@

# OpenTelemetry to Zipkin Transformation

This document defines the transformation between OpenTelemetry and Zipkin Spans.
Zipkin's v2 API is defined in the [zipkin2-api.yaml](https://github.com/openzipkin/zipkin-api/blob/master/zipkin2-api.yaml)

## Summary

The following table summarizes the major transformations between OpenTelemetry
and Zipkin.

|OpenTelemetry|Zipkin
|--|--|--|
|`span.kind=INTERNAL`|`span.kind=SERVER`
|`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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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


## Mappings

This section discusses the details of the transformations between OpenTelemetry
and Zipkin.

### SpanKind

Zipkin doesn't support OpenTelemetry's `SpanKind.INTERNAL`.
`SpanKind.INTERNAL` type MUST be reported to Zipkin as `SERVER`.
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@hekike hekike Aug 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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


The following table lists all the `SpanKind` mappings between OpenTelemetry and
Zipkin.

|OpenTelemetry|Zipkin
|--|--|
`SpanKind.CLIENT`|`SpanKind.CLIENT`
`SpanKind.SERVER`|`SpanKind.SERVER`
`SpanKind.CONSUMER`|`SpanKind.CONSUMER`
`SpanKind.PRODUCER`|`SpanKind.PRODUCER`
`SpanKind.INTERNAL`|`SpanKind.SERVER`

### Attribute

OpenTelemetry Span `Attribute`(s) MUST be reported as `tags` to Zipkin.

### Status

Span `Status` SHOULD be reported as a key-value pair in `tags` to Zipkin.

The following table defines the OpenTelemetry `Status` to Zipkin `tags` mapping.

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

Choose a reason for hiding this comment

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

This is opentracing standardization, does Zipkin follow that?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

and Zipkin pre-dates OpenTracing and has its own conventions

Copy link

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@tigrannajaryan tigrannajaryan Oct 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

|Message *(optional)* | `ot.status_description` | `{message}`

The `ot.status_code` tag value MUST follow the [Standard GRPC Code Names](https://github.com/grpc/grpc/blob/master/doc/statuscodes.md).

### Event

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.
Copy link
Member

Choose a reason for hiding this comment

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

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


## Unit of Time

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

Choose a reason for hiding this comment

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

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

in microseconds with decimal accuracy.

## Request Payload

For performance considerations, Zipkin fields that can be absent SHOULD be
omitted from the payload when they are empty in the OpenTelemetry `Span`.

For example, an OpenTelemetry `Span` without any `Event` should not have an
`annotations` field in the Zipkin payload.