Add correlation id middleware #82
Conversation
|
We have created an issue in Pivotal Tracker to manage this: https://www.pivotaltracker.com/story/show/168536968 The labels on this github issue will be updated when the story is started. |
|
Hello @ataleksandrov! Thanks for the PR. The first thing we want to clarify is that the brokerapi project aims to be OSBAPI compliant, and we think the request header property |
cf-gitbot
added
scheduled
in progress
and removed
unscheduled
scheduled
in progress
labels
|
Hello @winnab! I went with correlation id because I need an id that is unique to a transaction, which can span multiple requests ( e.g. async provision ). The way I understand the |
|
Great. Thanks @ataleksandrov! Our team will review and get back to you as soon as we can. |
|
|
||
| const CorrelationIDKey = "correlation-id" | ||
|
|
||
| var correlationIDHeaders = []string{"X-Correlation-ID", "X-CorrelationID", "X-ForRequest-ID", "X-Request-ID", "X-Vcap-Request-Id"} |
There was a problem hiding this comment.
Is it necessary to verify all of these headers? If so, could you add tests for the missing ones (a table test or a for loop would do it)?
Do we care if more than one is passed?
There was a problem hiding this comment.
Yes, you are absolutely right, there should be tests for each header. About the other question I don't think there should ever be more than one header, that carries such meaning for a request. But if somehow there is more than one header this indicates an integration problem between system components. We check more than one header because different systems can set the correlation id in different headers and these are the ones that are commonly used. The first one we find will be read, the rest will be ignored.
middlewares/correlation_id_header.go
Outdated
| } | ||
|
|
||
| w.Header().Set(headerName, correlationID) |
There was a problem hiding this comment.
The correlation id header should not be set to the response.
Gopkg.lock
Outdated
| @@ -22,6 +22,14 @@ | |||
| revision = "ab2d9a74b97eda058004c8deef80ac624432f408" | |||
| version = "v1.0.0" | |||
|
|
|||
| [[projects]] | |||
| digest = "1:141cc9fc6279592458b304038bd16a05ef477d125c6dad281216345a11746fd7" | |||
| name = "github.com/gofrs/uuid" | |||
There was a problem hiding this comment.
Hi @ataleksandrov! Sorry, we just noticed you added a new UUID generation dependency. We are already using https://github.com/pborman/uuid in this project. Do you have a strong objection to using the package we already have? If not, could you update your code to use the pborman dependency? If you do want to use this new package, could you swap out the pborman package where we use it for this one to minimise dependencies and make sure to update the Gopkg.toml?
|
Hi @ataleksandrov! We have now merged this PR :) Thanks |
Fixes #81.
Added:
Pull request: