Performance enhancements for the rules matchers. #3563
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ldez
added
kind/enhancement
mmatur
requested changes
Jul 6, 2018
middlewares/reqHost.go
Outdated
|
|
||
| var reqHostKey struct{} | ||
|
|
||
| // ReqHostMiddleware is the struct for the middleware that adds the CanonicalDomain of the requst Host into a context for later use. |
There was a problem hiding this comment.
Could you please replace requst by request
middlewares/reqHost.go
Outdated
| // ReqHostMiddleware is the struct for the middleware that adds the CanonicalDomain of the requst Host into a context for later use. | ||
| type ReqHostMiddleware struct{} | ||
|
|
||
| // NewReqHostMiddleware is a middleware that adds the CanonicalDomain of the requst Host into a context for later use. |
There was a problem hiding this comment.
Could you please replace requst by request
middlewares/reqHost.go
Outdated
| } | ||
| } | ||
|
|
||
| // GetCannonHost plucks the cannonized host key from the requst of a context that was put through the middleware |
There was a problem hiding this comment.
Could you please replace requst by request and cannonized by canonized
middlewares/reqHost.go
Outdated
| } | ||
|
|
||
| // GetCannonHost plucks the cannonized host key from the requst of a context that was put through the middleware | ||
| func GetCannonHost(ctx context.Context) (string, bool) { |
There was a problem hiding this comment.
Should maybe rename into GetCanonHost
middlewares/reqHost.go
Outdated
| var reqHostKey struct{} | ||
|
|
||
| // ReqHostMiddleware is the struct for the middleware that adds the CanonicalDomain of the requst Host into a context for later use. | ||
| type ReqHostMiddleware struct{} |
There was a problem hiding this comment.
Could you rename ReqHostMiddleware by ReqHost please
middlewares/reqHost.go
Outdated
| type ReqHostMiddleware struct{} | ||
|
|
||
| // NewReqHostMiddleware is a middleware that adds the CanonicalDomain of the requst Host into a context for later use. | ||
| func NewReqHostMiddleware() *ReqHostMiddleware { |
There was a problem hiding this comment.
This function is not necessary, you could directly use &ReqHostMiddleware{} instead of creating a function for that. Like it is done for Compress or AddPrefix middlewares
middlewares/reqHost.go
Outdated
| return &ReqHostMiddleware{} | ||
| } | ||
|
|
||
| func (rhm *ReqHostMiddleware) ServeHTTP(rw http.ResponseWriter, r *http.Request, next http.HandlerFunc) { |
There was a problem hiding this comment.
You could maybe rename rhm with rh
middlewares/reqHost.go
Outdated
| } | ||
|
|
||
| func (rhm *ReqHostMiddleware) ServeHTTP(rw http.ResponseWriter, r *http.Request, next http.HandlerFunc) { | ||
| reqHost := r.Host |
There was a problem hiding this comment.
You could maybe simplify like that
func (rh *ReqHostMiddleware) ServeHTTP(rw http.ResponseWriter, r *http.Request, next http.HandlerFunc) {
reqHost := types.CanonicalDomain(parseHost(r.Host))
if next != nil {
next.ServeHTTP(rw, r.WithContext(context.WithValue(r.Context(), reqHostKey, reqHost)))
}
}
func parseHost(addr string) string {
if !strings.Contains(addr, ":") {
return addr
}
host, _, err := net.SplitHostPort(addr)
if err != nil {
return addr
}
return host
}WDYT?
And you should be able to test parseHost function
mmatur
changed the title
|
The code freeze happen today, so I will do the change to be able to merge your PR today before the code freeze |
ldez
force-pushed
the
upsmaster
branch
2 times, most recently
from
2a2c370 to
3b19e92
Compare
ldez
suggested changes
Jul 6, 2018
ldez
force-pushed
the
upsmaster
branch
2 times, most recently
from
833b7de to
c80d5bd
Compare
ldez
approved these changes
Jul 9, 2018
added 2 commits
July 9, 2018 12:50
This commit works towards calling types.CannonicalDomain less and reducing the amount of calls that are in the hotpath for a request. The big change here is moving the cannon'd request Host header into a middleware so it only has be cannonized once per request.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit works towards calling
types.CannonicalDomainless and reducing the amount of calls that are in the hotpath for a request.The big change here is moving the cannon'd request Host header into a middleware so it only has be cannonized once per request.
What does this PR do?
This PR creates a middleware that gets hit early on in the request lifecycle and puts then
CanonicalDomainrequest host into a context to be read during the route Matcher funcs.Along with that this re-works the matcherFuncs to call
CanonicalDomainless by using the context, and callingCanonicalDomainfor the hosts at the initialization of the matcherFunc not inside the matcher func where it's expensive.Motivation
Performance tuning in our production environment showed that
CanonicalDomainwas using a good chunk of CPU time.See the include flameGraph photos of the
matcherFuncsection.Before:
After:
More
Additional Notes