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.

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

feat(otlp-exporter): add timeout env var #2738

Merged
merged 82 commits into from
May 11, 2022
Merged
Show file tree
Hide file tree
Changes from 37 commits
Commits
Show all changes
82 commits
Select commit Hold shift + click to select a range
4e81f83
feat(otlp-exporter): add timeout env var
svetlanabrennan Jan 24, 2022
11ea770
feat(otlp-exporter): add timeout env var test
svetlanabrennan Jan 25, 2022
a9a526f
Merge branch 'main' into add-exporter-timeout
svetlanabrennan Jan 26, 2022
4f659fb
Merge branch 'main' into add-exporter-timeout
svetlanabrennan Jan 26, 2022
2bc61e4
feat(otlp-exporter): add trace_timeout env var
svetlanabrennan Jan 26, 2022
48a25d7
Merge branch 'add-exporter-timeout' of github.com:svetlanabrennan/ope…
svetlanabrennan Jan 26, 2022
da3211f
feat(otlp-exporter): add how timeout is selected
svetlanabrennan Jan 26, 2022
0c1011d
feat(otlp-exporter): add negative value check to env vars
svetlanabrennan Jan 27, 2022
ed4079f
feat(otlp-exporter): add negative value check to env vars
svetlanabrennan Jan 27, 2022
fc486a1
feat(otlp-exporter): add timeout config information to readme
svetlanabrennan Jan 28, 2022
7292244
feat(otlp-exporter): add const var for default timeout value
svetlanabrennan Jan 28, 2022
ca98326
Merge branch 'main' into add-exporter-timeout
svetlanabrennan Jan 31, 2022
5e7d2f5
Merge branch 'main' into add-exporter-timeout
svetlanabrennan Jan 31, 2022
e8dc21b
feat(otlp-exporter): refactor code based on feedback
svetlanabrennan Feb 1, 2022
6c3a42d
Merge branch 'add-exporter-timeout' of github.com:svetlanabrennan/ope…
svetlanabrennan Feb 1, 2022
111cb83
feat(otlp-exporter): move configuring timeout to utils and tests
svetlanabrennan Feb 3, 2022
bbb75cb
feat(otlp-exporter): add timeout to http and xhr request
svetlanabrennan Feb 8, 2022
8866035
Merge branch 'main' into add-exporter-timeout
svetlanabrennan Feb 8, 2022
b8543c0
Fix typo in environment.ts
svetlanabrennan Feb 8, 2022
3873a85
feat(otlp-exporter): update timeout to general timers
svetlanabrennan Feb 14, 2022
1fe1847
feat(otlp-exporter): update node http timeout tests
svetlanabrennan Feb 15, 2022
a207d50
Merge branch 'main' into add-exporter-timeout
svetlanabrennan Feb 15, 2022
991beeb
feat(otlp-exporter): fix dependency version for http exporter
svetlanabrennan Feb 15, 2022
4e5af76
feat(otlp-exporter): add grpc deadline test
svetlanabrennan Feb 15, 2022
b27ca5f
feat(otlp-exporter): fix typos
svetlanabrennan Feb 15, 2022
a428d9d
Merge branch 'main' into add-exporter-timeout
svetlanabrennan Feb 17, 2022
21f596e
feat(otlp-exporter): add browser timeout test
svetlanabrennan Feb 18, 2022
3043e10
Merge branch 'main' into add-exporter-timeout
svetlanabrennan Feb 18, 2022
d66cb4a
feat(otlp-exporter): fix lint errors
svetlanabrennan Feb 18, 2022
fd4406b
Merge branch 'add-exporter-timeout' of github.com:svetlanabrennan/ope…
svetlanabrennan Feb 18, 2022
d42991e
Merge branch 'main' into add-exporter-timeout
svetlanabrennan Feb 23, 2022
c7fe6cc
Merge branch 'main' into add-exporter-timeout
Flarna Feb 24, 2022
10880be
feat(otlp-exporter): fix tests after main merge
svetlanabrennan Feb 25, 2022
3e8360d
feat(otlp-exporter): fix one test and lint on readme
svetlanabrennan Feb 26, 2022
481795d
feat(otlp-exporter): fix tests that call export method
svetlanabrennan Mar 1, 2022
e8db3dc
Merge branch 'main' into add-exporter-timeout
svetlanabrennan Mar 1, 2022
4f9a556
Merge branch 'main' into add-exporter-timeout
svetlanabrennan Mar 2, 2022
d9a665f
feat(otlp-exporter): update various items based on feedback
svetlanabrennan Mar 2, 2022
4222cc6
feat(otlp-exporter): add additional tests
svetlanabrennan Mar 2, 2022
0d9de49
feat(otlp-exporter): add two more tests
svetlanabrennan Mar 2, 2022
80c1b0a
Merge branch 'main' into add-exporter-timeout
svetlanabrennan Mar 10, 2022
eff3298
feat(otlp-exporter): add 1 more test
svetlanabrennan Mar 11, 2022
84aa2ec
Merge branch 'add-exporter-timeout' of github.com:svetlanabrennan/ope…
svetlanabrennan Mar 11, 2022
70ca58d
Merge branch 'main' into add-exporter-timeout
svetlanabrennan Mar 15, 2022
b00c3e6
Merge branch 'main' into add-exporter-timeout
svetlanabrennan Mar 16, 2022
83092e6
Merge branch 'main' into add-exporter-timeout
svetlanabrennan Mar 17, 2022
44eeb56
feat(otlp-exporter): add logic around request aborted after response …
svetlanabrennan Mar 24, 2022
cfeb182
feat(otlp-exporter): update grpc deadline calculation
svetlanabrennan Mar 24, 2022
8b6a005
feat(otlp-exporter): add real http request test to proto exporter
svetlanabrennan Mar 24, 2022
d955eac
Merge branch 'main' into add-exporter-timeout
svetlanabrennan Mar 24, 2022
5547628
feat(otlp-exporter): fix timeout test
svetlanabrennan Mar 24, 2022
bc6bb16
feat(otlp-exporter): fix grpc readme
svetlanabrennan Mar 24, 2022
f5309fd
Merge branch 'main' into add-exporter-timeout
svetlanabrennan Mar 28, 2022
8c0eda7
Update packages/exporter-trace-otlp-http/src/platform/node/util.ts
svetlanabrennan Mar 28, 2022
5f24eb9
Merge branch 'add-exporter-timeout' of github.com:svetlanabrennan/ope…
svetlanabrennan Mar 28, 2022
7f103cb
feat(otlp-exporter): remove comment from sendWithHttp
svetlanabrennan Mar 31, 2022
1a3b0bf
Merge branch 'main' into add-exporter-timeout
rauno56 Apr 13, 2022
a9f6a9a
Merge branch 'main' into add-exporter-timeout
svetlanabrennan Apr 19, 2022
f0055a0
feat(otlp-exporter): fix metric tests and add changelong
svetlanabrennan Apr 19, 2022
c79c521
feat(otlp-exporter): fix lint
svetlanabrennan Apr 19, 2022
d887480
Merge branch 'main' into add-exporter-timeout
svetlanabrennan Apr 20, 2022
d18c587
feat(otlp-exporter): fix metric tests
svetlanabrennan Apr 20, 2022
d1ccc95
Merge branch 'add-exporter-timeout' of github.com:svetlanabrennan/ope…
svetlanabrennan Apr 20, 2022
b97a418
feat(otlp-exporter): fix proto tests
svetlanabrennan Apr 20, 2022
0b8f7da
feat(otlp-exporter): fix lint
svetlanabrennan Apr 20, 2022
f053d09
feat(otlp-exporter): Merge branch 'main' into add-exporter-timeout
svetlanabrennan Apr 21, 2022
78d4e93
feat(otlp-exporter): fix tests after main merge
svetlanabrennan Apr 22, 2022
f32b965
Merge branch 'main' into add-exporter-timeout
svetlanabrennan Apr 25, 2022
0b4c617
Merge branch 'main' into add-exporter-timeout
Flarna Apr 26, 2022
e81f9dd
feat(otlp-exporter): fix lint
svetlanabrennan Apr 26, 2022
ae49cd3
Merge branch 'main' into add-exporter-timeout
svetlanabrennan Apr 27, 2022
828993e
Merge branch 'main' into add-exporter-timeout
vmarchaud Apr 30, 2022
dc647fd
Merge branch 'main' into add-exporter-timeout
svetlanabrennan May 5, 2022
fbaa457
feat(otlp-exporter): fix accidentally removed code after main merge
svetlanabrennan May 5, 2022
14ba7d3
feat(otlp-exporter): replace nextTick with queueMicroTask for browser…
svetlanabrennan May 9, 2022
1ebc461
feat(otlp-exporter): add more details to changelog item
svetlanabrennan May 9, 2022
27b2b86
Merge branch 'main' into add-exporter-timeout
svetlanabrennan May 9, 2022
b186cbf
Merge branch 'main' into add-exporter-timeout
svetlanabrennan May 10, 2022
511999a
feat(otlp-exporter): fix server timing issue in real http request tes…
svetlanabrennan May 10, 2022
b553759
feat(otlp-exporter): fix real http request tests
svetlanabrennan May 10, 2022
5da6ce2
feat(otlp-exporter): add comment to two ways of destroying request
svetlanabrennan May 11, 2022
6ee89b0
Merge branch 'main' into add-exporter-timeout
dyladan May 11, 2022
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
31 changes: 28 additions & 3 deletions packages/exporter-trace-otlp-grpc/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,31 @@ provider.register();

