@opentelemetry/exporter-collector fails to remove fulfilled promises

[aabmass]

Please answer these questions before submitting a bug report.

What version of OpenTelemetry are you using?

"dependencies": {
  "@opentelemetry/exporter-collector": "^0.14.0",
  "@opentelemetry/tracing": "^0.14.0",
  "@opentelemetry/web": "^0.14.0"
}

What version of Node are you using?

v12.19.0, but the bug appears in the browser (tested on Chrome and Firefox). Strangely, I was not able to repro the problem even though the node exporter looks like it has the same bug.

Please provide the code you used to setup the OpenTelemetry SDK

Code is in a repo

import {BatchSpanProcessor} from '@opentelemetry/tracing';
import {WebTracerProvider} from '@opentelemetry/web';
import {CollectorTraceExporter} from '@opentelemetry/exporter-collector';

const collectorOptions = {
  url: '/v1/trace',
  concurrencyLimit: 5,
};

const provider = new WebTracerProvider();
const exporter = new CollectorTraceExporter(collectorOptions);
provider.addSpanProcessor(
  new BatchSpanProcessor(exporter, {
    bufferSize: 10,
    bufferTimeout: 500,
  })
);

provider.register();

const tracer = provider.getTracer('index.tsx');

setInterval(() => {
  tracer.startSpan('Test').end();
  console.log((exporter as any)._sendingPromises);
}, 1000);

What did you do?

Created spans in the browser and tried to export them with CollectorTraceExporter. The first few spans send alright, but then I see {"stack":"Error: Concurrent export limit reached\n at CollectorTraceExporter.export ... exceptions (from here).

What did you expect to see?

Spans should export indefinitely without any issue. CollectorTraceExporter._sendingPromises list should pop promises as they finish.

What did you see instead?

Instead, the list grows indefinitely with fulfilled promises. This code never runs:

opentelemetry-js/packages/opentelemetry-exporter-collector/src/platform/browser/CollectorExporterBrowserBase.ts

Lines 80 to 84 in a2304c9

	const _onFinish = () => {
	resolve();
	const index = this._sendingPromises.indexOf(promise);
	this._sendingPromises.splice(index, 1);
	};

Additional context

The issue is promise is not initialized when it is used here:

opentelemetry-js/packages/opentelemetry-exporter-collector/src/platform/browser/CollectorExporterBrowserBase.ts

Line 82 in a2304c9

const index = this._sendingPromises.indexOf(promise);

I was able to determine this by wrapping that line in a try/catch and logging the error (it's not great that the error is swallowed without doing this, makes it very difficult to debug):

ReferenceError: Cannot access 'promise' before initialization
    at _onFinish (CollectorExporterBrowserBase.ts:83)
    at _onSuccess (CollectorExporterBrowserBase.ts:74)
    at Object.sendWithBeacon (util.ts:34)
    at CollectorExporterBrowserBase.ts:102
    at new Promise (<anonymous>)
    at CollectorTraceExporter.send (CollectorExporterBrowserBase.ts:71)
    at CollectorExporterBase.ts:107
    at new Promise (<anonymous>)
    at CollectorTraceExporter._export (CollectorExporterBase.ts:104)
    at CollectorTraceExporter.export (CollectorExporterBase.ts:94)

Restructuring this code to have a proper reference to the promise solves the issue. See fix PR #1775

[aabmass]

I'm not able to repro this in node.js for some reason

[aabmass]

Does anyone have an idea what's going on here? It seems like something strange is happening between reference scoping rules, browser JS engines, and compilation. I actually did not see this issue with a production build, so presumably it restructured the code in a way to prevent the issue.

I'm happy to flesh out that PR #1775 if that is the way to go.

[dyladan]

Sorry I meant to look into this but as always there are a lot of balls in the air and I just let this one slip through the cracks.

@obecny have you seen this?

[obecny]

seems like it was missed. I have checked and if this is happening then there are several places where it needs to be fixed not only in CollectorExporterBrowserBase.ts, happy to take it over.

[obecny]

@aabmass your PR is still a draft for so long time so reality is no one will review it when the work is in progress. It needs to be converted to regular PR once you decide it is finished and ready for review. Otherwise people don't know whether it is finished or not.

[aabmass]

@obecny ya it wasn't ready for review, I didn't know if this was just an issue on my side with webpack or babel config. I will update the PR to fix the other occurrence and mark it ready.

[obecny]

@aabmass whatever suits you better, but yr solution can be applied then to few other places, it can be in yr PR, or I can add this after yr PR will be merged, both works fine. The places I'm talking about can be found by simply searching for _sendingPromises in codebase.