-
Notifications
You must be signed in to change notification settings - Fork 97
Add tracing #80
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 tracing #80
Conversation
Codecov Report
@@ Coverage Diff @@
## master #80 +/- ##
============================================
+ Coverage 68.83% 69.32% +0.49%
- Complexity 198 217 +19
============================================
Files 32 35 +3
Lines 770 841 +71
Branches 33 34 +1
============================================
+ Hits 530 583 +53
- Misses 217 233 +16
- Partials 23 25 +2
Continue to review full report at Codecov.
|
Wouldn't passing an instrumented http client to the github client accomplish the same but without extra dependencies? (sry if I'm missing smth - I only glanced through the changes) |
So this doesn't trace the actual outgoing calls towards GHE. It just starts a span whenever a request is made and ends it whenever the request is finished (future.get() etc.). |
d003cae
to
15059e9
Compare
d9f2326
to
7373638
Compare
7373638
to
ae9ceff
Compare
src/test/java/com/spotify/github/opencensus/OpenCensusTracerTest.java
Outdated
Show resolved
Hide resolved
a6473e6
to
4c6403c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
This adds support for distributed tracing.
By default a the NoopTracer is used which doesn't do anything. I've added a OpenCensusTracer implementation which can be used as well by setting client.withTracer(new OpenCensusTracer()).
This was very much inspired by the tracing inside the folsom client: github.com/spotify/folsom
Using lighstep it would look something like this:

om