feat: prometheus metrics #62
Conversation
| ).orNotFound | ||
| ) | ||
|
|
||
| lazy val router: HttpApp[F] = |
There was a problem hiding this comment.
now this is named router but it's an HttpApp and above is named searchAndOp.Routes and is created using the Router class. It looks a bit strange, but maybe it's just me :). Perhaps this could be app or application or just routes? The last orNotFound could also be added when the HttpApp is really needed (when added to EmberBuilder)…?
There was a problem hiding this comment.
Yes, that's a good point. I'll try to streamline things a bit.
|
|
||
| object Microservice extends IOApp: | ||
|
|
||
| private val port = port"8080" |
There was a problem hiding this comment.
Maybe we can make this configurable as well (port and bind address)? (just a thought, because it's being addressed here anyways :))
| .map(new HttpApplication[F](_).router) | ||
|
|
||
| final class HttpApplication[F[_]: Async](metricsRoutes: HttpRoutes[F]) | ||
| extends Http4sDsl[F]: |
| metrics <- Prometheus.metricsOps[F](registry, prefix = "server") | ||
| yield PrometheusExportService(registry).routes <+> Metrics[F](metrics)(measuredRoutes) | ||
|
|
||
| def makeRoutes: Resource[F, HttpRoutes[F]] = |
There was a problem hiding this comment.
Not sure if I understand these overloaded methods. What would be the default to measure when no measuredRoutes are supplied? Or are they for different things?
There was a problem hiding this comment.
So the reason why we need MetricsRoutes in two flavours is:
- in the case of
search-apiservice we collect metrics of the HTTP API (latencies, number of calls, etc) so we pass themeasuredRoutes(business APIs) for which we'd like to gather the metrics. - in the case of
search-provisionwe don't want any HTTP metrics to be collected but still want to create routes that are exposed by the microservice.
Would that make sense?
There was a problem hiding this comment.
Ah I see, thank you! Sorry, I somehow couldn't make the connection it refers to the two services
| import io.prometheus.client.{CollectorRegistry, Collector as JCollector} | ||
| import org.http4s.metrics.prometheus.PrometheusExportService | ||
|
|
||
| final class CollectorRegistryBuilder[F[_]: Sync]: |
There was a problem hiding this comment.
Since this is mutable and we usually don't have this, should we maybe make it very clear by putting Mutable in the name? Like it is sometimes done in scala standard api (e.g. ListBuffer or something similar so you see immediately it's mutable), for me not looking at the source and having these with… methods, I would assume an immutable thing :). Or could be made immutable?
There was a problem hiding this comment.
Right, I tried to make it immutable at first but ended up with code I wasn't happy with. Now, I tried again now and I think it's a bit better.
| val script = | ||
| """local xrange = redis.call('XRANGE', KEYS[1], ARGV[1], ARGV[2]) | ||
| |return #xrange - 1""".stripMargin | ||
| createStringCommands.use( |
This PR:
GET /metricsendpoint exposing Prometheus metrics to both servicessearch-apiservice)search-provisionservice)