search

clj-commons / metrics-clojure

A thin façade around Coda Hale's metrics library.
http://metrics-clojure.rtfd.org/
MIT License
343 stars 82 forks source link

Metrics per endpoint #120

Closed cddr closed 7 years ago

cddr commented 7 years ago

Closes #119

Update:

Added tests that actually check the expected metrics have been registered and extracted the bit that adds them into it's own function since cider was complaining that the method was too big to debug. I could put that back now that I'm finished debugging but it seems like a reasonable refactor in any case.

cddr commented 7 years ago

Actually I think this needs a bit of a rethink. I tried to integrate it with our own app and it was very inconvenient to have to wrap each individual endpoint. I'm going to make a change that will allow you to just wrap a single handler but still track metrics per endpoint.

michaelklishin commented 7 years ago

@cddr thank you! Having this kind of feedback before a PR is merged is incredibly valuable.

cddr commented 7 years ago

Hi Michael,

Thanks for looking at this so quickly. Apologies for re-opening it before finishing the docs. That's me done now. I put the arglist on it's own line as you wished :-)

I am a little concerned at the risk this introduces for unbounded memory growth (an attacker could hit the app with many unique uris and we'd create the metrics map for each distinct one). However I can't think of an easy way to limit this only to URIs that would actually be served by the app. This isn't a problem for us but could be if you have a public facing website/api.

I suppose this is one of the benefits of the router being expressed as data rather than code (e.g. bidi instead of compojure).

michaelklishin commented 7 years ago

Thank you!