refactor: class HttpMetricsServer as close only

packages/beacon-node/src/index.ts

@@ -5,7 +5,7 @@ export {createNodeJsLibp2p, NodeJsLibp2pOpts} from "./network/index.js";
 export * from "./node/index.js";
 
 // Export metrics utilities to de-duplicate validator metrics
-export {RegistryMetricCreator, collectNodeJSMetrics, HttpMetricsServer} from "./metrics/index.js";
+export {RegistryMetricCreator, collectNodeJSMetrics, HttpMetricsServer, getHttpMetricsServer} from "./metrics/index.js";
 
 // Export monitoring service to make it usable by validator
 export {MonitoringService} from "./monitoring/index.js";

packages/beacon-node/src/metrics/server/http.ts

@@ -2,7 +2,6 @@ import http from "node:http";
 import {Registry} from "prom-client";
 import {Logger} from "@lodestar/utils";
 import {wrapError} from "../../util/wrapError.js";
-import {HistogramExtra} from "../utils/histogram.js";
 import {HttpActiveSocketsTracker} from "../../api/rest/activeSockets.js";
 import {RegistryMetricCreator} from "../utils/registryMetricCreator.js";
 
@@ -11,99 +10,91 @@ export type HttpMetricsServerOpts = {
   address?: string;
 };
 
-export class HttpMetricsServer {
-  private readonly server: http.Server;
-  private readonly register: Registry;
-  private readonly getOtherMetrics: () => Promise<string>;
-  private readonly logger: Logger;
-  private readonly activeSockets: HttpActiveSocketsTracker;
-
-  private readonly httpServerRegister: RegistryMetricCreator;
-  private readonly scrapeTimeMetric: HistogramExtra<"status">;
-
-  constructor(
-    private readonly opts: HttpMetricsServerOpts,
-    {
-      register,
-      getOtherMetrics = async () => "",
-      logger,
-    }: {register: Registry; getOtherMetrics?: () => Promise<string>; logger: Logger}
-  ) {
-    this.logger = logger;
-    this.register = register;
-    this.getOtherMetrics = getOtherMetrics;
-    this.server = http.createServer(this.onRequest.bind(this));
-
-    // New registry to metric the metrics. Using the same registry would deadlock the .metrics promise
-    this.httpServerRegister = new RegistryMetricCreator();
-
-    this.scrapeTimeMetric = this.httpServerRegister.histogram<"status">({
-      name: "lodestar_metrics_scrape_seconds",
-      help: "Lodestar metrics server async time to scrape metrics",
-      labelNames: ["status"],
-      buckets: [0.1, 1, 10],
-    });
-
-    const socketsMetrics = {
-      activeSockets: this.httpServerRegister.gauge({
-        name: "lodestar_metrics_server_active_sockets_count",
-        help: "Metrics server current count of active sockets",
-      }),
-      socketsBytesRead: this.httpServerRegister.gauge({
-        name: "lodestar_metrics_server_sockets_bytes_read_total",
-        help: "Metrics server total count of bytes read on all sockets",
-      }),
-      socketsBytesWritten: this.httpServerRegister.gauge({
-        name: "lodestar_metrics_server_sockets_bytes_written_total",
-        help: "Metrics server total count of bytes written on all sockets",
-      }),
-    };
-
-    this.activeSockets = new HttpActiveSocketsTracker(this.server, socketsMetrics);
-  }
-
-  async start(): Promise<void> {
-    const {port, address} = this.opts;
-    this.logger.info("Starting metrics HTTP server", {port, address: address ?? "127.0.0.1"});
-    return new Promise((resolve, reject) => {
-      this.server.once("error", reject);
-      this.server.listen(port, address, resolve);
-    });
-  }
+export type HttpMetricsServer = {
+  close(): Promise<void>;
+};
 
-  async stop(): Promise<void> {
-    // In NodeJS land calling close() only causes new connections to be rejected.
-    // Existing connections can prevent .close() from resolving for potentially forever.
-    // In Lodestar case when the BeaconNode wants to close we will just abruptly terminate
-    // all existing connections for a fast shutdown.
-    // Inspired by https://github.com/gajus/http-terminator/
-    this.activeSockets.destroyAll();
+export async function getHttpMetricsServer(
+  opts: HttpMetricsServerOpts,
+  {
+    register,
+    getOtherMetrics = async () => "",
+    logger,
+  }: {register: Registry; getOtherMetrics?: () => Promise<string>; logger: Logger}
+): Promise<HttpMetricsServer> {
+  // New registry to metric the metrics. Using the same registry would deadlock the .metrics promise
+  const httpServerRegister = new RegistryMetricCreator();
 
-    await new Promise<void>((resolve, reject) => {
-      this.server.close((err) => {
-        if (err) reject(err);
-        else resolve();
-      });
-    });
-  }
+  const scrapeTimeMetric = httpServerRegister.histogram<"status">({
+    name: "lodestar_metrics_scrape_seconds",
+    help: "Lodestar metrics server async time to scrape metrics",
+    labelNames: ["status"],
+    buckets: [0.1, 1, 10],
+  });
 
-  private async onRequest(req: http.IncomingMessage, res: http.ServerResponse): Promise<void> {
+  const server = http.createServer(async function onRequest(
+    req: http.IncomingMessage,
+    res: http.ServerResponse
+  ): Promise<void> {
     if (req.method === "GET" && req.url && req.url.includes("/metrics")) {
-      const timer = this.scrapeTimeMetric.startTimer();
-      const metricsRes = await Promise.all([wrapError(this.register.metrics()), this.getOtherMetrics()]);
+      const timer = scrapeTimeMetric.startTimer();
+      const metricsRes = await Promise.all([wrapError(register.metrics()), getOtherMetrics()]);
       timer({status: metricsRes[0].err ? "error" : "success"});
 
       // Ensure we only writeHead once
       if (metricsRes[0].err) {
         res.writeHead(500, {"content-type": "text/plain"}).end(metricsRes[0].err.stack);
       } else {
         // Get scrape time metrics
-        const httpServerMetrics = await this.httpServerRegister.metrics();
+        const httpServerMetrics = await httpServerRegister.metrics();
         const metricsStr = `${metricsRes[0].result}\n\n${metricsRes[1]}\n\n${httpServerMetrics}`;
-        res.writeHead(200, {"content-type": this.register.contentType}).end(metricsStr);
+        res.writeHead(200, {"content-type": register.contentType}).end(metricsStr);
       }
     } else {
       res.writeHead(404).end();
     }
-  }
+  });
+
+  const socketsMetrics = {
+    activeSockets: httpServerRegister.gauge({
+      name: "lodestar_metrics_server_active_sockets_count",
+      help: "Metrics server current count of active sockets",
+    }),
+    socketsBytesRead: httpServerRegister.gauge({
+      name: "lodestar_metrics_server_sockets_bytes_read_total",
+      help: "Metrics server total count of bytes read on all sockets",
+    }),
+    socketsBytesWritten: httpServerRegister.gauge({
+      name: "lodestar_metrics_server_sockets_bytes_written_total",
+      help: "Metrics server total count of bytes written on all sockets",
+    }),
+  };
+
+  const activeSockets = new HttpActiveSocketsTracker(server, socketsMetrics);
+
+  const {port, address} = opts;
+  logger.info("Starting metrics HTTP server", {port, address: address ?? "127.0.0.1"});
+
+  await new Promise<void>((resolve, reject) => {
+    server.once("error", reject);
+    server.listen(port, address, resolve);
+  });
+
+  return {
+    async close(): Promise<void> {
+      // In NodeJS land calling close() only causes new connections to be rejected.
+      // Existing connections can prevent .close() from resolving for potentially forever.
+      // In Lodestar case when the BeaconNode wants to close we will just abruptly terminate
+      // all existing connections for a fast shutdown.
+      // Inspired by https://github.com/gajus/http-terminator/
+      activeSockets.destroyAll();
+
+      await new Promise<void>((resolve, reject) => {
+        server.close((err) => {
+          if (err) reject(err);
+          else resolve();
+        });
+      });
+    },
+  };
 }

packages/beacon-node/src/node/nodejs.ts

@@ -15,7 +15,7 @@ import {Network, getReqRespHandlers} from "../network/index.js";
 import {BeaconSync, IBeaconSync} from "../sync/index.js";
 import {BackfillSync} from "../sync/backfill/index.js";
 import {BeaconChain, IBeaconChain, initBeaconMetrics} from "../chain/index.js";
-import {createMetrics, Metrics, HttpMetricsServer} from "../metrics/index.js";
+import {createMetrics, Metrics, HttpMetricsServer, getHttpMetricsServer} from "../metrics/index.js";
 import {MonitoringService} from "../monitoring/index.js";
 import {getApi, BeaconRestApiServer} from "../api/index.js";
 import {initializeExecutionEngine, initializeExecutionBuilder} from "../execution/index.js";
@@ -36,7 +36,7 @@ export type BeaconNodeModules = {
   api: {[K in keyof Api]: ServerApi<Api[K]>};
   sync: IBeaconSync;
   backfillSync: BackfillSync | null;
-  metricsServer?: HttpMetricsServer;
+  metricsServer: HttpMetricsServer | null;
   monitoring: MonitoringService | null;
   restApi?: BeaconRestApiServer;
   controller?: AbortController;
@@ -90,7 +90,7 @@ export class BeaconNode {
   config: BeaconConfig;
   db: IBeaconDb;
   metrics: Metrics | null;
-  metricsServer?: HttpMetricsServer;
+  metricsServer: HttpMetricsServer | null;
   monitoring: MonitoringService | null;
   network: Network;
   chain: IBeaconChain;
@@ -272,15 +272,12 @@ export class BeaconNode {
 
     // only start server if metrics are explicitly enabled
     const metricsServer = opts.metrics.enabled
-      ? new HttpMetricsServer(opts.metrics, {
+      ? await getHttpMetricsServer(opts.metrics, {
           register: (metrics as Metrics).register,
           getOtherMetrics: () => network.scrapeMetrics(),
           logger: logger.child({module: LoggerModule.metrics}),
         })
-      : undefined;
-    if (metricsServer) {
-      await metricsServer.start();
-    }
+      : null;
 
     const restApi = new BeaconRestApiServer(opts.api.rest, {
       config,
@@ -320,7 +317,7 @@ export class BeaconNode {
       this.sync.close();
       this.backfillSync?.close();
       await this.network.close();
-      if (this.metricsServer) await this.metricsServer.stop();
+      if (this.metricsServer) await this.metricsServer.close();
       if (this.monitoring) this.monitoring.stop();
       if (this.restApi) await this.restApi.close();
       await this.chain.persistToDisk();

packages/beacon-node/test/unit/metrics/server/http.test.ts

@@ -1,22 +1,23 @@
-import request from "supertest";
-import {HttpMetricsServer} from "../../../../src/metrics/index.js";
+import fetch from "cross-fetch";
+import {getHttpMetricsServer, HttpMetricsServer} from "../../../../src/metrics/index.js";
 import {testLogger} from "../../../utils/logger.js";
 import {createMetricsTest} from "../utils.js";
 
 describe("HttpMetricsServer", () => {
   const logger = testLogger();
 
   let server: HttpMetricsServer | null = null;
+  const port = 14500;
 
   it("should serve metrics on /metrics", async () => {
     const metrics = createMetricsTest();
-    server = new HttpMetricsServer({port: 0}, {register: metrics.register, logger});
+    server = await getHttpMetricsServer({port}, {register: metrics.register, logger});
 
-    await server.start();
-    await request(server["server"]).get("/metrics").expect(200);
+    const res = await fetch(`http://127.0.0.1:${port}/metrics`);
+    await res.text();
   });
 
   after(async () => {
-    if (server) await server.stop();
+    if (server) await server.close();
   });
 });

packages/cli/src/cmds/validator/handler.ts

@@ -9,7 +9,12 @@ import {
   BuilderSelection,
 } from "@lodestar/validator";
 import {getMetrics, MetricsRegister} from "@lodestar/validator";
-import {RegistryMetricCreator, collectNodeJSMetrics, HttpMetricsServer, MonitoringService} from "@lodestar/beacon-node";
+import {
+  RegistryMetricCreator,
+  collectNodeJSMetrics,
+  getHttpMetricsServer,
+  MonitoringService,
+} from "@lodestar/beacon-node";
 import {getNodeLogger} from "@lodestar/logger/node";
 import {getBeaconConfigFromArgs} from "../../config/index.js";
 import {GlobalArgs} from "../../options/index.js";
@@ -122,10 +127,9 @@ export async function validatorHandler(args: IValidatorCliArgs & GlobalArgs): Pr
     if (args["metrics"]) {
       const port = args["metrics.port"] ?? validatorMetricsDefaultOptions.port;
       const address = args["metrics.address"] ?? validatorMetricsDefaultOptions.address;
-      const metricsServer = new HttpMetricsServer({port, address}, {register, logger});
+      const metricsServer = await getHttpMetricsServer({port, address}, {register, logger});
 
-      onGracefulShutdownCbs.push(() => metricsServer.stop());
-      await metricsServer.start();
+      onGracefulShutdownCbs.push(() => metricsServer.close());
     }
   }