Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: new interface for ExportResult #1569 #1643

Merged
merged 9 commits into from
Nov 4, 2020
Merged

refactor: new interface for ExportResult #1569 #1643

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a7199e3
refactor: use interface with optional error for ExportResult
vmarchaud Oct 31, 2020
b2d4d10
refactor: use new export result for exporter-collector
vmarchaud Oct 31, 2020
2b45bc7
refactor: use new export result for exporter-jaeger
vmarchaud Oct 31, 2020
67831bd
refactor: use new export result for exporter-prometheus
vmarchaud Oct 31, 2020
a3cca78
refactor: use new export result for exporter-zipkin
vmarchaud Oct 31, 2020
acbf14a
refactor: use new export result for metrics sdk
vmarchaud Oct 31, 2020
06f3b7a
refactor: use new export result for tracing sdk
vmarchaud Oct 31, 2020
be5e4cf
chore: fix tests
vmarchaud Oct 31, 2020
6af9a7e
Merge branch 'master' into export-result-update
dyladan Nov 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions packages/opentelemetry-core/src/ExportResult.ts
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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})`,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason for this change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depending on what the collector report (which itself depend on upstream exporters) you can have a empty text. I prefered to opt for this generic text so user get an error message every time.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

xhr.status already is passed to CollectorExporterError. Maybe it is better to have xhr.responseText || "Failed to export with XHR"?

xhr.status
);

Expand Down
41 changes: 7 additions & 34 deletions packages/opentelemetry-exporter-collector/test/browser/CollectorMetricExporter.test.ts
View file
Open in desktop
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'));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the message is still sendBeacon - cannot send so why shorter check then ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i just rewrote the code without thinking about what was in place, i will add it back

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
Open in desktop
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'));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here the same sendBeacon - 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
Open in desktop
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