-
Notifications
You must be signed in to change notification settings - Fork 600
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 async http instrumentation #2272
Conversation
protocol http object
test/multiverse/suites/async_http/async_http_instrumentation_test.rb
Outdated
Show resolved
Hide resolved
Co-authored-by: James Bunch <[email protected]>
Co-authored-by: James Bunch <[email protected]>
Co-authored-by: James Bunch <[email protected]>
…newrelic-ruby-agent into add_async_http_instrumentation
Co-authored-by: James Bunch <[email protected]>
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.
GitHub is showing me "4" comments, so I think some of what shows up will be moot based on more recent changes!
# Controls auto-instrumentation of Async::HTTP at start up. | ||
# May be one of [auto|prepend|chain|disabled] | ||
# instrumentation.async_http: auto | ||
|
||
# Controls auto-instrumentation of the concurrent-ruby library at start up. May be |
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.
@tannalynn - Out of curiosity, did you add this manually or use the script? (If you used the script, I'm a bit concerned about why the first version was outdented one level)
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.
I used the generator originally, but there was a merge conflict so i had to copy it and put it back, it's very possible that i messed up the indentation when i resolved the merge conflict.
LHOST = 'host' | ||
UHOST = 'Host' | ||
COLON = ':' |
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.
Not for this PR, but maybe it's time to move these constants to a shared location.
Co-authored-by: James Bunch <[email protected]>
SimpleCov Report
|
Exciting stuff, thank you! /cc @ioquatix in case he's interested in having a look 👀 |
Thanks for working on this. I don't want to come across as negative as I think it's awesome someone is getting value from this, but:
I'm cautious about monkey patching approaches as unless tested in non-production (non-instrumented) environments, can suddenly break on deploying to production. Ideally, we can implement |
@ioquatix An initial question I have after a quick look at it: Is there a way to insert into the headers? I see there is a w3c trace context method built in, but the new relic ruby agent has other types of headers that we will need to insert into outgoing request headers in certain situations. I'm not sure if that would be possible though. I'll have to spend a little time setting up my test app to use traces and see how the things looks when creating a span with the new relic ruby agent with the traces interface. Once I've had a chance to do that I'll have a better idea of if we'll be able to move our instrumentation there. Thank you for pointing this out and letting us know about traces! |
If you use It is not currently possible to inject headers. I don't think we should change this but maybe that's something that can be monkey patched for legacy code? |
Creating a new relic implementation of traces seems like it would be a great thing for us to do to get more visibility, so I've created an issue #2283 for us to work on it and tentatively plan to work on it next quarter. As for being able to insert headers, the agent does that on the HTTP client side in order to then parse it out on the HTTP server side (presuming both client and server are instrumented by New Relic). This is something we need to do to support other new relic products (for example: synthetics monitoring https://newrelic.com/platform/synthetics). If that's not something we can do with traces, we'll have to carry on monkey patching for async http still. |
Hello, I know it sounds like even more good stuff is coming later with #2283 but I wanted to say thank you for working on this feature. I just rolled out a NR gem update in an app where I'm using Async::HTTP heavily and things are working great. It's lovely having this extra visibility now -- I really missed that when migrating from Typhoeus. Thank you! |
Adds instrumentation for async-http
resolves #1176