Add instrumentation for HTTP.rb #853

ericmustin · 2019-11-06T12:13:54Z

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

lib/ddtrace/contrib/httprb/datadog_wrap.rb

lib/ddtrace/contrib/httprb/patcher.rb

marcotc · 2019-11-07T17:23:25Z

lib/ddtrace/contrib/httprb/datadog_wrap.rb

+            return super(request) unless tracer_enabled?
+
+            tracer = datadog_configuration[:tracer]
+            datadog_span = tracer.active_span


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.

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.

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

…formatting

…le and appraisals

…, webmocks causing issues with when it intercepts requests

ericmustin

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

… error, wip tests for connection erroor

lib/ddtrace/contrib/httprb/patcher.rb

delner · 2019-11-15T02:22:25Z

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

merge master

…it tagging into request and response related tags

ericmustin · 2020-04-13T21:36:54Z

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 · 2020-04-17T11:08:46Z

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

…sts for distributed tracing and sampling

delner · 2020-04-20T21:47:41Z

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.

…ixes, clean up documentation around split_by_domain

ericmustin · 2020-04-21T09:29:02Z

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

… party repo

renchap · 2020-05-28T09:08:30Z

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

joaovitoras · 2020-06-23T13:41:56Z

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 · 2020-06-24T15:03:15Z

docs/GettingStarted.md

