Skip to content

Conversation

qdongxu
Copy link

@qdongxu qdongxu commented Jun 29, 2025

This PR is for Issue #7087

  1. record replacer "http.request.size" as well as "http.response.size"
  2. let metrics record accurate reqeust size by read from req.Body. ( the current metrics read from Content-Length field, which not applicable to web socket or chunked Body)
  3. cache RequestRecorder to handle records in multiple modules. ( logRequest and metrics creates its own ResponseRecorder.)
  4. refactor, put the recording function in a single place. lengthReader as a member of ResponseRecorder. it takes quite a time to understand when they separated and share a common int pointer.

@CLAassistant
Copy link

CLAassistant commented Jun 29, 2025

CLA assistant check
All committers have signed the CLA.

@mohammed90
Copy link
Member

There are 2 things that are unclear to me:
1- can you explain the reasoning for the double pointer on http.Request?
2- I don't understand how could you have used the ` for the imports

@qdongxu
Copy link
Author

qdongxu commented Jun 30, 2025

There are 2 things that are unclear to me:
1- can you explain the reasoning for the double pointer on http.Request?
2- I don't understand how could you have used the ` for the imports

As for the import, it shocks me too🤣. It was mostly generated by the goland IDE. I am sorry I didn't check formattings before checking in.

As for the double pointer, the http.Request.WithContext works in a immutable way, it should returns a new instance when change the context. It looks cohensive to make the change inside the NewResponseRecorder implementation.

    c := context.WithValue(r.Context(), ResponseRecorderVarKey, rr)
    *req = r.WithContext(c)

A alternative way is to let the invoker to take care of the new r . The side effect is not obvious for the invoker(The input r changed??)

    func NewResponseRecorder(w http.ResponseWriter, req *http.Request, buf *bytes.Buffer,
	shouldBuffer ShouldBufferFunc) (wrec ResponseRecorder, newReq *http.Request , cached bool)
    // ....
    wrec, r, cached := NewResponseRecorder(w, r, nil, nil)

It's weird to input a double pointer but it's may get the invoker's attention as I had thought.

What's your opinion on the alternatives ?

@qdongxu qdongxu marked this pull request as draft July 2, 2025 14:24
@qdongxu
Copy link
Author

qdongxu commented Jul 2, 2025

I converted this PR to draft. During review the caching of responseRecorder, I found its better to decouple responseRecorder into responseBuffer and responseMeter. I will do some more investigation.

2. make metrics records accurate reqeust size by read from req.Body. ( the current metrics read from Content-Length field, which not applicable to websocket or chunked Body)
@qdongxu qdongxu marked this pull request as ready for review July 13, 2025 10:02
@qdongxu
Copy link
Author

qdongxu commented Jul 13, 2025

I converted this PR to draft. During review the caching of responseRecorder, I found its better to decouple responseRecorder into responseBuffer and responseMeter. I will do some more investigation.

I have to admit it's complicate than I had expencted 🤣. It's better to make this PR scope smaller and focus on the issue of adding http.request.size only. I will submit a single refactor-purpose PR for the responsewriter splitting.

@qdongxu
Copy link
Author

qdongxu commented Aug 11, 2025

I will submit it for a later time.

@qdongxu qdongxu closed this Aug 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants