fix(core): code review comments

gh-0

Author: samarpan-b

packages/observability/src/__tests__/integration/runtime.integration.ts

@@ -82,6 +82,6 @@ describe('observability runtime', () => {
       ok(headers.traceparent);
     });
 
-    strictEqual(initResult.exporterName, 'loadOtlpExporter');
+    strictEqual(initResult.exporterName, 'InMemorySpanExporter');
   });
 });

packages/observability/src/__tests__/unit/runtime.unit.ts

@@ -0,0 +1,52 @@
+import {strictEqual} from 'assert';
+import {shutdownObservability} from '../../bootstrap';
+import {DEFAULT_OBSERVABILITY_CONFIG} from '../../config/defaults';
+import {
+  clearRuntimeState,
+  getRuntimeState,
+  updateRuntimeState,
+} from '../../runtime';
+
+describe('runtime state', () => {
+  afterEach(() => {
+    clearRuntimeState();
+  });
+
+  it('returns a snapshot instead of the mutable backing state', () => {
+    updateRuntimeState({
+      config: {
+        ...DEFAULT_OBSERVABILITY_CONFIG,
+        enabled: true,
+        profile: 'default',
+        serviceName: 'runtime-state-service',
+      },
+    });
+
+    const runtimeState = getRuntimeState();
+    runtimeState.config!.serviceName = 'mutated-from-test';
+
+    strictEqual(getRuntimeState().config?.serviceName, 'runtime-state-service');
+  });
+
+  it('swallows shutdown errors and clears runtime state', async () => {
+    updateRuntimeState({
+      runtime: {
+        enabled: true,
+        profile: 'default',
+        config: {
+          ...DEFAULT_OBSERVABILITY_CONFIG,
+          enabled: true,
+          profile: 'default',
+          serviceName: 'shutdown-runtime-service',
+        },
+        shutdown: async () => {
+          throw new Error('shutdown-failed');
+        },
+      },
+    });
+
+    await shutdownObservability();
+
+    strictEqual(getRuntimeState().runtime, undefined);
+  });
+});

packages/observability/src/__tests__/unit/tracing.unit.ts

@@ -0,0 +1,66 @@
+import {strictEqual} from 'assert';
+import {SpanStatusCode, trace, Tracer, TracerOptions} from '@opentelemetry/api';
+import {withSpan} from '../../tracing';
+
+describe('withSpan', () => {
+  it('marks failed spans with error status before rethrowing', async () => {
+    const originalGetTracer = trace.getTracer;
+    const spanState: {
+      status?: {code: SpanStatusCode; message?: string};
+      ended: boolean;
+      recordedError?: unknown;
+    } = {
+      ended: false,
+    };
+
+    const fakeSpan = {
+      end: () => {
+        spanState.ended = true;
+      },
+      recordException: (error: unknown) => {
+        spanState.recordedError = error;
+      },
+      setAttributes: () => undefined,
+      setStatus: (status: {code: SpanStatusCode; message?: string}) => {
+        spanState.status = status;
+      },
+    };
+
+    const fakeTracer = {
+      startActiveSpan: (
+        _name: string,
+        arg2: unknown,
+        arg3?: unknown,
+        arg4?: unknown,
+      ) => {
+        const callback =
+          typeof arg2 === 'function'
+            ? arg2
+            : typeof arg3 === 'function'
+              ? arg3
+              : arg4;
+
+        return (callback as (span: typeof fakeSpan) => unknown)(fakeSpan);
+      },
+    } as unknown as Tracer;
+
+    trace.getTracer = (
+      _name: string,
+      _version?: string,
+      _options?: TracerOptions,
+    ) => fakeTracer as Tracer;
+
+    try {
+      await withSpan('failing-span', async () => {
+        throw new Error('boom');
+      }).catch(() => undefined);
+    } finally {
+      trace.getTracer = originalGetTracer;
+    }
+
+    strictEqual(spanState.ended, true);
+    strictEqual(spanState.recordedError instanceof Error, true);
+    strictEqual(spanState.status?.code, SpanStatusCode.ERROR);
+    strictEqual(spanState.status?.message, 'boom');
+  });
+});

