Skip to content

Commit

Permalink
refactor: new interface for ExportResult #1569 (#1643)
Browse files Browse the repository at this point in the history 
Co-authored-by: Daniel Dyla <[email protected]>
  • Loading branch information
@vmarchaud @dyladan
vmarchaud and dyladan authored Nov 4, 2020
1 parent b1e2bed commit 14c92da
Show file tree
Hide file tree
Showing 28 changed files with 260 additions and 344 deletions.
10 changes: 7 additions & 3 deletions packages/opentelemetry-core/src/ExportResult.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,12 @@
* limitations under the License.
*/

export enum ExportResult {
export interface ExportResult {
code: ExportResultCode;
error?: Error;
}

export enum ExportResultCode {
SUCCESS,
FAILED_NOT_RETRYABLE,
FAILED_RETRYABLE,
FAILED,
}
23 changes: 7 additions & 16 deletions packages/opentelemetry-exporter-collector-proto/test/CollectorMetricExporter.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import {
ensureExportedValueRecorderIsCorrect,
} from './helper';
import { MetricRecord } from '@opentelemetry/metrics';
import { ExportResult, ExportResultCode } from '@opentelemetry/core';

const fakeRequest = {
end: function () {},
Expand Down Expand Up @@ -157,7 +158,6 @@ describe('CollectorMetricExporter - node with proto 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();
Expand All @@ -168,25 +168,15 @@ describe('CollectorMetricExporter - node with proto over http', () => {
const callback = args[1];
callback(mockRes);
setTimeout(() => {
const response: any = spyLoggerDebug.args[1][0];
assert.strictEqual(response, 'statusCode: 200');
const result = responseSpy.args[0][0] as ExportResult;
assert.strictEqual(result.code, ExportResultCode.SUCCESS);
assert.strictEqual(spyLoggerError.args.length, 0);
assert.strictEqual(responseSpy.args[0][0], 0);
done();
});
}, waitTimeMS);
});

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);

Expand All @@ -195,9 +185,10 @@ describe('CollectorMetricExporter - node with proto over http', () => {
const callback = args[1];
callback(mockResError);
setTimeout(() => {
const response = spyLoggerError.args[0][0] as string;
assert.ok(response.includes('"code":"400"'));
assert.strictEqual(responseSpy.args[0][0], 1);
const result = responseSpy.args[0][0] as ExportResult;
assert.strictEqual(result.code, ExportResultCode.FAILED);
// @ts-expect-error
assert.strictEqual(result.error?.code, 400);
done();
});
}, waitTimeMS);
Expand Down
24 changes: 7 additions & 17 deletions packages/opentelemetry-exporter-collector-proto/test/CollectorTraceExporter.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import {
ensureProtoSpanIsCorrect,
mockedReadableSpan,
} from './helper';
import { ExportResult, ExportResultCode } from '@opentelemetry/core';

const fakeRequest = {
end: function () {},
Expand Down Expand Up @@ -123,7 +124,6 @@ describe('CollectorTraceExporter - node with proto 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();
Expand All @@ -134,25 +134,15 @@ describe('CollectorTraceExporter - node with proto over http', () => {
const callback = args[1];
callback(mockRes);
setTimeout(() => {
const response: any = spyLoggerDebug.args[1][0];
assert.strictEqual(response, 'statusCode: 200');
const result = responseSpy.args[0][0] as ExportResult;
assert.strictEqual(result.code, ExportResultCode.SUCCESS);
assert.strictEqual(spyLoggerError.args.length, 0);
assert.strictEqual(responseSpy.args[0][0], 0);
done();
});
}, waitTimeMS);
});

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(spans, responseSpy);

