http4s / http4s-prometheus-metrics

Support for Prometheus Metrics
https://http4s.github.io/http4s-prometheus-metrics/
10 stars 10 forks source link

Documentation improvement on composable HttpRoutes with metrics #113

Open vjroby opened 1 year ago

vjroby commented 1 year ago

Hi team,

I discovered a weird behaviour if metrics are enabled on an HttpRoutes value that is composed with some other value.

For example let's say there are apiRoutes and adminRoutes defined and compose like this:

val metrics: MetricsOps[F] = ???
val apiRoutes: HttpRoutes[F] = ???
val adminRoutes: HttpRoutes[F] = ???

val apiRoutesWithMetrics = Metrics[IO](metrics)(apiRoutes)

httpApp = (
        apiRoutesWithMetrics <+> adminRoutes.  //  <+> SemigroupK Cats
      ).mapF(_.getOrElseF(NotFound()))

return httpApp // <- this is used to create the http4s webserver

Whenever there is a request meant for the adminRoutes the metrics defined for apiRoutesWithMetrics gets incremented for 4xx requests although the request is send back to the client with 200 OK status.

If the routes are composed the other way around adminRoutes <+> apiRoutesWithMetrics this behaviour does not happen.

I'm guessing is because the request goes through the first routes and exists with NotFound (400) and the metrics are incremented and then the request go to the admin routes and there is a route defined but the metrics are already incremented.

I'm not saying it's a bug in the library but maybe there can be added to the documentation. I'm happy to add it.

What is your opinion on this?

Regards, Robert

hamnis commented 1 year ago

your guess is correct.

You could try to use the Router and mount the routes separately there.

Example:

Router(
  "/admin" -> adminRoutes,
  "/" -> apiRoutesWithMetrics
)

that way you will not have that behaviour.

vjroby commented 1 year ago

Thanks for replying.

If I'm going that way I will have incremented metrics also for /admin routes, right? I'm asking because this is something I want to avoid.

hamnis commented 1 year ago

No that does not mean that.

Since you have only installed the metrics middleware on the api routes the /admin routes is not part of the routing.

I would strongly suggest using something like

Router(
  "/admin" -> adminRoutes,
  "/api" -> apiRoutesWithMetrics
)

to make this destinction even more clear.

This means however, a route defined in /api would start matching on Root, since /api is now the new Root node.

For more information see the Router documentation.