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

Zipkin Exporter: Adjust span transformation to comply with the spec #1688

Merged

Conversation

matej-g
Copy link
Contributor

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

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

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

  • Instrumentation library name / version key should be otel.library.* instead of otel.instrumentation_library.* (relevant section)
  • Remove otel.status_description and use error instead; Do not report Unset status and do not include status message for Ok / Unset span status (relevant section).
  • Serialize array attribute to string as JSON list (relevant section).
  • Populate remoteEndpoint according to the specification (relevant section)

Includes test changes as well.

Related issue

An issue for setting the service name is still pending in #1549

Resolves (partially) #1376.

@codecov
Copy link

codecov bot commented Mar 10, 2021

Codecov Report

Merging #1688 (ccdf86c) into main (2817c09) will increase coverage by 0.1%.
The diff coverage is 100.0%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #1688     +/-   ##
=======================================
+ Coverage   78.7%   78.8%   +0.1%     
=======================================
  Files        134     134             
  Lines       7116    7157     +41     
=======================================
+ Hits        5602    5643     +41     
  Misses      1268    1268             
  Partials     246     246             
Impacted Files Coverage Δ
exporters/trace/zipkin/model.go 99.1% <100.0%> (+0.4%) ⬆️
sdk/trace/batch_span_processor.go 83.9% <0.0%> (ø)

@matej-g matej-g force-pushed the zipkin-exporter-span-adjustments-spec branch from 6adf24e to d4181bf Compare March 11, 2021 08:09
@matej-g matej-g marked this pull request as ready for review March 11, 2021 12:27
@jcchavezs
Copy link
Contributor

Great work!

@jcchavezs
Copy link
Contributor

jcchavezs commented Mar 14, 2021

exporters/trace/zipkin/model.go Outdated Show resolved Hide resolved
exporters/trace/zipkin/model.go Outdated Show resolved Hide resolved
exporters/trace/zipkin/model.go Outdated Show resolved Hide resolved
exporters/trace/zipkin/model_test.go Show resolved Hide resolved
- Simplify deletion of redundant error code
- Simplify endpoint rank determination
@Aneurysm9 Aneurysm9 merged commit 7d8e6bd into open-telemetry:main Apr 7, 2021
@MrAlias MrAlias mentioned this pull request Apr 23, 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.

6 participants