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

Add option to thread matched route key on request context #286

Conversation

GeorgeMac
Copy link
Contributor

@GeorgeMac GeorgeMac commented Nov 21, 2019

👋 first off, thank you for such a great router!

I have a usecase where it would be really great (in terms of visibility) when handling a request to retrieve which router match occurred and called my handler. In my instance I want to create a middleware which can automatically enrich traces with this router path.

This is my little attempt to surface that information. There is probably a cleaner way than my approach.

Also here are the benchmarks:

goos: darwin
goarch: amd64
pkg: github.com/julienschmidt/httprouter
BenchmarkAllowed/Global-16         	300000000	         5.56 ns/op	       0 B/op	       0 allocs/op
BenchmarkAllowed/Path-16           	10000000	       137 ns/op	      32 B/op	       1 allocs/op
PASS
ok  	github.com/julienschmidt/httprouter	3.752s

OK I just saw #139 so perhaps this change is a little undesireable or just unlikely to go through. But I am going to try anyway.
This at least offers an interface which is backwards compatible with the existing one.

tree.go Outdated Show resolved Hide resolved
@julienschmidt
Copy link
Owner

julienschmidt commented Jan 6, 2020

Thank you for this PR. I understand the need for it and we'll somehow find a way to integrate this without a big performance penalty / memory overhead for the common case.

As a first thought: The full path only needs to be stored in nodes that have a handler.
I see 3 general possibilities:

  1. Store it in an additional map[*node]string, where the node address maps to the fullPath
    Edit: One can actually just put them into a var fullPaths []string slice and save just the index in the node, i.e. give all the nodes which have a handler a unique and monotonically increasing ID, which should be small enough to use some bytes which are currently just padding anyway.
  2. Somehow start packing the additional data for such nodes into a referenced, not directly embedded struct (currently only containing the Handler and the fullPath then)
  3. Somehow pack it into one of the existing strings or store the full path + an offset instead of just the node's path for these nodes

I currently don't have much time for it, but I hope to get back to this soon. Feel free to do some further experiments in the mean time.

@julienschmidt
Copy link
Owner

julienschmidt commented Jan 6, 2020

The minimum constraints for adding this feature are from my point of view:

  1. No breaking changes. Thus, I like this approach better than Flexible Handle Type #39. It also avoids having to do two lookups in the tree.
  2. Keep the memory overhead minimal.
  3. (nearly) no measurable impact on the performance of the general case, where the matched route is not of interest.

CC @jehiah

var MatchedRouteKey = matchKey{}

// MatchedRouteFromContext retrieves the matched route path from the context.
func MatchedRouteFromContext(ctx context.Context) string {
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure if it's even consistent in the existing code, but generally a route should be a tuple (Method, Path) in the context of this router. This is just the matched path.

@julienschmidt
Copy link
Owner

julienschmidt commented Jan 12, 2020

I just had an idea... take a look at the matched_path branch, which is based on this PR @GeorgeMac @jehiah.

Overhead for the common case: 0
Overhead if the option is enabled: 1 closure call + the modification of the context

Doesn't that already fulfill what you're looking for?

@julienschmidt
Copy link
Owner

julienschmidt commented Jan 12, 2020

Modifying the context of a http.Request seems to be in general quite expensive:
image

We're talking about +330 ns/op, +448 B/op, +4 allocs/op on a 4 years old laptop CPU (Intel Core i5-6267U), but still, one could avoid all that, by just passing the path directly to a different handler type with an additional function parameter.

The alternative could be to pass it as a special value in Params.

@julienschmidt
Copy link
Owner

Here is a quick-and-dirty implementation of the alternative using Params: matched_path_ps.

It indeed brings to cost down to almost 0, except for matched routes that don't contain params, where Params have to be retrieved from the pool / be allocated first.

@jehiah
Copy link

jehiah commented Jan 13, 2020

@julienschmidt adding the value into Params is clever, that implementation looks good to me.

@julienschmidt
Copy link
Owner

The params approach is now in master. Please test and provide feedback.

@GeorgeMac
Copy link
Contributor Author

Thank you @julienschmidt and @jehiah ! This great news. Sorry for the slow reply.

@GeorgeMac GeorgeMac deleted the gm/thread-path-match-on-context branch January 14, 2020 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants