feat: add global error handler

[mwear]

Which problem is this PR solving?

• Adds global error handler Add global error handler #1415

Short description of the changes

• This PR adds a global error handler and updates SDK components to use it. The default global error handler is configured with a ConsoleLogger at LogLevel.ERROR. This might be a questionable default and we can discuss alternatives.
• CollectorExporterError was changed from an interface to a subclass. Other string based log messages were converted to error objects as well. This change means these errors will get a stack that is logged by the default handler.

[codecov]

Codecov Report

Merging #1514 into master will increase coverage by 0.25%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1514      +/-   ##
==========================================
+ Coverage   92.93%   93.19%   +0.25%     
==========================================
  Files         156       82      -74     
  Lines        4871     2161    -2710     
  Branches      988      428     -560     
==========================================
- Hits         4527     2014    -2513     
+ Misses        344      147     -197     

Impacted Files	Coverage Δ
packages/opentelemetry-core/src/common/types.ts	100.00% <ø> (ø)
...ntelemetry-core/src/common/global-error-handler.ts	100.00% <100.00%> (ø)
...telemetry-core/src/common/logging-error-handler.ts	100.00% <100.00%> (ø)
...ry-exporter-collector/src/CollectorExporterBase.ts	91.83% <100.00%> (-0.17%)	⬇️
...ages/opentelemetry-exporter-collector/src/types.ts	100.00% <100.00%> (ø)
...ckages/opentelemetry-exporter-zipkin/src/zipkin.ts	82.22% <0.00%> (-17.78%)	⬇️
...ges/opentelemetry-exporter-jaeger/src/transform.ts
packages/opentelemetry-sdk-node/src/sdk.ts
...-plugin-grpc-js/src/server/clientStreamAndUnary.ts
...plugin-grpc-js/src/client/loadPackageDefinition.ts
... and 69 more

[obecny]

I think the global error handler should be a part of api, otherwise we will always depend on sdk which I think is not desired. We should decouple sdk from api as much as possible.
@dyladan what do you think about moving this to api ?

[mwear]

I think the global error handler should be a part of api, otherwise we will always depend on sdk which I think is not desired. We should decouple sdk from api as much as possible.

I'm fine with wherever we decide to put this, but the global error handler is meant to log errors that come from the SDK. Quoting from #1415:

This error handler is for when the SDK itself errors, not when a monitored entity throws an error.

[dyladan]

I think the global error handler should be a part of api, otherwise we will always depend on sdk which I think is not desired. We should decouple sdk from api as much as possible.

I'm fine with wherever we decide to put this, but the global error handler is meant to log errors that come from the SDK. Quoting from #1415:

This error handler is for when the SDK itself errors, not when a monitored entity throws an error.

I see this one both ways. Where have other SIGs implemented it?

[mwear]

Surveying Python and Go and it's a 50/50 split. Python has it as part of the SDK: open-telemetry/opentelemetry-python#1080. Go has it as part of the API: https://github.com/open-telemetry/opentelemetry-go/blob/master/api/global/handler.go.

[dyladan]

Surveying Python and Go and it's a 50/50 split. Python has it has part of the SDK: open-telemetry/opentelemetry-python#1080. Go has it as part of the API: https://github.com/open-telemetry/opentelemetry-go/blob/master/api/global/handler.go.

Asked in gitter https://gitter.im/open-telemetry/maintainers?at=5f68edb9ce5bbc7ffddc7b9c

[dyladan]

As mentioned by @mwear in gitter, the discussion in open-telemetry/opentelemetry-specification#696 seems to point to including this in the SDK.

edit: I also agree with this as I see this as more of an SDK configuration topic than a tracing API

[vmarchaud]

well i believe it's the case here too, globalErrorHandler should only be called inside the CollectorExporterBase i think

[mwear]

You're right, thanks again! I made another pass through and cleaned up redundant calls to the globalErrorHandler.

[vmarchaud]

I actually forgotten about those path (see #1561):

opentelemetry-js/packages/opentelemetry-metrics/src/export/Controller.ts

Lines 48 to 60 in d8b1be8

	return new Promise((resolve, reject) => {
	this._exporter.export(
	this._meter.getBatcher().checkPointSet(),
	result => {
	if (result === ExportResult.SUCCESS) {
	resolve();
	} else {
	// @todo log error
	reject();
	}
	}
	);
	});

https://github.com/open-telemetry/opentelemetry-js/blob/master/packages/opentelemetry-tracing/src/MultiSpanProcessor.ts#L36

[mwear]

@vmarchaud I made the changes you requested. Let me know if that's what you had in mind. Codecov is coming in slim after the latest round of changes though.

[dyladan]

Codecov is having a service outage but the error handler itself is tested so I think this is fine. If you fix the conflicts this can be merged.

[mwear]

FWIW, I increased the test coverage before rebasing.