Middleware method RequestID() modified to support X-Request-ID from HTTP header#367
Conversation
|
travis build is failing with an error https://travis-ci.org/go-chi/chi/jobs/460622264#L479 on Go 1.8 only but this seems unrelated to the changes above |
| // RequestIDFromHeader is a middleware similar to RequestID | ||
| // except it uses the value from request header X-Request-Id | ||
| // if one is available. Else revert to default behavior. | ||
| func RequestIDFromHeader(next http.Handler) http.Handler { |
There was a problem hiding this comment.
Can we provide a SetReqID() function, instead of hardcoding the X-Request-Id header name?
There was a problem hiding this comment.
Would be nice to provide and example for #368 as part of this PR.
There was a problem hiding this comment.
I thought about this first. However I thought it was more intrusive change. The RequestID is maintained in a context variable keyed by a non-exported variable. Exporting that variable doesn't sound good either. So it would have a SetReqID() method inside the middleware which also takes the context as an explicit parameter, and set the given string as the request-id. Is that what you are looking for? As for #368 , isn't there already a GetReqID() method available in the middleware?
There was a problem hiding this comment.
You know what.. I think it's not a bad idea to use the X-Request-ID header directly after all. This middleware is opinionated anyway.
Imho, the is the desired lifecycle of "tracing" the request-id across multiple services is as follows:
- browser request
->
- optionally, an external service, creates reqID and passes it as X-Request-ID header
->
- our program reads the header and sets the context value, ie.
r.Use(middleware.RequestID)
// which behind the scenes does this:
reqID := r.Header.Get("X-Request-ID")
if reqID == "" {
reqID = generateReqId()
}
setReqId(ctx, reqID)->
-
middlewares/handlers use the reqID value (from ctx) and pass it to logger or to external HTTP calls as X-Request-ID header
we just provide a getter for this:
reqID := middleware.GetReqId(ctx)
// log.With().String("requestId", reqID) // zerolog structured log example
// req.Header.Set("X-Request-ID", reqID) // HTTP calls to other services that should be aware of this request IDThere was a problem hiding this comment.
Item 4 already exists. Please confirm if that is what you meant.
So the remaining change is to merge RequestIDFromHeader in to RequestID middleware. I wasn't sure if it would break any existing usage.
|
@VojtechVitek please see updated pull request. Thanks. |
VojtechVitek
left a comment
There was a problem hiding this comment.
LGTM
Thanks for your contribution!
Following middleware RequestIDFromHeader uses the value of "X-Request-ID" header. I am looking at ways to make it a bit more generic and make the header value configurable. Wikipedia mentions X-Request-Id as well X-Correlation-ID