finagle / finch

Scala combinator library for building Finagle HTTP services
https://finagle.github.io/finch
Apache License 2.0
1.6k stars 221 forks source link

Metrics for endpoints #781

Closed sergeykolbasov closed 6 years ago

sergeykolbasov commented 7 years ago

Hey hey!

Could we use finagle-stats for reporting metrics of endpoints out-of-the-box?

WHY To get http stats of individual endpoints for monitoring how performant they are

HOW Use StatsReceiver provided by finagle-stats, since this project is dependant on finagle already

Of course, this could be achieved manually, adding some boilerplate, but I wonder if it's possible to make some endpoint that could write metrics for underlying endpoints

spockz commented 7 years ago

I did something for this in our application. We add a metrics filter in finagle that puts a MetricContext case class on a local Context. Each endpoint definition is wrapped in a withEndpointMetrics method that used the toString to get the endpoint definition/path and put it in the metriccontext class. On the return in the metrics filter we used the metric context to lookup the name and used it for the metrics in the stats receiver.

Would you be interested if we made a PR out of this?

On 30 May 2017, at 10:46, Sergey Kolbasov notifications@github.com wrote:

Hey hey!

Could we use finagle-stats for reporting metrics of endpoints out-of-the-box?

WHY To get http stats of individual endpoints for monitoring how performant they are

HOW Use StatsReceiver provided by finagle-stats, since this project is dependant on finagle already

Of course, this could be achieved manually, adding some boilerplate, but I wonder if it's possible to make some endpoint that could write metrics for underlying endpoints

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

sergeykolbasov commented 7 years ago

Well, I believe Endpoint.transform is a more natural way to measure things inside endpoints, but it's impossible to get response size with this method because I have to operate with Output[A], not a Response

BenWhitehead commented 7 years ago

I created a filter to address this exact issue, and automatically add stats for any request that was handled by the finagle server in my finch-server project.

The finagle filter can be seen here: StatsFilter and can be woven into the server similar to the following:

Http.server.serve(":8080", StatsFilter andThen api.toServiceAs[Text.Plain])

I can open a PR to add this stats filter into finch itself if it is something people would find useful.

rpless commented 7 years ago

One challenge I see with adding metrics at the endpoint level is that it is kinda difficult to choose a place in Endpoint or a single piece of boilerplate that makes sense given all of the different ways people use Finch (whether that's using finchrich, a controller style abstraction, or just sticking with functions). I don't know that there is a one size fits all solution in Endpoint or as boilerplate around endpoint. There are a number of ways I can thing of to implement metrics, although a Finagle Filter currently makes the most sense to me (I use a filter based on the one @BenWhitehead linked to).

I'm also not sure I like the idea of bringing finagle-stats in as a dependency to the core project. Finch in general has stayed away from telling users how to organize their endpoints and I feel like we want to stick with a similar idea when it comes to metrics. Finagle Stats isn't the only metric library / framework out there and I don't think core should pull finagle-stats as a dependency if some users are not going to use it (whether that's because they don't have a metric infrastructure yet, or already use a different library/infrastructure for metrics, or even another I reason that I'm unaware of).

I'm all for PRs to the cookbook or linking out to any blog posts / projects that folks may write, but I think that the choice of how to metric kinda belongs on a slightly higher level then Finch's core project. I'm curious to hear more thoughts on the matter and see if there are other ways that people have come up with for metrics.

vkostyukov commented 7 years ago

StatsReceiver is defined in util-stats, which finagle-core depends on so we don't need any new dependency for this. Although I agree with @rpless I'm not yet sold this has to be part of finch-core.

I'd be happy to link the gist implementation from cookbook though.

vkostyukov commented 6 years ago

Implemented in #960 and #957.