internal/datastreams: fix memory leak

[piochelepiotr]

What does this PR do?

Context here: https://stackoverflow.com/questions/18598780/is-resp-body-close-necessary-if-we-dont-read-anything-from-the-body/18601625#18601625

Motivation

Memory leak.

Reviewer's Checklist

• Changed code has unit tests for its functionality at or near 100% coverage.
• There is a benchmark for any new code, or changes to existing code.
• If this interacts with the agent in a new way, a system test has been added.

For Datadog employees:

• If this PR touches code that handles credentials of any kind, such as Datadog API keys, I've requested a review from @DataDog/security-design-and-guidance.
• This PR doesn't touch any of that.

Unsure? Have a question? Request a review!

[pr-commenter]

Benchmarks

Benchmark execution time: 2023-11-06 19:58:19

Comparing candidate commit ef405b7 in PR branch piotr-wolski/fix-memory-leak with baseline commit 3b5b794 in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 41 metrics, 0 unstable metrics.

[github-actions]

This PR is stale because it has been open 20 days with no activity. Remove stale label or comment or this will be closed in 10 days.

[15rsirvin]

Any chance on this getting merged? Also, is data-streams-go out of alpha now that it has been moved into the dd-trace-go package? It isn't clear to me that this feature is prod ready.

[katiehockman]

Suggested change

	msg := make([]byte, 1000)
	n, _ := resp.Body.Read(msg)
	resp.Body.Close()
	if code := resp.StatusCode; code >= 400 {
	// error, check the body for context information and
	// return a nice error.
	defer resp.Body.Close()
	if code := resp.StatusCode; code >= 400 {
	// error, check the body for context information and
	// return a nice error.
	msg := make([]byte, 1000)
	n, _ := resp.Body.Read(msg)

If there's a chance you're going to need to throw away n, then don't read it until you know you need to. I suggest just deferring the close and then reading the body if and when you really need it

[piochelepiotr]

So, ideally I would like to do that, but it looks like we need to read the bytes for the structure to be re-used:

// Body which the user is expected to close. If the Body is not both
// read to EOF and closed, the Client's underlying RoundTripper
// (typically Transport) may not be able to re-use a persistent TCP
// connection to the server for a subsequent "keep-alive" request.

From here:
https://github.com/golang/go/blob/master/src/net/http/client.go#L562

[knusbaum]

This doesn't guarantee we read to EOF, just that we read <=1000 bytes.

Let's just use the standard io.Copy and io.Discard like we do in the agent: https://github.com/DataDog/datadog-agent/blob/main/pkg/trace/api/api.go#L467
ideally with a defer along with the Close.

Then we can only allocate a buffer for the message and read into it if we actually need it.

[knusbaum]

This doesn't guarantee we read to EOF, just that we read <=1000 bytes.

Let's just use the standard io.Copy and io.Discard like we do in the agent: https://github.com/DataDog/datadog-agent/blob/main/pkg/trace/api/api.go#L467
ideally with a defer along with the Close.

Then we can only allocate a buffer for the message and read into it if we actually need it.

[knusbaum]

Looks good.