Skip to content

Convert Zipkin receiver and exporter to use OTLP and fix translation bugs #1446

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

Merged
merged 37 commits into from
Aug 11, 2020
Merged

Convert Zipkin receiver and exporter to use OTLP and fix translation bugs #1446

merged 37 commits into from
Aug 11, 2020

Conversation

kbrockhoff
Copy link
Member

@kbrockhoff kbrockhoff commented Jul 27, 2020

Description: New translators were created for Zipkin formats to OTLP and for OTLP to Zipkin v2. The Zipkin receiver and exporter were converted to use the new translators.

Link to tracking Issue:
Fixes #526
Fixes #572
Fixes #1138
Fixes #1303
Fixes #1304

Testing: All existing tests pass

Documentation: N/A

@codecov
Copy link

codecov bot commented Aug 1, 2020

Codecov Report

Merging #1446 into master will decrease coverage by 0.10%.
The diff coverage is 88.63%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1446      +/-   ##
==========================================
- Coverage   91.35%   91.25%   -0.11%     
==========================================
  Files         240      246       +6     
  Lines       16754    17270     +516     
==========================================
+ Hits        15305    15759     +454     
- Misses       1045     1094      +49     
- Partials      404      417      +13     
Impacted Files Coverage Δ
exporter/zipkinexporter/factory.go 100.00% <ø> (ø)
exporter/zipkinexporter/zipkin.go 66.66% <14.28%> (-4.17%) ⬇️
translator/trace/protospan_translation.go 20.00% <20.00%> (ø)
translator/trace/zipkin/zipkinv1_to_protospan.go 94.49% <66.66%> (+0.57%) ⬆️
exporter/zipkinexporter/test_utils.go 85.71% <85.71%> (ø)
translator/trace/zipkin/zipkinv2_to_traces.go 91.46% <91.46%> (ø)
translator/trace/zipkin/traces_to_zipkinv2.go 92.56% <92.56%> (ø)
receiver/zipkinreceiver/trace_receiver.go 90.90% <93.33%> (+0.69%) ⬆️
internal/goldendataset/resource_generator.go 98.23% <100.00%> (+0.19%) ⬆️
internal/goldendataset/span_generator.go 98.92% <100.00%> (+0.04%) ⬆️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d051556...50616c8. Read the comment docs.

@bogdandrutu
Copy link
Member

Seems that correctness tests for Zipkin translation are failing

@kbrockhoff
Copy link
Member Author

Timestamps are not converting correctly on Linux but are on OS X. I am working on debugging.

@bogdandrutu
Copy link
Member

Can be some time zone issue, it is just a guess

b[i], b[j] = b[j], b[i]
}

// V2SpansToInternalTraces translates Zipkin v2 spans into internal trace data.
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't appear that this method shares any implementation details with the Zipkin v1, can we keep them somewhat unified? I had just merged them together recently #1002

return err
}

func populateSpanStatus(tags map[string]string, status pdata.SpanStatus) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The previous implementation used statusMapper to infer the status from additional tags like http.status_code https://github.com/open-telemetry/opentelemetry-collector/blob/master/translator/trace/zipkin/zipkinv2_to_protospan.go#L239-L252

Copy link
Member Author

Choose a reason for hiding this comment

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

Status is going to be removed from the specification. We should wait until that happens to revise this.

@kbrockhoff kbrockhoff changed the title Convert Zipkin receiver and exporter to use OTLP and fix translation bugs [WIP] Convert Zipkin receiver and exporter to use OTLP and fix translation bugs Aug 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants