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

Add Zipkin Thrift as kafka ingestion format #1256

Merged
merged 6 commits into from Dec 18, 2018

Conversation

@geobeau
Copy link
Contributor

geobeau commented Dec 16, 2018

Which problem is this PR solving?

  • Ingesting Zipkin thrift span from Kafka: This is useful for a seamless migration from Zipkin. It is mentioned in #171

Short description of the changes

  • Add an unmarshaller to process spans in zipkin thrift using what was used for collector zipkin API
Copy link
Member

yurishkuro left a comment

Thanks to contributing! This will be the last missing piece for Zipkin compat.

I left a few stylistic comments. The build appears to be failing on go fmt

Go fmt, license check, or import ordering failures, run 'make fmt'
./plugin/storage/kafka/options.go
./plugin/storage/kafka/unmarshaller.go

Last piece is to add unit test for the marshaling.

cmd/ingester/app/flags.go Outdated Show resolved Hide resolved
cmd/ingester/app/flags.go Outdated Show resolved Hide resolved
cmd/ingester/app/builder/builder.go Outdated Show resolved Hide resolved
plugin/storage/kafka/options.go Outdated Show resolved Hide resolved
@codecov

This comment has been minimized.

Copy link

codecov bot commented Dec 16, 2018

Codecov Report

Merging #1256 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1256   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         159     160    +1     
  Lines        7163    7181   +18     
======================================
+ Hits         7163    7181   +18
Impacted Files Coverage Δ
plugin/storage/kafka/unmarshaller.go 100% <100%> (ø) ⬆️
model/converter/thrift/zipkin/deserialize.go 100% <100%> (ø)
plugin/storage/kafka/factory.go 100% <100%> (ø) ⬆️
cmd/ingester/app/flags.go 100% <100%> (ø) ⬆️
plugin/storage/kafka/options.go 100% <100%> (ø) ⬆️
cmd/collector/app/zipkin/http_handler.go 100% <100%> (ø) ⬆️

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 8f809ed...9038399. Read the comment docs.

@geobeau geobeau force-pushed the geobeau:add-thrift-ingester branch from 0ffd48e to 83c8a2d Dec 16, 2018
geobeau added 3 commits Dec 16, 2018
Signed-off-by: Geoffrey Beausire <g.beausire@criteo.com>
Signed-off-by: Geoffrey Beausire <g.beausire@criteo.com>
Signed-off-by: Geoffrey Beausire <g.beausire@criteo.com>
@geobeau geobeau force-pushed the geobeau:add-thrift-ingester branch from 83c8a2d to 50b11f1 Dec 16, 2018
if err != nil {
return nil, err
}
return mSpan[0], err

This comment has been minimized.

Copy link
@geobeau

geobeau Dec 16, 2018

Author Contributor

@yurishkuro I am not sure it's some good code, any advices to avoid using [0]. I thought about changing the interface to return []model.Span and handle the array in the calling function.

This comment has been minimized.

Copy link
@yurishkuro

yurishkuro Dec 16, 2018

Member

I think the ingester is currently built with the assumption that a single Kafka message contains just a single span. Even in this case a single Zipkin span can sometimes be transformed to two Jaeger spans, so it would be better to extend the ingester to support the notion of many spans per message, at least after unmarshaling. However, I would not recommend doing it in this PR, it should be a separate PR as it might get quite large. For now I would suggest logging a warning.

There are good reasons to expect a single span per Kafka message, it makes the streaming jobs easier to write.

This comment has been minimized.

Copy link
@geobeau

geobeau Dec 16, 2018

Author Contributor

Thanks

@geobeau

This comment has been minimized.

Copy link
Contributor Author

geobeau commented Dec 16, 2018

Formating done, now going for tests

geobeau added 3 commits Dec 16, 2018
Signed-off-by: Geoffrey Beausire <g.beausire@criteo.com>
Signed-off-by: Geoffrey Beausire <g.beausire@criteo.com>
Signed-off-by: Geoffrey Beausire <g.beausire@criteo.com>
@geobeau geobeau force-pushed the geobeau:add-thrift-ingester branch from a273a5c to 9038399 Dec 17, 2018
@geobeau

This comment has been minimized.

Copy link
Contributor Author

geobeau commented Dec 18, 2018

Should be good now, @yurishkuro

Copy link
Member

yurishkuro left a comment

Looks great! Thanks for contributing 🎉

Have you been able to test this with real Zipkin clients writing to Kafka?

@geobeau

This comment has been minimized.

Copy link
Contributor Author

geobeau commented Dec 18, 2018

Yes, I tested it mostly that way. It has not run for a long continuous time thought. And I am not sure we are using a clean version of Zipkin. I should probably build a small PoC using docker images to test better

@yurishkuro

This comment has been minimized.

Copy link
Member

yurishkuro commented Dec 18, 2018

I don't mind merging this as is, but it would be nice to add an integration test for Travis. We can probably extend https://github.com/jaegertracing/xdock-zipkin-brave to support configuration to use Kafka as the span reporting transport.

@yurishkuro yurishkuro merged commit d23d18f into jaegertracing:master Dec 18, 2018
5 checks passed
5 checks passed
DCO DCO
Details
WIP Ready for review
Details
codecov/patch 100% of diff hit (target 100%)
Details
codecov/project 100% (target 100%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.