fix(#355 review E1=B + F1-F8): gate client telemetry OFF by default + throttler/lifecycle/overflow fixes

Maintainer resolved E1 as variant B: the public vitals sink + client collection must be OFF by default (else client_metrics grows unbounded on a self-host deploy with no external pruner, via an unauthenticated public endpoint). - F1: new operator flag CLIENT_TELEMETRY_ENABLED (default OFF), SEPARATE from METRICS_PORT (Grafana reads the table directly, independent of the scrape port). ClientTelemetryModule.register() provides VitalsController ONLY when the flag is true (route absent otherwise); the flag reaches the client via window.CONFIG (config.ts isClientTelemetryEnabled), and initVitals() early-returns when off. - F2/F3 [throttler]: this repo's ThrottlerGuard applies EVERY named throttler to every guarded route unless skipped. The new VITALS bucket therefore (a) newly bound collab-token → 429 behind shared/NAT IPs, and (b) the vitals route didn't skip the stricter public-share-ai (5/min) bucket → effective 5/min not 120. Fix (additive, global config unchanged): vitals.controller @SkipThrottle the other buckets + @Throttle VITALS 120/min; collab-token adds VITALS_THROTTLER to its existing @SkipThrottle (restoring its prior effectively-unthrottled state). - F4: metrics node:http server is closed on shutdown (MetricsServerLifecycle OnModuleDestroy → closeMetricsServer(), fired by enableShutdownHooks). - F5: docSize outside [0, int4-max] drops to null (keeping the event) instead of overflowing int4 and failing the WHOLE batch insert (+ 2 tests). - F6: .env.example documents METRICS_PORT (no default — unset = subsystem OFF) + CLIENT_TELEMETRY_ENABLED; fixed the inaccurate "default 9464" wording. - F7: disabled/non-sampled sessions install ZERO observers — isVitalsActive() (enabled && sampled) gates reportClientMetric AND the page-editor measurePageOpen + dispatchTransaction wrapping. - F8: kept db.d.ts hand-added (wontfix) — this repo HAND-CURATES db.d.ts (verified across recent fork migrations a32fba63/8c5b57eb/fdeede00); codegen would be the deviation. The ClientMetrics interface maps the migration 1:1. Gate: server tsc 0, client tsc 0, server metrics/vitals/telemetry/throttle 21 tests, client route-template 5. No new deps. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-07-05 00:00:03 +03:00
parent b9f3de80f5
commit d3209b5aab
16 changed files with 241 additions and 47 deletions
@@ -96,7 +96,9 @@ try {
    AiModule,
    AiChatModule,
    MetricsModule,
-    ClientTelemetryModule,
+    // Gated OFF by default: only registers the public vitals sink controller
+    // when CLIENT_TELEMETRY_ENABLED=true (maintainer decision E1=B).
+    ClientTelemetryModule.register(),
    ...enterpriseModules,
  ],
  controllers: [AppController],
@@ -16,6 +16,7 @@ import {
  AUTH_THROTTLER,
  PAGE_TEMPLATE_THROTTLER,
  PUBLIC_SHARE_AI_THROTTLER,
+  VITALS_THROTTLER,
 } from '../../integrations/throttle/throttler-names';
 import { LoginDto } from './dto/login.dto';
 import { AuthService } from './services/auth.service';
@@ -184,16 +185,21 @@ export class AuthController {
  }

  // The global ThrottlerGuard applies ALL named throttlers to every route by
-  // default, so each non-AUTH bucket (AI chat, page template, public-share AI)
-  // is explicitly skipped here. collab-token is auth-guarded (JwtAuthGuard),
-  // per-user and client-cached, so those feature buckets are irrelevant to it;
-  // skipping them avoids spurious 429s when a user opens many pages in a short
-  // window. The AUTH bucket is skipped too for the same per-user, cached reason.
+  // default, so each non-AUTH bucket (AI chat, page template, public-share AI,
+  // client vitals) is explicitly skipped here. collab-token is auth-guarded
+  // (JwtAuthGuard), per-user and client-cached, so those feature buckets are
+  // irrelevant to it; skipping them avoids spurious 429s when a user opens many
+  // pages in a short window. The VITALS bucket must be skipped too: it is a
+  // process-wide named throttler, so without this skip its per-IP limit would
+  // silently cap collab-token (the one route that opts out of every other
+  // bucket) and break editing behind shared/NAT IPs. The AUTH bucket is skipped
+  // for the same per-user, cached reason.
  @SkipThrottle({
    [AUTH_THROTTLER]: true,
    [AI_CHAT_THROTTLER]: true,
    [PAGE_TEMPLATE_THROTTLER]: true,
    [PUBLIC_SHARE_AI_THROTTLER]: true,
+    [VITALS_THROTTLER]: true,
  })
  @UseGuards(JwtAuthGuard)
  @HttpCode(HttpStatus.OK)
@@ -37,6 +37,12 @@ export const MAX_ATTR_LENGTH = 120;
 // route label sanity cap (client sends a template like /s/:space/p/:slug).
 export const MAX_ROUTE_LENGTH = 200;

+// `client_metrics.doc_size` is a Postgres `int` (int4). A garbage/huge docSize
+// on a single event would overflow int4 and make Postgres reject the WHOLE
+// batch INSERT, losing every event in it. Values outside this range are DROPPED
+// to null (the event is still kept) so one bad field never loses the batch.
+export const DOC_SIZE_MAX = 2147483647; // 2^31 - 1 (int4 max)
+
 export interface ClientMetricRow {
  name: string;
  value: number;
@@ -88,6 +94,12 @@ export function sanitizeVitalEvent(
    // Accept snake_case too, in case a client sends the raw column name.
    docSize = Math.trunc(e.doc_size as number);
  }
+  // Guard the int4 column: an out-of-range docSize would overflow int4 and make
+  // Postgres reject the whole batch INSERT. Drop the field (keep the event)
+  // rather than lose every other event in the batch.
+  if (docSize !== null && (docSize < 0 || docSize > DOC_SIZE_MAX)) {
+    docSize = null;
+  }

  return { name, value, rating, route, attr, docSize, workspaceId };
 }
@@ -1,4 +1,4 @@
-import { Module } from '@nestjs/common';
+import { DynamicModule, Module } from '@nestjs/common';
 import { VitalsController } from './vitals.controller';
 import { VitalsService } from './vitals.service';

@@ -7,9 +7,26 @@ import { VitalsService } from './vitals.service';
 * persists web-vitals + custom client metrics into `client_metrics`.
 * Named ClientTelemetryModule to avoid confusion with the unrelated
 * integrations/telemetry (product usage ping) module.
+ *
+ * GATED OFF BY DEFAULT (maintainer decision E1=B). The public, unauthenticated
+ * endpoint is only registered when CLIENT_TELEMETRY_ENABLED=true — otherwise the
+ * route does NOT exist at all (no anonymous disk-fill surface, and no unbounded
+ * `client_metrics` growth on a self-host deploy without an external pruner). The
+ * client is told the same flag via window.CONFIG and skips sending when off.
 */
-@Module({
-  controllers: [VitalsController],
-  providers: [VitalsService],
-})
-export class ClientTelemetryModule {}
+@Module({})
+export class ClientTelemetryModule {
+  static register(): DynamicModule {
+    // Read process.env directly (not EnvironmentService) so the toggle is
+    // resolved at module-registration time, identical to how the metrics
+    // subsystem reads METRICS_PORT. Absent/anything-but-"true" => OFF.
+    const enabled =
+      (process.env.CLIENT_TELEMETRY_ENABLED ?? '').toLowerCase() === 'true';
+
+    return {
+      module: ClientTelemetryModule,
+      controllers: enabled ? [VitalsController] : [],
+      providers: enabled ? [VitalsService] : [],
+    };
+  }
+}
@@ -6,10 +6,16 @@ import {
  Req,
  UseGuards,
 } from '@nestjs/common';
-import { Throttle, ThrottlerGuard } from '@nestjs/throttler';
+import { SkipThrottle, Throttle, ThrottlerGuard } from '@nestjs/throttler';
 import { FastifyRequest } from 'fastify';
 import { Public } from '../../common/decorators/public.decorator';
-import { VITALS_THROTTLER } from '../../integrations/throttle/throttler-names';
+import {
+  AI_CHAT_THROTTLER,
+  AUTH_THROTTLER,
+  PAGE_TEMPLATE_THROTTLER,
+  PUBLIC_SHARE_AI_THROTTLER,
+  VITALS_THROTTLER,
+} from '../../integrations/throttle/throttler-names';
 import { VitalsService } from './vitals.service';

 /**
@@ -25,6 +31,17 @@ export class VitalsController {

  @Public()
  @UseGuards(ThrottlerGuard)
+  // The global ThrottlerGuard applies ALL named throttlers to every route, so
+  // every OTHER bucket must be skipped here — otherwise the strictest of them
+  // (public-share AI at 5/min) would override the intended vitals limit and cap
+  // this route at 5/min instead of 120/min. Skip them all so ONLY the VITALS
+  // bucket below applies.
+  @SkipThrottle({
+    [AUTH_THROTTLER]: true,
+    [AI_CHAT_THROTTLER]: true,
+    [PAGE_TEMPLATE_THROTTLER]: true,
+    [PUBLIC_SHARE_AI_THROTTLER]: true,
+  })
  @Throttle({ [VITALS_THROTTLER]: { limit: 120, ttl: 60_000 } })
  @Post('vitals')
  @HttpCode(200)
@@ -119,4 +119,31 @@ describe('VitalsService.buildRows', () => {
    const rows = svc.buildRows({ events: [{ name: 'LCP', value: 1 }] }, null);
    expect(rows[0].workspaceId).toBeNull();
  });
+
+  it('drops an out-of-int4-range docSize to null without losing the batch', () => {
+    const rows = svc.buildRows(
+      {
+        events: [
+          // Garbage docSize overflowing int4 must NOT reject the whole batch:
+          // the field is dropped to null and the event is kept.
+          { name: 'editor_tx_ms', value: 10, docSize: 9_999_999_999 },
+          { name: 'editor_tx_ms', value: 20, docSize: -5 },
+          { name: 'editor_tx_ms', value: 30, docSize: 4096 },
+        ],
+      },
+      WS,
+    );
+    expect(rows).toHaveLength(3);
+    expect(rows[0].docSize).toBeNull();
+    expect(rows[1].docSize).toBeNull();
+    expect(rows[2].docSize).toBe(4096);
+  });
+
+  it('keeps a docSize exactly at the int4 max', () => {
+    const rows = svc.buildRows(
+      { events: [{ name: 'editor_tx_ms', value: 1, docSize: 2147483647 }] },
+      WS,
+    );
+    expect(rows[0].docSize).toBe(2147483647);
+  });
 });
@@ -227,6 +227,22 @@ export class EnvironmentService {
    return compactTree === 'true';
  }

+  /**
+   * Operator toggle for the public client-telemetry sink (#355). DEFAULT OFF:
+   * the unauthenticated POST /api/telemetry/vitals endpoint + client vitals
+   * collection are only wired when this is explicitly true. Kept SEPARATE from
+   * METRICS_PORT (the server Prometheus half) because Grafana reads the
+   * `client_metrics` table directly, independent of the scrape port — and
+   * because `client_metrics` has no app-side retention, so an operator must opt
+   * in and run an external pruner.
+   */
+  isClientTelemetryEnabled(): boolean {
+    const enabled = this.configService
+      .get<string>('CLIENT_TELEMETRY_ENABLED', 'false')
+      .toLowerCase();
+    return enabled === 'true';
+  }
+
  getStripePublishableKey(): string {
    return this.configService.get<string>('STRIPE_PUBLISHABLE_KEY');
  }
@@ -0,0 +1,16 @@
+import { Injectable, OnModuleDestroy } from '@nestjs/common';
+import { closeMetricsServer } from './metrics.server';
+
+/**
+ * Ties the bare node:http metrics scrape server (started in main.ts after the
+ * Fastify app is up, outside the DI container) into Nest's shutdown lifecycle.
+ * With `app.enableShutdownHooks()`, onModuleDestroy fires on SIGTERM/SIGINT and
+ * closes the listener so it is not left dangling (jest/e2e never exits, and a
+ * prod restart doesn't leak the port). No-op when metrics are disabled.
+ */
+@Injectable()
+export class MetricsServerLifecycle implements OnModuleDestroy {
+  async onModuleDestroy(): Promise<void> {
+    await closeMetricsServer();
+  }
+}
@@ -1,13 +1,15 @@
 import { Module } from '@nestjs/common';
 import { MetricsBullService } from './metrics-bull.service';
+import { MetricsServerLifecycle } from './metrics-server.lifecycle';

 /**
 * Wires the BullMQ collectors (#355). The queues are provided by the @Global
 * QueueModule (which exports BullModule), so no re-registration is needed here.
 * The HTTP histogram, DB-query and collab-store collectors live in module-level
 * singletons (metrics.registry) and are wired directly at their call sites.
+ * MetricsServerLifecycle closes the scrape server on shutdown.
 */
@Module({
-  providers: [MetricsBullService],
+  providers: [MetricsBullService, MetricsServerLifecycle],
 })
 export class MetricsModule {}
@@ -3,13 +3,19 @@ import { Logger } from '@nestjs/common';
 import { getMetricsRegistry, isMetricsEnabled } from './metrics.registry';

 /**
- * Start the Prometheus scrape endpoint on a SEPARATE port (default 9464,
- * overridable via `METRICS_PORT`). This is a bare node:http server, NOT part of
- * the Fastify app, so `/metrics` never exists on the public :3000 listener.
+ * Start the Prometheus scrape endpoint on a SEPARATE port, taken from
+ * `METRICS_PORT`. There is NO default port: when `METRICS_PORT` is unset the
+ * whole metrics subsystem is OFF and this returns null. This is a bare node:http
+ * server, NOT part of the Fastify app, so `/metrics` never exists on the public
+ * :3000 listener.
 *
 * Returns the http.Server (so callers can close it on shutdown) or null when
- * metrics are disabled.
+ * metrics are disabled. The reference is also kept module-side so the Nest
+ * lifecycle (see MetricsModule) can close it on application shutdown without
+ * threading the handle back through the non-DI bootstrap.
 */
+let metricsServer: Server | null = null;
+
 export function startMetricsServer(): Server | null {
  if (!isMetricsEnabled()) return null;

@@ -52,5 +58,20 @@ export function startMetricsServer(): Server | null {
    logger.error(`Metrics server error: ${err?.message}`);
  });

+  metricsServer = server;
  return server;
 }
+
+/**
+ * Close the metrics scrape server if one is running. Idempotent and safe to call
+ * when metrics are disabled (no server was ever started). Wired into Nest's
+ * shutdown lifecycle so the listener is not left dangling on shutdown.
+ */
+export function closeMetricsServer(): Promise<void> {
+  const server = metricsServer;
+  metricsServer = null;
+  if (!server) return Promise.resolve();
+  return new Promise((resolve) => {
+    server.close(() => resolve());
+  });
+}
@@ -50,6 +50,10 @@ export class StaticModule implements OnModuleInit {
          : undefined,
        POSTHOG_HOST: this.environmentService.getPostHogHost(),
        POSTHOG_KEY: this.environmentService.getPostHogKey(),
+        // #355 — mirrors the server-side CLIENT_TELEMETRY_ENABLED gate so the
+        // client only collects/sends vitals when the operator opts in.
+        CLIENT_TELEMETRY_ENABLED:
+          this.environmentService.isClientTelemetryEnabled(),
      };

      const windowScriptContent = `<script>window.CONFIG=${JSON.stringify(configString)};</script>`;
@@ -195,8 +195,9 @@ async function bootstrap() {
    );
  });

-  // #355 — Prometheus scrape endpoint on a SEPARATE port (METRICS_PORT, default
-  // 9464), started after the app is up. No-op when METRICS_PORT is unset.
+  // #355 — Prometheus scrape endpoint on a SEPARATE port (METRICS_PORT),
+  // started after the app is up. No default port: a no-op when METRICS_PORT is
+  // unset. Closed on shutdown by MetricsServerLifecycle (MetricsModule).
  startMetricsServer();
 }