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

Jump to bottom

Fix Jaeger exporter to correctly translate span.kind attribute #1329

Merged
merged 1 commit into from
Nov 10, 2020
Merged

Fix Jaeger exporter to correctly translate span.kind attribute #1329

merged 1 commit into from
Nov 10, 2020

Conversation

owais
Copy link
Contributor

@owais owais commented Nov 3, 2020 •
edited
Loading

According to opentracing semantic conventions, span.kind attribute
should be one of the following four values: "server", "client",
"producer", "consumer". The spec does not mention the casing of the
strings but all examples show them in lower case. Moreoever, the
OpenTelemetry collector recognizes these values only when they are
lower case. Today if opentelemetry-python exports a span using the
jaeger exporter to the OpenTelemetry collector, it loses the span.kind
information. Using the debug logger exporter in the collector prints the
following to the screen:

Span #0
    Trace ID       : 31789e02d79452eaedb72769bed5fac7
    Parent ID      : a6398bc507d8a982
    ID             : 4a00eaa815e69785
    Name           : custom
    Kind           : SPAN_KIND_UNSPECIFIED

Span #1
    Trace ID       : 31789e02d79452eaedb72769bed5fac7
    Parent ID      :
    ID             : a6398bc507d8a982
    Name           : HelloWorldResource.on_get
    Kind           : SPAN_KIND_UNSPECIFIED

After the patch, span kind is correctly understood and represented by
the collector:

Span #0
    Trace ID       : 8e4aeaa621f7e67c813a9cf26c56a202
    Parent ID      : 13f4cc8db8dfe5b3
    ID             : e8032193bb9dca98
    Name           : custom
    Kind           : SPAN_KIND_INTERNAL

Span #1
    Trace ID       : 8e4aeaa621f7e67c813a9cf26c56a202
    Parent ID      :
    ID             : 13f4cc8db8dfe5b3
    Name           : HelloWorldResource.on_get
    Kind           : SPAN_KIND_SERVER

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Updated existing tests
  • Manually tested by exporting to Jaeger collector and OpenTelemetry collector

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added

