Add Metrics to RetryableHTTPClient #4545

mustansir14 · 2025-11-07T14:46:27Z

Description:

This PR adds a new HTTP response body size metric to InstrumentedTransport, which is the current Transport for SaneHTTPClient. InstrumentedTransport is also set to be used as the Transport for RetryableHTTPClient.

Challenge with Response Body Size Calculation:

The Response object has a ContentLength field that may have exactly what we're looking for, but it is not guaranteed to be provided by the server. Also it is never available for a chunked response.
A simple approach would be to read the full body in memory and get it's size, but this is inefficient as responses can be large. Also it may add a delay to the RoundTrip() call, and would require restoring the body so that downstream code can read it again.

Solution:

To address this, a responseSizeCounterReadCloser is introduced to measure the size of the response body WHILE the downstream code reads it. This ensures no overhead is added in the RoundTrip() call. This however will not work if the downstream code never reads the body (e.g. detectors where only the StatusCode is used to verify), so to handle this, we add a check in the Close() method of the ReadCloser to drain the body if it's not consumed.

Checklist:

Tests passing (make test-community)?
Lint passing (make lint this requires golangci-lint)?

…tedTransport to RetryableHTTPClient

amanfcp · 2025-11-07T15:47:34Z

pkg/common/http.go

+
+		// wrap the response body to count bytes read
+		resp.Body = &responseSizeCounterReadCloser{
+			ReadCloser: resp.Body,
+			recordSizeFunc: func(size int) {
+				// Record response body size
+				recordResponseBodySize(sanitizedURL, size)
+			},
+		}


This would also start generating metrics for SaneHTTPClient. Just wanna make sure if this is intentional.

Yes, this is intended. We also want to add this metric to SaneHTTPClient

rosecodym

This is pretty interesting, but I did leave an inline question.

Also, please add doc comments for your new exported functions. Thanks!

rosecodym · 2025-11-10T13:59:26Z

pkg/common/http.go

+	if r.recordSizeFunc != nil {
+		if r.bytesRead == nil {
+			// this means downstream code never consumed the body, we must drain the body to get its size
+			_, err := io.Copy(io.Discard, r)


Does this cause Read to be called? If so, can you put that in a comment?

rosecodym · 2025-11-10T14:04:37Z

pkg/common/http.go

+
+func (r *responseSizeCounterReadCloser) Close() error {
+	if r.recordSizeFunc != nil {
+		if r.bytesRead == nil {


If a previous call to Read didn't read the entire payload, then the leftover size won't be counted, right? E.g.

buf := make([]byte, 10) reader.Read(buf) // bytesRead is now 10 reader.Close() // bytesRead is not updated

This is kind of a niche case, so I'm not sure how much of a problem it is, but if I'm correct, this should at least be called out in a comment somewhere.

Hmm, that's an interesting case. You're right, a partial read will cause us to not have a correct size. But do you think this is a problem? I can't think of any case where a detector or any piece of code would partially read a response.

I can surely call this out in a comment though.

rosecodym · 2025-11-10T14:09:58Z

pkg/common/http_test.go

+		name               string
+		path               string
+		expectedStatusCode int
+		expectsNon200      bool


What is this field for? It looks unused.

Yep, good catch. I basically copy-pasted the test for SaneHTTPClient so this got copied over as well. Will remove it

rosecodym · 2025-11-10T14:10:35Z

pkg/common/http_test.go

+
+			// Get initial metric values
+			sanitizedURL := sanitizeURL(requestURL)
+			initialRequestsTotal := testutil.ToFloat64(httpRequestsTotal.WithLabelValues(sanitizedURL))


Whoa I didn't know we could test Prometheus metrics like this!

camgunz

I really like how this is super accurate, which wasn't easy to do! But instead of counting what we read (and potentially manually reading), I think we should start out by just using ContentLength from the Response object. It's definitely less reliable, but I don't want to get out the big hammer until we see how much less reliable it really is.

mustansir14 · 2025-11-21T09:15:29Z

I really like how this is super accurate, which wasn't easy to do! But instead of counting what we read (and potentially manually reading), I think we should start out by just using ContentLength from the Response object. It's definitely less reliable, but I don't want to get out the big hammer until we see how much less reliable it really is.

I really liked that code 🥹. But I guess you're right, it is overkill for just metrics. I'll scrap it

shahzadhaider1 · 2025-11-24T08:14:46Z

pkg/common/http_metrics.go

+	// Record response body size if known
+	if contentLength >= 0 {
+		httpResponseBodySizeBytes.WithLabelValues(sanitizedURL).Observe(float64(contentLength))
+	}


this if will always be true?

I assume this check to ensure that the contentLength value being passed into the function is valid (i.e. >0)

Here's what net/http documentation says about the ContentLength field:

// ContentLength records the length of the associated content. The
// value -1 indicates that the length is unknown. Unless Request.Method
// is "HEAD", values >= 0 indicate that the given number of bytes may
// be read from Body.
ContentLength int64

So there is a possibility of the value being -1

nabeelalam

Thanks @mustansir14

add response body size metric to InstrumentedTransport, add Instrumen…

86d0fb0

…tedTransport to RetryableHTTPClient

mustansir14 requested a review from a team November 7, 2025 14:46

mustansir14 requested a review from a team as a code owner November 7, 2025 14:46

amanfcp reviewed Nov 7, 2025

View reviewed changes

rosecodym reviewed Nov 10, 2025

View reviewed changes

mustansir14 added 3 commits November 11, 2025 14:29

add doc to responseSizeCounterReadCloser, remove unused field from test

5ca7474

Merge branch 'main' into INS-162-Add-metrics-to-detector-HTTP-requests

bd4c865

Merge branch 'main' into INS-162-Add-metrics-to-detector-HTTP-requests

7b62061

mustansir14 requested review from amanfcp and rosecodym November 18, 2025 05:26

Merge branch 'main' into INS-162-Add-metrics-to-detector-HTTP-requests

66addf9

camgunz requested changes Nov 20, 2025

View reviewed changes

record response using ContentLength

117f95d

mustansir14 requested a review from camgunz November 21, 2025 09:28

shahzadhaider1 approved these changes Nov 24, 2025

View reviewed changes

nabeelalam approved these changes Nov 24, 2025

View reviewed changes

Merge branch 'main' into INS-162-Add-metrics-to-detector-HTTP-requests

0dc81d8

Add Metrics to RetryableHTTPClient #4545

Are you sure you want to change the base?

Add Metrics to RetryableHTTPClient #4545

Conversation

mustansir14 commented Nov 7, 2025

Description:

Challenge with Response Body Size Calculation:

Solution:

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rosecodym left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mustansir14 Nov 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

camgunz left a comment

Choose a reason for hiding this comment

Uh oh!

mustansir14 commented Nov 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nabeelalam left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mustansir14 Nov 11, 2025 •

edited

Loading