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 gRPC-OpenTracing interceptors for Go #3

Merged
merged 5 commits into from
Sep 10, 2016
Merged

Add gRPC-OpenTracing interceptors for Go #3

merged 5 commits into from
Sep 10, 2016

Conversation

bhs
Copy link
Contributor

@bhs bhs commented Sep 5, 2016

See the README.md for usage examples, etc.

This is possible thanks to grpc/grpc-go#867.

@bhs
Copy link
Contributor Author

bhs commented Sep 5, 2016

cc: @dominikh per our conversation on Gitter about this

I would like to include more information about the peer (via tags/etc), but unfortunately I'm not sure where to access that given what's (not) exported in gRPC.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@bhs
Copy link
Contributor Author

bhs commented Sep 5, 2016

Per the google CLA thing: "I did it"

@googlebot
Copy link

CLAs look good, thanks!

@bg451
Copy link

bg451 commented Sep 8, 2016

Everything looks good to me, although I would recommend adding an Option that allows users to specify a set of tags that'll be applied whenever a new span is created in the interceptor.

md = metadata.New(nil)
}
mdWriter := metadataReaderWriter{md}
tracer.Inject(clientSpan.Context(), opentracing.TextMap, mdWriter)
Copy link

Choose a reason for hiding this comment

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

There's an error to handle, or a comment explaining why not.

@jmacd
Copy link

jmacd commented Sep 8, 2016

👍

}
mdWriter := metadataReaderWriter{md}
err = tracer.Inject(clientSpan.Context(), opentracing.TextMap, mdWriter)
// We have no better place to record an error than the Span itself :-/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jmacd this is how I dealt with Inject errors...

image

It gets the job done. Registering an error handler as an Option seemed like overkill.

@bhs
Copy link
Contributor Author

bhs commented Sep 10, 2016

Comments addressed... merging.

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