caddyhttp: Log request body bytes read #5461
Conversation
francislavoie
force-pushed
the
req-body-size
branch
from
c8c23e6 to
fd4df96
Compare
There was a problem hiding this comment.
Thanks for tackling this -- I can see why it'd be useful to have this info.
Because of that, I'm not sure what the best naming is. req_size could make sense because we already have size for the response size, but that's weirdly incongruent with resp_header using a prefix for the other half. bytes_read is more accurate in the sense that if the body isn't read then it would report 0 even if there was a body in the request, but it doesn't have req_; should it, or would that be too long?
Ugh, you're right, that's kind of a bother. I like bytes_read you have already, for the preciseness. If I had planned ahead better I would have designed the field names to be more congruent/symmetric. Oh well :(
| @@ -248,6 +249,14 @@ func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) { | |||
| wrec := NewResponseRecorder(w, nil, nil) | |||
| w = wrec | |||
|
|
|||
| // wrap the request body in a LengthReader | |||
| // so we can track the number of bytes read from it | |||
| var bodyReader *lengthReader | |||
There was a problem hiding this comment.
Might be a long shot, but does this have to be a pointer? Just wondering if we can avoid an allocation.
There was a problem hiding this comment.
I don't think so because we need to use a pointer receiver for Read
cannot use bodyReader (variable of type lengthReader) as io.ReadCloser value in assignment: lengthReader does not implement io.ReadCloser (method Close has pointer receiver) compiler(InvalidIfaceAssign)
There was a problem hiding this comment.
Well maybe if I make Read and Close not use a pointer receiver, but does that even work?
Edit: No it wouldn't because Read unfortunately needs to write the Length back to the struct
There was a problem hiding this comment.
Ah yeah, that was my guess/concern. Makes sense. Oh well!
There was a problem hiding this comment.
You were kinda right though - we should probably use sync.Pool for lengthReader. Opened #5756
francislavoie
force-pushed
the
req-body-size
branch
from
fd4df96 to
da12ba2
Compare
Closes #4494
I'm not super satisfied with this unfortunately... the problem is we can't really add this to the
requestlog object, because it gets marshaled when we call.With(loggableReq)and doesn't wait until thedeferso it logs the size as 0 because it hasn't been read yet. So we need to put it in the top-level of the log. Later on, maybe something like.LazyWith()might get added uber-go/zap#1121 but for now I don't see a way to prevent it.Because of that, I'm not sure what the best naming is.
req_sizecould make sense because we already havesizefor the response size, but that's weirdly incongruent withresp_headerusing a prefix for the other half.bytes_readis more accurate in the sense that if the body isn't read then it would report 0 even if there was a body in the request, but it doesn't havereq_; should it, or would that be too long?