Expand All @@ -161,10 +151,10 @@ describe('CollectorTraceExporter - node with proto over http', () => {
const callback = args[1];
callback(mockResError);
setTimeout(() => {
const response = spyLoggerError.args[0][0] as string;

assert.ok(response.includes('"code":"400"'));
assert.strictEqual(responseSpy.args[0][0], 1);
const result = responseSpy.args[0][0] as ExportResult;
assert.strictEqual(result.code, ExportResultCode.FAILED);
// @ts-expect-error
assert.strictEqual(result.error.code, 400);
done();
});
}, waitTimeMS);
Expand Down
16 changes: 7 additions & 9 deletions packages/opentelemetry-exporter-collector/src/CollectorExporterBase.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@
import { Attributes, Logger } from '@opentelemetry/api';
import {
ExportResult,
ExportResultCode,
NoopLogger,
globalErrorHandler,
} from '@opentelemetry/core';
import {
CollectorExporterError,
Expand Down Expand Up @@ -70,21 +70,19 @@ export abstract class CollectorExporterBase<
*/
export(items: ExportItem[], resultCallback: (result: ExportResult) => void) {
if (this._isShutdown) {
resultCallback(ExportResult.FAILED_NOT_RETRYABLE);
resultCallback({
code: ExportResultCode.FAILED,
error: new Error('Exporter has been shutdown'),
});
return;
}

this._export(items)
.then(() => {
resultCallback(ExportResult.SUCCESS);
resultCallback({ code: ExportResultCode.SUCCESS });
})
.catch((error: ExportServiceError) => {
globalErrorHandler(error);
if (error.code && error.code < 500) {
resultCallback(ExportResult.FAILED_NOT_RETRYABLE);
} else {
resultCallback(ExportResult.FAILED_RETRYABLE);
}
resultCallback({ code: ExportResultCode.FAILED, error });
});
}

Expand Down
2 changes: 1 addition & 1 deletion packages/opentelemetry-exporter-collector/src/platform/browser/util.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ export function sendWithXhr(
onSuccess();
} else {
const error = new collectorTypes.CollectorExporterError(
xhr.responseText,
`Failed to export with XHR (status: ${xhr.status})`,
xhr.status
);

Expand Down
41 changes: 7 additions & 34 deletions packages/opentelemetry-exporter-collector/test/browser/CollectorMetricExporter.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,7 @@
* limitations under the License.
*/

import {
NoopLogger,
setGlobalErrorHandler,
loggingErrorHandler,
} from '@opentelemetry/core';
import { ExportResultCode, NoopLogger } from '@opentelemetry/core';
import * as assert from 'assert';
import * as sinon from 'sinon';
import { CollectorMetricExporter } from '../../src/platform/browser/index';
Expand Down Expand Up @@ -166,25 +162,12 @@ describe('CollectorMetricExporter - web', () => {
});

it('should log the error message', done => {
const spyLoggerError = sinon.spy();
const handler = loggingErrorHandler({
debug: sinon.fake(),
info: sinon.fake(),
warn: sinon.fake(),
error: spyLoggerError,
});
setGlobalErrorHandler(handler);
const spyLoggerDebug = sinon.stub(collectorExporter.logger, 'debug');
spyBeacon.restore();
spyBeacon = sinon.stub(window.navigator, 'sendBeacon').returns(false);

collectorExporter.export(metrics, () => {});

setTimeout(() => {
const response: any = spyLoggerError.args[0][0] as string;
assert.ok(response.includes('sendBeacon - cannot send'));
assert.strictEqual(spyLoggerDebug.args.length, 1);

collectorExporter.export(metrics, result => {
assert.deepStrictEqual(result.code, ExportResultCode.FAILED);
assert.ok(result.error?.message.includes('cannot send'));
done();
});
});
Expand Down Expand Up @@ -290,19 +273,9 @@ describe('CollectorMetricExporter - web', () => {
});

it('should log the error message', done => {
const spyLoggerError = sinon.spy();
const handler = loggingErrorHandler({
debug: sinon.fake(),
info: sinon.fake(),
warn: sinon.fake(),
error: spyLoggerError,
});
setGlobalErrorHandler(handler);

collectorExporter.export(metrics, () => {
const response = spyLoggerError.args[0][0] as string;
assert.ok(response.includes('"code":"400"'));

collectorExporter.export(metrics, result => {
assert.deepStrictEqual(result.code, ExportResultCode.FAILED);
assert.ok(result.error?.message.includes('Failed to export'));
assert.strictEqual(spyBeacon.callCount, 0);
done();
});
Expand Down
47 changes: 7 additions & 40 deletions packages/opentelemetry-exporter-collector/test/browser/CollectorTraceExporter.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,7 @@
* limitations under the License.
*/

import {
NoopLogger,
setGlobalErrorHandler,
loggingErrorHandler,
} from '@opentelemetry/core';
import { NoopLogger, ExportResultCode } from '@opentelemetry/core';
import { ReadableSpan } from '@opentelemetry/tracing';
import * as assert from 'assert';
import * as sinon from 'sinon';
Expand Down Expand Up @@ -135,29 +131,12 @@ describe('CollectorTraceExporter - web', () => {
});

it('should log the error message', done => {
const spyLoggerError = sinon.spy();
const handler = loggingErrorHandler({
debug: sinon.fake(),
info: sinon.fake(),
warn: sinon.fake(),
error: spyLoggerError,
});
setGlobalErrorHandler(handler);

const spyLoggerDebug = sinon.stub(
collectorTraceExporter.logger,
'debug'
);
spyBeacon.restore();
spyBeacon = sinon.stub(window.navigator, 'sendBeacon').returns(false);

collectorTraceExporter.export(spans, () => {});

setTimeout(() => {
const response = spyLoggerError.args[0][0] as string;
assert.ok(response.includes('sendBeacon - cannot send'));
assert.strictEqual(spyLoggerDebug.args.length, 1);

collectorTraceExporter.export(spans, result => {
assert.deepStrictEqual(result.code, ExportResultCode.FAILED);
assert.ok(result.error?.message.includes('cannot send'));
done();
});
});
Expand Down Expand Up @@ -236,21 +215,9 @@ describe('CollectorTraceExporter - web', () => {
});

it('should log the error message', done => {
const spyLoggerError = sinon.spy();
const handler = loggingErrorHandler({
debug: sinon.fake(),
info: sinon.fake(),
warn: sinon.fake(),
error: spyLoggerError,
});
setGlobalErrorHandler(handler);

collectorTraceExporter.export(spans, () => {
const response = spyLoggerError.args[0][0] as string;

assert.ok(response.includes('"code":"400"'));

assert.strictEqual(spyBeacon.callCount, 0);
collectorTraceExporter.export(spans, result => {
assert.deepStrictEqual(result.code, ExportResultCode.FAILED);
assert.ok(result.error?.message.includes('Failed to export'));
done();
});

Expand Down
39 changes: 11 additions & 28 deletions packages/opentelemetry-exporter-collector/test/common/CollectorMetricExporter.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
* limitations under the License.
*/

import { ExportResult, NoopLogger } from '@opentelemetry/core';
import { ExportResultCode, NoopLogger } from '@opentelemetry/core';
import * as assert from 'assert';
import * as sinon from 'sinon';
import { CollectorExporterBase } from '../../src/CollectorExporterBase';
Expand Down Expand Up @@ -149,53 +149,36 @@ describe('CollectorMetricExporter - common', () => {
collectorExporter.export(metrics, callbackSpy);
const returnCode = callbackSpy.args[0][0];
assert.strictEqual(
returnCode,
ExportResult.FAILED_NOT_RETRYABLE,
returnCode.code,
ExportResultCode.FAILED,
'return value is wrong'
);
assert.strictEqual(spySend.callCount, 0, 'should not call send');
}
);
});
describe('when an error occurs', () => {
it('should return a Not Retryable Error', done => {
it('should return failed export result', done => {
spySend.throws({
code: 100,
code: 600,
details: 'Test error',
metadata: {},
message: 'Non-retryable',
message: 'Non-Retryable',
stack: 'Stack',
});
const callbackSpy = sinon.spy();
collectorExporter.export(metrics, callbackSpy);
setTimeout(() => {
const returnCode = callbackSpy.args[0][0];
assert.strictEqual(
returnCode,
ExportResult.FAILED_NOT_RETRYABLE,
returnCode.code,
ExportResultCode.FAILED,
'return value is wrong'
);
assert.strictEqual(spySend.callCount, 1, 'should call send');
done();
});
});

it('should return a Retryable Error', done => {
spySend.throws({
code: 600,
details: 'Test error',
metadata: {},
message: 'Retryable',
stack: 'Stack',
});
const callbackSpy = sinon.spy();
collectorExporter.export(metrics, callbackSpy);
setTimeout(() => {
const returnCode = callbackSpy.args[0][0];
assert.strictEqual(
returnCode,
ExportResult.FAILED_RETRYABLE,
'return value is wrong'
returnCode.error.message,
'Non-Retryable',
'return error message is wrong'
);
assert.strictEqual(spySend.callCount, 1, 'should call send');
done();
Expand Down
Loading

0 comments on commit 14c92da

Please sign in to comment.