Note, that this will only work if TLS is also configured on the server.

## Exporter Timeout Configuration
svetlanabrennan marked this conversation as resolved.
Show resolved Hide resolved

The OTLPTraceExporter has a timeout configuration option which is the maximum time, in milliseconds, the OTLP exporter will wait for each batch export. The default value is 10000ms.
svetlanabrennan marked this conversation as resolved.
Show resolved Hide resolved

To override the default timeout duration, use the following options:

+ Set with environment variables:

| Environment variable | Description |
|----------------------|-------------|
| OTEL_EXPORTER_OTLP_TRACES_TIMEOUT | The maximum waiting time, in milliseconds, allowed to send each OTLP trace batch. Default is 10000. |
| OTEL_EXPORTER_OTLP_TIMEOUT | The maximum waiting time, in milliseconds, allowed to send each OTLP trace and metric batch. Default is 10000. |

> `OTEL_EXPORTER_OTLP_TRACES_TIMEOUT` takes precedence and overrides `OTEL_EXPORTER_OTLP_TIMEOUT`.

+ Provide `timeoutMillis` to OTLPTraceExporter with `collectorOptions`:

```js
const collectorOptions = {
svetlanabrennan marked this conversation as resolved.
Show resolved Hide resolved
timeoutMillis: 15000
};
```

