Address OpenTelemetry review feedback

Author: justin808

packages/react-on-rails-pro-node-renderer/package.json

@@ -78,7 +78,7 @@
   },
   "peerDependencies": {
     "@honeybadger-io/js": ">=4.0.0",
-    "@opentelemetry/api": ">=1.9.0 <1.10.0",
+    "@opentelemetry/api": ">=1.9.0 <2.0.0",
     "@opentelemetry/exporter-trace-otlp-http": ">=0.214.0 <1.0.0",
     "@fastify/otel": ">=0.18.0 <1.0.0",
     "@opentelemetry/instrumentation": ">=0.214.0 <1.0.0",

packages/react-on-rails-pro-node-renderer/src/integrations/opentelemetry.ts

@@ -170,6 +170,7 @@ export function init(opts: OpenTelemetryInitOptions = {}): void {
   let registeredProvider: NodeTracerProviderType | undefined;
   let unregisterFastifyConfig: (() => void) | undefined;
   let unregisterWorkerShutdownHook: (() => void) | undefined;
+  let ownsOpenTelemetryGlobals = false;
 
   try {
     /* eslint-disable @typescript-eslint/no-require-imports, global-require --
@@ -219,8 +220,6 @@ export function init(opts: OpenTelemetryInitOptions = {}): void {
       spanProcessors: [spanProcessor],
     });
 
-    configureOpenTelemetryDiagnostics(loadedOtelApi);
-
     if (opts.fastify) {
       /* eslint-disable @typescript-eslint/no-require-imports, global-require */
       const { registerInstrumentations } =
@@ -285,15 +284,21 @@ export function init(opts: OpenTelemetryInitOptions = {}): void {
             }
           });
         installedAdapters.subSpan = setupSubSpan(subSpanImpl);
+      } else {
+        message(
+          '[OpenTelemetry] tracing integration was not installed because another tracing integration is ' +
+            'active; skipping OpenTelemetry sub-spans.',
+        );
       }
     }
 
     try {
       provider.register();
       setOpenTelemetryTracerProvider(provider);
       registeredProvider = provider;
+      ownsOpenTelemetryGlobals = true;
+      configureOpenTelemetryDiagnostics(loadedOtelApi);
     } catch (err) {
-      disableOpenTelemetryGlobals(loadedOtelApi);
       installedAdapters = resetInstalledTracingAdapters(installedAdapters);
       // When opts.fastify is enabled, OpenTelemetry does not expose a public
       // rollback API for module patches applied by registerInstrumentations().
@@ -310,6 +315,7 @@ export function init(opts: OpenTelemetryInitOptions = {}): void {
           if (getOpenTelemetryTracerProvider() === provider) {
             setOpenTelemetryTracerProvider(null);
             disableOpenTelemetryGlobals(loadedOtelApi);
+            ownsOpenTelemetryGlobals = false;
             installedAdapters = resetInstalledTracingAdapters(installedAdapters);
           }
         } finally {
@@ -333,11 +339,15 @@ export function init(opts: OpenTelemetryInitOptions = {}): void {
   } catch (err) {
     unregisterFastifyConfig?.();
     unregisterWorkerShutdownHook?.();
-    if (registeredProvider && getOpenTelemetryTracerProvider() === registeredProvider) {
+    if (
+      ownsOpenTelemetryGlobals &&
+      registeredProvider &&
+      otelApi &&
+      getOpenTelemetryTracerProvider() === registeredProvider
+    ) {
       setOpenTelemetryTracerProvider(null);
-    }
-    if (otelApi) {
       disableOpenTelemetryGlobals(otelApi);
+      ownsOpenTelemetryGlobals = false;
     }
     installedAdapters = resetInstalledTracingAdapters(installedAdapters);
     message(`[OpenTelemetry] init failed: ${String(err)}`);

packages/react-on-rails-pro-node-renderer/src/worker.ts

@@ -40,7 +40,7 @@ import {
   Asset,
   getAssetPath,
 } from './shared/utils.js';
-import { startSsrRequestOptions, subSpan, trace } from './shared/tracing.js';
+import { startSsrRequestOptions, subSpan, trace, type TracingContext } from './shared/tracing.js';
 import { applyFastifyConfigFunctions } from './worker/fastifyConfig.js';
 
 export { configureFastify, type FastifyConfigFunction } from './worker/fastifyConfig.js';
@@ -382,10 +382,12 @@ export default function run(config: Partial<Config>) {
     // Track whether we've already started sending a response (streaming or otherwise)
     // If true, we can't send an error response on failure - headers are already sent
     let responseStarted = false;
+    let incrementalTracingContext: TracingContext | undefined;
 
     try {
       await trace(
-        async () => {
+        async (context) => {
+          incrementalTracingContext = context;
           // Handle the incremental render stream
           await handleIncrementalRenderStream({
             request: req,
@@ -430,6 +432,7 @@ export default function run(config: Partial<Config>) {
                     err,
                     'Error while handling incremental render request',
                   ),
+                  context,
                 );
                 return {
                   response: errorResponse,
@@ -502,7 +505,7 @@ export default function run(config: Partial<Config>) {
         }
       } else {
         // Response hasn't started yet, we can send an error response
-        const errorResponse = errorResponseResult(errorMessage);
+        const errorResponse = errorResponseResult(errorMessage, incrementalTracingContext);
         await setResponse(errorResponse, res);
       }
     }
@@ -649,6 +652,8 @@ export default function run(config: Partial<Config>) {
     });
   }
 
+  // Integration hooks registered before the worker loads are applied here, immediately after
+  // listen() is scheduled and before Fastify finishes booting.
   applyFastifyConfigFunctions(app);
 
   return app;

packages/react-on-rails-pro-node-renderer/src/worker/fastifyConfig.ts

@@ -21,6 +21,11 @@ export function registerFastifyConfigFunction(configFunction: FastifyConfigFunct
   };
 }
 
+/**
+ * Public one-way registration API for custom entrypoints and integrations.
+ * Internal callers use registerFastifyConfigFunction() when they need the
+ * unregister callback during failed initialization or shutdown cleanup.
+ */
 export function configureFastify(configFunction: FastifyConfigFunction): void {
   registerFastifyConfigFunction(configFunction);
 }

packages/react-on-rails-pro-node-renderer/src/worker/shutdownHooks.ts

@@ -2,6 +2,26 @@ export type WorkerShutdownHook = () => void | Promise<void>;
 
 const workerShutdownHooks: WorkerShutdownHook[] = [];
 
+type AggregateErrorWithErrors = Error & { errors: readonly unknown[] };
+type AggregateErrorConstructor = new (
+  errors: readonly unknown[],
+  message?: string,
+) => AggregateErrorWithErrors;
+
+function createAggregateShutdownHookError(errors: readonly unknown[]): AggregateErrorWithErrors {
+  const { AggregateError: AggregateErrorCtor } = globalThis as typeof globalThis & {
+    AggregateError?: AggregateErrorConstructor;
+  };
+  const aggregateError = AggregateErrorCtor
+    ? new AggregateErrorCtor(errors, 'Multiple worker shutdown hooks failed')
+    : Object.assign(new Error('Multiple worker shutdown hooks failed'), {
+        errors,
+        name: 'AggregateError',
+      });
+
+  return aggregateError;
+}
+
 export function registerWorkerShutdownHook(hook: WorkerShutdownHook): () => void {
   workerShutdownHooks.push(hook);
   return () => {
@@ -18,12 +38,16 @@ export async function runWorkerShutdownHooks(): Promise<void> {
       await hook();
     }),
   );
-  const rejectedResult = results.find(
+  const rejectedResults = results.filter(
     (result): result is PromiseRejectedResult => result.status === 'rejected',
   );
 
-  if (rejectedResult) {
-    throw rejectedResult.reason;
+  const firstRejectedResult = rejectedResults[0];
+  if (rejectedResults.length === 1 && firstRejectedResult) {
+    throw firstRejectedResult.reason;
+  }
+  if (rejectedResults.length > 1) {
+    throw createAggregateShutdownHookError(rejectedResults.map((result) => result.reason));
   }
 }
 

packages/react-on-rails-pro-node-renderer/tests/integrations/opentelemetry-init.test.ts

@@ -1,6 +1,7 @@
 import { jest } from '@jest/globals';
 import {
   context as otelContext,
+  DiagLogLevel,
   diag as otelDiag,
   propagation as otelPropagation,
   trace as otelTrace,
@@ -394,18 +395,15 @@ describe('opentelemetry integration: init() failure path', () => {
     await resetOpenTelemetryForTest();
   });
 
-  test('init() disables OTel diagnostics when provider.register() fails', async () => {
-    const log = {
+  test('init() preserves existing OTel globals when provider.register() fails', async () => {
+    const existingDiagLogger = {
       debug: jest.fn(),
       error: jest.fn(),
       info: jest.fn(),
+      verbose: jest.fn(),
       warn: jest.fn(),
     };
 
-    jest.doMock('../../src/integrations/api.js', () => ({
-      log,
-      message: jest.fn(),
-    }));
     jest.doMock('@opentelemetry/sdk-trace-node', () => ({
       NodeTracerProvider: jest.fn(function NodeTracerProvider() {
         return {
@@ -417,19 +415,31 @@ describe('opentelemetry integration: init() failure path', () => {
       }),
     }));
 
-    const { diag } = await import('@opentelemetry/api');
-    const { InMemorySpanExporter, SimpleSpanProcessor } = await import('@opentelemetry/sdk-trace-base');
+    const { BasicTracerProvider, InMemorySpanExporter, SimpleSpanProcessor } = await import(
+      '@opentelemetry/sdk-trace-base'
+    );
+    const existingExporter = new InMemorySpanExporter();
+    const existingProvider = new BasicTracerProvider({
+      spanProcessors: [new SimpleSpanProcessor(existingExporter)],
+    });
+    expect(otelTrace.setGlobalTracerProvider(existingProvider)).toBe(true);
+    otelDiag.setLogger(existingDiagLogger, DiagLogLevel.WARN);
+    existingDiagLogger.warn.mockClear();
+
     const otel = await import('../../src/integrations/opentelemetry');
 
     otel.init({
       spanProcessor: new SimpleSpanProcessor(new InMemorySpanExporter()),
     });
-    diag.warn('should not reach renderer log');
 
-    expect(log.warn).not.toHaveBeenCalledWith(
-      { otel: true, level: 'warn', args: [] },
-      'should not reach renderer log',
-    );
+    otelTrace.getTracer('existing').startActiveSpan('span.after.failed.init', (span) => {
+      span.end();
+    });
+    otelDiag.warn('diagnostic after failed init');
+
+    expect(existingExporter.getFinishedSpans().map((span) => span.name)).toEqual(['span.after.failed.init']);
+    expect(existingDiagLogger.warn).toHaveBeenCalledWith('diagnostic after failed init');
+    await existingProvider.shutdown();
   });
 
   test('Fastify onClose suppresses late provider.shutdown() rejection after timeout', async () => {

packages/react-on-rails-pro-node-renderer/tests/integrations/opentelemetry.test.ts

@@ -6,6 +6,7 @@ import { resetOpenTelemetryForTest } from '../../src/testUtils/opentelemetry';
 import worker, { disableHttp2 } from '../../src/worker';
 import { applyFastifyConfigFunctions } from '../../src/worker/fastifyConfig';
 import packageJson from '../../src/shared/packageJson';
+import * as errorReporter from '../../src/shared/errorReporter';
 import {
   trace,
   subSpan,
@@ -336,22 +337,29 @@ describe('opentelemetry integration: tracing wiring', () => {
   });
 
   test('init({ tracing: true }) does not install subSpan when another tracing integration is active', async () => {
+    const messageSpy = jest.spyOn(errorReporter, 'message').mockImplementation(jest.fn());
     const existingExecutor = jest.fn(async (fn) => fn());
-    expect(setupTracing({ executor: existingExecutor })).toBe(true);
 
-    init({
-      tracing: true,
-      spanProcessor: new SimpleSpanProcessor(exporter),
-    });
+    try {
+      expect(setupTracing({ executor: existingExecutor })).toBe(true);
 
-    const result = await trace(
-      () => subSpan({ name: 'ror.vm.execute' }, async () => 'ok'),
-      startSsrRequestOptions({ renderingRequest: 'irrelevant' }),
-    );
+      init({
+        tracing: true,
+        spanProcessor: new SimpleSpanProcessor(exporter),
+      });
 
-    expect(result).toBe('ok');
-    expect(existingExecutor).toHaveBeenCalledTimes(1);
-    expect(exporter.getFinishedSpans()).toHaveLength(0);
+      const result = await trace(
+        () => subSpan({ name: 'ror.vm.execute' }, async () => 'ok'),
+        startSsrRequestOptions({ renderingRequest: 'irrelevant' }),
+      );
+
+      expect(result).toBe('ok');
+      expect(existingExecutor).toHaveBeenCalledTimes(1);
+      expect(exporter.getFinishedSpans()).toHaveLength(0);
+      expect(messageSpy).toHaveBeenCalledWith(expect.stringContaining('skipping OpenTelemetry sub-spans'));
+    } finally {
+      messageSpy.mockRestore();
+    }
   });
 
   test('subSpan does not leak renderingRequest payload into span attributes (sensitive data audit)', async () => {

packages/react-on-rails-pro-node-renderer/tests/shutdownHooks.test.ts

@@ -0,0 +1,41 @@
+import {
+  __resetWorkerShutdownHooksForTest,
+  registerWorkerShutdownHook,
+  runWorkerShutdownHooks,
+} from '../src/worker/shutdownHooks';
+
+describe('worker shutdown hooks', () => {
+  beforeEach(() => {
+    __resetWorkerShutdownHooksForTest();
+  });
+
+  afterEach(() => {
+    __resetWorkerShutdownHooksForTest();
+  });
+
+  test('throws the original error when a single shutdown hook fails', async () => {
+    const error = new Error('shutdown failed');
+    registerWorkerShutdownHook(async () => {
+      throw error;
+    });
+
+    await expect(runWorkerShutdownHooks()).rejects.toBe(error);
+  });
+
+  test('surfaces all errors when multiple shutdown hooks fail', async () => {
+    const firstError = new Error('first shutdown failed');
+    const secondError = new Error('second shutdown failed');
+    registerWorkerShutdownHook(async () => {
+      throw firstError;
+    });
+    registerWorkerShutdownHook(async () => {
+      throw secondError;
+    });
+
+    await expect(runWorkerShutdownHooks()).rejects.toMatchObject({
+      errors: [firstError, secondError],
+      message: 'Multiple worker shutdown hooks failed',
+      name: 'AggregateError',
+    });
+  });
+});

packages/react-on-rails-pro-node-renderer/tests/worker.test.ts

@@ -8,6 +8,7 @@ import packageJson from '../package.json';
 import worker, { configureFastify, disableHttp2 } from '../src/worker';
 import * as vm from '../src/worker/vm';
 import * as errorReporter from '../src/shared/errorReporter';
+import { __resetTracingForTest, setupTracing, type TracingContext } from '../src/shared/tracing';
 import {
   BUNDLE_TIMESTAMP,
   SECONDARY_BUNDLE_TIMESTAMP,
@@ -1100,6 +1101,41 @@ describe('worker', () => {
       expect(res.payload).toContain('Invalid first incremental render request chunk received');
     });
 
+    test('reports initial render errors with the active tracing context', async () => {
+      const app = createWorkerApp();
+      const tracingContext = { testContext: true } as TracingContext;
+      const reportMessageSpy = jest.spyOn(errorReporter, 'message').mockImplementation(jest.fn());
+
+      try {
+        __resetTracingForTest();
+        expect(setupTracing({ executor: async (fn) => fn(tracingContext) })).toBe(true);
+
+        const incompletePayload = {
+          gemVersion,
+          protocolVersion,
+          password: 'my_password',
+          dependencyBundleTimestamps: [String(BUNDLE_TIMESTAMP)],
+        };
+
+        const res = await callIncrementalRender(
+          app,
+          BUNDLE_TIMESTAMP,
+          'd41d8cd98f00b204e9800998ecf8427e',
+          incompletePayload,
+          400,
+        );
+
+        expect(res.payload).toContain('Invalid first incremental render request chunk received');
+        expect(reportMessageSpy).toHaveBeenCalledWith(
+          expect.stringContaining('Invalid first incremental render request chunk received'),
+          tracingContext,
+        );
+      } finally {
+        __resetTracingForTest();
+        reportMessageSpy.mockRestore();
+      }
+    });
+
     test('fails when password is missing', async () => {
       const app = createWorkerApp();
       await uploadBundle(app);