packages/observability/src/bootstrap.ts

@@ -56,6 +56,11 @@ function buildDisabledRuntime(): ObservabilityRuntime {
   };
 }
 
+function discardProfileState(profile: ObservabilityProfile): void {
+  profile.shutdown?.().catch(() => undefined);
+  clearRuntimeState();
+}
+
 export function bootstrapObservability(
   overrides?: Partial<ObservabilityConfig>,
 ): ObservabilityRuntime {
@@ -78,9 +83,15 @@ export function bootstrapObservability(
   const profileConfig = profile.applyDefaults(resolvedConfig);
   validateObservabilityConfig(profileConfig);
 
-  const initResult = profile.initialize(profileConfig, {
-    createExporter: createOtlpExporter,
-  });
+  let initResult;
+  try {
+    initResult = profile.initialize(profileConfig, {
+      createExporter: createOtlpExporter,
+    });
+  } catch (error) {
+    discardProfileState(profile);
+    throw error;
+  }
 
   updateRuntimeState({
     profile,
@@ -93,8 +104,13 @@ export function bootstrapObservability(
     config: profileConfig,
     tracerProvider: initResult.tracerProvider,
     async shutdown() {
-      await profile.shutdown?.();
-      clearRuntimeState();
+      try {
+        await profile.shutdown?.();
+      } catch {
+        return;
+      } finally {
+        clearRuntimeState();
+      }
     },
   };
 
@@ -112,7 +128,11 @@ export async function shutdownObservability(): Promise<void> {
     return;
   }
 
-  await runtime.shutdown();
+  try {
+    await runtime.shutdown();
+  } catch {
+    clearRuntimeState();
+  }
 }
 
 export function isObservabilityEnabled(): boolean {

packages/observability/src/config/resolve-config.ts

@@ -41,6 +41,28 @@ function parseNumber(value: string | undefined): number | undefined {
   return Number.isFinite(parsed) ? parsed : undefined;
 }
 
+function parseExporterProtocol(
+  value: string | undefined,
+): ExporterProtocol | undefined {
+  if (value === 'grpc' || value === 'http/protobuf') {
+    return value;
+  }
+
+  return undefined;
+}
+
+function parseSamplerName(value: string | undefined): SamplerName | undefined {
+  if (
+    value === 'always_on' ||
+    value === 'always_off' ||
+    value === 'traceidratio'
+  ) {
+    return value;
+  }
+
+  return undefined;
+}
+
 function parseHeaders(value: string | undefined): Record<string, string> {
   if (!value) {
     return {};
@@ -98,13 +120,10 @@ function startupInstrumentationConfig(
     DEFAULT_INSTRUMENTATIONS,
   ) as (keyof InstrumentationToggles)[];
 
-  return keys.reduce(
-    (result, key) => {
-      result[key] = resolveInstrumentationToggle(key, overrides);
-      return result;
-    },
-    {} as InstrumentationToggles,
-  );
+  return keys.reduce((result, key) => {
+    result[key] = resolveInstrumentationToggle(key, overrides);
+    return result;
+  }, {} as InstrumentationToggles);
 }
 
 function resolveServiceInfo(
@@ -136,13 +155,15 @@ function resolveExporterConfig(
   overrides: Partial<ObservabilityConfig> | undefined,
   env: NodeJS.ProcessEnv,
 ) {
-  const exporterProtocol = (overrides?.exporterProtocol ??
-    (env.OTEL_EXPORTER_OTLP_PROTOCOL as ExporterProtocol | undefined) ??
-    DEFAULT_OBSERVABILITY_CONFIG.exporterProtocol) as ExporterProtocol;
+  const exporterProtocol =
+    overrides?.exporterProtocol ??
+    parseExporterProtocol(env.OTEL_EXPORTER_OTLP_PROTOCOL) ??
+    DEFAULT_OBSERVABILITY_CONFIG.exporterProtocol;
 
-  const sampler = (overrides?.sampler ??
-    (env.OTEL_TRACES_SAMPLER as SamplerName | undefined) ??
-    DEFAULT_OBSERVABILITY_CONFIG.sampler) as SamplerName;
+  const sampler =
+    overrides?.sampler ??
+    parseSamplerName(env.OTEL_TRACES_SAMPLER) ??
+    DEFAULT_OBSERVABILITY_CONFIG.sampler;
 
   const samplerArg =
     overrides?.samplerArg ??
@@ -159,7 +180,13 @@ function resolveExporterConfig(
     ...overrides?.otlpHeaders,
   };
 
-  return {exporterProtocol, sampler, samplerArg, otlpEndpoint, otlpHeaders};
+  return {
+    exporterProtocol,
+    sampler,
+    samplerArg,
+    otlpEndpoint,
+    otlpHeaders,
+  };
 }
 
 export function resolveBootstrapConfig(

packages/observability/src/config/validate-config.ts

@@ -4,25 +4,15 @@ import {
   InstrumentationModuleRequirement,
   InstrumentationName,
   INSTRUMENTATION_MODULE_REQUIREMENTS,
+  isModuleInstalled,
 } from '../profiles/instrumentations';
 
 function assertDependencyInstalled(moduleName: string, message: string): void {
-  try {
-    require.resolve(moduleName);
-  } catch {
+  if (!isModuleInstalled(moduleName)) {
     throw new Error(message);
   }
 }
 
-function isDependencyInstalled(moduleName: string): boolean {
-  try {
-    require.resolve(moduleName);
-    return true;
-  } catch {
-    return false;
-  }
-}
-
 export function validateObservabilityConfig(
   config: ResolvedObservabilityConfig,
 ): void {
@@ -72,7 +62,7 @@ function validateInstrumentationRequirements(
       continue;
     }
 
-    if (!requirement.modules.some(isDependencyInstalled)) {
+    if (!requirement.modules.some(isModuleInstalled)) {
       throw new Error(
         `Install one of the optional peer dependencies "${requirement.modules.join(
           '" or "',

packages/observability/src/profiles/datadog.profile.ts

@@ -1,8 +1,6 @@
 import {
   ObservabilityProfileName,
   ObservabilityProfile,
-  ProfileBootstrapContext,
-  ProfileInitResult,
   ResolvedObservabilityConfig,
 } from '../types';
 import {BaseOtlpObservabilityProfile} from './otlp.profile';
@@ -40,11 +38,4 @@ export class DatadogObservabilityProfile
       },
     };
   }
-
-  initialize(
-    config: ResolvedObservabilityConfig,
-    context: ProfileBootstrapContext,
-  ): ProfileInitResult {
-    return super.initialize(this.applyDefaults(config), context);
-  }
 }

packages/observability/src/profiles/instrumentations.ts

@@ -42,7 +42,7 @@ export type InstrumentationLoader = (
   name: InstrumentationName,
 ) => ManagedInstrumentation[];
 
-function isModuleInstalled(moduleName: string): boolean {
+export function isModuleInstalled(moduleName: string): boolean {
   try {
     require.resolve(moduleName);
     return true;

packages/observability/src/profiles/newrelic.profile.ts

@@ -1,8 +1,6 @@
 import {
   ObservabilityProfileName,
   ObservabilityProfile,
-  ProfileBootstrapContext,
-  ProfileInitResult,
   ResolvedObservabilityConfig,
 } from '../types';
 import {BaseOtlpObservabilityProfile} from './otlp.profile';
@@ -40,11 +38,4 @@ export class NewRelicObservabilityProfile
       },
     };
   }
-
-  initialize(
-    config: ResolvedObservabilityConfig,
-    context: ProfileBootstrapContext,
-  ): ProfileInitResult {
-    return super.initialize(this.applyDefaults(config), context);
-  }
 }