Add async http instrumentation #2272

tannalynn · 2023-10-18T20:34:38Z

Adds instrumentation for async-http

resolves #1176

protocol http object

test/multiverse/suites/async_http/async_http_instrumentation_test.rb

lib/new_relic/agent/http_clients/async_http_wrappers.rb

lib/new_relic/agent/instrumentation/async_http.rb

lib/new_relic/agent/instrumentation/async_http/instrumentation.rb

newrelic.yml

test/multiverse/suites/async_http/Envfile

Co-authored-by: James Bunch <fallwith@gmail.com>

…newrelic-ruby-agent into add_async_http_instrumentation

Co-authored-by: James Bunch <fallwith@gmail.com>

kaylareopelle

GitHub is showing me "4" comments, so I think some of what shows up will be moot based on more recent changes!

lib/new_relic/agent/instrumentation/async_http.rb

newrelic.yml

kaylareopelle · 2023-10-19T19:23:24Z

newrelic.yml

+  # 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


@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)

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.

kaylareopelle · 2023-10-19T19:32:10Z

lib/new_relic/agent/http_clients/async_http_wrappers.rb

+        LHOST = 'host'
+        UHOST = 'Host'
+        COLON = ':'


Not for this PR, but maybe it's time to move these constants to a shared location.

test/multiverse/suites/async_http/Envfile

Co-authored-by: James Bunch <fallwith@gmail.com>

github-actions · 2023-10-20T00:25:53Z

SimpleCov Report

	Coverage	Threshold
Line	94.1%	94%
Branch	85.66%	85%

trevorturk · 2023-10-20T00:44:33Z

Exciting stuff, thank you!

/cc @ioquatix in case he's interested in having a look 👀

ioquatix · 2023-10-20T05:09:38Z

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:

There is a chance instrumentation that's injected like this will break due to internal changes or interface changes.
In order to deal with this, I created a gem, traces.
That gem provides a generic interface for tracing traces and spans, and provides implementation specific backends e.g. traces-backend-open_telemetry, traces-backend-datadog.
This ensures any tracing (including across service boundaries) is handled correctly and is compatible, as the tracing is provided by the library itself.

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 traces-backend-newrelic?

tannalynn · 2023-10-20T22:01:05Z

@ioquatix
Thanks for bringing this to our attention! I didn't realize that existed, I must have missed it when I looked originally, sorry about that! Before I started, I looked at the readme for async-http to see if there was anything built in for observability (like subscriptions or something like this) but didn't see anything mentioned so I ended up just approaching it the way we instrument most gems (with monkey patching and prepend options).

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!

ioquatix · 2023-10-20T22:28:28Z

If you use traces, you will get instrumentation across all the socketry gems where it's implemented.

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?

tannalynn · 2023-10-23T22:30:47Z

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.

trevorturk · 2023-11-06T23:11:04Z

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!

tannalynn added 8 commits October 10, 2023 13:52

Create async http instrumentation files

0587815

add http wrapper for async http

37e3044

instrument call method

2219100

http test cases working for async

76915eb

ensure headers work if hash, array or

d24d89a

protocol http object

Merge branch 'dev' into add_async_http_instrumentation

740bd51

delete comments

8d0b0cb

add changelog entry

ca6da27

tannalynn marked this pull request as ready for review October 18, 2023 21:06

tannalynn requested review from fallwith, hannahramadan and kaylareopelle as code owners October 18, 2023 21:06

fallwith reviewed Oct 18, 2023

View reviewed changes

tannalynn and others added 10 commits October 19, 2023 10:05

remove rescue from test

0b3edb6

Update newrelic.yml

00f0fce

Co-authored-by: James Bunch <fallwith@gmail.com>

Update lib/new_relic/agent/http_clients/async_http_wrappers.rb

33333f6

Co-authored-by: James Bunch <fallwith@gmail.com>

Update lib/new_relic/agent/instrumentation/async_http.rb

6be1dc1

Co-authored-by: James Bunch <fallwith@gmail.com>

move require

fef6ba7

Merge branch 'add_async_http_instrumentation' of github.com:newrelic/…

6717a9f

…newrelic-ruby-agent into add_async_http_instrumentation

Update test/multiverse/suites/async_http/Envfile

1906c10

Co-authored-by: James Bunch <fallwith@gmail.com>

add begin fro ruby 2.4

f148bfd

add to http client multiverse group

bad57ad

min version supported 0.59.0

3285c5b

kaylareopelle previously approved these changes Oct 19, 2023

View reviewed changes

kaylareopelle self-requested a review October 19, 2023 19:36

kaylareopelle reviewed Oct 19, 2023

View reviewed changes

test/multiverse/suites/async_http/Envfile Outdated Show resolved Hide resolved

kaylareopelle self-requested a review October 19, 2023 19:38

min ruby version 2.5

e911c78

tannalynn dismissed kaylareopelle’s stale review via e911c78 October 19, 2023 19:49

kaylareopelle previously approved these changes Oct 19, 2023

View reviewed changes

tannalynn dismissed kaylareopelle’s stale review via 57c81fe October 20, 2023 00:13

Update lib/new_relic/agent/instrumentation/async_http.rb

57c81fe

Co-authored-by: James Bunch <fallwith@gmail.com>

fallwith approved these changes Oct 20, 2023

View reviewed changes

tannalynn merged commit aab41ab into dev Oct 20, 2023
26 checks passed

fallwith deleted the add_async_http_instrumentation branch October 20, 2023 00:45

tannalynn mentioned this pull request Oct 23, 2023

Add instrumentation for Async/socketry gems #2283

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add async http instrumentation #2272

Add async http instrumentation #2272

tannalynn commented Oct 18, 2023 •

edited

Loading

kaylareopelle left a comment

kaylareopelle Oct 19, 2023

tannalynn Oct 19, 2023

kaylareopelle Oct 19, 2023

github-actions bot commented Oct 20, 2023

trevorturk commented Oct 20, 2023

ioquatix commented Oct 20, 2023 •

edited

Loading

tannalynn commented Oct 20, 2023

ioquatix commented Oct 20, 2023 •

edited

Loading

tannalynn commented Oct 23, 2023

trevorturk commented Nov 6, 2023

Add async http instrumentation #2272

Add async http instrumentation #2272

Conversation

tannalynn commented Oct 18, 2023 • edited Loading

kaylareopelle left a comment

Choose a reason for hiding this comment

kaylareopelle Oct 19, 2023

Choose a reason for hiding this comment

tannalynn Oct 19, 2023

Choose a reason for hiding this comment

kaylareopelle Oct 19, 2023

Choose a reason for hiding this comment

github-actions bot commented Oct 20, 2023

SimpleCov Report

trevorturk commented Oct 20, 2023

ioquatix commented Oct 20, 2023 • edited Loading

tannalynn commented Oct 20, 2023

ioquatix commented Oct 20, 2023 • edited Loading

tannalynn commented Oct 23, 2023

trevorturk commented Nov 6, 2023

tannalynn commented Oct 18, 2023 •

edited

Loading

ioquatix commented Oct 20, 2023 •

edited

Loading

ioquatix commented Oct 20, 2023 •

edited

Loading