Use exceptions to report shutdown result (#2599)

* Use exceptions to report shutdown result

Fixes #2406

* Fix mypy

* Make exceptions private

* Fix exceptions module path

* Remove exceptions and show failed metric readers

Author: ocelotl

opentelemetry-sdk/src/opentelemetry/sdk/_metrics/__init__.py

@@ -315,25 +315,39 @@ def _shutdown():
 
         if not did_shutdown:
             _logger.warning("shutdown can only be called once")
-            return False
+            return
 
-        overall_result = True
+        metric_reader_error = {}
 
         for metric_reader in self._sdk_config.metric_readers:
-            metric_reader_result = metric_reader.shutdown()
+            try:
+                metric_reader.shutdown()
 
-            if not metric_reader_result:
-                _logger.warning(
-                    "MetricReader %s failed to shutdown", metric_reader
-                )
+            # pylint: disable=broad-except
+            except Exception as error:
 
-            overall_result = overall_result and metric_reader_result
+                metric_reader_error[metric_reader] = error
 
         if self._atexit_handler is not None:
             unregister(self._atexit_handler)
             self._atexit_handler = None
 
-        return overall_result
+        if metric_reader_error:
+
+            metric_reader_error_string = "\n".join(
+                [
+                    f"{metric_reader.__class__.__name__}: {repr(error)}"
+                    for metric_reader, error in metric_reader_error.items()
+                ]
+            )
+
+            raise Exception(
+                (
+                    "MeterProvider.shutdown failed because the following "
+                    "metric readers failed during shutdown:\n"
+                    f"{metric_reader_error_string}"
+                )
+            )
 
     def get_meter(
         self,

opentelemetry-sdk/src/opentelemetry/sdk/_metrics/export/__init__.py

@@ -122,8 +122,8 @@ def _receive_metrics(self, metrics: Iterable[Metric]):
         with self._lock:
             self._metrics = list(metrics)
 
-    def shutdown(self) -> bool:
-        return True
+    def shutdown(self):
+        pass
 
 
 class PeriodicExportingMetricReader(MetricReader):
@@ -193,16 +193,15 @@ def _receive_metrics(self, metrics: Iterable[Metric]) -> None:
             _logger.exception("Exception while exporting metrics %s", str(e))
         detach(token)
 
-    def shutdown(self) -> bool:
+    def shutdown(self):
         def _shutdown():
             self._shutdown = True
 
         did_set = self._shutdown_once.do_once(_shutdown)
         if not did_set:
             _logger.warning("Can't shutdown multiple times")
-            return False
+            return
 
         self._shutdown_event.set()
         self._daemon_thread.join()
         self._exporter.shutdown()
-        return True

opentelemetry-sdk/src/opentelemetry/sdk/_metrics/metric_reader.py

@@ -65,7 +65,7 @@ def _receive_metrics(self, metrics: Iterable[Metric]):
         """Called by `MetricReader.collect` when it receives a batch of metrics"""
 
     @abstractmethod
-    def shutdown(self) -> bool:
+    def shutdown(self):
         """Shuts down the MetricReader. This method provides a way
         for the MetricReader to do any cleanup required. A metric reader can
         only be shutdown once, any subsequent calls are ignored and return

opentelemetry-sdk/tests/metrics/test_in_memory_metric_reader.py

@@ -63,7 +63,7 @@ def test_converts_metrics_to_list(self):
 
     def test_shutdown(self):
         # shutdown should always be successful
-        self.assertTrue(InMemoryMetricReader().shutdown())
+        self.assertIsNone(InMemoryMetricReader().shutdown())
 
     def test_integration(self):
         reader = InMemoryMetricReader()

opentelemetry-sdk/tests/metrics/test_metrics.py

@@ -151,33 +151,45 @@ def test_shutdown(self):
 
         mock_metric_reader_0 = MagicMock(
             **{
-                "shutdown.return_value": False,
-                "__str__.return_value": "mock_metric_reader_0",
+                "shutdown.side_effect": ZeroDivisionError(),
+            }
+        )
+        mock_metric_reader_1 = MagicMock(
+            **{
+                "shutdown.side_effect": AssertionError(),
             }
         )
-        mock_metric_reader_1 = Mock(**{"shutdown.return_value": True})
 
         meter_provider = MeterProvider(
             metric_readers=[mock_metric_reader_0, mock_metric_reader_1]
         )
 
-        with self.assertLogs(level=WARNING) as log:
-            self.assertFalse(meter_provider.shutdown())
-            self.assertEqual(
-                log.records[0].getMessage(),
-                "MetricReader mock_metric_reader_0 failed to shutdown",
-            )
+        with self.assertRaises(Exception) as error:
+            meter_provider.shutdown()
+
+        error = error.exception
+
+        self.assertEqual(
+            str(error),
+            (
+                "MeterProvider.shutdown failed because the following "
+                "metric readers failed during shutdown:\n"
+                "MagicMock: ZeroDivisionError()\n"
+                "MagicMock: AssertionError()"
+            ),
+        )
+
         mock_metric_reader_0.shutdown.assert_called_once()
         mock_metric_reader_1.shutdown.assert_called_once()
 
-        mock_metric_reader_0 = Mock(**{"shutdown.return_value": True})
-        mock_metric_reader_1 = Mock(**{"shutdown.return_value": True})
+        mock_metric_reader_0 = Mock()
+        mock_metric_reader_1 = Mock()
 
         meter_provider = MeterProvider(
             metric_readers=[mock_metric_reader_0, mock_metric_reader_1]
         )
 
-        self.assertTrue(meter_provider.shutdown())
+        self.assertIsNone(meter_provider.shutdown())
         mock_metric_reader_0.shutdown.assert_called_once()
         mock_metric_reader_1.shutdown.assert_called_once()