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

Record matched pattern in context as web.C.Pattern. #70

Closed
wants to merge 1 commit into from
Closed

Record matched pattern in context as web.C.Pattern. #70

wants to merge 1 commit into from

Conversation

mredivo
Copy link
Contributor

@mredivo mredivo commented Oct 6, 2014

This makes the matched StringPattern available to method handlers and to loggers.

This makes the matched StringPattern available to method handlers and to loggers.
@mredivo
Copy link
Contributor Author

mredivo commented Oct 7, 2014

Some explanation of the use case might help here.

A REST service will have many requests that contain resource IDs:

/a/$RESOURCE_ID/b/$SUB_ID

To generate stats by endpoint, unless the above is logged with placeholders instead of actual IDs, there will be a combinatorial explosion of paths that defeats analysis. For a logger to fold those IDs to a placeholder value requires more knowledge about the app than a logger should have.

It turns out that the pattern matcher already has exactly what we're looking for: the pattern that was parsed to extract the values. By saving that pattern in the environment, it becomes trivial to log the value that we really want:

/a/:resid/b:subid

I added this feature to the string pattern matcher, because I'm using that one. At this point I don't know how this consideration applies to regexp matches, so I left that one alone. The disruptive part (adding "Pattern" to web.C) will have already been done when someone needs it for regexps.

There may be other cases where the handler itself is interested in the pattern, but I'm not far enough into my app to see a use case yet.

Does that all sound reasonable?

@zenazn
Copy link
Owner

zenazn commented Oct 7, 2014

Yeah, that sounds reasonable. I think modifying web.C to support this is a non-starter though. Plus, it's not clear you even want this, since routing happens after all middleware, including the logging middleware.

I thought of a clever solution to this after some followup comments on #32: if you insert a special Router middleware into the middleware stack, routing will be performed at that time, and a reified version of the selected route will be placed into c.Env. When the middleware stack is done, that route will be dispatched to. I haven't had the time to actually implement this behavior, but when I do, stashing the matched web.Pattern into c.Env alongside the route sounds like a very reasonable thing to do.

@mredivo
Copy link
Contributor Author

mredivo commented Oct 7, 2014

It's true that the middleware gets processed before routing, but after the chaining call, the pattern matching has been done. For example:

https://github.com/zenazn/goji/blob/master/web/middleware/logger.go#L28

The modified web.C is available after the call to h.ServeHTTP(lw, r), and that's where I'm logging it.

I would stick the route into web.C.Env instead of the added Pattern member, but that still involves forking pattern.go to get it there. Do you have a suggestion for how to get access to s.raw or its equivalent in some other way?

@mredivo
Copy link
Contributor Author

mredivo commented Oct 7, 2014

I found a workaround. I have the following code in my implementation of
https://github.com/zenazn/goji/blob/master/web/middleware/logger.go#L28 :

h.ServeHTTP(lw, r)

// Replace the resource IDs in the path with their variable names.
path := r.URL.Path
for k, v := range c.URLParams {
    path = strings.Replace(path, v, ":"+k, 1)
}

This isn't as reliable as getting the value from s.raw in pattern.go, but should cover the majority of cases.

@mredivo mredivo closed this Oct 7, 2014
@zenazn zenazn mentioned this pull request Nov 2, 2014
@zenazn
Copy link
Owner

zenazn commented Apr 12, 2015

This was fixed in 707cf7e

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.

2 participants