💤 Swap Zipkin server for Zipkin Receiver from OTel Collector Contrib #5045

yurishkuro · 2023-12-26T21:43:15Z

Which problem is this PR solving?

Remove duplicated code for handling Zipkin formats that requires a dependency on open-api packages
This will also help with jaeger-v2 since we'll just reuse the same OTEL Zipkin receiver, no more breaking changes

Description of the changes

Swap custom Zipkin server for Zipkin Receiver from OTel Collector Contrib
- (breaking 🛑) Zipkin Proto receiver requires 128bit trace ID, otherwise TraceID: has length 8 yet wanted length 16. Jaeger receiver did not have such limitation.
- (breaking 🛑) the --collector.zipkin.keep-alive setting is not supported in the OTEL receiver
Delete a bunch of packages and files that are no longer needed (5k lines, including code-gen)
go mod tidy removed all go-openapi dependencies and a few others

How was this change tested?

Added unit tests cmd/collector/app/handler/zipkin_receiver_test.go that submit various Zipkin formats to the receiver
Crossdock tests also cover similar exports in CI:

Executing Matrix...
✓ [endtoend] test_driver→ (services=go) ⇒ Pass
✓ [endtoend] test_driver→ (services=java) ⇒ Pass
✓ [endtoend] test_driver→ (services=node) ⇒ Pass
✓ [endtoend] test_driver→ (services=python) ⇒ Pass
✓ [endtoend] test_driver→ (services=zipkin-brave-json) ⇒ Pass
✓ [endtoend] test_driver→ (services=zipkin-brave-json-v2) ⇒ Pass
✓ [endtoend] test_driver→ (services=zipkin-brave-proto) ⇒ Pass
✓ [endtoend] test_driver→ (services=zipkin-brave-thrift) ⇒ Pass

Checklist

I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
I have signed all commits
I have added unit tests for the new functionality
I have run lint and test steps successfully
- for jaeger: make lint test
- for jaeger-ui: yarn lint and yarn test

Signed-off-by: Yuri Shkuro <[email protected]>

yurishkuro · 2023-12-27T01:04:55Z

cmd/collector/app/handler/testdata/zipkin_thrift_v1_merged_spans.json

@@ -0,0 +1,79 @@
+[


I used the existing zipkin_v1_merged_spans.json file (Zipkin JSON v1), converged into object model, and re-serialized as JSON. It uses different representations for many fields, like raw numbers instead of hex strings for IDs.

codecov · 2023-12-27T01:11:29Z

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (9138be4) 95.58% compared to head (5b43341) 95.55%.

Files	Patch %	Lines
cmd/collector/app/collector.go	58.33%	2 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5045      +/-   ##
==========================================
- Coverage   95.58%   95.55%   -0.04%     
==========================================
  Files         319      313       -6     
  Lines       18794    18161     -633     
==========================================
- Hits        17965    17353     -612     
+ Misses        665      647      -18     
+ Partials      164      161       -3

Flag	Coverage Δ
cassandra-3.x	`25.61% <ø> (ø)`
cassandra-4.x	`25.61% <ø> (ø)`
elasticsearch-5.x	`19.88% <ø> (ø)`
elasticsearch-6.x	`19.87% <ø> (ø)`
elasticsearch-7.x	`20.02% <ø> (+0.01%)`	⬆️
elasticsearch-8.x	`20.09% <ø> (-0.02%)`	⬇️
grpc-badger	`19.50% <ø> (ø)`
kafka	`14.10% <ø> (ø)`
opensearch-1.x	`20.02% <ø> (+0.01%)`	⬆️
opensearch-2.x	`20.02% <ø> (ø)`
unittests	`93.21% <91.22%> (-0.12%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Yuri Shkuro <[email protected]>

yurishkuro · 2023-12-27T01:12:15Z

cmd/collector/app/handler/testdata/zipkin_proto_02.json

@@ -1,7 +1,7 @@
 {
  "spans": [
    {
-      "trace_id": "AAAAAAAAAAI=",
+      "trace_id":"QoVouqoJRn5CjmiKqglGfw==",


(breaking 🛑) had to change this because of the error TraceID: has length 8 yet wanted length 16. Jaeger receiver did not have such limitation.

Signed-off-by: Yuri Shkuro <[email protected]>

yurishkuro · 2023-12-27T01:18:29Z

cmd/collector/app/handler/zipkin_receiver_tls_test.go

@@ -12,59 +12,29 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.

-package server
+package handler


This was testing exhaustively TLS settings on Zipkin endpoint, so I decided to keep it, just move from a different location. It works against the Zipkin receiver now.

Signed-off-by: Yuri Shkuro <[email protected]>

jkowall

lgtm. One potential issue is keeping this sync with the upstream Otel changes. I do not think this will happen much with Zipkin being stagnant but it's a potential point of additional work.

yurishkuro · 2023-12-27T17:33:35Z

What sync are you referring to? Basically my plan is to only use OTEL's implementation going forward and if any issues direct them upstream.

We still support Zipkin Thrift in the agent, which is a separate implementation, but we should be able to remove it once we sunset the agent.

yurishkuro added 2 commits December 26, 2023 16:01

Regenerate Zipkin types using newer Swagger image

196b4c3

Signed-off-by: Yuri Shkuro <[email protected]>

Swap Zipkin server for receiver from OTel Collector Contrib

8ebe334

Signed-off-by: Yuri Shkuro <[email protected]>

yurishkuro requested a review from a team as a code owner December 26, 2023 21:43

yurishkuro requested a review from albertteoh December 26, 2023 21:43

yurishkuro changed the title ~~Swap Zipkin server for receiver from OTel Collector Contrib~~ Swap Zipkin server for Zipkin Receiver from OTel Collector Contrib Dec 26, 2023

yurishkuro added the changelog:breaking-change Change that is breaking public APIs or established behavior label Dec 26, 2023

yurishkuro added 7 commits December 26, 2023 16:59

fmt

335a277

Signed-off-by: Yuri Shkuro <[email protected]>

receiver-with-tests

400dae5

Signed-off-by: Yuri Shkuro <[email protected]>

Add tests and delete zipkin packages

a64c03b

Signed-off-by: Yuri Shkuro <[email protected]>

Delete zipkin swagger

c9917a3

Signed-off-by: Yuri Shkuro <[email protected]>

remove-commented-code

a761791

Signed-off-by: Yuri Shkuro <[email protected]>

go mod tidy

aef4ba6

Signed-off-by: Yuri Shkuro <[email protected]>

fix typo

0f33eb0

Signed-off-by: Yuri Shkuro <[email protected]>

yurishkuro commented Dec 27, 2023

View reviewed changes

fix all tests

b49c03e

Signed-off-by: Yuri Shkuro <[email protected]>

yurishkuro commented Dec 27, 2023

View reviewed changes

remove deprecated func

ef123db

Signed-off-by: Yuri Shkuro <[email protected]>

yurishkuro commented Dec 27, 2023

View reviewed changes

yurishkuro and others added 2 commits December 26, 2023 20:21

clean-up

284bd34

Signed-off-by: Yuri Shkuro <[email protected]>

Merge branch 'main' into zipkin-swagger

cf07733

yurishkuro changed the title ~~Swap Zipkin server for Zipkin Receiver from OTel Collector Contrib~~ 💤 Swap Zipkin server for Zipkin Receiver from OTel Collector Contrib Dec 27, 2023

yurishkuro and others added 2 commits December 27, 2023 11:37

Merge branch 'main' into zipkin-swagger

7d24ebd

remove openapi group from dependabot

5b43341

Signed-off-by: Yuri Shkuro <[email protected]>

jkowall approved these changes Dec 27, 2023

View reviewed changes

yurishkuro merged commit 8b6ede9 into jaegertracing:main Dec 27, 2023
36 of 37 checks passed

yurishkuro deleted the zipkin-swagger branch December 27, 2023 17:34

This was referenced Dec 27, 2023

Change test stdout redirect to tee #5050

Closed

bugfix: Set pipefail when running unit tests to preserve exit code #5057

Merged

yurishkuro mentioned this pull request May 9, 2024

[Bug]: Zipkin json binaryAnnotation value : cannot unmarshall #5434

Closed

yurishkuro mentioned this pull request Dec 6, 2024

[Bug]: From Jaeger version 1.53.0 'error.message' tag value is missing, 'error' tag contains both true value and error message. This makes it difficult to retrieve the tags proper value. #6317

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

💤 Swap Zipkin server for Zipkin Receiver from OTel Collector Contrib #5045

💤 Swap Zipkin server for Zipkin Receiver from OTel Collector Contrib #5045

yurishkuro commented Dec 26, 2023 •

edited

Loading

yurishkuro Dec 27, 2023

codecov bot commented Dec 27, 2023 •

edited

Loading

yurishkuro Dec 27, 2023 •

edited

Loading

yurishkuro Dec 27, 2023

jkowall left a comment

yurishkuro commented Dec 27, 2023 •

edited

Loading

💤 Swap Zipkin server for Zipkin Receiver from OTel Collector Contrib #5045

💤 Swap Zipkin server for Zipkin Receiver from OTel Collector Contrib #5045

Conversation

yurishkuro commented Dec 26, 2023 • edited Loading

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

yurishkuro Dec 27, 2023

Choose a reason for hiding this comment

codecov bot commented Dec 27, 2023 • edited Loading

Codecov Report

yurishkuro Dec 27, 2023 • edited Loading

Choose a reason for hiding this comment

yurishkuro Dec 27, 2023

Choose a reason for hiding this comment

jkowall left a comment

Choose a reason for hiding this comment

yurishkuro commented Dec 27, 2023 • edited Loading

yurishkuro commented Dec 26, 2023 •

edited

Loading

codecov bot commented Dec 27, 2023 •

edited

Loading

yurishkuro Dec 27, 2023 •

edited

Loading

yurishkuro commented Dec 27, 2023 •

edited

Loading