-
Notifications
You must be signed in to change notification settings - Fork 327
Conversation
Several reorganization related and readability improvements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, thanks @rakyll. LGTM with some nits. Thank you!
if !h.NoTrace { | ||
var end func() | ||
r, end = h.startTrace(w, r) | ||
defer end() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aha, I like this, I read the previous code on my way back home and had the urge for this kind of change, so thank you!
} | ||
|
||
span.SetAttributes(requestAttrs(r)...) | ||
return r.WithContext(trace.WithSpan(r.Context(), span)), span.End |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks for changing it from func() { span.End }
to span.End
!
writer http.ResponseWriter | ||
} | ||
|
||
var _ http.ResponseWriter = (*trackingResponseWriter)(nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, thanks for this cleanup.
start time.Time | ||
statusCode int | ||
endOnce sync.Once | ||
writer http.ResponseWriter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
writer seems quite ambigious for the field of http.ResponseWriter.
How about either of?
- rw
- responseWriter
Several reorganization related and readability improvements.