[CORL-1075] Metrics Cleanup (#2955)

* feat: added support for metrics on another port

* fix: handle cluster better

* fix: linting

src/core/server/app/index.ts

@@ -1,6 +1,6 @@
 import cons from "consolidate";
 import cors from "cors";
-import { Express } from "express";
+import express, { Express } from "express";
 import enforceHTTPSMiddleware from "express-enforces-ssl";
 import { GraphQLSchema } from "graphql";
 import { RedisPubSub } from "graphql-redis-subscriptions";
@@ -9,9 +9,16 @@ import http from "http";
 import { Db } from "mongodb";
 import nunjucks from "nunjucks";
 import path from "path";
+import { AggregatorRegistry, register } from "prom-client";
 
-import { cacheHeadersMiddleware } from "coral-server/app/middleware/cacheHeaders";
-import { HTMLErrorHandler } from "coral-server/app/middleware/error";
+import {
+  cacheHeadersMiddleware,
+  noCacheMiddleware,
+} from "coral-server/app/middleware/cacheHeaders";
+import {
+  HTMLErrorHandler,
+  JSONErrorHandler,
+} from "coral-server/app/middleware/error";
 import { notFoundMiddleware } from "coral-server/app/middleware/notFound";
 import { createPassport } from "coral-server/app/middleware/passport";
 import { Config } from "coral-server/config";
@@ -32,6 +39,7 @@ import TenantCache from "coral-server/services/tenant/cache";
 
 import { healthHandler, versionHandler } from "./handlers";
 import { compileTrust } from "./helpers";
+import { basicAuth } from "./middleware/basicAuth";
 import { accessLogger, errorLogger } from "./middleware/logging";
 import { metricsRecorder } from "./middleware/metrics";
 import serveStatic from "./middleware/serveStatic";
@@ -46,7 +54,7 @@ export interface AppOptions {
   rejectorQueue: RejectorQueue;
   webhookQueue: WebhookQueue;
   notifierQueue: NotifierQueue;
-  metrics?: Metrics;
+  metrics: Metrics;
   mongo: Db;
   parent: Express;
   persistedQueriesRequired: boolean;
@@ -73,10 +81,8 @@ export async function createApp(options: AppOptions): Promise<Express> {
   // Logging
   parent.use(accessLogger);
 
-  if (options.metrics) {
-    // Capturing metrics.
-    parent.use(metricsRecorder(options.metrics));
-  }
+  // Capturing metrics.
+  parent.use(metricsRecorder(options.metrics));
 
   // Configure the health check endpoint.
   parent.get("/api/health", healthHandler);
@@ -198,3 +204,59 @@ function configureApplicationViews(options: AppOptions) {
   // set .html as the default extension.
   parent.set("view engine", "html");
 }
+
+export default function createMetricsServer(config: Config) {
+  const server = express();
+
+  // Setup access logger.
+  server.use(accessLogger);
+  server.use(noCacheMiddleware);
+
+  // Add basic auth if provided.
+  const username = config.get("metrics_username");
+  const password = config.get("metrics_password");
+  if (username && password) {
+    server.use(basicAuth(username, password));
+    logger.info("adding authentication to metrics endpoint");
+  } else {
+    logger.info(
+      "not adding authentication to metrics endpoint, credentials not provided"
+    );
+  }
+
+  // If we are running in concurrency mode, we should setup the aggregator for
+  // the cluster metrics.
+  if (config.get("concurrency") > 1) {
+    // Create the aggregator registry for metrics.
+    const aggregatorRegistry = new AggregatorRegistry();
+
+    // Use the aggregator registry to handle serving metrics.
+    server.get("/cluster_metrics", (req, res, next) => {
+      aggregatorRegistry.clusterMetrics((err, metrics) => {
+        if (err) {
+          return next(err);
+        }
+
+        res.set("Content-Type", aggregatorRegistry.contentType);
+        res.send(metrics);
+      });
+    });
+
+    logger.info({ path: "/cluster_metrics" }, "mounted metrics handler");
+  } else {
+    // Use the memory register to handle serving metrics.
+    server.get("/metrics", (req, res) => {
+      res.set("Content-Type", register.contentType);
+      res.end(register.metrics());
+    });
+
+    logger.info({ path: "/metrics" }, "mounted metrics handler");
+  }
+
+  // Error handling.
+  server.use(notFoundMiddleware);
+  server.use(errorLogger);
+  server.use(JSONErrorHandler());
+
+  return server;
+}

src/core/server/app/middleware/graphql/graphqlMiddleware.ts

@@ -40,22 +40,15 @@ const NoIntrospection = (context: ValidationContext) => ({
 const graphqlMiddleware = (
   config: Config,
   requestOptions: ExpressGraphQLOptionsFunction,
-  metrics?: Metrics
+  metrics: Metrics
 ): Handler => {
   const extensions: Array<() => GraphQLExtension> = [
     () => new ErrorWrappingExtension(),
     () => new LoggerExtension(),
+    // Pass the metrics to the extension so it can increment.
+    () => new MetricsExtension(metrics),
   ];
 
-  // Add the metrics extension if provided.
-  if (metrics) {
-    extensions.push(
-      () =>
-        // Pass the metrics to the extension so it can increment.
-        new MetricsExtension(metrics)
-    );
-  }
-
   // Create a new baseOptions that will be merged into the new options.
   const baseOptions: Omit<GraphQLOptions, "schema"> = {
     // Disable the debug mode, as we already add in our logging function.

src/core/server/app/router/index.ts

@@ -1,15 +1,13 @@
 import cookies from "cookie-parser";
 import express, { Router } from "express";
-import { register } from "prom-client";
 
 import { LanguageCode } from "coral-common/helpers/i18n/locales";
 import { AppOptions } from "coral-server/app";
-import { noCacheMiddleware } from "coral-server/app/middleware/cacheHeaders";
+
 import playground from "coral-server/app/middleware/playground";
 import { RouterOptions } from "coral-server/app/router/types";
 import logger from "coral-server/logger";
 
-import { basicAuth } from "../middleware/basicAuth";
 import { createAPIRouter } from "./api";
 import { mountClientRoutes } from "./client";
 
@@ -38,26 +36,6 @@ export function createRouter(app: AppOptions, options: RouterOptions) {
     logger.warn("client routes are disabled");
   }
 
-  if (app.metrics) {
-    // Add basic auth if provided.
-    const username = app.config.get("metrics_username");
-    const password = app.config.get("metrics_password");
-    if (username && password) {
-      router.use("/metrics", basicAuth(username, password));
-      logger.info("adding authentication to metrics endpoint");
-    } else {
-      logger.info(
-        "not adding authentication to metrics endpoint, credentials not provided"
-      );
-    }
-
-    router.get("/metrics", noCacheMiddleware, (req, res) => {
-      res.set("Content-Type", register.contentType);
-      res.end(register.metrics());
-    });
-    logger.info({ path: "/metrics" }, "mounting metrics path on app");
-  }
-
   return router;
 }
 

src/core/server/config.ts

@@ -107,12 +107,6 @@ const config = convict({
     default: 3000,
     env: "PORT",
   },
-  cluster_metrics_port: {
-    doc: "The port to bind for cluster metrics.",
-    format: "port",
-    default: 3001,
-    env: "CLUSTER_METRICS_PORT",
-  },
   metrics_username: {
     doc: "The username to use to authenticate to the metrics endpoint.",
     format: "String",
@@ -124,9 +118,14 @@ const config = convict({
     format: "String",
     default: "",
     env: "METRICS_PASSWORD",
-
     sensitive: true,
   },
+  metrics_port: {
+    doc: "The port that the metrics handler should be mounted.",
+    format: "port",
+    default: 9000,
+    env: "METRICS_PORT",
+  },
   dev_port: {
     doc: "The port to bind for the Webpack Dev Server.",
     format: "port",

src/core/server/graph/subscriptions/server.ts

@@ -180,16 +180,13 @@ export function formatResponse(
     // Log out the query.
     logQuery(context, query, persisted);
 
-    // Increment the metrics if enabled.
-    if (metrics) {
-      // Get the request metadata.
-      const { operation, operationName } = getOperationMetadata(query);
-      if (operation && operationName) {
-        // Increment the graph query value, tagging with the name of the query.
-        metrics.executedGraphQueriesTotalCounter
-          .labels(operation, operationName)
-          .inc();
-      }
+    // Get the request metadata.
+    const { operation, operationName } = getOperationMetadata(query);
+    if (operation && operationName) {
+      // Increment the graph query value, tagging with the name of the query.
+      metrics.executedGraphQueriesTotalCounter
+        .labels(operation, operationName)
+        .inc();
     }
 
     if (value.errors && value.errors.length > 0) {

src/core/server/index.ts

@@ -4,16 +4,15 @@ import { GraphQLSchema } from "graphql";
 import { RedisPubSub } from "graphql-redis-subscriptions";
 import http from "http";
 import { Db } from "mongodb";
-import { AggregatorRegistry, collectDefaultMetrics } from "prom-client";
+import { collectDefaultMetrics } from "prom-client";
 import { SubscriptionServer } from "subscriptions-transport-ws";
 
 import { LanguageCode } from "coral-common/helpers/i18n/locales";
-import { AppOptions, createApp, listenAndServe } from "coral-server/app";
-import { basicAuth } from "coral-server/app/middleware/basicAuth";
-import { noCacheMiddleware } from "coral-server/app/middleware/cacheHeaders";
-import { JSONErrorHandler } from "coral-server/app/middleware/error";
-import { accessLogger, errorLogger } from "coral-server/app/middleware/logging";
-import { notFoundMiddleware } from "coral-server/app/middleware/notFound";
+import createMetricsServer, {
+  AppOptions,
+  createApp,
+  listenAndServe,
+} from "coral-server/app";
 import config, { Config } from "coral-server/config";
 import startScheduledTasks, { ScheduledJobGroups } from "coral-server/cron";
 import getTenantSchema from "coral-server/graph/schema";
@@ -274,60 +273,13 @@ class Server {
       signingConfig: this.signingConfig,
     });
 
-    // If we are running in concurrency mode, and we are the master, we should
-    // setup the aggregator for the cluster metrics.
-    if (cluster.isMaster && this.config.get("concurrency") > 1) {
-      // Create the aggregator registry for metrics.
-      const aggregatorRegistry = new AggregatorRegistry();
-
-      // Setup the cluster metrics server.
-      const metricsServer = express();
-
-      // Setup access logger.
-      metricsServer.use(accessLogger);
-
-      // Add basic auth if provided.
-      const username = this.config.get("metrics_username");
-      const password = this.config.get("metrics_password");
-      if (username && password) {
-        metricsServer.use("/cluster_metrics", basicAuth(username, password));
-        logger.info("adding authentication to metrics endpoint");
-      } else {
-        logger.info(
-          "not adding authentication to metrics endpoint, credentials not provided"
-        );
-      }
-
-      // Cluster metrics will be served on /cluster_metrics.
-      metricsServer.get(
-        "/cluster_metrics",
-        noCacheMiddleware,
-        (req, res, next) => {
-          aggregatorRegistry.clusterMetrics((err, metrics) => {
-            if (err) {
-              return next(err);
-            }
-
-            res.set("Content-Type", aggregatorRegistry.contentType);
-            res.send(metrics);
-          });
-        }
-      );
-
-      // Error handling.
-      metricsServer.use(notFoundMiddleware);
-      metricsServer.use(errorLogger);
-      metricsServer.use(JSONErrorHandler());
-
-      const port = this.config.get("cluster_metrics_port");
-
-      // Star the server listening for cluster metrics.
-      await listenAndServe(metricsServer, port);
-
-      logger.info(
-        { port, path: "/cluster_metrics" },
-        "now listening for cluster metrics"
-      );
+    // We only want to setup a metrics server iff the concurrency is 1 or the
+    // concurrency is greater than one and this is the master process.
+    if (config.get("concurrency") === 1 || cluster.isMaster) {
+      // Configure the metrics server and start it.
+      const port = this.config.get("metrics_port");
+      await listenAndServe(createMetricsServer(this.config), port);
+      logger.info({ port }, "now listening for metrics");
     }
   }
 
@@ -370,17 +322,13 @@ class Server {
       notifierQueue: this.tasks.notifier,
       disableClientRoutes,
       persistedQueryCache: this.persistedQueryCache,
+      metrics: createMetrics(),
       persistedQueriesRequired:
         this.config.get("env") === "production" &&
         !this.config.get("enable_graphiql"),
       migrationManager: this.migrationManager,
     };
 
-    // Only enable the metrics server if concurrency is set to 1.
-    if (this.config.get("concurrency") === 1) {
-      options.metrics = createMetrics();
-    }
-
     // Create the Coral App, branching off from the parent app.
     const app: Express = await createApp(options);