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

Expose Request.matchuri() #959

Closed
codefromthecrypt opened this issue Dec 21, 2017 · 4 comments
Closed

Expose Request.matchuri() #959

codefromthecrypt opened this issue Dec 21, 2017 · 4 comments

Comments

@codefromthecrypt
Copy link

The templated route uri looks like a decent choice for a low-cardinality grouper for metrics and traces. Seems like Request.matchuri() could be exposed (albeit nullable) to suit the job. This could be used in tools like zipkin and micrometer https://github.com/micrometer-metrics/micrometer/blob/master/micrometer-spring-legacy/src/main/java/io/micrometer/spring/web/servlet/WebMvcTags.java#L81

@codefromthecrypt
Copy link
Author

FYI I will go with a reflection approach for now as it seems not great to have spark based web requests look not as good as others in tracing

@codefromthecrypt
Copy link
Author

Actually I'm not clever enough to figure out how to do this without bytecode.

I've considered making my own wrapper for Request, but that would be fragile to versions (unless it is expected to have no new methods added). A reflective proxy could also work, but I don't know if I can ever control the type of request (as filters in spark are callbacks, not interceptors). The failed idea here is hooking into Request.changeMatch and stashing matchuri temporarily as a request attribute.

Only other thing could be to hook into Access.changeMatch, which for bytecode is a cleaner way to accomplish the same.

Please ping back if anyone knows any other options, or if someone believes that exposing this is likely to happen (even if that answer is pull requests welcome)

@tipsy
Copy link
Contributor

tipsy commented Mar 12, 2018

I don't see why this shouldn't be included, I'll merge a PR if you create one.

@tipsy
Copy link
Contributor

tipsy commented Mar 21, 2019

This has been merged now!

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

No branches or pull requests

2 participants