Add time to write header handler middleware #304
Conversation
stuartnelson3
force-pushed
the
stn/time-to-write-header-middleware
branch
from
ac5eb32 to
91ef55a
Compare
|
On my review queue… |
| // judiciously. | ||
| // | ||
| // If the wrapped Handler does not set a status code via WriteHeader, no value | ||
| // is reported. |
There was a problem hiding this comment.
See https://golang.org/pkg/net/http/#ResponseWriter . I understand this as WriteHeader being called eventually in any case, so we should be fine and don't need this note.
|
|
||
| return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| now := time.Now() | ||
| d := &wroteHeaderDelegator{ |
There was a problem hiding this comment.
This breaks interface upgrades (as the type of the embedded delegator is just the plain delegator interface, and the check for interface upgrades only type-switches on the outer type, not the type of the embedded struct element, see https://play.golang.org/p/uuqlJpUG7G as proof).
I suggest to chenge the general responseWriterDelegator implementation to include an observeWriteHeader function, which we check for non-nil in the WriteHeader method.
There was a problem hiding this comment.
K, I updated it, not sure if this was what you imagined. Since newDelegator returns an interface type, I just went one more layer deep on what gets sent into that function so I could attach the observeWriteHeader method.
| @@ -132,12 +129,12 @@ func InstrumentHandlerTimeToWriteHeader(obs prometheus.ObserverVec, next http.Ha | |||
|
|
|||
| return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |||
| now := time.Now() | |||
| d := &wroteHeaderDelegator{ | |||
| delegator: newDelegator(w), | |||
| d := newDelegator(&responseWriterDelegator{ | |||
There was a problem hiding this comment.
This will again kill the interface upgrade. I think the way to go is to change newDelegator to also accept the observeWriteHeader func (which can be nil) and set it in the created responseWriterDelegator. It doesn't look beautiful, but at least newDelegator is not exported.
There was a problem hiding this comment.
Roger, pushed.
Constructor now accepts an additional parameter that sets the function to call, if not nil, when writing the header.
|
This should work. Thanks. |
@beorn7 first stab at writing the "time to writing header" middleware.
Things to note are: