Skip to content

fix for gh-1331 MetricFilter now supports uri template variables when av... #1333

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

Conversation

twicksell
Copy link

Proposed fix for #1331

@dsyer
Copy link
Member

dsyer commented Aug 4, 2014

Looks pretty neat. Is there a nicer looking replacement for brackets than hyphen (replaceAll("[{}]", "-"))?

@twicksell
Copy link
Author

Not sure, I just avoided the brackets since the default response was in json and things start looking a little ugly with them. Open to suggestions, or I can just remove that bit entirely and trust that things will be escaped appropriately.

@dsyer
Copy link
Member

dsyer commented Aug 4, 2014

I definitely agree that escaping JSON delimiters is a good thing. Maybe just replacing them with nothing is best (/{foo}/{bar} goes to .foo.bar after the existing rules are applied)?

@twicksell
Copy link
Author

Its pretty useful to be able to distinguish between known path variables and people just passing in strange URLs that are similarly named, but I'm not married to it either way.

Thinking of that I just realized a further issue that I'm not solving. Any 404's will still be logged as their original URL. Meaning people can bring down a boot app by spamming obviously garbage URL's which are sure to return 404. Need to consider a how to handle that eventuality.

@dsyer
Copy link
Member

dsyer commented Aug 5, 2014

Good point. I'll have a think. Can you fill in the contributor's agreement if you didn't already?

@twicksell
Copy link
Author

Added some support so that all 404's which aren't intentionally coming from a Controller will be logged under a single metric "unknownPath". Not sure if its the best solution, but it seems reasonable compared to the current alternative. Perhaps we can make this property driven if we're really concerned about changing functionality that has been out for a while?

Also, I've signed the contributor's agreement ages ago. Attempted to sign again just to be safe, but the site linked from the docs appears to be unresponsive.

@dsyer dsyer closed this in 733b22f Aug 5, 2014
wilkinsona added a commit that referenced this pull request Aug 6, 2014
The request is being made to '/' and, while the application does have
a mapping for '/', that mapping is not looked for before Spring
Security's filter rejects the request with a 401. This means that the
request is considered to be unmapped and this is reflected in the
metric's name.

See #1331 and #1333
@philwebb philwebb added this to the 1.1.5 milestone Aug 13, 2014
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.

3 participants