BatchSpanProcessor ExportResult other than SUCCESS causes UnhandledPromiseRejectionWarning: 1

[johanneswuerbach]

With @opentelemtry/tracing v0.12.0 we are observing unhandled promise rejections caused by the BatchSpanExporter not exporting results successfully https://github.com/open-telemetry/opentelemetry-js/blob/master/packages/opentelemetry-tracing/src/export/BatchSpanProcessor.ts#L110

While we use Node 14, we already exit on unhandled promise rejections as done by default on Node 15 (nodejs/TSC#916) so an unhandled rejection is causing our processes to exit. Additionally the issue is challenging to pin down as the rejection value is a plain number, so the generated stack traces aren't really valuable.

Example trace:

Error: 1
  File "/app/node_modules/raven/lib/client.js", line 409, col 15, in Raven.captureException
    err = new Error(err);
  File "/app/node_modules/REMOVED/built/lib/instrument-process.js", line 13, col 30, in sentryId
    sentryClient.captureException(error, {
  ?, in new Promise
  File "/app/node_modules/REMOVED/built/lib/instrument-process.js", line 12, col 36, in null.<anonymous>
    const sentryId = await new Promise((resolve, reject) => {
  File "/app/node_modules/REMOVED/built/lib/instrument-process.js", line 67, col 16, in process.<anonymous>
    return exitFatal(err, 'unhandledRejection');
  File "events.js", line 315, col 20, in process.emit
  File "domain.js", line 486, col 12, in process.EventEmitter.emit
  File "internal/process/promises.js", line 245, col 33, in processPromiseRejections
  File "internal/process/task_queues.js", line 94, col 32, in processTicksAndRejections

How would a fix here look like? Add .catch() to each invocation of this._flush() as done here https://github.com/open-telemetry/opentelemetry-js/blob/master/packages/opentelemetry-tracing/src/export/BatchSpanProcessor.ts#L121? Or just remove the reject https://github.com/open-telemetry/opentelemetry-js/blob/master/packages/opentelemetry-tracing/src/export/BatchSpanProcessor.ts#L110 and rely on the global error logger (which similar to #1569 I assume)?

We are using the jaeger exporter, which is already logging the error itself https://github.com/open-telemetry/opentelemetry-js/blob/master/packages/opentelemetry-exporter-jaeger/src/jaeger.ts#L139-L141

[vmarchaud]

Do you plan to work on this @johanneswuerbach ?

[johanneswuerbach]

I would have some capacity, but I would need some guidance what the solution should be here. Just remove the rejection?

[vmarchaud]

@johanneswuerbach The way to go should be to use the globalErrorHandler like the metric do: https://github.com/open-telemetry/opentelemetry-js/blob/master/packages/opentelemetry-metrics/src/export/Controller.ts#L57

[johanneswuerbach]

Thanks @vmarchaud, PR created #1622