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

Update distributed tracing doc to reflect changes in 0.8.0 #1415

Merged
merged 2 commits into from
Jun 4, 2018

Conversation

objectiser
Copy link
Contributor

Fixes istio/istio#5971

This PR also includes a minor formatting fix in the quick start page.

@istio-testing
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: objectiser
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: linsun

Assign the PR to them by writing /assign @linsun in a comment when ready.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@istio-testing
Copy link
Contributor

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

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.

@geeknoid
Copy link
Contributor

geeknoid commented Jun 1, 2018

Deploy preview for preliminary-istio failed.

Built with commit aaea14b

https://app.netlify.com/sites/preliminary-istio/deploys/5b11a0d8b13fb137a55243cc

@objectiser
Copy link
Contributor Author

/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.

@linsun linsun merged commit b989221 into istio:master Jun 4, 2018
@objectiser objectiser deleted the tracing branch June 4, 2018 13:15
geeknoid pushed a commit that referenced this pull request Jun 4, 2018
* Update distributed tracing doc to reflect changes in 0.8.0

* Updated images and reference from GKE setup page

(cherry picked from commit b989221)
ayj pushed a commit to ayj/istio.github.io that referenced this pull request Jun 9, 2018
* Update distributed tracing doc to reflect changes in 0.8.0

* Updated images and reference from GKE setup page

(cherry picked from commit b989221)
@codefromthecrypt
Copy link

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.

@objectiser
Copy link
Contributor Author

@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.

@codefromthecrypt
Copy link

@costinm @mandarjog can you make public your conversation? cc also @rshriram who approved the deletion?

@codefromthecrypt
Copy link

@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

@douglas-reid
Copy link
Contributor

@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.

@codefromthecrypt
Copy link

@douglas-reid in my book, once resolved, all-good. Appreciate the frankness. I'll ask folks who were confused to review your work.

@codefromthecrypt
Copy link

This appears to be the PR to resolve this: istio/istio#9910

@codefromthecrypt
Copy link

codefromthecrypt commented Nov 13, 2018

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
Expedia Haystack can be pushed spans via HotelsDotCom PitchFork which probably starts very fast :P cc @worldtiki

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

@douglas-reid
Copy link
Contributor

@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.

@codefromthecrypt
Copy link

codefromthecrypt commented Nov 13, 2018 via email

3ks added a commit to 3ks/istio.io that referenced this pull request Dec 13, 2019
istio-testing pushed a commit that referenced this pull request Dec 15, 2019
* zh-translation: /faq/mixer/header-rules.md #1415

* adopt gauliang's advise
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.

Distributed tracing page out of date with 0.8.0
7 participants