+| `distributed_tracing` | Enables [distributed tracing](#distributed-tracing) | `true` |
+| `service_name` | Service name for `httprb` instrumentation. | `'httprb'` |
+| `split_by_domain` | Uses the request domain as the service name when set to `true`. | `false` |
+| `tracer` | `Datadog::Tracer` used to perform instrumentation. Usually you don't need to set this. | `Datadog.tracer` |


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

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

merge master

ericmustin · 2020-06-25T08:46:32Z

@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 · 2020-07-08T20:14:30Z

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 · 2020-07-09T08:33:34Z

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

marcotc · 2020-07-09T19:13:09Z

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

marcotc

🚀 🚀 🚀

ericmustin added 4 commits November 5, 2019 23:37

add basic integration for httprb and patching code using features hooks

8323fde

fix accidental deletion of integration module

18b86d5

whitespace fix

0d27fa3

ensure block and check for tracer enabled in wrap request and response

9cd4215

ericmustin requested a review from a team November 6, 2019 12:13

brettlangdon added the do-not-merge/WIP Not ready for merge label Nov 6, 2019

marcotc reviewed Nov 7, 2019

View reviewed changes

ericmustin added 5 commits November 9, 2019 15:04

[wip] clean up request response wrapping and fix begin rescue ensure …

a154fe8

…formatting

clean up datadog wrap class, add base of specs and add gems to rakefi…

e9e0495

…le and appraisals

[wip] code cleanup and remove usage of .try

dc9d8ab

fix typo, start specs for request and response wrappers using webrick…

7515042

…, webmocks causing issues with when it intercepts requests

update tagging details and continue adding specs

009baa2

ericmustin commented Nov 12, 2019

View reviewed changes

ericmustin added 2 commits November 13, 2019 10:21

add specs for error responses and handling add wrapper for connection…

08dbf94

… error, wip tests for connection erroor

add spec for headers and sampling priority

57d7bc9

delner reviewed Nov 15, 2019

View reviewed changes

lib/ddtrace/contrib/httprb/patcher.rb Outdated Show resolved Hide resolved

delner mentioned this pull request Feb 25, 2020

Feature request: HTTP.rb gem trace support #529

Closed

ericmustin added 6 commits February 28, 2020 14:34

Merge branch 'master' into issue_529_httprb

fdd27fd

merge master

in prog moving to AS notifications

6b7aaed

remove event based integration attempt

8f43b89

refactor to use pin

da97a72

Merge branch 'master' into issue_529_httprb

485b228

merge master

[httprb] refactor tests clean up formatting handle error tags and spl…

a30e501

…it tagging into request and response related tags

marcotc reviewed Apr 14, 2020

View reviewed changes

[httprb] add documentation for httprb instrumentation and add more te…

e3a0650

…sts for distributed tracing and sampling

ericmustin changed the title ~~[WIP] Add instrumentation for HTTP.rb~~ Add instrumentation for HTTP.rb Apr 19, 2020

marcotc assigned ericmustin Apr 20, 2020

ericmustin added 2 commits April 21, 2020 03:04

[httprb] inprog adding multiplexing

df7122d

[httprb] ensure general distributed tracing config is used, rubocop f…

9409d8d

…ixes, clean up documentation around split_by_domain

[http.rb] lowercase the naming convention per style guidelines of 3rd…

974f434

… party repo

ericmustin requested a review from marcotc June 24, 2020 14:08

marcotc reviewed Jun 24, 2020

View reviewed changes

ericmustin added 3 commits June 25, 2020 09:56

[httprb] add integration test suite and update documentation

e380040

Merge branch 'master' into issue_529_httprb

c520129

merge master

merge master and bring tests up to date with current framework helpers

04e431d

ericmustin requested a review from marcotc July 8, 2020 08:54

marcotc previously approved these changes Jul 8, 2020

View reviewed changes

ericmustin added feature Involves a product feature and removed do-not-merge/WIP Not ready for merge labels Jul 9, 2020

[httprb] resolve conflicts

24d2036

ericmustin dismissed marcotc’s stale review via 24d2036 July 9, 2020 21:16

marcotc approved these changes Jul 9, 2020

View reviewed changes

ericmustin merged commit 5929c13 into DataDog:master Jul 10, 2020

marcotc added this to the 0.38.0 milestone Jul 10, 2020

marcotc added the integrations Involves tracing integrations label Jul 10, 2020

agrobbin mentioned this pull request Jan 16, 2021

Integrate with httpclient #1311

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add instrumentation for HTTP.rb #853

Add instrumentation for HTTP.rb #853

ericmustin commented Nov 6, 2019

marcotc Nov 7, 2019

delner Nov 15, 2019

ericmustin Apr 21, 2020

ericmustin left a comment

delner commented Nov 15, 2019

ericmustin commented Apr 13, 2020

marcotc left a comment

ericmustin commented Apr 17, 2020

delner commented Apr 20, 2020 •

edited

Loading

ericmustin commented Apr 21, 2020

renchap commented May 28, 2020

joaovitoras commented Jun 23, 2020

marcotc left a comment

marcotc Jun 24, 2020

ericmustin Jun 25, 2020

ericmustin commented Jun 25, 2020

marcotc left a comment

marcotc commented Jul 8, 2020

ericmustin commented Jul 9, 2020

marcotc commented Jul 9, 2020

marcotc left a comment

Add instrumentation for HTTP.rb #853

Add instrumentation for HTTP.rb #853

Conversation

ericmustin commented Nov 6, 2019

marcotc Nov 7, 2019

Choose a reason for hiding this comment

delner Nov 15, 2019

Choose a reason for hiding this comment

ericmustin Apr 21, 2020

Choose a reason for hiding this comment

ericmustin left a comment

Choose a reason for hiding this comment

delner commented Nov 15, 2019

ericmustin commented Apr 13, 2020

marcotc left a comment

Choose a reason for hiding this comment

ericmustin commented Apr 17, 2020

delner commented Apr 20, 2020 • edited Loading

ericmustin commented Apr 21, 2020

renchap commented May 28, 2020

joaovitoras commented Jun 23, 2020

marcotc left a comment

Choose a reason for hiding this comment

marcotc Jun 24, 2020

Choose a reason for hiding this comment

ericmustin Jun 25, 2020

Choose a reason for hiding this comment

ericmustin commented Jun 25, 2020

marcotc left a comment

Choose a reason for hiding this comment

marcotc commented Jul 8, 2020

ericmustin commented Jul 9, 2020

marcotc commented Jul 9, 2020

marcotc left a comment

Choose a reason for hiding this comment

delner commented Apr 20, 2020 •

edited

Loading