From e99a62b5f45edad63427ddf104509fd5d10291db Mon Sep 17 00:00:00 2001 From: Marc Pichler Date: Fri, 20 Feb 2026 10:51:50 +0100 Subject: [PATCH] refactor(otlp-exporter-base): promisify sendWithHttp --- experimental/CHANGELOG.md | 2 + .../src/transport/http-exporter-transport.ts | 25 +-- .../src/transport/http-transport-utils.ts | 171 +++++++++--------- .../test/node/http-transport-utils.test.ts | 44 ++--- 4 files changed, 112 insertions(+), 130 deletions(-) diff --git a/experimental/CHANGELOG.md b/experimental/CHANGELOG.md index 261f772d07a..956d67bbca0 100644 --- a/experimental/CHANGELOG.md +++ b/experimental/CHANGELOG.md @@ -24,6 +24,8 @@ For notes on migrating to 2.x / 0.200.x see [the upgrade guide](doc/upgrade-to-2 ### :house: Internal +* refactor(otlp-exporter-base): promisify sendWithHttp() [#????](https://github.com/open-telemetry/opentelemetry-js/pull/6412) @pichlermarc + ## 0.212.0 ### :boom: Breaking Changes diff --git a/experimental/packages/otlp-exporter-base/src/transport/http-exporter-transport.ts b/experimental/packages/otlp-exporter-base/src/transport/http-exporter-transport.ts index 9a2694074cd..1012c4a7335 100644 --- a/experimental/packages/otlp-exporter-base/src/transport/http-exporter-transport.ts +++ b/experimental/packages/otlp-exporter-base/src/transport/http-exporter-transport.ts @@ -29,21 +29,16 @@ class HttpExporterTransport implements IExporterTransport { const { agent, request } = await this._loadUtils(); const headers = await this._parameters.headers(); - return new Promise(resolve => { - sendWithHttp( - request, - this._parameters.url, - headers, - this._parameters.compression, - this._parameters.userAgent, - agent, - data, - result => { - resolve(result); - }, - timeoutMillis - ); - }); + return sendWithHttp( + request, + this._parameters.url, + headers, + this._parameters.compression, + this._parameters.userAgent, + agent, + data, + timeoutMillis + ); } shutdown() { diff --git a/experimental/packages/otlp-exporter-base/src/transport/http-transport-utils.ts b/experimental/packages/otlp-exporter-base/src/transport/http-transport-utils.ts index 6f5c8729ab1..68fb8f0eac6 100644 --- a/experimental/packages/otlp-exporter-base/src/transport/http-transport-utils.ts +++ b/experimental/packages/otlp-exporter-base/src/transport/http-transport-utils.ts @@ -25,7 +25,6 @@ const DEFAULT_USER_AGENT = `OTel-OTLP-Exporter-JavaScript/${VERSION}`; * @param userAgent * @param agent * @param data - * @param onDone * @param timeoutMillis */ export function sendWithHttp( @@ -36,105 +35,111 @@ export function sendWithHttp( userAgent: string | undefined, agent: http.Agent | https.Agent, data: Uint8Array, - onDone: (response: ExportResponse) => void, timeoutMillis: number -): void { - const parsedUrl = new URL(url); +): Promise { + return new Promise(resolve => { + const parsedUrl = new URL(url); - if (userAgent) { - headers['User-Agent'] = `${userAgent} ${DEFAULT_USER_AGENT}`; - } else { - headers['User-Agent'] = DEFAULT_USER_AGENT; - } + if (userAgent) { + headers['User-Agent'] = `${userAgent} ${DEFAULT_USER_AGENT}`; + } else { + headers['User-Agent'] = DEFAULT_USER_AGENT; + } - const options: http.RequestOptions | https.RequestOptions = { - hostname: parsedUrl.hostname, - port: parsedUrl.port, - path: parsedUrl.pathname, - method: 'POST', - headers, - agent, - }; - - const req = request(options, (res: http.IncomingMessage) => { - const responseData: Buffer[] = []; - res.on('data', chunk => responseData.push(chunk)); - - res.on('end', () => { - if (res.statusCode && res.statusCode <= 299) { - onDone({ - status: 'success', - data: Buffer.concat(responseData), - }); - } else if (res.statusCode && isExportHTTPErrorRetryable(res.statusCode)) { - onDone({ - status: 'retryable', - retryInMillis: parseRetryAfterToMills(res.headers['retry-after']), - }); - } else { - const error = new OTLPExporterError( - res.statusMessage, - res.statusCode, - Buffer.concat(responseData).toString() - ); - onDone({ - status: 'failure', - error, - }); - } + const options: http.RequestOptions | https.RequestOptions = { + hostname: parsedUrl.hostname, + port: parsedUrl.port, + path: parsedUrl.pathname, + method: 'POST', + headers, + agent, + }; + + const req = request(options, (res: http.IncomingMessage) => { + const responseData: Buffer[] = []; + res.on('data', chunk => responseData.push(chunk)); + + res.on('end', () => { + if (res.statusCode && res.statusCode <= 299) { + resolve({ + status: 'success', + data: Buffer.concat(responseData), + }); + } else if ( + res.statusCode && + isExportHTTPErrorRetryable(res.statusCode) + ) { + resolve({ + status: 'retryable', + retryInMillis: parseRetryAfterToMills(res.headers['retry-after']), + }); + } else { + const error = new OTLPExporterError( + res.statusMessage, + res.statusCode, + Buffer.concat(responseData).toString() + ); + resolve({ + status: 'failure', + error, + }); + } + }); + + res.on('error', (error: Error) => { + // Note: 'end' may still be emitted after 'error' on the same response object, since we're resolving a promise, + // the first call to resolve() will determine the final state. + if (res.statusCode && res.statusCode <= 299) { + // If the response is successful but an error occurs while reading the response, + // we consider it a success since the data has been sent successfully. + resolve({ + status: 'success', + }); + } else if ( + res.statusCode && + isExportHTTPErrorRetryable(res.statusCode) + ) { + resolve({ + status: 'retryable', + error: error, + retryInMillis: parseRetryAfterToMills(res.headers['retry-after']), + }); + } else { + resolve({ + status: 'failure', + error, + }); + } + }); }); - res.on('error', (error: Error) => { - // Note: 'end' may still be emitted after 'error' on the same response object. - // However, since onDone maps to a Promise resolve/reject, only the first call takes effect. - // This will be addressed in https://github.com/open-telemetry/opentelemetry-js/issues/5990 - if (res.statusCode && res.statusCode <= 299) { - // If the response is successful but an error occurs while reading the response, - // we consider it a success since the data has been sent successfully. - onDone({ - status: 'success', - }); - } else if (res.statusCode && isExportHTTPErrorRetryable(res.statusCode)) { - onDone({ + req.setTimeout(timeoutMillis, () => { + req.destroy(); + resolve({ + status: 'retryable', + error: new Error('Request timed out'), + }); + }); + + req.on('error', (error: Error) => { + if (isHttpTransportNetworkErrorRetryable(error)) { + resolve({ status: 'retryable', - error: error, - retryInMillis: parseRetryAfterToMills(res.headers['retry-after']), + error, }); } else { - onDone({ + resolve({ status: 'failure', error, }); } }); - }); - - req.setTimeout(timeoutMillis, () => { - req.destroy(); - onDone({ - status: 'retryable', - error: new Error('Request timed out'), - }); - }); - req.on('error', (error: Error) => { - if (isHttpTransportNetworkErrorRetryable(error)) { - onDone({ - status: 'retryable', - error, - }); - } else { - onDone({ + compressAndSend(req, compression, data, (error: Error) => { + resolve({ status: 'failure', error, }); - } - }); - - compressAndSend(req, compression, data, (error: Error) => { - onDone({ - status: 'failure', - error, }); }); } diff --git a/experimental/packages/otlp-exporter-base/test/node/http-transport-utils.test.ts b/experimental/packages/otlp-exporter-base/test/node/http-transport-utils.test.ts index 6a613b53dcb..ae377bdac24 100644 --- a/experimental/packages/otlp-exporter-base/test/node/http-transport-utils.test.ts +++ b/experimental/packages/otlp-exporter-base/test/node/http-transport-utils.test.ts @@ -39,9 +39,8 @@ describe('sendWithHttp', function () { sentUserAgent = ''; }); - it('sends a request setting the default user-agent header', function (done) { - let firstCallback = true; - sendWithHttp( + it('sends a request setting the default user-agent header', async function () { + await sendWithHttp( requestFn, 'http://localhost:8080', {}, @@ -49,26 +48,16 @@ describe('sendWithHttp', function () { undefined, new http.Agent(), Buffer.from([1, 2, 3]), - // TODO: the `onDone` callback is called twice because there are two error handlers - // - first is attached on the request created in `sendWithHttp` - // - second is attached on the pipe within `compressAndSend` - () => { - if (firstCallback) { - firstCallback = false; - assert.strictEqual( - sentUserAgent, - `OTel-OTLP-Exporter-JavaScript/${VERSION}` - ); - done(); - } - }, 100 ); + assert.strictEqual( + sentUserAgent, + `OTel-OTLP-Exporter-JavaScript/${VERSION}` + ); }); - it('sends a request prepending the provided user-agent to the default one', function (done) { - let firstCallback = true; - sendWithHttp( + it('sends a request prepending the provided user-agent to the default one', async function () { + await sendWithHttp( requestFn, 'http://localhost:8080', {}, @@ -76,20 +65,11 @@ describe('sendWithHttp', function () { 'Transport-User-Agent/1.2.3', new http.Agent(), Buffer.from([1, 2, 3]), - // TODO: the `onDone` callback is called twice because there are two error handlers - // - first is attached on the request created in `sendWithHttp` - // - second is attached on the pipe within `compressAndSend` - () => { - if (firstCallback) { - firstCallback = false; - assert.strictEqual( - sentUserAgent, - `Transport-User-Agent/1.2.3 OTel-OTLP-Exporter-JavaScript/${VERSION}` - ); - done(); - } - }, 100 ); + assert.strictEqual( + sentUserAgent, + `Transport-User-Agent/1.2.3 OTel-OTLP-Exporter-JavaScript/${VERSION}` + ); }); });