Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,63 @@
* limitations under the License.
*/

export function isExportRetryable(statusCode: number): boolean {
export function isExportHTTPErrorRetryable(statusCode: number): boolean {
const retryCodes = [429, 502, 503, 504];
return retryCodes.includes(statusCode);
}

function getErrorCode(error: unknown): string | undefined {
if (!error || typeof error !== 'object') {
return undefined;
}

if ('code' in error && typeof error.code === 'string') {
return error.code;
}

const err = error as Error;
if (err.cause && typeof err.cause === 'object' && 'code' in err.cause) {
const code = (err.cause as { code: unknown }).code;
if (typeof code === 'string') {
return code;
}
}

return undefined;
}

export function isExportNetworkErrorRetryable(error: Error): boolean {
const RETRYABLE_ERROR_CODES = new Set([
'ECONNRESET',
'ECONNREFUSED',
'EPIPE',
'ETIMEDOUT',
'EAI_AGAIN',
'ENOTFOUND',
'ENETUNREACH',
'EHOSTUNREACH',
'UND_ERR_CONNECT_TIMEOUT',
'UND_ERR_HEADERS_TIMEOUT',
'UND_ERR_BODY_TIMEOUT',
'UND_ERR_SOCKET',
Copy link
Member

Choose a reason for hiding this comment

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

I don't think there's any code path right now that would produce undici errors.

]);

if (error.name === 'AbortError') {
return false;
}

const code = getErrorCode(error);
if (code && RETRYABLE_ERROR_CODES.has(code)) {
return true;
}

if (error instanceof TypeError && !error.cause) {
return true;
}

return false;
}

export function parseRetryAfterToMills(
retryAfter?: string | undefined | null
): number | undefined {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@ import { IExporterTransport } from '../exporter-transport';
import { ExportResponse } from '../export-response';
import { diag } from '@opentelemetry/api';
import {
isExportRetryable,
isExportHTTPErrorRetryable,
isExportNetworkErrorRetryable,
parseRetryAfterToMills,
} from '../is-export-retryable';
import { HeadersFactory } from '../configuration/otlp-http-configuration';
Expand Down Expand Up @@ -53,7 +54,7 @@ class FetchTransport implements IExporterTransport {
if (response.status >= 200 && response.status <= 299) {
diag.debug('response success');
return { status: 'success' };
} else if (isExportRetryable(response.status)) {
} else if (isExportHTTPErrorRetryable(response.status)) {
const retryAfter = response.headers.get('Retry-After');
const retryInMillis = parseRetryAfterToMills(retryAfter);
return { status: 'retryable', retryInMillis };
Expand All @@ -63,10 +64,10 @@ class FetchTransport implements IExporterTransport {
error: new Error('Fetch request failed with non-retryable status'),
};
} catch (error) {
if (error?.name === 'AbortError') {
if (isExportNetworkErrorRetryable(error)) {
return {
status: 'failure',
error: new Error('Fetch request timed out', { cause: error }),
status: 'retryable',
retryInMillis: 0,
};
}
Comment on lines 67 to 72
Copy link
Member

Choose a reason for hiding this comment

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

this transport is fully intended for the browser. It's for browser and webworkers. It will therefore never receive any undici errors.

return {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ import * as zlib from 'zlib';
import { Readable } from 'stream';
import { ExportResponse } from '../export-response';
import {
isExportRetryable,
isExportHTTPErrorRetryable,
isExportNetworkErrorRetryable,
parseRetryAfterToMills,
} from '../is-export-retryable';
import { OTLPExporterError } from '../types';
Expand Down Expand Up @@ -74,7 +75,7 @@ export function sendWithHttp(
status: 'success',
data: Buffer.concat(responseData),
});
} else if (res.statusCode && isExportRetryable(res.statusCode)) {
} else if (res.statusCode && isExportHTTPErrorRetryable(res.statusCode)) {
onDone({
status: 'retryable',
retryInMillis: parseRetryAfterToMills(res.headers['retry-after']),
Expand All @@ -96,16 +97,23 @@ export function sendWithHttp(
req.setTimeout(timeoutMillis, () => {
req.destroy();
onDone({
status: 'failure',
error: new Error('Request Timeout'),
status: 'retryable',
retryInMillis: 0,
});
});

req.on('error', (error: Error) => {
onDone({
status: 'failure',
error,
});
if (isExportNetworkErrorRetryable(error)) {
onDone({
status: 'retryable',
retryInMillis: 0,
});
Comment on lines 107 to 110
Copy link
Member

Choose a reason for hiding this comment

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

This changes the behavoir quite significantly:

  • errors are completely swallowed. If this always fails, the end-user will never be able to see a log what actually went wrong.
    • suggestion: I think to solve this we should allow attaching an optional error to a retryable status so that the RetryingTransport can propagate these back to the export delegate, which then logs it.
  • retryInMillis: 0 might not be what we want. the RetryingTransport implements an exponential backoff as required by the spec. IMO we should have this backoff happen to avoid overwhelming the endpoint.
  • suggestion: I think we should omit retryInMillis here to let the retryingtransport handle it.

} else {
onDone({
status: 'failure',
error,
});
}
});

compressAndSend(req, compression, data, (error: Error) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import { IExporterTransport } from '../exporter-transport';
import { ExportResponse } from '../export-response';
import { diag } from '@opentelemetry/api';
import {
isExportRetryable,
isExportHTTPErrorRetryable,
parseRetryAfterToMills,
} from '../is-export-retryable';
// eslint-disable-next-line @typescript-eslint/no-unused-vars
Expand Down Expand Up @@ -49,8 +49,8 @@ class XhrTransport implements IExporterTransport {

xhr.ontimeout = _ => {
resolve({
status: 'failure',
error: new Error('XHR request timed out'),
status: 'retryable',
retryInMillis: 0,
Copy link
Member

Choose a reason for hiding this comment

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

this timeout here is also the full maximum of the request - if this is ever triggered, there's no time left for the export to retry.

});
};

Expand All @@ -60,7 +60,7 @@ class XhrTransport implements IExporterTransport {
resolve({
status: 'success',
});
} else if (xhr.status && isExportRetryable(xhr.status)) {
} else if (xhr.status && isExportHTTPErrorRetryable(xhr.status)) {
resolve({
status: 'retryable',
retryInMillis: parseRetryAfterToMills(
Expand All @@ -82,9 +82,10 @@ class XhrTransport implements IExporterTransport {
});
};
xhr.onerror = () => {
// XHR onerror typically indicates network failures which are retryable
resolve({
status: 'failure',
error: new Error('XHR request errored'),
status: 'retryable',
retryInMillis: 0,
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: I would omit retryInMillis here to let the RetryingTransport have the exponential backoff.

});
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ describe('FetchTransport', function () {
}, done /* catch any rejections */);
});

it('returns failure when request times out', function (done) {
it('returns failure when request is aborted', function (done) {
// arrange
const abortError = new Error('aborted request');
abortError.name = 'AbortError';
Expand All @@ -137,7 +137,7 @@ describe('FetchTransport', function () {
assert.strictEqual(response.status, 'failure');
assert.strictEqual(
(response as ExportResponseFailure).error.message,
'Fetch request timed out'
'Fetch request errored'
);
} catch (e) {
done(e);
Expand All @@ -147,7 +147,7 @@ describe('FetchTransport', function () {
clock.tick(requestTimeout + 100);
});

it('returns failure when no server exists', function (done) {
it('returns failure when fetch throws non-network error', function (done) {
// arrange
sinon.stub(globalThis, 'fetch').throws(new Error('fetch failed'));
const clock = sinon.useFakeTimers();
Expand All @@ -169,5 +169,51 @@ describe('FetchTransport', function () {
}, done /* catch any rejections */);
clock.tick(requestTimeout + 100);
});

it('returns retryable when browser fetch throws network error', function (done) {
// arrange
// Browser fetch throws TypeError for network errors
sinon.stub(globalThis, 'fetch').rejects(new TypeError('Failed to fetch'));
const transport = createFetchTransport(testTransportParameters);

//act
transport.send(testPayload, requestTimeout).then(response => {
// assert
try {
assert.strictEqual(response.status, 'retryable');
assert.strictEqual(
(response as ExportResponseRetryable).retryInMillis,
0
);
} catch (e) {
done(e);
}
done();
}, done /* catch any rejections */);
});

it('returns retryable when fetch throws network error with code', function (done) {
// arrange
const cause = new Error('network error') as NodeJS.ErrnoException;
cause.code = 'ECONNRESET';
const networkError = new TypeError('fetch failed', { cause });
sinon.stub(globalThis, 'fetch').rejects(networkError);
const transport = createFetchTransport(testTransportParameters);

//act
transport.send(testPayload, requestTimeout).then(response => {
// assert
try {
assert.strictEqual(response.status, 'retryable');
assert.strictEqual(
(response as ExportResponseRetryable).retryInMillis,
0
);
} catch (e) {
done(e);
}
done();
}, done /* catch any rejections */);
});
});
Copy link
Member

Choose a reason for hiding this comment

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

no need to test any Node.js things here - this is a browser-targeted test for a browser-targeted component.

});
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@ import * as assert from 'assert';
import { createXhrTransport } from '../../src/transport/xhr-transport';
import {
ExportResponseRetryable,
ExportResponseFailure,
ExportResponseSuccess,
ExportResponseFailure,
} from '../../src';
import { ensureHeadersContain } from '../testHelper';

Expand Down Expand Up @@ -58,6 +58,14 @@ function hasOnTimeout(request: unknown): request is { ontimeout: () => void } {
return 'ontimeout' in request && typeof request['ontimeout'] === 'function';
}

function hasOnAbort(request: unknown): request is { onabort: () => void } {
if (request == null || typeof request != 'object') {
return false;
}

return 'onabort' in request && typeof request['onabort'] === 'function';
}

describe('XhrTransport', function () {
afterEach(function () {
sinon.restore();
Expand Down Expand Up @@ -151,7 +159,7 @@ describe('XhrTransport', function () {
}, done /* catch any rejections */);
});

it('returns failure when request times out', function (done) {
it('returns retryable when request times out', function (done) {
// arrange
// A fake server needed, otherwise the message will not be a timeout but a failure to connect.
const server = sinon.useFakeServer();
Expand All @@ -170,10 +178,10 @@ describe('XhrTransport', function () {
transport.send(testPayload, requestTimeout).then(response => {
// assert
try {
assert.strictEqual(response.status, 'failure');
assert.strictEqual(response.status, 'retryable');
assert.strictEqual(
(response as ExportResponseFailure).error.message,
'XHR request timed out'
(response as ExportResponseRetryable).retryInMillis,
0
);
} catch (e) {
done(e);
Expand All @@ -182,26 +190,56 @@ describe('XhrTransport', function () {
}, done /* catch any rejections */);
});

it('returns failure when no server exists', function (done) {
it('returns retryable when network error occurs', function (done) {
// arrange
const clock = sinon.useFakeTimers();
const transport = createXhrTransport(testTransportParameters);

//act
transport.send(testPayload, requestTimeout).then(response => {
// assert
try {
assert.strictEqual(response.status, 'retryable');
assert.strictEqual(
(response as ExportResponseRetryable).retryInMillis,
0
);
} catch (e) {
done(e);
}
done();
}, done /* catch any rejections */);
clock.tick(requestTimeout + 100);
});

it('returns failure when request is aborted', function (done) {
// arrange
const server = sinon.useFakeServer();
const transport = createXhrTransport(testTransportParameters);

respondWhenRequestExists(server, () => {
// this executes after the act block
const request = server.requests[0];
// Manually trigger the onabort event
if (hasOnAbort(request)) {
request.onabort();
}
});

//act
transport.send(testPayload, requestTimeout).then(response => {
// assert
try {
assert.strictEqual(response.status, 'failure');
assert.strictEqual(
(response as ExportResponseFailure).error.message,
'XHR request errored'
'XHR request aborted'
);
} catch (e) {
done(e);
}
done();
}, done /* catch any rejections */);
clock.tick(requestTimeout + 100);
});
});
});
Loading
Loading