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

Jaeger Exporter: Fix minor mapping discrepancies #1626

Merged

Conversation

matej-g
Copy link
Contributor

@matej-g matej-g commented Mar 1, 2021

The specification for Jaeger exporter specifies a number of requirements on how the span data should be mapped between OpenTelemetry and Jaeger.

This PR intends to correct some minor discrepancies which have been found previously, in particular:

  • Instrumentation library name / version key should be otel.library.* instead of otel.instrumentation_library.* (relevant section)
  • SpanKind.INTERNAL should not be added to tags (relevant section).
  • Do not include status code tag if the span status is UNSET (relevant section).
  • Do not ignore array attributes; serialize them to string as JSON list (relevant section).
  • Links should have reference type FOLLOWS_FROM (relevant section).

Includes also test changes.

Related issue

One aspect that remain unclear to me, which is not addressed in this PR, I have described in #1625

Resolves (partially) #1376.

@codecov
Copy link

codecov bot commented Mar 1, 2021

Codecov Report

Merging #1626 (163355b) into main (238e7c6) will increase coverage by 0.0%.
The diff coverage is 100.0%.

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #1626   +/-   ##
=====================================
  Coverage   77.3%   77.3%           
=====================================
  Files        128     128           
  Lines       6682    6693   +11     
=====================================
+ Hits        5167    5180   +13     
+ Misses      1268    1266    -2     
  Partials     247     247           
Impacted Files Coverage Δ
exporters/trace/jaeger/jaeger.go 93.4% <100.0%> (+0.4%) ⬆️
exporters/otlp/otlpgrpc/connection.go 88.7% <0.0%> (+1.8%) ⬆️

@matej-g matej-g marked this pull request as ready for review March 2, 2021 07:49
@ijsong
Copy link
Contributor

ijsong commented Mar 2, 2021

@matej-g
It seems that the jaeger exporter has a different tag for InstrumentationLibrary. I am not sure what is correct.

	TagInstrumentationName    = "otlp.instrumentation.library.name"
	TagInstrumentationVersion = "otlp.instrumentation.library.version"

https://github.com/open-telemetry/opentelemetry-collector/blob/27b27b75a53e5dd596a8862d931a7aad4992c3d6/translator/trace/protospan_translation.go#L44-L45

@matej-g
Copy link
Contributor Author

matej-g commented Mar 2, 2021

It seems that the jaeger exporter has a different tag for InstrumentationLibrary. I am not sure what is correct.

Not sure what the requirements are for the collector, but as it stands in the specification, I believe otel.library.* should be correct here. Same key is being used by implementations in other languages e.g. Java or Rust.

@MrAlias MrAlias added this to the RC1 milestone Mar 4, 2021
@MrAlias MrAlias merged commit 05252f4 into open-telemetry:main Mar 8, 2021
@XSAM XSAM mentioned this pull request Mar 9, 2021
@MrAlias MrAlias mentioned this pull request Mar 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants