Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PathPrefix and Full Path support rewriteTarget #552

Closed
aniaan opened this issue Mar 29, 2022 · 2 comments · Fixed by #553
Closed

PathPrefix and Full Path support rewriteTarget #552

aniaan opened this issue Mar 29, 2022 · 2 comments · Fixed by #553

Comments

@aniaan
Copy link
Collaborator

aniaan commented Mar 29, 2022

Is your feature request related to a problem? Please describe.

Currently only "pathRegexp" supports rewriteTarget, in some scenarios, it would be better for path and pathPrefix to support rewriteTarget

https://github.com/megaease/easegress/blob/0cd6b15620601de3021abbca1e310120a566795d/pkg/object/httpserver/mux.go#L508-L513

Describe the solution you'd like

Due to some historical reasons, the API design is not standardized. At present, when accessing easegress, it is necessary to develop a new standard API interface, rewrite the old API into a new API, and then forward it through proxy.

During use, it was found that only pathRegexp supports rewriteTarget. In most of our scenarios, using prefix can basically solve most problems, a small amount can be solved using full path, and pathRegexp is basically not used, although prefix can solve the problem, Regexp can also be solved, but in terms of program execution efficiency, prefix performance will be higher, so I think, we need to support the path rewriting of prefix and full path

eg:

// old api: /tianji/exchange/activity/claim/participate
// new api: /api/activity/ex/clain/participate
// old api rewrite target to new api
// use pathRegexp or prefix

package main

import (
	"fmt"
	"regexp"
	"strings"
	"time"
)

func main() {
	n := 100000
	pt := prefix(n)
	rt := regex(n)

	fmt.Println("prefixPath: ", pt)
	fmt.Println("regexPath: ", rt)

}

func prefix(n int) time.Duration {
	repl := "/api/activity/ex/"
	prefix := "/tianji/exchange/activity/"
	path := "/tianji/exchange/activity/claim/participate"
	now := time.Now()

	for i := 0; i < n; i++ {
		path = strings.Replace(path, prefix, repl, 1)
	}

	return time.Since(now)

}

func regex(n int) time.Duration {
	repl := "/api/activity/ex/$1"
	pattern := "^/tianji/exchange/activity/(.*)$"
	re, _ := regexp.Compile(pattern)

	path := "/tianji/exchange/activity/claim/participate"
	now := time.Now()

	for i := 0; i < n; i++ {
		path = re.ReplaceAllString(path, repl)
	}

	return time.Since(now)
}

Additional context

If you agree with the change, I will follow up with the corresponding PR


Thanks for contributing 🎉!

@aniaan aniaan changed the title PathPrefix and Full Path support rewriteTarget **PathPrefix** and **Full Path** support rewriteTarget Mar 29, 2022
@aniaan aniaan changed the title **PathPrefix** and **Full Path** support rewriteTarget PathPrefix and Full Path support rewriteTarget Mar 29, 2022
@samutamm
Copy link
Contributor

Thanks for opening the issue!

When I run this on my workstation, I get following output

prefixPath:  1.969323ms
regexPath:  18.592825ms

so running re.ReplaceAllString 100000 times takes about 10x more time than same operation using strings.Replace. This seems like a significant improvement, but often there is also delay of the backend application where Easegress routes the requests from HTTPServer. For example, if the backend application has delay of one ms, wouldn't the total cpu time for 100000 request be like 100000ms + Easegress execution time? In that case, if we save 17ms from that over 100000ms of cpu time, it's not so significant improvement any more.

But of course, every performance improvement is a positive thing. If this change don't add extra complexity, maybe it's worth it.

@aniaan
Copy link
Collaborator Author

aniaan commented Mar 29, 2022

Another important point is that the prefix is relatively simple and easy to understand, while the regex is relatively cumbersome to write.

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 a pull request may close this issue.

2 participants