Fix 1116, remove parsing of resp body from http.rb instrumentation

[ericmustin]

Resolves #1116

This piggybacks off the discussion here #1117

Ultimately, it is not safe to read the http.rb response body in ddtrace: we end up consuming the body reading stream, so the host application cannot properly read it. The httprb suggests usage of .readpartial which we would also impact.

To address this issue we should stop reading the response body message altogether. This will remove some information from our error reporting, but it's ultimately required for the safe operation of ddtrace with http.rb.

[marcotc]

🐛 🔨

[ericmustin]

@marcotc accidentally dismissed review trying to kick tests :( . will merge tmrw, no rush

[cdmo]

This is an old and closed issue, but, is there a specific example of how checking the response body makes it unavailable to the host app? We are monkeypatching this gem in an app to add some cases where we don't want ddog to tag things as errors because for our application there are many non-200 responses that are part of the standard day-in day-out operation based on how the webservice we consume reacts. When we do this, the host app can still access the body, so curious about what context would prevent the host app from being able to access the body 🤔

[marcotc]

👋 @cdmo, what we noticed is that some versions of http.rb would completely consume the payload when executing JSON.parse(response.body), leaving the body unreadable for the host application.

This only happens in cases where the payload is a stream, thus would need to be manually rewound by the host application to be read again (if that's even possible).

Also, it's generally unadvised to read a response.body, as we don't know if it's only a few bytes or gigabytes of data.

If your application does not suffer from this issue, similar to how our test suite didn't, you are likely safe to read response.body as many time as you'd like. For us, we can't be sure, and it's not guaranteed to be safe to read the payload.

[cdmo]

@marcotc thanks for the quick and thorough response and it makes total sense. We have guarantees that the payload will always be small for this application and for the version of httprb and ruby and all other pieces of the app, the http response body is still available. We'll be monkeypatching this gem and will just have to be careful to check on functionality of the app and datadog reporting when we update this gem or httprb (or maybe ruby too). Thanks again.

[marcotc]

The changes in psu-libraries/myaccount#341 make think about adding some sort of interface with a user accessible callback for each span created, including arguments and local variables available at the time.

I'll keep this in mind 🤔

[ericmustin]

@marcotc i think a hooks interface is worth surfacing imo.

There's prior art here in the node tracer: https://datadoghq.dev/dd-trace-js/interfaces/plugins.http.html#hooks

Additionally we have a sort've orphaned/undocumented feature similar to this for net/http #724

iirc we had concerns about the net/http hook being misused for resource renaming to create resources with cardinality issues, but for use cases like the one above this is far more efficient than what we'd currently expose to modify spans, a trace processor.

lastly i went to penn state and spent a lot of time in the paterno library, so, it'd be cool to ship something that was useful for them 😅.

[cdmo]

Cool! Thanks to you both for considering this. If you ever find yourself back in Happy Valley, feel free to stop by the library and say hi :) I guess it's true what the student ambassadors say, there are Penn State grads everywhere!