Disable timeouts on healthcheck calls

Currently, healthchecks reside behind the same timeout settings as any other endpoint. We observed that when autoscaling under massive load, it is possible for collector to be taken down because of long health check responses. Which previously did not happen. We therefore move healthchecks above the timeout middleware to return to previous behavior. Additionally, this allows us to set arbitrarily short (or long) response times for the regular endpoints when necessary. --- Part of [PDP-1408].
snowplow · Aug 13, 2024 · 6a68947 · 6a68947
1 parent 55f020c
commit 6a68947
Show file tree

Hide file tree

Showing 4 changed files with 41 additions and 34 deletions.
diff --git a/core/src/main/scala/com.snowplowanalytics.snowplow.collector.core/HttpServer.scala b/core/src/main/scala/com.snowplowanalytics.snowplow.collector.core/HttpServer.scala
@@ -33,6 +33,7 @@ object HttpServer {
 
   def build[F[_]: Async](
     routes: HttpRoutes[F],
+    healthRoutes: HttpRoutes[F],
     port: Int,
     secure: Boolean,
     hsts: Config.HSTS,
@@ -42,7 +43,7 @@ object HttpServer {
   ): Resource[F, Server] =
     for {
       withMetricsMiddleware <- createMetricsMiddleware(routes, metricsConfig)
-      server                <- buildBlazeServer[F](withMetricsMiddleware, port, secure, hsts, networking, debugHttp)
+      server                <- buildBlazeServer[F](withMetricsMiddleware, healthRoutes, port, secure, hsts, networking, debugHttp)
     } yield server
 
   private def createMetricsMiddleware[F[_]: Async](
@@ -64,26 +65,27 @@ object HttpServer {
     StatsDMetricFactoryConfig(Some(metricsConfig.statsd.prefix), server, defaultTags = tags)
   }
 
-  private[core] def hstsMiddleware[F[_]: Async](hsts: Config.HSTS, routes: HttpApp[F]): HttpApp[F] =
+  private[core] def hstsApp[F[_]: Async](hsts: Config.HSTS, routes: HttpRoutes[F]): HttpApp[F] =
     if (hsts.enable)
-      HSTS(routes, `Strict-Transport-Security`.unsafeFromDuration(hsts.maxAge))
-    else routes
+      HSTS(routes.orNotFound, `Strict-Transport-Security`.unsafeFromDuration(hsts.maxAge))
+    else routes.orNotFound
 
-  private def loggerMiddleware[F[_]: Async](routes: HttpApp[F], config: Config.Debug.Http): HttpApp[F] =
+  private def loggerMiddleware[F[_]: Async](routes: HttpRoutes[F], config: Config.Debug.Http): HttpRoutes[F] =
     if (config.enable) {
-      LoggerMiddleware.httpApp[F](
+      LoggerMiddleware.httpRoutes[F](
         logHeaders        = config.logHeaders,
         logBody           = config.logBody,
         redactHeadersWhen = config.redactHeaders.map(CIString(_)).contains(_),
         logAction         = Some((msg: String) => Logger[F].debug(msg))
       )(routes)
     } else routes
 
-  private def timeoutMiddleware[F[_]: Async](routes: HttpApp[F], networking: Config.Networking): HttpApp[F] =
-    Timeout.httpApp[F](timeout = networking.responseHeaderTimeout)(routes)
+  private def timeoutMiddleware[F[_]: Async](routes: HttpRoutes[F], networking: Config.Networking): HttpRoutes[F] =
+    Timeout.httpRoutes[F](timeout = networking.responseHeaderTimeout)(routes)
 
   private def buildBlazeServer[F[_]: Async](
     routes: HttpRoutes[F],
+    healthRoutes: HttpRoutes[F],
     port: Int,
     secure: Boolean,
     hsts: Config.HSTS,
@@ -94,7 +96,11 @@ object HttpServer {
       BlazeServerBuilder[F]
         .bindSocketAddress(new InetSocketAddress(port))
         .withHttpApp(
-          loggerMiddleware(timeoutMiddleware(hstsMiddleware(hsts, routes.orNotFound), networking), debugHttp)
+          hstsApp(
+            hsts,
+            loggerMiddleware(timeoutMiddleware(routes, networking), debugHttp)
+              <+> loggerMiddleware(healthRoutes, debugHttp)
+          )
         )
         .withIdleTimeout(networking.idleTimeout)
         .withMaxConnections(networking.maxConnections)

diff --git a/core/src/main/scala/com.snowplowanalytics.snowplow.collector.core/Routes.scala b/core/src/main/scala/com.snowplowanalytics.snowplow.collector.core/Routes.scala
@@ -28,20 +28,6 @@ class Routes[F[_]: Async](
 
   implicit val dns: Dns[F] = Dns.forSync[F]
 
-  private val healthRoutes = HttpRoutes.of[F] {
-    case GET -> Root / "health" =>
-      Ok("ok")
-    case GET -> Root / "sink-health" =>
-      service
-        .sinksHealthy
-        .ifM(
-          ifTrue  = Ok("ok"),
-          ifFalse = ServiceUnavailable("Service Unavailable")
-        )
-    case GET -> Root / "robots.txt" =>
-      Ok("User-agent: *\nDisallow: /\n\nUser-agent: Googlebot\nDisallow: /\n\nUser-agent: AdsBot-Google\nDisallow: /")
-  }
-
   private val corsRoute = HttpRoutes.of[F] {
     case req @ OPTIONS -> _ =>
       service.preflightResponse(req)
@@ -93,8 +79,22 @@ class Routes[F[_]: Async](
       service.crossdomainResponse
   }
 
+  val health = HttpRoutes.of[F] {
+    case GET -> Root / "health" =>
+      Ok("ok")
+    case GET -> Root / "sink-health" =>
+      service
+        .sinksHealthy
+        .ifM(
+          ifTrue  = Ok("ok"),
+          ifFalse = ServiceUnavailable("Service Unavailable")
+        )
+    case GET -> Root / "robots.txt" =>
+      Ok("User-agent: *\nDisallow: /\n\nUser-agent: Googlebot\nDisallow: /\n\nUser-agent: AdsBot-Google\nDisallow: /")
+  }
+
   val value: HttpRoutes[F] = {
-    val routes = healthRoutes <+> corsRoute <+> cookieRoutes <+> rootRoute <+> crossdomainRoute
+    val routes = corsRoute <+> cookieRoutes <+> rootRoute <+> crossdomainRoute
     if (enableDefaultRedirect) routes else rejectRedirect <+> routes
   }
 }
diff --git a/core/src/main/scala/com.snowplowanalytics.snowplow.collector.core/Run.scala b/core/src/main/scala/com.snowplowanalytics.snowplow.collector.core/Run.scala
@@ -93,14 +93,16 @@ object Run {
         Sinks(sinks.good, sinks.bad),
         appInfo
       )
+      routes = new Routes[F](
+        config.enableDefaultRedirect,
+        config.rootResponse.enabled,
+        config.crossDomain.enabled,
+        config.networking.responseHeaderTimeout,
+        collectorService
+      )
       httpServer = HttpServer.build[F](
-        new Routes[F](
-          config.enableDefaultRedirect,
-          config.rootResponse.enabled,
-          config.crossDomain.enabled,
-          config.networking.responseHeaderTimeout,
-          collectorService
-        ).value,
+        routes.value,
+        routes.health,
         if (config.ssl.enable) config.ssl.port else config.port,
         config.ssl.enable,
         config.hsts,

diff --git a/core/src/test/scala/com.snowplowanalytics.snowplow.collector.core/RoutesSpec.scala b/core/src/test/scala/com.snowplowanalytics.snowplow.collector.core/RoutesSpec.scala
@@ -5,6 +5,7 @@ import cats.data.NonEmptyList
 import scala.collection.mutable.ListBuffer
 import org.specs2.mutable.Specification
 import cats.effect.IO
+import cats.syntax.all._
 import cats.effect.unsafe.implicits.global
 import org.http4s.implicits._
 import org.http4s._
@@ -74,9 +75,7 @@ class RoutesSpec extends Specification {
     val service = new TestService()
     val routes =
       new Routes(enabledDefaultRedirect, enableRootResponse, enableCrossdomainTracking, 500.millis, service)
-        .value
-        .orNotFound
-    val routesWithHsts = HttpServer.hstsMiddleware(Config.HSTS(enableHsts, 180.days), routes)
+    val routesWithHsts = HttpServer.hstsApp(Config.HSTS(enableHsts, 180.days), (routes.value <+> routes.health))
     (service, routesWithHsts)
   }