From 05d1e25ec9db4e771b469da1e35f26ceafc27328 Mon Sep 17 00:00:00 2001 From: vmarchaud Date: Tue, 20 Oct 2020 17:45:59 +0200 Subject: [PATCH 1/3] feat(exporter-collector): log upstream error #1459 --- .../src/platform/node/util.ts | 25 ++++++---- .../src/types.ts | 4 +- .../test/helper.ts | 20 ++++++++ .../test/node/CollectorMetricExporter.test.ts | 48 +++++++++++++++---- .../test/node/CollectorTraceExporter.test.ts | 40 ++++++++++++---- 5 files changed, 109 insertions(+), 28 deletions(-) diff --git a/packages/opentelemetry-exporter-collector/src/platform/node/util.ts b/packages/opentelemetry-exporter-collector/src/platform/node/util.ts index 0e20cfc8ce..29d46e2943 100644 --- a/packages/opentelemetry-exporter-collector/src/platform/node/util.ts +++ b/packages/opentelemetry-exporter-collector/src/platform/node/util.ts @@ -50,16 +50,21 @@ export function sendWithHttp( const request = parsedUrl.protocol === 'http:' ? http.request : https.request; const req = request(options, (res: http.IncomingMessage) => { - if (res.statusCode && res.statusCode < 299) { - collector.logger.debug(`statusCode: ${res.statusCode}`); - onSuccess(); - } else { - const error = new collectorTypes.CollectorExporterError( - res.statusMessage, - res.statusCode - ); - onError(error); - } + let data = ''; + res.on('data', chunk => (data += chunk)); + res.on('end', () => { + if (res.statusCode && res.statusCode < 299) { + collector.logger.debug(`statusCode: ${res.statusCode}`, data); + onSuccess(); + } else { + const error = new collectorTypes.CollectorExporterError( + res.statusMessage, + res.statusCode, + data + ); + onError(error); + } + }); }); req.on('error', (error: Error) => { diff --git a/packages/opentelemetry-exporter-collector/src/types.ts b/packages/opentelemetry-exporter-collector/src/types.ts index ba8c474caf..694c60d8d9 100644 --- a/packages/opentelemetry-exporter-collector/src/types.ts +++ b/packages/opentelemetry-exporter-collector/src/types.ts @@ -310,9 +310,11 @@ export namespace opentelemetryProto { export class CollectorExporterError extends Error { readonly code?: number; readonly name: string = 'CollectorExporterError'; + readonly data?: string; - constructor(message?: string, code?: number) { + constructor(message?: string, code?: number, data?: string) { super(message); + this.data = data; this.code = code; } } diff --git a/packages/opentelemetry-exporter-collector/test/helper.ts b/packages/opentelemetry-exporter-collector/test/helper.ts index f430b441bc..1506f1cfbb 100644 --- a/packages/opentelemetry-exporter-collector/test/helper.ts +++ b/packages/opentelemetry-exporter-collector/test/helper.ts @@ -22,6 +22,7 @@ import { hexToBase64, InstrumentationLibrary } from '@opentelemetry/core'; import * as assert from 'assert'; import { opentelemetryProto } from '../src/types'; import * as collectorTypes from '../src/types'; +import { Stream } from 'stream'; const meterProvider = new MeterProvider({ interval: 30000, @@ -680,3 +681,22 @@ export function ensureHeadersContain( ); }); } + +export class MockedResponse extends Stream { + constructor(private _code: number, private _msg?: string) { + super(); + } + + send(data: string) { + this.emit('data', data); + this.emit('end'); + } + + get statusCode() { + return this._code; + } + + get statusMessage() { + return this._msg; + } +} diff --git a/packages/opentelemetry-exporter-collector/test/node/CollectorMetricExporter.test.ts b/packages/opentelemetry-exporter-collector/test/node/CollectorMetricExporter.test.ts index 14d9314c79..c303314121 100644 --- a/packages/opentelemetry-exporter-collector/test/node/CollectorMetricExporter.test.ts +++ b/packages/opentelemetry-exporter-collector/test/node/CollectorMetricExporter.test.ts @@ -14,7 +14,12 @@ * limitations under the License. */ -import { ConsoleLogger, ExportResultCode, LogLevel } from '@opentelemetry/core'; +import { + ConsoleLogger, + ExportResult, + ExportResultCode, + LogLevel, +} from '@opentelemetry/core'; import * as core from '@opentelemetry/core'; import * as http from 'http'; import * as assert from 'assert'; @@ -31,6 +36,7 @@ import { mockValueRecorder, ensureValueRecorderIsCorrect, ensureObserverIsCorrect, + MockedResponse, } from '../helper'; import { MetricRecord } from '@opentelemetry/metrics'; @@ -40,10 +46,6 @@ const fakeRequest = { write: function () {}, }; -const mockRes = { - statusCode: 200, -}; - const address = 'localhost:1501'; describe('CollectorMetricExporter - node with json over http', () => { @@ -167,19 +169,18 @@ describe('CollectorMetricExporter - node with json over http', () => { }); it('should log the successful message', done => { - const spyLoggerDebug = sinon.stub(collectorExporter.logger, 'debug'); const spyLoggerError = sinon.stub(collectorExporter.logger, 'error'); const responseSpy = sinon.spy(); collectorExporter.export(metrics, responseSpy); setTimeout(() => { + const mockRes = new MockedResponse(200); const args = spyRequest.args[0]; const callback = args[1]; callback(mockRes); + mockRes.send('success'); setTimeout(() => { - const response: any = spyLoggerDebug.args[1][0]; - assert.strictEqual(response, 'statusCode: 200'); assert.strictEqual(spyLoggerError.args.length, 0); assert.strictEqual( responseSpy.args[0][0].code, @@ -189,6 +190,37 @@ describe('CollectorMetricExporter - node with json over http', () => { }); }); }); + + it('should log the error message', done => { + const spyLoggerError = sinon.spy(); + const handler = core.loggingErrorHandler({ + debug: sinon.fake(), + info: sinon.fake(), + warn: sinon.fake(), + error: spyLoggerError, + }); + core.setGlobalErrorHandler(handler); + + const responseSpy = sinon.spy(); + collectorExporter.export(metrics, responseSpy); + + setTimeout(() => { + const mockRes = new MockedResponse(400); + const args = spyRequest.args[0]; + const callback = args[1]; + callback(mockRes); + mockRes.send('failed'); + setTimeout(() => { + const result = responseSpy.args[0][0] as ExportResult; + assert.strictEqual(result.code, ExportResultCode.FAILED); + const error = result.error as collectorTypes.CollectorExporterError; + assert.ok(error !== undefined); + assert.strictEqual(error.code, 400); + assert.strictEqual(error.data, 'failed'); + done(); + }); + }); + }); }); describe('CollectorMetricExporter - node (getDefaultUrl)', () => { it('should default to localhost', done => { diff --git a/packages/opentelemetry-exporter-collector/test/node/CollectorTraceExporter.test.ts b/packages/opentelemetry-exporter-collector/test/node/CollectorTraceExporter.test.ts index aa9076a896..2e3cbbc068 100644 --- a/packages/opentelemetry-exporter-collector/test/node/CollectorTraceExporter.test.ts +++ b/packages/opentelemetry-exporter-collector/test/node/CollectorTraceExporter.test.ts @@ -14,7 +14,12 @@ * limitations under the License. */ -import { ConsoleLogger, ExportResultCode, LogLevel } from '@opentelemetry/core'; +import { + ConsoleLogger, + ExportResultCode, + ExportResult, + LogLevel, +} from '@opentelemetry/core'; import * as core from '@opentelemetry/core'; import { ReadableSpan } from '@opentelemetry/tracing'; import * as http from 'http'; @@ -23,6 +28,7 @@ import * as sinon from 'sinon'; import { CollectorTraceExporter } from '../../src/platform/node'; import { CollectorExporterConfigBase } from '../../src/types'; import * as collectorTypes from '../../src/types'; +import { MockedResponse } from '../helper'; import { ensureExportTraceServiceRequestIsSet, @@ -36,10 +42,6 @@ const fakeRequest = { write: function () {}, }; -const mockRes = { - statusCode: 200, -}; - const address = 'localhost:1501'; describe('CollectorTraceExporter - node with json over http', () => { @@ -134,19 +136,17 @@ describe('CollectorTraceExporter - node with json over http', () => { }); it('should log the successful message', done => { - const spyLoggerDebug = sinon.stub(collectorExporter.logger, 'debug'); const spyLoggerError = sinon.stub(collectorExporter.logger, 'error'); - const responseSpy = sinon.spy(); collectorExporter.export(spans, responseSpy); setTimeout(() => { + const mockRes = new MockedResponse(200); const args = spyRequest.args[0]; const callback = args[1]; callback(mockRes); + mockRes.send('success'); setTimeout(() => { - const response: any = spyLoggerDebug.args[1][0]; - assert.strictEqual(response, 'statusCode: 200'); assert.strictEqual(spyLoggerError.args.length, 0); assert.strictEqual( responseSpy.args[0][0].code, @@ -156,6 +156,28 @@ describe('CollectorTraceExporter - node with json over http', () => { }); }); }); + + it('should log the error message', done => { + const responseSpy = sinon.spy(); + collectorExporter.export(spans, responseSpy); + + setTimeout(() => { + const mockResError = new MockedResponse(400); + const args = spyRequest.args[0]; + const callback = args[1]; + callback(mockResError); + mockResError.send('failed'); + setTimeout(() => { + const result = responseSpy.args[0][0] as ExportResult; + assert.strictEqual(result.code, ExportResultCode.FAILED); + const error = result.error as collectorTypes.CollectorExporterError; + assert.ok(error !== undefined); + assert.strictEqual(error.code, 400); + assert.strictEqual(error.data, 'failed'); + done(); + }); + }); + }); }); describe('CollectorTraceExporter - node (getDefaultUrl)', () => { it('should default to localhost', done => { From 1060faac0fef6e1332813abbef81816034ab4921 Mon Sep 17 00:00:00 2001 From: vmarchaud Date: Sat, 24 Oct 2020 10:32:13 +0200 Subject: [PATCH 2/3] chore: fix tests of exporter-collector-proto --- .../test/CollectorMetricExporter.test.ts | 21 +++++++++---------- .../test/CollectorTraceExporter.test.ts | 13 +++++------- .../test/helper.ts | 20 ++++++++++++++++++ 3 files changed, 35 insertions(+), 19 deletions(-) diff --git a/packages/opentelemetry-exporter-collector-proto/test/CollectorMetricExporter.test.ts b/packages/opentelemetry-exporter-collector-proto/test/CollectorMetricExporter.test.ts index 621ba2d1b9..e415d6662e 100644 --- a/packages/opentelemetry-exporter-collector-proto/test/CollectorMetricExporter.test.ts +++ b/packages/opentelemetry-exporter-collector-proto/test/CollectorMetricExporter.test.ts @@ -30,9 +30,11 @@ import { ensureExportedCounterIsCorrect, ensureExportedObserverIsCorrect, ensureExportedValueRecorderIsCorrect, + MockedResponse, } from './helper'; import { MetricRecord } from '@opentelemetry/metrics'; import { ExportResult, ExportResultCode } from '@opentelemetry/core'; +import { CollectorExporterError } from '@opentelemetry/exporter-collector/build/src/types'; const fakeRequest = { end: function () {}, @@ -40,14 +42,6 @@ const fakeRequest = { write: function () {}, }; -const mockRes = { - statusCode: 200, -}; - -const mockResError = { - statusCode: 400, -}; - // send is lazy loading file so need to wait a bit const waitTimeMS = 20; @@ -164,9 +158,11 @@ describe('CollectorMetricExporter - node with proto over http', () => { collectorExporter.export(metrics, responseSpy); setTimeout(() => { + const mockRes = new MockedResponse(200); const args = spyRequest.args[0]; const callback = args[1]; callback(mockRes); + mockRes.send('success'); setTimeout(() => { const result = responseSpy.args[0][0] as ExportResult; assert.strictEqual(result.code, ExportResultCode.SUCCESS); @@ -181,14 +177,17 @@ describe('CollectorMetricExporter - node with proto over http', () => { collectorExporter.export(metrics, responseSpy); setTimeout(() => { + const mockRes = new MockedResponse(400); const args = spyRequest.args[0]; const callback = args[1]; - callback(mockResError); + callback(mockRes); + mockRes.send('failed'); setTimeout(() => { const result = responseSpy.args[0][0] as ExportResult; assert.strictEqual(result.code, ExportResultCode.FAILED); - // @ts-expect-error - assert.strictEqual(result.error?.code, 400); + const error = result.error as CollectorExporterError; + assert.strictEqual(error.code, 400); + assert.strictEqual(error.data, 'failed'); done(); }); }, waitTimeMS); diff --git a/packages/opentelemetry-exporter-collector-proto/test/CollectorTraceExporter.test.ts b/packages/opentelemetry-exporter-collector-proto/test/CollectorTraceExporter.test.ts index 22e6d69586..6f534d9558 100644 --- a/packages/opentelemetry-exporter-collector-proto/test/CollectorTraceExporter.test.ts +++ b/packages/opentelemetry-exporter-collector-proto/test/CollectorTraceExporter.test.ts @@ -28,6 +28,7 @@ import { ensureExportTraceServiceRequestIsSet, ensureProtoSpanIsCorrect, mockedReadableSpan, + MockedResponse, } from './helper'; import { ExportResult, ExportResultCode } from '@opentelemetry/core'; @@ -37,14 +38,6 @@ const fakeRequest = { write: function () {}, }; -const mockRes = { - statusCode: 200, -}; - -const mockResError = { - statusCode: 400, -}; - // send is lazy loading file so need to wait a bit const waitTimeMS = 20; @@ -130,9 +123,11 @@ describe('CollectorTraceExporter - node with proto over http', () => { collectorExporter.export(spans, responseSpy); setTimeout(() => { + const mockRes = new MockedResponse(200); const args = spyRequest.args[0]; const callback = args[1]; callback(mockRes); + mockRes.send('success'); setTimeout(() => { const result = responseSpy.args[0][0] as ExportResult; assert.strictEqual(result.code, ExportResultCode.SUCCESS); @@ -147,9 +142,11 @@ describe('CollectorTraceExporter - node with proto over http', () => { collectorExporter.export(spans, responseSpy); setTimeout(() => { + const mockResError = new MockedResponse(400); const args = spyRequest.args[0]; const callback = args[1]; callback(mockResError); + mockResError.send('failed'); setTimeout(() => { const result = responseSpy.args[0][0] as ExportResult; assert.strictEqual(result.code, ExportResultCode.FAILED); diff --git a/packages/opentelemetry-exporter-collector-proto/test/helper.ts b/packages/opentelemetry-exporter-collector-proto/test/helper.ts index 51d970226d..1afa2da253 100644 --- a/packages/opentelemetry-exporter-collector-proto/test/helper.ts +++ b/packages/opentelemetry-exporter-collector-proto/test/helper.ts @@ -21,6 +21,7 @@ import { Resource } from '@opentelemetry/resources'; import { collectorTypes } from '@opentelemetry/exporter-collector'; import * as assert from 'assert'; import { MeterProvider, MetricRecord } from '@opentelemetry/metrics'; +import { Stream } from 'stream'; const meterProvider = new MeterProvider({ interval: 30000, @@ -424,3 +425,22 @@ export function ensureExportMetricsServiceRequestIsSet( const metrics = resourceMetrics[0].instrumentationLibraryMetrics[0].metrics; assert.strictEqual(metrics.length, 3, 'Metrics are missing'); } + +export class MockedResponse extends Stream { + constructor(private _code: number, private _msg?: string) { + super(); + } + + send(data: string) { + this.emit('data', data); + this.emit('end'); + } + + get statusCode() { + return this._code; + } + + get statusMessage() { + return this._msg; + } +} From 354ea244f92dd765d9320ac4b20aca4fba65107a Mon Sep 17 00:00:00 2001 From: vmarchaud Date: Sat, 24 Oct 2020 11:00:47 +0200 Subject: [PATCH 3/3] chore: move MockedResponse to nodeHelper --- .../test/helper.ts | 20 ----------- .../test/node/CollectorMetricExporter.test.ts | 3 +- .../test/node/CollectorTraceExporter.test.ts | 2 +- .../test/node/nodeHelpers.ts | 36 +++++++++++++++++++ 4 files changed, 38 insertions(+), 23 deletions(-) create mode 100644 packages/opentelemetry-exporter-collector/test/node/nodeHelpers.ts diff --git a/packages/opentelemetry-exporter-collector/test/helper.ts b/packages/opentelemetry-exporter-collector/test/helper.ts index 1506f1cfbb..f430b441bc 100644 --- a/packages/opentelemetry-exporter-collector/test/helper.ts +++ b/packages/opentelemetry-exporter-collector/test/helper.ts @@ -22,7 +22,6 @@ import { hexToBase64, InstrumentationLibrary } from '@opentelemetry/core'; import * as assert from 'assert'; import { opentelemetryProto } from '../src/types'; import * as collectorTypes from '../src/types'; -import { Stream } from 'stream'; const meterProvider = new MeterProvider({ interval: 30000, @@ -681,22 +680,3 @@ export function ensureHeadersContain( ); }); } - -export class MockedResponse extends Stream { - constructor(private _code: number, private _msg?: string) { - super(); - } - - send(data: string) { - this.emit('data', data); - this.emit('end'); - } - - get statusCode() { - return this._code; - } - - get statusMessage() { - return this._msg; - } -} diff --git a/packages/opentelemetry-exporter-collector/test/node/CollectorMetricExporter.test.ts b/packages/opentelemetry-exporter-collector/test/node/CollectorMetricExporter.test.ts index c303314121..1e626c56d3 100644 --- a/packages/opentelemetry-exporter-collector/test/node/CollectorMetricExporter.test.ts +++ b/packages/opentelemetry-exporter-collector/test/node/CollectorMetricExporter.test.ts @@ -27,7 +27,7 @@ import * as sinon from 'sinon'; import { CollectorMetricExporter } from '../../src/platform/node'; import { CollectorExporterConfigBase } from '../../src/types'; import * as collectorTypes from '../../src/types'; - +import { MockedResponse } from './nodeHelpers'; import { mockCounter, mockObserver, @@ -36,7 +36,6 @@ import { mockValueRecorder, ensureValueRecorderIsCorrect, ensureObserverIsCorrect, - MockedResponse, } from '../helper'; import { MetricRecord } from '@opentelemetry/metrics'; diff --git a/packages/opentelemetry-exporter-collector/test/node/CollectorTraceExporter.test.ts b/packages/opentelemetry-exporter-collector/test/node/CollectorTraceExporter.test.ts index 2e3cbbc068..bb9fb74a06 100644 --- a/packages/opentelemetry-exporter-collector/test/node/CollectorTraceExporter.test.ts +++ b/packages/opentelemetry-exporter-collector/test/node/CollectorTraceExporter.test.ts @@ -28,7 +28,7 @@ import * as sinon from 'sinon'; import { CollectorTraceExporter } from '../../src/platform/node'; import { CollectorExporterConfigBase } from '../../src/types'; import * as collectorTypes from '../../src/types'; -import { MockedResponse } from '../helper'; +import { MockedResponse } from './nodeHelpers'; import { ensureExportTraceServiceRequestIsSet, diff --git a/packages/opentelemetry-exporter-collector/test/node/nodeHelpers.ts b/packages/opentelemetry-exporter-collector/test/node/nodeHelpers.ts new file mode 100644 index 0000000000..1219f2d976 --- /dev/null +++ b/packages/opentelemetry-exporter-collector/test/node/nodeHelpers.ts @@ -0,0 +1,36 @@ +/* + * Copyright The OpenTelemetry Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { Stream } from 'stream'; + +export class MockedResponse extends Stream { + constructor(private _code: number, private _msg?: string) { + super(); + } + + send(data: string) { + this.emit('data', data); + this.emit('end'); + } + + get statusCode() { + return this._code; + } + + get statusMessage() { + return this._msg; + } +}