> Providing `timeoutMillis` with `collectorOptions` takes precedence and overrides timeout set with environment variables.

## Running opentelemetry-collector locally to see the traces

1. Go to examples/otlp-exporter-node
Expand All @@ -115,9 +140,9 @@ Note, that this will only work if TLS is also configured on the server.

## Useful links

- For more information on OpenTelemetry, visit: <https://opentelemetry.io/>
- For more about OpenTelemetry JavaScript: <https://github.com/open-telemetry/opentelemetry-js>
- For help or feedback on this project, join us in [GitHub Discussions][discussions-url]
+ For more information on OpenTelemetry, visit: <https://opentelemetry.io/>
svetlanabrennan marked this conversation as resolved.
Show resolved Hide resolved
+ For more about OpenTelemetry JavaScript: <https://github.com/open-telemetry/opentelemetry-js>
+ For help or feedback on this project, join us in [GitHub Discussions][discussions-url]

## License

Expand Down
1 change: 1 addition & 0 deletions packages/exporter-trace-otlp-grpc/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ export interface ServiceClient extends grpc.Client {
export: (
request: any,
metadata: grpc.Metadata,
options: object,
svetlanabrennan marked this conversation as resolved.
Show resolved Hide resolved
callback: Function
) => {};
}
Expand Down
2 changes: 2 additions & 0 deletions packages/exporter-trace-otlp-grpc/src/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -84,10 +84,12 @@ export function send<ExportItem, ServiceRequest>(
): void {
if (collector.serviceClient) {
const serviceRequest = collector.convert(objects);
const deadline = new Date().setSeconds(new Date().getSeconds() + (collector._timeoutMillis / 1000));

collector.serviceClient.export(
serviceRequest,
collector.metadata || new grpc.Metadata(),
{deadline: deadline},
(err: otlpTypes.ExportServiceError) => {
if (err) {
diag.error('Service request', serviceRequest);
Expand Down
29 changes: 29 additions & 0 deletions packages/exporter-trace-otlp-grpc/test/OTLPTraceExporter.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ import {
mockedReadableSpan,
} from './traceHelper';

import * as core from '@opentelemetry/core';

const traceServiceProtoPath =
'opentelemetry/proto/collector/trace/v1/trace_service.proto';
const includeDirs = [path.resolve(__dirname, '../protos')];
Expand Down Expand Up @@ -197,6 +199,33 @@ const testCollectorExporter = (params: TestParams) =>
done();
}, 200);
});
it('should log deadline exceeded error', done => {
const credentials = params.useTLS
? grpc.credentials.createSsl(
fs.readFileSync('./test/certs/ca.crt'),
fs.readFileSync('./test/certs/client.key'),
fs.readFileSync('./test/certs/client.crt')
)
: undefined;

const collectorExporterWithTimeout = new OTLPTraceExporter({
url: 'grpcs://' + address,
credentials,
metadata: params.metadata,
timeoutMillis: 100,
});

const responseSpy = sinon.spy();
const spans = [Object.assign({}, mockedReadableSpan)];
collectorExporterWithTimeout.export(spans, responseSpy);

setTimeout(() => {
const result = responseSpy.args[0][0] as core.ExportResult;
assert.strictEqual(result.code, core.ExportResultCode.FAILED);
assert.strictEqual(responseSpy.args[0][0].error.details, 'Deadline exceeded');
done();
}, 300);
svetlanabrennan marked this conversation as resolved.
Show resolved Hide resolved
});
});
});

