twitter / finatra

Fast, testable, Scala services built on TwitterServer and Finagle
https://twitter.github.io/finatra/
Apache License 2.0
2.27k stars 406 forks source link

No way of using production StatsReceiver with EmbeddedHttpServer #496

Closed ryanb93 closed 5 years ago

ryanb93 commented 5 years ago

If using a custom StatsReceiver in production, is it not possible to use it in tests due to EmbeddedHttpServer overriding it with an InMemoryStatsReceiver.

Given a custom StatsReceiver has been defined, such as using the finagle-metrics library. When the application is started using EmbeddedHttpServer the following code is executed:

    injectableServer.addFrameworkOverrideModules(InMemoryStatsReceiverModule)

This overrides the injector.instance[StatsReceiver] in the application with an InMemoryStatsReceiver.

If you try and override it yourself

 new EmbeddedHttpServer(
          twitterServer = new PpsServer {
            override protected def overrideModules: Seq[Module] =
              Seq(new TwitterModule {
                override def configure(): Unit = {
                  bind[StatsReceiver].toInstance(new MetricsStatsReceiver())
                }
              })
          }
)

The following error is thrown:

1) A binding to com.twitter.finagle.stats.StatsReceiver was already configured at com.hotels.pps.common.acceptance.AcceptanceTestContext$$anon$1$$anon$2.configure(AcceptanceTestContext.scala:77) (via modules: com.google.inject.util.Modules$OverrideModule -> com.hotels.pps.common.acceptance.AcceptanceTestContext$$anon$1$$anon$2).
  at com.twitter.inject.modules.InMemoryStatsReceiverModule$.configure(InMemoryStatsReceiverModule.scala:8) (via modules: com.google.inject.util.Modules$OverrideModule -> com.twitter.inject.modules.InMemoryStatsReceiverModule$)

Our production code is written to use the codahale MetricsRegistry directly, which is then shared with the MetricsStatsReceiver wrapper. When we hit our custom/metrics endpoint in tests, which prints the MetricsRegistry out to JSON, we can see our application metrics however we are missing the Finatra specific metrics, for example route.endpoint.status.200 and all the jvm. metrics.

This only happens in testing but makes it hard to verify production behaviour without starting the application manually and hitting the endpoint (then everything shows as expected).

There should be a way to disable the InMemoryStatsReceiverModule being loaded and to use the one defined in the application.

ryanoneill commented 5 years ago

Talked this over with a teammate, and we're going to look into adding this functionality to the EmbeddedHttpServer.

sinanspd commented 5 years ago

Are there any plans to generalize this behavior for maybe user defined modules? I ran into a few cases where I am running into the same issue with the modules I have created when I try to use them across multiple test files & src

cacoco commented 5 years ago

@sinanspd I'm not sure what issues you are running into as the StatsReceiver is the only instance that was bound unchangeably by the testing framework. Have you looked at the documentation around testing? Specifically the OverrideModules? https://twitter.github.io/finatra/user-guide/testing/override_modules.html