Metrics per endpoint #120
Metrics per endpoint #120
Conversation
| 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. |
| @cddr thank you! Having this kind of feedback before a PR is merged is incredibly valuable. |
| (when-let [metric (metric-map k (metric-map :other))] | ||
| (mark! metric))) | ||
| | ||
| (defn- ring-metrics [reg {:keys [prefix] :as options}] |
There was a problem hiding this comment.
Sorry to nitpick but can you please move the argument vector to the next line? We don't do this consistently in this project
but every function that has a doc string or has multiple heads (overrides) does that.
cddr force-pushed the metrics-per-endpoint branch from c7a7ae2 to 462211d Compare | 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). |
| Thank you! |
3 participants
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.