Prometheus Finatra Compatibility

sinanspd commented 5 years ago

It would be nice to have compatibility with Twitter Finatra https://twitter.github.io/finatra/

The core library seems to play well, however test suite that comes with Finatra causes multiple issues.

Notably, Finatra allows multiple feature/integrations tests that mock the application through EmbeddedHttpServer. For example:

override val server = new EmbeddedHttpServer(new Main { override val overrideModules: Seq[TwitterModule] = Seq(new FlagTestProviderModule("occurrencetests")) override def warmup(): Unit = { /* no-op */ } })

these mock servers run in parallel by default. I believe prometheus attempts to collect the metrics for these mock servers in a single thread causing the following issue:

[info]   java.lang.IllegalArgumentException: Collector already registered that provides name: finagle_timer_deviation_ms_count
[info]   at io.prometheus.client.CollectorRegistry.register(CollectorRegistry.java:54)
[info]   at io.prometheus.client.Collector.register(Collector.java:139)
[info]   at io.prometheus.client.Collector.register(Collector.java:132)
[info]   at com.github.kovszilard.twitter.server.prometheus.PrometheusExporter.$init$(PrometheusExporter.scala:11)
[info]   at com.armoredthings.gregor.Main.<init>(Main.scala:19)

mgd43b commented 4 years ago

Specifically this appears when running multiple embedded (http) servers in tests, despite not sharing test fixtures - see https://twitter.github.io/finatra/user-guide/testing/feature_tests.html#sharing-a-server-fixture-between-many-feature-tests

sinanspd commented 4 years ago

I am gonna start working on a fix for this. Although not directly the source of the problem, I think there are a number of bad practices the way things are implemented here. We should link the entire thing to twitter-server lifecycle. The fact that this code executes before the server properly gets up is concerning. Something like this would suffice.


    premain{
        PrometheusMetricsCollector().register()
        val metricsRoute: Route = Route.isolate(Route(
           path = "/metrics",
           handler = new PrometheusMetricsExporterService(),
           alias = "Prometheus Metrics",
           group = Some(Grouping.Metrics),
           includeInIndex = true
      ))
      addAdminRoute(metricsRoute)
   }

We should be using LoadedStatsReceiver instead of the DefaultRegistry, than we can skip or perform a custom action on duplicate keys

kovszilard / twitter-server-prometheus

Prometheus Finatra Compatibility #7