-
Notifications
You must be signed in to change notification settings - Fork 368
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
Integrate with httpclient #1311
Conversation
0d62b87
to
0bce74b
Compare
Codecov Report
@@ Coverage Diff @@
## master #1311 +/- ##
==========================================
- Coverage 98.13% 98.12% -0.02%
==========================================
Files 752 759 +7
Lines 35783 36120 +337
==========================================
+ Hits 35117 35444 +327
- Misses 666 676 +10
Continue to review full report at Codecov.
|
0bce74b
to
2ce6a09
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.
Thank you so much for your contribution, @agrobbin.
Everything looks great already!
If you are able to test these changes internally in your application it would give us a lot of confidence that the value it provides is what users (like yourself) want from httpclient
.
Let me know if any progress with using it yourself, or if you are unable to.
Again, awesome stuff! 🎉
I've implemented this in a very similar way as other HTTP client libraries that don't offer an official integration feature (i.e. a middleware stack).
2ce6a09
to
9497b21
Compare
Thanks @marcotc! I've was able to get this out into our staging environment (which is instrumented by Datadog), and here is the first trace from this new integration: I've hidden a few bits of information that could be considered sensitive to our application, but tried to showcase as much information as possible. At first glance, this looks correct, but I'd love to get your 👀 on it to be sure! |
@agrobbin Thanks for tackling this. We are also using |
My pleasure @djlmills! It does seem like google-api-ruby-client is planning to move from httpclient to Faraday (googleapis/google-api-ruby-client#2348), but I'm sure that won't happen overnight, so this is definitely going to be valuable! |
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.
Thank you again, @agrobbin!
Even if Google is moving away from httpclient
, the gem still has almost 100 million downloads of its latest version (admittedly likely due to Google's API).
This is my first dd-trace-rb contribution, so please let me know if I've missed anything!
httpclient is the underlying transport framework used by google-api-ruby-client, and it'd be great to have it instrumented by Datadog like the other libraries it already supports. This is based on #853, when http.rb integration was added. I've implemented this in a very similar way as other HTTP client libraries that don't offer an official integration feature (i.e. a middleware stack).