Review all exporters for unhandled exceptions

[cijothomas]

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/error-handling.md#basic-error-handling-principles

"The SDK MUST NOT throw unhandled exceptions for errors in their own operations. For example, an exporter should not throw an exception when it cannot reach the endpoint to which it sends telemetry data."

This was exposed in #2870 where OTLP exporter crashed the whole process, but opening a dedicated issue to track fixing this. This is separate from the issue about when/how should SDK handle unhandled exceptions when user components behave bad.

This issue is only tracking the core components (as required by the spec) :

• OTLPExporter - Fixed in OTLPExporter unhandled exceptions #2915
• Jaeger - Fix not required. See Review all exporters for unhandled exceptions #2872 (comment)
• Zipkin - Fixed Zipkin Exporter fixes #2935
• Prometheus - Fix not required. See Review all exporters for unhandled exceptions #2872 (comment)

[cijothomas]

@Yun-Ting for OTLPExporter. @alanwest can guide here.

[TimothyMothra]

I can work on Prometheus

[TimothyMothra]

I reviewed the Prometheus exporter.
TLDR; If this is used as designed, there should be no issues.
However, if the exporter is ever invoked while OnExport is set to null, this would throw an exception.

If we want to proceed with caution, we can add a try/catch to the Exporter or add a null check.
I'll follow up with @cijothomas offline to discuss.

---

1. PrometheusExporter.Export will call PrometheusExporter.OnExport without a null check.
   

   opentelemetry-dotnet/src/OpenTelemetry.Exporter.Prometheus/PrometheusExporter.cs

   Lines 76 to 79 in 1b5765b

   	public override ExportResult Export(in Batch<Metric> metrics)
   	{
   	return this.OnExport(metrics);
   	}

2. PrometheusExporter.OnExport should be set to PrometheusCollectionManager.OnCollect which DOES has a try/catch.
   This eventually calls BaseExportingMetricReader.ProcessMetrics which will call exporter.Export

   opentelemetry-dotnet/src/OpenTelemetry.Exporter.Prometheus/Implementation/PrometheusCollectionManager.cs

   Lines 177 to 182 in 1b5765b

   	private ExportResult OnCollect(Batch<Metric> metrics)
   	{
   	int cursor = 0;
   	try
   	{

   opentelemetry-dotnet/src/OpenTelemetry/Metrics/BaseExportingMetricReader.cs

   Lines 81 to 86 in 1b5765b

   	internal override bool ProcessMetrics(in Batch<Metric> metrics, int timeoutMilliseconds)
   	{
   	// TODO: Do we need to consider timeout here?
   	try
   	{
   	return this.exporter.Export(metrics) == ExportResult.Success;

3. The PrometheusCollectionManager has a pattern I'm unfamiliar with.
   This sets PrometheusExporter.OnExport, then calls PrometheusExporter.Collect, and then sets PrometheusExporter.OnExport to null.
   

   opentelemetry-dotnet/src/OpenTelemetry.Exporter.Prometheus/Implementation/PrometheusCollectionManager.cs

   Lines 168 to 175 in 1b5765b

   	[MethodImpl(MethodImplOptions.AggressiveInlining)]
   	private bool ExecuteCollect()
   	{
   	this.exporter.OnExport = this.onCollectRef;
   	bool result = this.exporter.Collect(Timeout.Infinite);
   	this.exporter.OnExport = null;
   	return result;
   	}

[TimothyMothra]

I took a look the JaegerExporter.
It's export method is covered with a try/catch.

opentelemetry-dotnet/src/OpenTelemetry.Exporter.Jaeger/JaegerExporter.cs

Lines 95 to 98 in 6ca4c13

	public override ExportResult Export(in Batch<Activity> activityBatch)
	{
	try
	{

@cijothomas, I think this is safe and can be marked as complete above :)

[cijothomas]

https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry/Metrics/MetricReader.cs#L112-L124 MetricReader is catching all exceptions, so even if a MetricExporter throws, it wont crash. (This is different from Trace/Log as the BatchExporterProcessor doesn't catch exceptions from Exporter.)

[cijothomas]

#2905 another crash from OTLP.