Expand Down
31 changes: 28 additions & 3 deletions packages/exporter-trace-otlp-http/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,31 @@ OTEL_EXPORTER_OTLP_METRICS_ENDPOINT=https://metric-service:4317/v1/metrics

For more details, see [OpenTelemetry Specification on Protocol Exporter][opentelemetry-spec-protocol-exporter].

## Exporter Timeout Configuration

The OTLPTraceExporter has a timeout configuration option which is the maximum time, in milliseconds, the OTLP exporter will wait for each batch export. The default value is 10000ms.

To override the default timeout duration, use the following options:

+ Set with environment variables:

| Environment variable | Description |
|----------------------|-------------|
| OTEL_EXPORTER_OTLP_TRACES_TIMEOUT | The maximum waiting time, in milliseconds, allowed to send each OTLP trace batch. Default is 10000. |
| OTEL_EXPORTER_OTLP_TIMEOUT | The maximum waiting time, in milliseconds, allowed to send each OTLP trace and metric batch. Default is 10000. |

> `OTEL_EXPORTER_OTLP_TRACES_TIMEOUT` takes precedence and overrides `OTEL_EXPORTER_OTLP_TIMEOUT`.

+ Provide `timeoutMillis` to OTLPTraceExporter with `collectorOptions`:

```js
const collectorOptions = {
timeoutMillis: 15000
};
```

