From 08e125651c367794a6a0901ff8476d3e98d8c5ff Mon Sep 17 00:00:00 2001 From: kgilpin Date: Fri, 3 Jul 2026 17:27:50 -0400 Subject: [PATCH] fix(telemetry): make ApplicationInsights flush await in-flight requests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The application-insights backend's requests are fire-and-forget and its flush() was a no-op, so the response/error handlers (which console.warn) ran after the test finished — leaking warnings into the next test and racing assertions ("Cannot log after tests are done"; the 500 test saw the previous test's error warning instead of its own). Track pending requests and make flush() resolve once they settle (delete from the set after the warn fires), mirroring SplunkBackend. The two warning tests now await flush() before asserting — deterministic, because the warning is guaranteed to have run — and afterEach restoreAllMocks() isolates the console.warn spy across tests. Co-Authored-By: Claude Opus 4.8 --- .../src/backends/application-insights.ts | 32 +++++++++++++++++-- .../backends/application-insights.spec.ts | 13 +++++--- 2 files changed, 39 insertions(+), 6 deletions(-) diff --git a/packages/telemetry/src/backends/application-insights.ts b/packages/telemetry/src/backends/application-insights.ts index 8b40090e7c..dcb6485153 100644 --- a/packages/telemetry/src/backends/application-insights.ts +++ b/packages/telemetry/src/backends/application-insights.ts @@ -1,3 +1,4 @@ +import type { ClientRequest } from 'node:http'; import * as https from 'node:https'; import os from 'node:os'; @@ -8,6 +9,8 @@ import type { TelemetryData, } from '../types'; +const MaxFlushTime = 5000; // 5 seconds + // This key is meant to be publically shared. However, I'm adding a simple // obfuscation to mitigate key scraping bots on GitHub. The key is split on // hypens and base64 encoded without padding. @@ -26,6 +29,7 @@ export class ApplicationInsightsBackend implements TelemetryBackend { private readonly productName: string; private readonly hostname: string; private readonly uname: string; + private pendingRequests = new Set(); constructor( userId: string, @@ -61,9 +65,30 @@ export class ApplicationInsightsBackend implements TelemetryBackend { this._post([envelope]); } + // Resolve once all in-flight requests have settled (their response/error + // handlers, including any warning, have run), so callers — and tests — can + // await the full request lifecycle rather than just the request being sent. flush(callback?: FlushCallback): void { - // No batching, so nothing to flush. Just call callback. - callback?.(); + const startTime = process.hrtime.bigint(); + + const checkPending = () => { + if (this.pendingRequests.size === 0) { + callback?.(); + return; + } + + const elapsedMs = Number((process.hrtime.bigint() - startTime) / BigInt(1_000_000)); + if (elapsedMs > MaxFlushTime) { + console.warn(`ApplicationInsightsBackend: Flush timed out after ${MaxFlushTime}ms`); + this.pendingRequests.forEach((req) => req.destroy()); + callback?.(); + return; + } + + setTimeout(checkPending, 10); + }; + + checkPending(); } private _post(envelopes: unknown[]): void { @@ -85,10 +110,13 @@ export class ApplicationInsightsBackend implements TelemetryBackend { if (res.statusCode && (res.statusCode < 200 || res.statusCode >= 300)) { console.warn(`ApplicationInsightsBackend: Failed to send telemetry event. Status: ${res.statusCode}`); } + this.pendingRequests.delete(req); }); }); + this.pendingRequests.add(req); req.on('error', (e) => { console.warn('Error sending telemetry data to Application Insights', e); + this.pendingRequests.delete(req); }); req.write(body); req.end(); diff --git a/packages/telemetry/tests/backends/application-insights.spec.ts b/packages/telemetry/tests/backends/application-insights.spec.ts index 1c64db0157..a130b52aa8 100644 --- a/packages/telemetry/tests/backends/application-insights.spec.ts +++ b/packages/telemetry/tests/backends/application-insights.spec.ts @@ -34,6 +34,7 @@ describe('ApplicationInsightsBackend', () => { afterEach(() => { nock.cleanAll(); nock.enableNetConnect(); + jest.restoreAllMocks(); }); const waitForRequest = async (scope: nock.Scope) => { @@ -118,7 +119,7 @@ describe('ApplicationInsightsBackend', () => { it('handles https errors', async () => { nock.cleanAll(); // Don't use the default scope - const errorScope = nock(AI_ENDPOINT).post(AI_PATH).replyWithError('test error'); + nock(AI_ENDPOINT).post(AI_PATH).replyWithError('test error'); const warnSpy = jest.spyOn(console, 'warn').mockImplementation(); const backend = new ApplicationInsightsBackend('user-id', 'session-id', 'product-name', { type: 'application-insights', @@ -130,7 +131,9 @@ describe('ApplicationInsightsBackend', () => { backend.sendEvent(event); - await waitForRequest(errorScope); + // Wait for the request's error handler (and its warning) to run, not just + // for the request to be sent. + await new Promise((resolve) => backend.flush(resolve)); expect(warnSpy).toHaveBeenCalledWith( 'Error sending telemetry data to Application Insights', @@ -140,7 +143,7 @@ describe('ApplicationInsightsBackend', () => { it('logs a warning for non-2xx HTTP responses', async () => { nock.cleanAll(); - const errorScope = nock(AI_ENDPOINT).post(AI_PATH).reply(500, { message: 'Internal Server Error' }); + nock(AI_ENDPOINT).post(AI_PATH).reply(500, { message: 'Internal Server Error' }); const warnSpy = jest.spyOn(console, 'warn').mockImplementation(); const backend = new ApplicationInsightsBackend('user-id', 'session-id', 'product-name', { type: 'application-insights', @@ -152,7 +155,9 @@ describe('ApplicationInsightsBackend', () => { backend.sendEvent(event); - await waitForRequest(errorScope); + // Wait for the response handler (and its warning) to run, not just for the + // request to be sent. + await new Promise((resolve) => backend.flush(resolve)); expect(warnSpy).toHaveBeenCalledWith( 'ApplicationInsightsBackend: Failed to send telemetry event. Status: 500'