Add instrumentation for HTTP.rb

[ericmustin]

Basic Instrumentation for http.rb using the features middleware that underlies.

• https://github.com/httprb/http/wiki/Logging-and-Instrumentation#instrumentation
• https://github.com/httprb/http/pull/499
• https://github.com/httprb/http/tree/4-x-stable/lib/http/features

This PR addresses this community card and open issue 529

Essentially httprb offers middleware functionality for wrapping the request and response methods in custom methods, where we inject distributed tracing context , start/stop the spans, and add the relevant span tags metadata.

Submitting this a bit early so I can get some feedback on whether this approach is appropriate here, I think it's much cleaner than trying to overwrite the entire Http::Client. This is still pretty rough as I need to make the error handling a bit more defensive and ensure the appropriate params get returned when the tracer is disabled, but getting this in early to see if this approach is appropriate.

TODO:

• clean up implementation add more robust error handling
• tests/specs
• docs
• test against chaining/tracer disabled/edge case scenarios more thoroughly

[marcotc]

Question for @delner: what's our way to ensuring that tracer.active_span is the trace we expect in integrations like these (with separate start and end events)?

It seems like it would find the correct one no child integration "leaks", but I'm not sure how confident we are in this.

[delner]

I think this is a good observation @marcotc; I don't think this is a safe operation because although in theory, if everything else is working as intended it should be no problem. If a span is opened (but not closed) between wrap_request and wrap_response, this would return the wrong span.

If there's a better way to 'track' the span without leaking, we should do that instead. Using tracer.trace with a block is the most reliable way. I think Marco suggested the use of ActiveSupport notifications elsewhere, for which we already have a module that serves as a reliable means of doing this work for you: so we might want to consider that.

[ericmustin]

per latest comments, refactored to monkeypatching + wrapping with .trace, rather than AS approach

[ericmustin]

idk why this keeps starting a review every time i want to make a one comment reply 😭

[delner]

As its currently implemented, does instrumentation for HTTP.rb apply out of the box when you set use: httprb?

[ericmustin]

very sorry for the long delay here folks. My bandwidth has been stretched thinner than i'd like. Finally had a free day so was able to get to this.

After reviewing the feedback here from @delner : #853 (comment) I decided to do a rewrite of the httprb instrumentation using a tracer.trace to wrap the request. It involves monkey-patching, which i think is the most robust way to ensure the span is tracked without leaking.

For context, at first I did attempt to rewrite this to use the Active Support approach suggested by @marcotc here: #853 (comment) However, bc httprb has separate AS events for a request's start and end, it isn't a clean fit w/dd-trace-rb's AS Module, and makes "stitching them together" difficult, introducing many of the same issues w/span context leaking that the original PR had. Ultimately, I think in order to use AS reliably, it would involve modifying the request object to attach a private _datadog_span key to maintain span context, which felt hackier than simply wrapping the request via monkey-patching.

I've tried to bring this back up to a reviewable state, and up to date with master. I think I need to add some tests for analytics + distributed tracing, but given the refactor, wanted to get a review to make sure I'm on the right track. Some notes for reviewer

• It's possible I have missed some of the latest features or integration requirements (changes to sampling and measured span tags, to name a few, I wasn't sure about how to handle).

• The tests are admittedly brittle, i also noticed this warning log /usr/local/bundle/gems/dogstatsd-ruby-4.7.0/lib/datadog/statsd/connection.rb:12: warning: instance variable @socket not initialized seems to emit quite frequently with a number of tests? Not sure if that's related to this PR

• I did have a little bit of trouble getting span.set_error to work as expected. Let me know if there's a more elegant way to handle setting error tags for 400/500 level responses.

• I more or less have tried to model the instrumentation after the current http instrumentation.

let me know what you think. I'm sorry again about the long delay here, definitely will make sure to prioritise any feedback to get this over the finish line.

[marcotc]

@ericmustin the implementation looks in line with our existing HTTP client implementations, great job!

One thing that we'll want before we can merge this PR is to add documentation about this integration to our GettingStarted.md.

[ericmustin]

@marcotc gotcha, ty for taking a look. ok let me clean this up otw, and will try to review/improve testing suite as well.

[delner]

One last request: do you think it is possible to support multiplexing, like we do for Net/HTTP

To this end I would recommend looking at the PatternResolver we implemented for some of the other HTTP libraries, which provides this functionality. You should be able to copy the implementation mostly from them. See #953.

[ericmustin]

@marcotc @delner Make sense. I've taken a pass at adding multiplexing here for http.rb along with specs for it. Borrowed heavily from the net/http multiplexing implementation. Have also updated the docs, both per the feedback above and to include the split_by_domain config option. Lmk what y'all think, thanks again for taking a look!

[renchap]

Friendly bump? I would love to have this instrumentation in the gem!

[joaovitoras]

Is this work done? I would love to have it available

[marcotc]

Looks very close to completion!

I notice we are missing an Integration test, similar to: https://github.com/DataDog/dd-trace-rb/blob/master/spec/ddtrace/contrib/rack/integration_spec.rb

There's also a comment regarding documentation.

[marcotc]

We've removed the tracer option from integrations in #1079, we can remove it here too.

[ericmustin]

ah i see, ty. updated, had to clean up the tests and some of the pin code to account for this as well after bringing this up to date with master. I think it's good now tho

[ericmustin]

@marcotc thanks for the quick look, I added the integration_spec test suite for this integration and also brought things up to date after noticing some of the deprecation warnings i was seeing after bringing this up to date with master

[marcotc]

🎉

[marcotc]

Anything still missing @ericmustin? The current version seems to have a complete implementation, with tests and documentation. (I noticed that this PR is still tagged as do-not-merge/WIP)

[ericmustin]

@marcotc i believe it's good to go, that label was added by brett when the pr was first made, i've updated

[marcotc]

@ericmustin a few conflicts need to be resolved, but we are good to go!

[marcotc]

🚀 🚀 🚀