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 retrieval of peer.service from Zipkin's MESSAGE_ADDR annotation #3312

Merged
merged 2 commits into from
Oct 14, 2021
Merged

Fix retrieval of peer.service from Zipkin's MESSAGE_ADDR annotation #3312

merged 2 commits into from
Oct 14, 2021

Conversation

Git-Jiro
Copy link
Contributor

@Git-Jiro Git-Jiro commented Oct 8, 2021 •
edited by yurishkuro
Loading

Which problem is this PR solving?

Resolves #2399

Short description of the changes

Adding a test case to address #2399

@Git-Jiro Git-Jiro requested a review from a team as a code owner October 8, 2021 19:26
@Git-Jiro Git-Jiro requested a review from joe-elliott October 8, 2021 19:26
@Git-Jiro Git-Jiro force-pushed the implement_test branch 4 times, most recently from b976e3b to da9789d Compare October 9, 2021 15:44
@codecov
Copy link

codecov bot commented Oct 10, 2021 •
edited
Loading

Codecov Report

Merging #3312 (74bfb75) into master (b8e8fc2) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3312      +/-   ##
==========================================
+ Coverage   95.96%   95.98%   +0.01%     
==========================================
  Files         259      259              
  Lines       15434    15434              
==========================================
+ Hits        14811    14814       +3     
+ Misses        527      526       -1     
+ Partials       96       94       -2
Impacted Files Coverage Δ
model/converter/thrift/zipkin/to_domain.go 100.00% <100.00%> (ø)
pkg/config/tlscfg/cert_watcher.go 92.63% <0.00%> (-2.11%) ⬇️
plugin/storage/integration/integration.go 78.88% <0.00%> (-0.40%) ⬇️
cmd/query/app/server.go 95.58% <0.00%> (+1.47%) ⬆️
cmd/query/app/static_handler.go 95.80% <0.00%> (+1.79%) ⬆️

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 b8e8fc2...74bfb75. Read the comment docs.

albertteoh
albertteoh previously approved these changes Oct 10, 2021
View reviewed changes
Copy link
Contributor

@albertteoh albertteoh left a comment

Choose a reason for hiding this comment

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

I don't know much about Zipkin, but the change looks reasonable to me. I'll leave this open a little longer for other reviewers with more Zipkin experience to take a look as well.

cmd/collector/app/zipkin/jsonv2_test.go Outdated Show resolved Hide resolved
cmd/collector/app/zipkin/jsonv2_test.go Outdated Show resolved Hide resolved
@Git-Jiro Git-Jiro force-pushed the implement_test branch 2 times, most recently from 347bf7c to c2044cb Compare October 11, 2021 16:16
yurishkuro
yurishkuro reviewed Oct 12, 2021
View reviewed changes
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

I think there is a missing dependency in these tests. You're trying to validate that a zipkin json will be transformed into Jaeger span with peer.servce=kafka tag? But the tests don't actually invoke zipkin thrift -> jaeger converter, so there's no end-to-end test, and this is why it's difficult to see the impact of this change.

@Git-Jiro
Copy link
Contributor Author

Might this be the code I need to build a more thorough test? https://github.com/jaegertracing/jaeger/blob/master/model/converter/thrift/zipkin/to_domain.go#L80

@yurishkuro
Copy link
Member

Yes

@Git-Jiro Git-Jiro force-pushed the implement_test branch 2 times, most recently from cec6c55 to 1a4044c Compare October 12, 2021 16:27
@Git-Jiro
Copy link
Contributor Author

@yurishkuro I think I fixed the problem now.

yurishkuro
yurishkuro previously approved these changes Oct 14, 2021
View reviewed changes
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

Thanks!

@yurishkuro yurishkuro changed the title Initial version of new test Fix retrieval of peer.service from Zipkin's MESSAGE_ADDR annotation Oct 14, 2021
@yurishkuro yurishkuro enabled auto-merge (squash) October 14, 2021 02:57
@Git-Jiro
Fix missing annotation when converting span from Zipkin to Jaeger
2655d7e 
Signed-off-by: Martin Schimandl <martin.schimandl@gmail.com>
auto-merge was automatically disabled October 14, 2021 03:46

Head branch was pushed to by a user without write access

@Git-Jiro
Copy link
Contributor Author

@yurishkuro had to push once more, because linter was not happy.

@yurishkuro yurishkuro enabled auto-merge (squash) October 14, 2021 14:43
@yurishkuro yurishkuro merged commit 0d1727c into jaegertracing:master Oct 14, 2021
@Git-Jiro Git-Jiro deleted the implement_test branch October 14, 2021 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pavolloffay pavolloffay pavolloffay approved these changes

@albertteoh albertteoh albertteoh left review comments

@yurishkuro yurishkuro yurishkuro left review comments

@joe-elliott joe-elliott Awaiting requested review from joe-elliott joe-elliott was automatically assigned from jaegertracing/jaeger-maintainers

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

Successfully merging this pull request may close these issues.

Remote endpoint field of Zipkin with only service name span can't be retrieved
4 participants
@Git-Jiro @yurishkuro @pavolloffay @albertteoh