WIP: refactor

apps/app/src/features/opentelemetry/server/node-sdk.spec.ts

@@ -1,3 +1,4 @@
+import { Resource } from '@opentelemetry/resources';
 import { NodeSDK } from '@opentelemetry/sdk-node';
 import {
   beforeEach, describe, expect, it, vi,
@@ -6,8 +7,9 @@ import {
 import { configManager } from '~/server/service/config-manager';
 
 import { detectServiceInstanceId, initInstrumentation } from './node-sdk';
+import { getSdkInstance, resetSdkInstance } from './node-sdk.testing';
 
-// Mock configManager
+// Only mock configManager as it's external to what we're testing
 vi.mock('~/server/service/config-manager', () => ({
   configManager: {
     getConfig: vi.fn(),
@@ -15,38 +17,11 @@ vi.mock('~/server/service/config-manager', () => ({
   },
 }));
 
-// Mock NodeSDK
-let mockSdkInstance: any;
-vi.mock('@opentelemetry/sdk-node', () => ({
-  NodeSDK: vi.fn().mockImplementation(() => {
-    const instance = {
-      _resource: {
-        attributes: () => ({
-          'service.instance.id': 'default-instance-id',
-        }),
-      },
-      start: vi.fn(),
-    };
-    mockSdkInstance = instance;
-    return instance;
-  }),
-}));
-
-// Mock node-sdk-configuration
-vi.mock('./node-sdk-configuration', () => ({
-  generateNodeSDKConfiguration: vi.fn().mockImplementation((serviceInstanceId?: string) => ({
-    resource: {
-      attributes: () => ({
-        'service.instance.id': serviceInstanceId ?? 'default-instance-id',
-      }),
-    },
-  })),
-}));
-
 describe('node-sdk', () => {
   beforeEach(() => {
     vi.clearAllMocks();
-    mockSdkInstance = undefined;
+    vi.resetModules();
+    resetSdkInstance();
 
     // Reset configManager mock implementation
     (configManager.getConfig as any).mockImplementation((key: string) => {
@@ -58,37 +33,73 @@ describe('node-sdk', () => {
   });
 
   describe('detectServiceInstanceId', () => {
-    it('should update service.instance.id in the resource attributes', async() => {
+    it('should update service.instance.id when app:serviceInstanceId is available', async() => {
       // Initialize SDK first
       await initInstrumentation();
 
-      // Get initial resource attributes
-      const initialAttributes = mockSdkInstance._resource.attributes();
-      expect(initialAttributes['service.instance.id']).toBe('default-instance-id');
+      // Get instance for testing
+      const sdkInstance = getSdkInstance();
+      expect(sdkInstance).toBeDefined();
+      expect(sdkInstance).toBeInstanceOf(NodeSDK);
+
+      // Verify initial state (service.instance.id should not be set)
+      const resource = (sdkInstance as any)._resource;
+      expect(resource).toBeInstanceOf(Resource);
+      expect(resource.attributes['service.instance.id']).toBeUndefined();
 
       // Call detectServiceInstanceId
       await detectServiceInstanceId();
 
-      // Get updated resource attributes
-      const updatedAttributes = mockSdkInstance._resource.attributes();
-      expect(updatedAttributes['service.instance.id']).toBe('test-instance-id');
+      // Verify that resource was updated with app:serviceInstanceId
+      const updatedResource = (sdkInstance as any)._resource;
+      expect(updatedResource.attributes['service.instance.id']).toBe('test-instance-id');
     });
 
-    it('should not update resource if instrumentation is disabled', async() => {
+    it('should update service.instance.id with otel:serviceInstanceId if available', async() => {
+      // Mock otel:serviceInstanceId is available
+      (configManager.getConfig as any).mockImplementation((key: string) => {
+        if (key === 'otel:enabled') return true;
+        if (key === 'otel:serviceInstanceId') return 'otel-instance-id';
+        if (key === 'app:serviceInstanceId') return 'test-instance-id';
+        return undefined;
+      });
+
+      // Initialize SDK
+      await initInstrumentation();
+
+      // Verify initial state
+      const sdkInstance = getSdkInstance();
+      const resource = (sdkInstance as any)._resource;
+      expect(resource.attributes['service.instance.id']).toBeUndefined();
+
+      // Call detectServiceInstanceId
+      await detectServiceInstanceId();
+
+      // Verify that otel:serviceInstanceId was used
+      const updatedResource = (sdkInstance as any)._resource;
+      expect(updatedResource.attributes['service.instance.id']).toBe('otel-instance-id');
+    });
+
+    it('should not create SDK instance if instrumentation is disabled', async() => {
       // Mock instrumentation as disabled
       (configManager.getConfig as any).mockImplementation((key: string) => {
         if (key === 'otel:enabled') return false;
         return undefined;
       });
 
-      // Initialize SDK first
+      // Initialize SDK
       await initInstrumentation();
 
+      // Verify that no SDK instance was created
+      const sdkInstance = getSdkInstance();
+      expect(sdkInstance).toBeUndefined();
+
       // Call detectServiceInstanceId
       await detectServiceInstanceId();
 
-      // Verify that SDK instance was not created
-      expect(mockSdkInstance).toBeUndefined();
+      // Verify that still no SDK instance exists
+      const updatedSdkInstance = getSdkInstance();
+      expect(updatedSdkInstance).toBeUndefined();
     });
   });
 });

apps/app/src/features/opentelemetry/server/node-sdk.testing.ts

@@ -0,0 +1,24 @@
+/**
+ * This module provides testing APIs for node-sdk.ts
+ * It should be imported only in test files
+ */
+
+import type { NodeSDK } from '@opentelemetry/sdk-node';
+
+import { __testing__ } from './node-sdk';
+
+/**
+ * Get the current SDK instance
+ * This function should only be used in tests
+ */
+export const getSdkInstance = (): NodeSDK | undefined => {
+  return __testing__.getSdkInstance();
+};
+
+/**
+ * Reset the SDK instance
+ * This function should be used to clean up between tests
+ */
+export const resetSdkInstance = (): void => {
+  __testing__.reset();
+};

apps/app/src/features/opentelemetry/server/node-sdk.ts

@@ -1,4 +1,5 @@
 import { ConfigSource } from '@growi/core/dist/interfaces';
+import { Resource } from '@opentelemetry/resources';
 import type { NodeSDK } from '@opentelemetry/sdk-node';
 
 import { configManager } from '~/server/service/config-manager';
@@ -6,8 +7,7 @@ import loggerFactory from '~/utils/logger';
 
 const logger = loggerFactory('growi:opentelemetry:server');
 
-
-let sdkInstance: NodeSDK;
+let sdkInstance: NodeSDK | undefined;
 
 /**
  * Overwrite "OTEL_SDK_DISABLED" env var before sdk.start() is invoked if needed.
@@ -33,7 +33,6 @@ function overwriteSdkDisabled(): void {
     process.env.OTEL_SDK_DISABLED = 'true';
     return;
   }
-
 }
 
 export const initInstrumentation = async(): Promise<void> => {
@@ -49,7 +48,6 @@ export const initInstrumentation = async(): Promise<void> => {
 
   const instrumentationEnabled = configManager.getConfig('otel:enabled', ConfigSource.env);
   if (instrumentationEnabled) {
-
     logger.info(`GROWI now collects anonymous telemetry.
 
 This data is used to help improve GROWI, but you can opt-out at any time.
@@ -75,22 +73,32 @@ For more information, see https://docs.growi.org/en/admin-guide/admin-cookbook/t
 export const detectServiceInstanceId = async(): Promise<void> => {
   const instrumentationEnabled = configManager.getConfig('otel:enabled', ConfigSource.env);
 
-  if (instrumentationEnabled) {
+  if (instrumentationEnabled && sdkInstance != null) {
     const { generateNodeSDKConfiguration } = await import('./node-sdk-configuration');
 
     const serviceInstanceId = configManager.getConfig('otel:serviceInstanceId')
       ?? configManager.getConfig('app:serviceInstanceId');
 
-    // overwrite resource
-    const updatedResource = generateNodeSDKConfiguration(serviceInstanceId).resource;
-    (sdkInstance as any)._resource = updatedResource;
+    // Update resource with new service instance id
+    const newConfig = generateNodeSDKConfiguration(serviceInstanceId);
+    if (newConfig.resource instanceof Resource) {
+      (sdkInstance as any)._resource = newConfig.resource;
+    }
   }
 };
 
 export const startOpenTelemetry = (): void => {
   const instrumentationEnabled = configManager.getConfig('otel:enabled', ConfigSource.env);
 
-  if (instrumentationEnabled) {
+  if (instrumentationEnabled && sdkInstance != null) {
     sdkInstance.start();
   }
 };
+
+// For testing purposes only
+export const __testing__ = {
+  getSdkInstance: () => sdkInstance,
+  reset: () => {
+    sdkInstance = undefined;
+  },
+};