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

Updated kit/tracing readme's and removed deprecated kit/tracing/zipkin package #275

Merged
merged 2 commits into from
May 26, 2016

Conversation

basvanbeek
Copy link
Member

I've removed the deprecated kit/tracing/zipkin package but have left the folder available. The folder now has some explanation on how to transition from our legacy Zipkin package to OpenTracing and it remains to hold the docker-compose script for getting a Zipkin back-end bootstrapped quickly for development.

`tracing/zipkin` package is now deprecated. In the `kit/tracing/zipkin`
directory you can still find the `docker-compose` script to bootstrap a Zipkin
development environment and a [README] detailing how to transition from the
old package to the new.
Copy link
Member

Choose a reason for hiding this comment

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

Clear and concise explanation. Perfect. Thank you.

Copy link

Choose a reason for hiding this comment

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

+1!

@peterbourgon
Copy link
Member

-17,350 💃

@freeformz
Copy link
Contributor

Thoughts on having a tracing/zipkin/doc.go that describes the change?

@basvanbeek
Copy link
Member Author

basvanbeek commented May 23, 2016

For a full example of how the OpenTracing middleware should be wired we have an updated examples/addsvc example, for more details on the specifics regarding Zipkin and how to add resource spans the update includes a new tracing/zipkin/README.md.

Can you elaborate on what in your mind is currently lacking in documenting the change so we can improve this PR if needed?

// let's annotate the end...
span.LogEvent("query:end")

// we're done with the span. send it to the SpanRecorder for collection...
Copy link

Choose a reason for hiding this comment

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

FYI, the OpenTracing spec doesn't address what happens to Spans after they are Finish()ed... (basictracer might, but that's just a convenience repo for folks implementing tracing systems)

Copy link
Member Author

Choose a reason for hiding this comment

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

This is how the Zipkin implementation works and that's the scope of this document, but I'll leave it out if it would confuse people.

@basvanbeek
Copy link
Member Author

updated the zipkin readme regarding span per node vs span per rpc.

@peterbourgon
Copy link
Member

Excellent. LGTM. I'll let you merge as you have the contributor bit now.

@basvanbeek basvanbeek merged commit f867f38 into go-kit:master May 26, 2016
guycook pushed a commit to codelingo/kit that referenced this pull request Oct 12, 2016
Updated kit/tracing readme's and removed deprecated kit/tracing/zipkin package
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.

4 participants