> Providing `timeoutMillis` with `collectorOptions` takes precedence and overrides timeout set with environment variables.

## Running opentelemetry-collector locally to see the traces

1. Go to examples/otlp-exporter-node
Expand All @@ -114,9 +139,9 @@ For more details, see [OpenTelemetry Specification on Protocol Exporter][opentel

## Useful links

- For more information on OpenTelemetry, visit: <https://opentelemetry.io/>
- For more about OpenTelemetry JavaScript: <https://github.com/open-telemetry/opentelemetry-js>
- For help or feedback on this project, join us in [GitHub Discussions][discussions-url]
+ For more information on OpenTelemetry, visit: <https://opentelemetry.io/>
+ For more about OpenTelemetry JavaScript: <https://github.com/open-telemetry/opentelemetry-js>
+ For help or feedback on this project, join us in [GitHub Discussions][discussions-url]

## License

Expand Down
4 changes: 4 additions & 0 deletions packages/exporter-trace-otlp-http/src/OTLPExporterBase.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {
OTLPExporterConfigBase,
ExportServiceError,
} from './types';
import { configureExporterTimeout } from './util';

/**
* Collector Exporter abstract base class
Expand All @@ -36,6 +37,7 @@ export abstract class OTLPExporterBase<
protected _concurrencyLimit: number;
protected _sendingPromises: Promise<unknown>[] = [];
protected _shutdownOnce: BindOnceFuture<void>;
public readonly _timeoutMillis: number;
svetlanabrennan marked this conversation as resolved.
Show resolved Hide resolved
svetlanabrennan marked this conversation as resolved.
Show resolved Hide resolved

/**
* @param config
Expand All @@ -56,6 +58,8 @@ export abstract class OTLPExporterBase<
? config.concurrencyLimit
: Infinity;

this._timeoutMillis = configureExporterTimeout(config.timeoutMillis);

// platform dependent
this.onInit(config);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ export abstract class OTLPExporterBrowserBase<

const promise = new Promise<void>((resolve, reject) => {
if (this._useXHR) {
sendWithXhr(body, this.url, this._headers, resolve, reject);
sendWithXhr(body, this.url, this._headers, this._timeoutMillis, resolve, reject);
} else {
sendWithBeacon(body, this.url, { type: 'application/json' }, resolve, reject);
}
Expand Down
16 changes: 15 additions & 1 deletion packages/exporter-trace-otlp-http/src/platform/browser/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,17 @@ export function sendWithXhr(
body: string,
url: string,
headers: Record<string, string>,
exporterTimeout: number,
onSuccess: () => void,
onError: (error: otlpTypes.OTLPExporterError) => void
): void {
let reqIsDestroyed: boolean;

const exporterTimer = setTimeout(() => {
reqIsDestroyed = true;
xhr.abort();
}, exporterTimeout);

const xhr = new XMLHttpRequest();
xhr.open('POST', url);
svetlanabrennan marked this conversation as resolved.
Show resolved Hide resolved

Expand All @@ -74,14 +82,20 @@ export function sendWithXhr(
xhr.onreadystatechange = () => {
if (xhr.readyState === XMLHttpRequest.DONE) {
if (xhr.status >= 200 && xhr.status <= 299) {
clearTimeout(exporterTimer);
diag.debug('xhr success', body);
onSuccess();
} else if (reqIsDestroyed) {
const error = new otlpTypes.OTLPExporterError(
'Request Timeout', xhr.status
);
onError(error);
} else {
const error = new otlpTypes.OTLPExporterError(
`Failed to export with XHR (status: ${xhr.status})`,
xhr.status
);

clearTimeout(exporterTimer);
onError(error);
}
}
Expand Down
22 changes: 19 additions & 3 deletions packages/exporter-trace-otlp-http/src/platform/node/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,14 @@ export function sendWithHttp<ExportItem, ServiceRequest>(
onSuccess: () => void,
onError: (error: otlpTypes.OTLPExporterError) => void
): void {
const exporterTimeout = collector._timeoutMillis;
const parsedUrl = new url.URL(collector.url);
let reqIsDestroyed: boolean;

const exporterTimer = setTimeout(() => {
svetlanabrennan marked this conversation as resolved.
Show resolved Hide resolved
reqIsDestroyed = true;
req.destroy();
}, exporterTimeout);

const options: http.RequestOptions | https.RequestOptions = {
hostname: parsedUrl.hostname,
Expand All @@ -65,21 +72,30 @@ export function sendWithHttp<ExportItem, ServiceRequest>(
res.on('end', () => {
if (res.statusCode && res.statusCode < 299) {
diag.debug(`statusCode: ${res.statusCode}`, responseData);
clearTimeout(exporterTimer);
svetlanabrennan marked this conversation as resolved.
Show resolved Hide resolved
onSuccess();
} else {
const error = new otlpTypes.OTLPExporterError(
res.statusMessage,
res.statusCode,
responseData
);
clearTimeout(exporterTimer);
onError(error);
}
});
});


req.on('error', (error: Error) => {
onError(error);
req.on('error', (error: Error | any) => {
if (reqIsDestroyed) {
const err = new otlpTypes.OTLPExporterError(
'Request Timeout', error.code
);
onError(err);
} else {
clearTimeout(exporterTimer);
onError(error);
}
});

switch (collector.compression) {
Expand Down
3 changes: 3 additions & 0 deletions packages/exporter-trace-otlp-http/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,9 @@ export interface OTLPExporterConfigBase {
attributes?: SpanAttributes;
url?: string;
concurrencyLimit?: number;
/** Maximum time the OTLP exporter will wait for each batch export.
* The default value is 10000ms. */
timeoutMillis?: number;
}

/**
Expand Down
44 changes: 44 additions & 0 deletions packages/exporter-trace-otlp-http/src/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@
*/

import { diag } from '@opentelemetry/api';
import { getEnv } from '@opentelemetry/core';

const DEFAULT_TRACE_TIMEOUT = 10000;

/**
* Parses headers from config leaving only those that have defined values
Expand All @@ -39,3 +42,44 @@ export function appendResourcePathToUrlIfNotPresent(url: string, path: string):

return url + path;
}

/**
* Configure exporter trace timeout value from passed in value or environment variables
* @param timeoutMillis
* @returns timeout value in milliseconds
*/

export function configureExporterTimeout(timeoutMillis: number | undefined): number {
if (typeof timeoutMillis === 'number') {
if (timeoutMillis <= 0) {
// OTLP exporter configured timeout - using default value of 10000ms
return invalidTimeout(timeoutMillis, DEFAULT_TRACE_TIMEOUT);
}
return timeoutMillis;
} else {
return getExporterTimeoutFromEnv();
}
}

function getExporterTimeoutFromEnv(): number {
const definedTimeout =
Number(getEnv().OTEL_EXPORTER_OTLP_TRACES_TIMEOUT ||
svetlanabrennan marked this conversation as resolved.
Show resolved Hide resolved
getEnv().OTEL_EXPORTER_OTLP_TIMEOUT);

if (definedTimeout) {
svetlanabrennan marked this conversation as resolved.
Show resolved Hide resolved
if (definedTimeout <= 0) {
// OTLP exporter configured timeout - using default value of 10000ms
return invalidTimeout(definedTimeout, DEFAULT_TRACE_TIMEOUT);
}
return definedTimeout;
} else {
return getEnv().OTEL_EXPORTER_OTLP_TRACES_TIMEOUT;
}
}

// OTLP exporter configured timeout - using default value of 10000ms
function invalidTimeout(timeout: number, defaultTimeout: number): number {
diag.warn('Timeout must be non-negative', timeout);

return defaultTimeout;
}
Loading