owais
owais commented Nov 3, 2020
View reviewed changes
exporter/opentelemetry-exporter-jaeger/src/opentelemetry/exporter/jaeger/__init__.py Outdated
@@ -226,7 +234,7 @@ def _translate_to_jaeger(spans: Span):
[
_get_long_tag("status.code", status.status_code.value),
_get_string_tag("status.message", status.description),
_get_string_tag("span.kind", span.kind.name),
_get_string_tag("span.kind", _otlp_to_jaeger_span_kind(span.kind)),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should be mapping OTLP status values to OpenTracing status values. Most values being the same string with different case is just coincidence or a historical artifact IMO. Also, OTLP defines the values as an enum with numeric values. The name is just a label which may change in future. Considering this, I've replaced kind.name with a otlp > jaeger mapping even if they look very similar.

@owais owais marked this pull request as ready for review November 3, 2020 12:44
@owais owais requested a review from a team as a code owner November 3, 2020 12:44
@owais owais requested review from aabmass and lzchen and removed request for a team November 3, 2020 12:44
@owais owais force-pushed the jaeger-exporter-status-code-translation branch from cf2b08d to 0900235 Compare November 3, 2020 12:44
lzchen
lzchen reviewed Nov 5, 2020
View reviewed changes
exporter/opentelemetry-exporter-jaeger/src/opentelemetry/exporter/jaeger/__init__.py Outdated
@@ -299,6 +309,13 @@ def _convert_int_to_i64(val):
return val


def _otlp_to_jaeger_span_kind(kind: SpanKind) -> str:
jaeger_kind = OTLP_JAEGER_SPAN_KIND.get(kind, "")
if not jaeger_kind:
Copy link
Contributor

@lzchen lzchen Nov 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Nit: Would it be better to explicitly check for INTERNAL instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The thinking was that some users or distributions might have custom values for the kind attribute so this would take care of all unknown cases including INTERNAL which is unknown as far as Jaeger/OpenTracing is concerned but looks like the spec and API do not support that (even if it is not enforced). So I'm replacing it with a map and adding a test case to ensure all possible values are mapped.

lzchen
lzchen reviewed Nov 5, 2020
View reviewed changes
exporter/opentelemetry-exporter-jaeger/src/opentelemetry/exporter/jaeger/__init__.py Outdated
@@ -299,6 +309,13 @@ def _convert_int_to_i64(val):
return val


def _otlp_to_jaeger_span_kind(kind: SpanKind) -> str:
Copy link
Contributor

Choose a reason for hiding this comment

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

Tests for this function?

@lzchen
Copy link
Contributor

lzchen commented Nov 5, 2020

@NathanielRN
Just a heads up, this PR would be affected by the migration.

@NathanielRN
Copy link
Contributor

@lzchen We should be good, because this exporter stays in this repo.

@owais owais force-pushed the jaeger-exporter-status-code-translation branch 4 times, most recently from 7ae7e89 to 3fa551b Compare November 10, 2020 00:13
@owais
Fix Jaeger exporter to correctly translate span.kind attribute
41141c9 
According to opentracing semantic conventions, span.kind attribute
should be one of the following four values: "server", "client",
"producer", "consumer". The spec does not mention the casing of the
strings but all examples show them in lower case. Moreoever, the
OpenTelemetry collector recognizes these values only when they are
lower case. Today if opentelemetry-python exports a span using the
jaeger exporter to the OpenTelemetry collector, it loses the span.kind
information. Using the debug logger exporter in the collector prints the
following to the screen:

    Span #0
        Trace ID       : 31789e02d79452eaedb72769bed5fac7
        Parent ID      : a6398bc507d8a982
        ID             : 4a00eaa815e69785
        Name           : span
        Kind           : SPAN_KIND_UNSPECIFIED
        Start time     : 2020-11-03 18:06:29.684287 +0530 IST
        End time       : 2020-11-03 18:06:29.684344 +0530 IST
        Status code    : STATUS_CODE_CANCELLED
        Status message :

    Span #1
        Trace ID       : 31789e02d79452eaedb72769bed5fac7
        Parent ID      :
        ID             : a6398bc507d8a982
        Name           : HelloWorldResource.on_get
        Kind           : SPAN_KIND_UNSPECIFIED
        Start time     : 2020-11-03 18:06:29.683881 +0530 IST
        End time       : 2020-11-03 18:06:29.684421 +0530 IST
        Status code    : STATUS_CODE_CANCELLED
        Status message :

After the patch, span kind is correctly understood and represented by
the collector:

    Span #0
        Trace ID       : 8e4aeaa621f7e67c813a9cf26c56a202
        Parent ID      : 13f4cc8db8dfe5b3
        ID             : e8032193bb9dca98
        Name           : span
        Kind           : SPAN_KIND_INTERNAL
        Start time     : 2020-11-03 18:03:39.964796 +0530 IST
        End time       : 2020-11-03 18:03:39.964857 +0530 IST
        Status code    : STATUS_CODE_CANCELLED
        Status message :

    Span #1
        Trace ID       : 8e4aeaa621f7e67c813a9cf26c56a202
        Parent ID      :
        ID             : 13f4cc8db8dfe5b3
        Name           : HelloWorldResource.on_get
        Kind           : SPAN_KIND_SERVER
        Start time     : 2020-11-03 18:03:39.964335 +0530 IST
        End time       : 2020-11-03 18:03:39.964936 +0530 IST
        Status code    : STATUS_CODE_CANCELLED
        Status message :
@owais owais force-pushed the jaeger-exporter-status-code-translation branch from 3fa551b to 41141c9 Compare November 10, 2020 00:16
@lzchen lzchen merged commit ad01a56 into open-telemetry:master Nov 10, 2020
@owais owais deleted the jaeger-exporter-status-code-translation branch November 10, 2020 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codeboten codeboten codeboten approved these changes

@lzchen lzchen lzchen approved these changes

@aabmass aabmass Awaiting requested review from aabmass

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@owais @lzchen @NathanielRN @codeboten