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

Remove zipkin service #75

Merged
merged 1 commit into from
Oct 30, 2018
Merged

Remove zipkin service #75

merged 1 commit into from
Oct 30, 2018

Conversation

objectiser
Copy link
Contributor

Originally the zipkin service was added to the jaeger-kubernetes template to provide a drop-in replacement for the zipkin deployment that was included as an addon within the Istio installation instructions.

Over time, the template was moved into the Istio helm chart, and a specific zipkin service was created to provide a "zipkin compatible" endpoint for any user selected tracing backend to support. However it was not removed from the upstream jaeger-kubernetes templates, which were no longer being used as part of the Istio installation.

The jaeger-collector provides access to the zipkin port, so I don't believe the jaeger templates/operator need to provide a separate service any more.

The main purpose of this change is that I am investigating the potential for using the jaeger-operator as part of the Istio helm installation - and it is now resulting in two "zipkin" related services being created. One representing the zipkin service that is used as the endpoint for all Istio components to report zipkin compatible tracing data to, and the other created by the operator e.g. <jaeger-instance-name>-zipkin.

As the jaeger-operator is still in early development, now would be a good time to clean up these issues. If users still require access to the zipkin service, separate from the jaeger-collector service, then it could always be added back in as an option.

Signed-off-by: Gary Brown [email protected]

Signed-off-by: Gary Brown <[email protected]>
@jpkrohling
Copy link
Contributor

This change is Reviewable

@codecov
Copy link

codecov bot commented Oct 23, 2018

Codecov Report

Merging #75 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #75      +/-   ##
==========================================
- Coverage   99.37%   99.34%   -0.03%     
==========================================
  Files          19       18       -1     
  Lines         801      769      -32     
==========================================
- Hits          796      764      -32     
  Misses          5        5
Impacted Files Coverage Δ
pkg/deployment/all-in-one.go 100% <ø> (ø) ⬆️
pkg/deployment/collector.go 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 92fce64...2b6cfee. Read the comment docs.

@pavolloffay
Copy link
Member

I think the original design was also to provide a smooth migration from https://github.com/fabric8io/kubernetes-zipkin to jaeger installation. We could expose the port by default and deploy the service based on the config option.

Copy link
Contributor

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

Is this only about the Kubernetes service? The env var COLLECTOR_ZIPKIN_HTTP_PORT is still there in the deployment, but I think those have to stay there, right?

This LGTM and it all tests are passing, including the e2e tests:

ok  	github.com/jaegertracing/jaeger-operator/test/e2e	206.654s

Reviewed 9 of 9 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@objectiser
Copy link
Contributor Author

@jpkrohling Yes only the service needs to be removed. If current users of the template find this to be an issue when switching to the operator, then we can added it back in as an option.

@jpkrohling jpkrohling merged commit ef89b39 into jaegertracing:master Oct 30, 2018
@objectiser objectiser deleted the rmzipkinservice branch January 29, 2019 12:40
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.

3 participants