Skip to content

Middleware method RequestID() modified to support X-Request-ID from HTTP header#367

Merged
pkieltyka merged 2 commits into
go-chi:masterfrom
harikb:master
Dec 10, 2018
Merged

Middleware method RequestID() modified to support X-Request-ID from HTTP header#367
pkieltyka merged 2 commits into
go-chi:masterfrom
harikb:master

Conversation

@harikb

@harikb harikb commented Nov 28, 2018

Copy link
Copy Markdown
Contributor

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

@harikb

harikb commented Nov 28, 2018

Copy link
Copy Markdown
Contributor Author

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

@VojtechVitek VojtechVitek left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feedback submitted. Thanks for your contribution

Comment thread middleware/request_id.go Outdated
// 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 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we provide a SetReqID() function, instead of hardcoding the X-Request-Id header name?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to provide and example for #368 as part of this PR.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. browser request

->

  1. optionally, an external service, creates reqID and passes it as X-Request-ID header

->

  1. 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)

->

  1. 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 ID

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@harikb

harikb commented Dec 10, 2018

Copy link
Copy Markdown
Contributor Author

@VojtechVitek please see updated pull request. Thanks.

@harikb harikb changed the title New middleware method RequestIDFromHeader Middleware method RequestID() modified to support X-Request-ID from HTTP header Dec 10, 2018

@VojtechVitek VojtechVitek left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Thanks for your contribution!

@pkieltyka pkieltyka merged commit def7567 into go-chi:master Dec 10, 2018
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.

3 participants