-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Update distributed tracing doc to reflect changes in 0.8.0 #1415
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: objectiser Assign the PR to them by writing The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Hi @objectiser. Thanks for your PR. I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Deploy preview for preliminary-istio failed. Built with commit aaea14b https://app.netlify.com/sites/preliminary-istio/deploys/5b11a0d8b13fb137a55243cc |
/assign @linsun Someone may need to review the GKE setup instructions and update as necessary (e.g. console image). I've just changed the tracing related refs/image. |
* Update distributed tracing doc to reflect changes in 0.8.0 * Updated images and reference from GKE setup page (cherry picked from commit b989221)
* Update distributed tracing doc to reflect changes in 0.8.0 * Updated images and reference from GKE setup page (cherry picked from commit b989221)
so there was template drift.. to address this you decided to delete zipkin. This was done by a jaeger committer. @linsun I doubt you meant this, but this action is beyond insensitive. Consider jaeger has skin in the game of deleting zipkin. Then consider how nice it is to delete zipkin off their own formats! This has already caused confusion in the user base. If this is about helm chart there was already one from the community. https://github.com/Financial-Times/zipkin-helm Consider there's only one full time staff in zipkin, we rely in part on the kindness of staffed projects like yours to do the right thing. Volunteers do quite a lot, but can't be tasked to track everything. Does anyone have time to make this right? Restore the docs, fix the template? You might be able to fix this quicker. Meanwhile I will try to help source someone. |
@adriancole The switch from zipkin to jaeger backend was actually done by @douglas-reid in this PR istio/istio#5656. I had previously tried to include Jaeger helm chart alongside Zipkin, but the PR was not accepted. It has long been the intention to support multiple tracing backends in Istio's helm chart, so would be good to have zipkin supported again. If I get some free time I will try to add it in - but if you find someone else that would be great. |
@costinm @mandarjog can you make public your conversation? cc also @rshriram who approved the deletion? |
@objectiser appreciate your letting know the background. consider you helping to remove all references is something not quite cool, even if others did worse. Appreciate for sure if you can help make this right. I hope you won't get blocked by google folks or otherwise again |
@adriancole if I may speak for @costinm and @mandarjog, iirc, around the 0.8 timeframe, there was concern with the startup time of the zipkin add-on, as this impacted overall runtime of end-to-end tests, etc. I was asked to switch to the default add-on to jaeger, as it was felt that this would improve the situation. I don't know if the switch ended up having the intended impact, as the details of that release have been purged from my memory. As @objectiser relates, there remains a strong intention to support multiple tracing integrations. Unfortunately, during release crunches, some of our best intentions go unfulfilled -- and we find ourselves in situations such as this. As I was the one that ultimately submitted the PR that led to this situation, I will take ownership of trying to restore. I will add you as a reviewer to ensure that I'm using the most recent images, etc. |
@douglas-reid in my book, once resolved, all-good. Appreciate the frankness. I'll ask folks who were confused to review your work. |
This appears to be the PR to resolve this: istio/istio#9910 |
Side point on multiple tracing systems: While it certainly is the most marketed, jaeger is not the only zipkin emulator that benefits from the work done. If the goal is to support more backends, you probably already do, maybe just don't know it. Some of these tools have very advanced analytical capabilities. Skywalking 6 accepts zipkin ingress cc @wu-sheng There are others as well. while usually not discussed publicly, quite a lot of vendors have mentioned zipkin ingress to me personally. All of the receivers would have less work if envoy supported zipkin's compact (v2) format in either json (supported by all the above) or protobuf (supported by all except jaeger, but they probably could easily support it). This is due to lack of help in the envoy project, eventhough there are many that want this. The main problem is sourcing c++ help. envoyproxy/envoy#4839 |
@adriancole I would love to have an integration that we could point to with Skywalking, etc. I've been meaning to get around to documenting, in full, the different ways that Istio can support multiple vendors and formats -- as well as to try to balance the various approaches to integrating different tracing formats and backends (more envoy drivers v. mixer adapters v. opencensus exporters v. ??). If you have some time to spare and are willing to help sort out recommendations, etc., I would appreciate any advice / guidance / feedback you are willing to provide. In a similar vein, I've been trying to get traction on a full Istio API for controlling tracing in general (see: istio/api#363). As of now, all we have are env vars and install options. Any help there would be doubly-welcome. I'm not sure I can offer much c++ help at the moment, but I'll definitely keep that in mind should the opportunity arise. Thanks again for engaging and helping to push us forward. |
advice is cheap :) yeah the ones mentioned accept zipkin format would be
easiest as you have to do similar to jaeger or zipkin to drop them in.
Ones that can't might be more tricky or end up a backend to trace forwarder
like pitchfork. You could imagine that pitchfork could send zipkin data to
Amazon X-Ray for example. Zipkin's server can proxy to x-ray as well.
can't do much more than advice right now but hope it helps.
…On Wed, 14 Nov 2018, 07:00 Douglas Reid, ***@***.***> wrote:
@adriancole <https://github.com/adriancole> I would love to have an
integration that we could point to with Skywalking, etc. I've been meaning
to get around to documenting, in full, the different ways that Istio can
support multiple vendors and formats -- as well as to try to balance the
various approaches to integrating different tracing formats and backends
(more envoy drivers v. mixer adapters v. opencensus exporters v. ??). If
you have some time to spare and are willing to help sort out
recommendations, etc., I would appreciate any advice / guidance / feedback
you are willing to provide.
In a similar vein, I've been trying to get traction on a full Istio API
for controlling tracing in general (see: istio/api#363
<istio/api#363>). As of now, all we have are env
vars and install options. Any help there would be doubly-welcome.
I'm not sure I can offer much c++ help at the moment, but I'll definitely
keep that in mind should the opportunity arise.
Thanks again for engaging and helping to push us forward.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1415 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAD617Mlf4s3Q5dV3ETVtB0BDcHmQN-Nks5uu08CgaJpZM4UXNoT>
.
|
Fixes istio/istio#5971
This PR also includes a minor formatting fix in the quick start page.