apply review suggestions

Co-authored-by: Marc Pichler <[email protected]>

Author: raphael-theriault-swi

experimental/packages/otlp-exporter-base/src/configuration/convert-legacy-node-http-options.ts

@@ -32,24 +32,13 @@ function convertLegacyAgentOptions(
     return config.httpAgentOptions;
   }
 
-  // populate keepAlive for use with new settings
-  if (config?.keepAlive != null) {
-    if (config.httpAgentOptions != null) {
-      if (config.httpAgentOptions.keepAlive == null) {
-        // specific setting is not set, populate with non-specific setting.
-        config.httpAgentOptions.keepAlive = config.keepAlive;
-      }
-      // do nothing, use specific setting otherwise
-    } else {
-      // populate specific option if AgentOptions does not exist.
-      config.httpAgentOptions = {
-        keepAlive: config.keepAlive,
-      };
-    }
+  let legacy = config.httpAgentOptions;
+  if (config.keepAlive != null) {
+    legacy = { keepAlive: config.keepAlive, ...legacy };
   }
 
-  if (config.httpAgentOptions != null) {
-    return httpAgentFactoryFromOptions(config.httpAgentOptions);
+  if (legacy != null) {
+    return httpAgentFactoryFromOptions(legacy);
   } else {
     return undefined;
   }
@@ -80,7 +69,7 @@ export function convertLegacyHttpOptions(
       concurrencyLimit: config.concurrencyLimit,
       timeoutMillis: config.timeoutMillis,
       compression: config.compression,
-      agent: convertLegacyAgentOptions(config),
+      agentFactory: convertLegacyAgentOptions(config),
     },
     getHttpConfigurationFromEnvironment(signalIdentifier, signalResourcePath),
     getHttpConfigurationDefaults(requiredHeaders, signalResourcePath)

experimental/packages/otlp-exporter-base/src/configuration/otlp-http-configuration.ts

@@ -34,7 +34,7 @@ export type HttpAgentFactory =
 export interface OtlpHttpConfiguration extends OtlpSharedConfiguration {
   url: string;
   headers: () => Record<string, string>;
-  agent: HttpAgentFactory;
+  agentFactory: HttpAgentFactory;
 }
 
 function mergeHeaders(
@@ -81,8 +81,8 @@ export function httpAgentFactoryFromOptions(
   options: http.AgentOptions | https.AgentOptions
 ): HttpAgentFactory {
   return async protocol => {
-    const { Agent } =
-      protocol === 'http:' ? await import('http') : await import('https');
+    const module = protocol === 'http:' ? import('http') : import('https');
+    const { Agent } = await module;
     return new Agent(options);
   };
 }
@@ -112,10 +112,10 @@ export function mergeOtlpHttpConfigurationWithDefaults(
       validateUserProvidedUrl(userProvidedConfiguration.url) ??
       fallbackConfiguration.url ??
       defaultConfiguration.url,
-    agent:
-      userProvidedConfiguration.agent ??
-      fallbackConfiguration.agent ??
-      defaultConfiguration.agent,
+    agentFactory:
+      userProvidedConfiguration.agentFactory ??
+      fallbackConfiguration.agentFactory ??
+      defaultConfiguration.agentFactory,
   };
 }
 
@@ -127,6 +127,6 @@ export function getHttpConfigurationDefaults(
     ...getSharedConfigurationDefaults(),
     headers: () => requiredHeaders,
     url: 'http://localhost:4318/' + signalResourcePath,
-    agent: httpAgentFactoryFromOptions({ keepAlive: true }),
+    agentFactory: httpAgentFactoryFromOptions({ keepAlive: true }),
   };
 }

experimental/packages/otlp-exporter-base/src/transport/http-exporter-transport.ts

@@ -14,21 +14,19 @@
  * limitations under the License.
  */
 
-import type {
-  HttpRequestParameters,
-  sendWithHttp,
-} from './http-transport-types';
+import type { HttpRequestParameters } from './http-transport-types';
 
 // NOTE: do not change these type imports to actual imports. Doing so WILL break `@opentelemetry/instrumentation-http`,
 // as they'd be imported before the http/https modules can be wrapped.
 import type * as https from 'https';
 import type * as http from 'http';
 import { ExportResponse } from '../export-response';
 import { IExporterTransport } from '../exporter-transport';
+import { sendWithHttp } from './http-transport-utils';
 
 interface Utils {
   agent: http.Agent | https.Agent;
-  send: sendWithHttp;
+  request: typeof http.request | typeof https.request;
 }
 
 class HttpExporterTransport implements IExporterTransport {
@@ -37,10 +35,11 @@ class HttpExporterTransport implements IExporterTransport {
   constructor(private _parameters: HttpRequestParameters) {}
 
   async send(data: Uint8Array, timeoutMillis: number): Promise<ExportResponse> {
-    const { agent, send } = await this._loadUtils();
+    const { agent, request } = await this._loadUtils();
 
     return new Promise<ExportResponse>(resolve => {
-      send(
+      sendWithHttp(
+        request,
         this._parameters,
         agent,
         data,
@@ -60,23 +59,25 @@ class HttpExporterTransport implements IExporterTransport {
     let utils = this._utils;
 
     if (utils === null) {
-      // Lazy import to ensure that http/https is not required before instrumentations can wrap it.
-      const imported = await import('./http-transport-utils.js');
-
+      const protocol = new URL(this._parameters.url).protocol;
       utils = this._utils = {
-        agent: await this._parameters.agent(
-          new URL(this._parameters.url).protocol
-        ),
-        // @ts-expect-error dynamic imports are never transpiled, but named exports only work when
-        // dynamically importing an esm file, and the utils file might be transpiled to cjs
-        send: imported.sendWithHttp ?? imported.default.sendWithHttp,
+        agent: await this._parameters.agentFactory(protocol),
+        request: await requestFactory(protocol),
       };
     }
 
     return utils;
   }
 }
 
+async function requestFactory(
+  protocol: string
+): Promise<typeof http.request | typeof https.request> {
+  const module = protocol === 'http:' ? import('http') : import('https');
+  const { request } = await module;
+  return request;
+}
+
 export function createHttpExporterTransport(
   parameters: HttpRequestParameters
 ): IExporterTransport {

experimental/packages/otlp-exporter-base/src/transport/http-transport-types.ts

@@ -14,22 +14,11 @@
  * limitations under the License.
  */
 
-import type * as http from 'http';
-import type * as https from 'https';
-import { ExportResponse } from '../export-response';
 import { HttpAgentFactory } from '../configuration/otlp-http-configuration';
 
-export type sendWithHttp = (
-  params: HttpRequestParameters,
-  agent: http.Agent | https.Agent,
-  data: Uint8Array,
-  onDone: (response: ExportResponse) => void,
-  timeoutMillis: number
-) => void;
-
 export interface HttpRequestParameters {
   url: string;
   headers: () => Record<string, string>;
   compression: 'gzip' | 'none';
-  agent: HttpAgentFactory;
+  agentFactory: HttpAgentFactory;
 }

experimental/packages/otlp-exporter-base/src/transport/http-transport-utils.ts

@@ -13,8 +13,8 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-import * as http from 'http';
-import * as https from 'https';
+import type * as http from 'http';
+import type * as https from 'https';
 import * as zlib from 'zlib';
 import { Readable } from 'stream';
 import { HttpRequestParameters } from './http-transport-types';
@@ -27,13 +27,15 @@ import { OTLPExporterError } from '../types';
 
 /**
  * Sends data using http
+ * @param requestFunction
  * @param params
  * @param agent
  * @param data
  * @param onDone
  * @param timeoutMillis
  */
 export function sendWithHttp(
+  request: typeof https.request | typeof http.request,
   params: HttpRequestParameters,
   agent: http.Agent | https.Agent,
   data: Uint8Array,
@@ -53,8 +55,6 @@ export function sendWithHttp(
     agent: agent,
   };
 
-  const request = parsedUrl.protocol === 'http:' ? http.request : https.request;
-
   const req = request(options, (res: http.IncomingMessage) => {
     const responseData: Buffer[] = [];
     res.on('data', chunk => responseData.push(chunk));

experimental/packages/otlp-exporter-base/test/common/configuration/otlp-http-configuration.test.ts

@@ -28,7 +28,7 @@ describe('mergeOtlpHttpConfigurationWithDefaults', function () {
     compression: 'none',
     concurrencyLimit: 2,
     headers: () => ({ 'User-Agent': 'default-user-agent' }),
-    agent: () => null!,
+    agentFactory: () => null!,
   };
 
   describe('headers', function () {

experimental/packages/otlp-exporter-base/test/node/configuration/convert-legacy-node-otlp-http-options.test.ts

@@ -52,7 +52,7 @@ describe('convertLegacyHttpOptions', function () {
     );
 
     // assert
-    assert.strictEqual(options.agent, factory);
+    assert.strictEqual(options.agentFactory, factory);
   });
 
   it('should keep specific keepAlive', async () => {
@@ -65,7 +65,7 @@ describe('convertLegacyHttpOptions', function () {
       'v1/signal',
       {}
     );
-    const agent = (await options.agent('https:')) as https.Agent;
+    const agent = (await options.agentFactory('https:')) as https.Agent;
 
     // assert
     assert.ok(agent.options.keepAlive);
@@ -85,7 +85,7 @@ describe('convertLegacyHttpOptions', function () {
       'v1/signal',
       {}
     );
-    const agent = (await options.agent('https:')) as https.Agent;
+    const agent = (await options.agentFactory('https:')) as https.Agent;
 
     // assert
     assert.ok(agent.options.keepAlive);

experimental/packages/otlp-exporter-base/test/node/http-exporter-transport.test.ts

@@ -58,7 +58,7 @@ describe('HttpExporterTransport', function () {
         url: 'http://localhost:8080',
         headers: () => ({}),
         compression: 'none',
-        agent: () => new http.Agent(),
+        agentFactory: () => new http.Agent(),
       });
 
       // act
@@ -95,7 +95,7 @@ describe('HttpExporterTransport', function () {
         url: 'http://jocajhost:8080',
         headers: () => ({}),
         compression: 'none',
-        agent: protocol => {
+        agentFactory: protocol => {
           assert.strictEqual(protocol, 'http:');
           return new SedAgent();
         },
@@ -124,7 +124,7 @@ describe('HttpExporterTransport', function () {
         url: 'http://localhost:8080',
         headers: () => ({}),
         compression: 'none',
-        agent: () => new http.Agent(),
+        agentFactory: () => new http.Agent(),
       });
 
       // act
@@ -148,7 +148,7 @@ describe('HttpExporterTransport', function () {
         url: 'http://localhost:8080',
         headers: () => ({}),
         compression: 'none',
-        agent: () => new http.Agent(),
+        agentFactory: () => new http.Agent(),
       });
 
       const result = await transport.send(sampleRequestData, 1000);
@@ -173,7 +173,7 @@ describe('HttpExporterTransport', function () {
         url: 'http://localhost:8080',
         headers: () => ({}),
         compression: 'none',
-        agent: () => new http.Agent(),
+        agentFactory: () => new http.Agent(),
       });
 
       // act
@@ -210,7 +210,7 @@ describe('HttpExporterTransport', function () {
         url: 'http://localhost:8080',
         headers: () => ({}),
         compression: 'none',
-        agent: () => new http.Agent(),
+        agentFactory: () => new http.Agent(),
       });
 
       // act
@@ -243,7 +243,7 @@ describe('HttpExporterTransport', function () {
         url: 'http://localhost:8080',
         headers: () => ({}),
         compression: 'none',
-        agent: () => new http.Agent(),
+        agentFactory: () => new http.Agent(),
       });
 
       // act
@@ -264,7 +264,7 @@ describe('HttpExporterTransport', function () {
         url: 'http://example.test',
         headers: () => ({}),
         compression: 'none',
-        agent: () => new http.Agent(),
+        agentFactory: () => new http.Agent(),
       });
 
       // act
@@ -314,7 +314,7 @@ describe('HttpExporterTransport', function () {
         url: 'http://localhost:8080',
         headers: () => ({ foo: 'foo-value', bar: 'bar-value' }),
         compression: 'none',
-        agent: () => new http.Agent(),
+        agentFactory: () => new http.Agent(),
       });
 
       // assert
@@ -363,7 +363,7 @@ describe('HttpExporterTransport', function () {
         url: 'http://localhost:8080',
         headers: () => ({ foo: 'foo-value', bar: 'bar-value' }),
         compression: 'gzip',
-        agent: () => new http.Agent(),
+        agentFactory: () => new http.Agent(),
       });
 
       // act

experimental/packages/otlp-exporter-base/test/node/otlp-http-export-delegate.test.ts

@@ -55,7 +55,7 @@ describe('createOtlpHttpExportDelegate', function () {
     const delegate = createOtlpHttpExportDelegate(
       {
         url: 'http://localhost:8083',
-        agent: () => new http.Agent(),
+        agentFactory: () => new http.Agent(),
         compression: 'none',
         concurrencyLimit: 30,
         headers: () => ({}),

experimental/packages/otlp-exporter-base/tsconfig.json

@@ -19,12 +19,5 @@
     {
       "path": "../otlp-transformer"
     }
-  ],
-  "ts-node": {
-    "experimentalResolver": true,
-    "compilerOptions": {
-      "module": "commonjs",
-      "moduleResolution": "node10"
-    }
-  }
+  ]
 }