Repository: DataDog/datadogpy

Issue:
Error handling: APIError is never raised
This library never returns the status code of the http request.
Instead, when the status code is 400, 403 or 404, it is supposed to raise an APIError exception with the content of the errors field received from the dispatcher.(https://github.com/DataDog/datadogpy/blob/master/datadog/api/base.py#L152).
For all other 3, 4, 5** status codes, an HTTPError is raised.

So catching errors and interacting with the API should be a matter of catching APIError and HTTPError exceptions.

BUT, the `_swallow` attribute prevents APIError to be raised (https://github.com/DataDog/datadogpy/blob/master/datadog/api/base.py#L171-L172). It's set to True in api/__init__.py and is not modifiable at the moment.

A temporary workaround is to use `from datadog import api api._swallow = False`.

We could evaluate the impact of a potential transition to swallow being True by default and also being set up when initializing the api.

Discussion:

Patch:
diff --git a/datadog/__init__.py b/datadog/__init__.py
index 92c4d55..2dbe54e 100644
--- a/datadog/__init__.py
+++ b/datadog/__init__.py
@@ -7,14 +7,17 @@ It contains:
 without hindering performance.
 * datadog.dogshell: a command-line tool, wrapping datadog.api, to interact with Datadog REST API.
 """
+# stdlib
 from pkg_resources import get_distribution, DistributionNotFound
 import os
 import os.path
 
+# datadog
 from datadog import api
 from datadog.dogstatsd import statsd
 from datadog.threadstats import ThreadStats  # noqa
 from datadog.util.hostname import get_hostname
+from datadog.util.compat import iteritems
 
 
 try:
@@ -32,7 +35,7 @@ else:
 
 
 def initialize(api_key=None, app_key=None, host_name=None, api_host=None,
-               proxies=None, statsd_host=None, statsd_port=None, cacert=True):
+               statsd_host=None, statsd_port=None, **kwargs):
     """
     Initialize and configure Datadog.api and Datadog.statsd modules
 
@@ -58,17 +61,23 @@ def initialize(api_key=None, app_key=None, host_name=None, api_host=None,
     certificates. Can also be set to True (default) to use the systems
     certificate store, or False to skip SSL verification
     :type cacert: path or boolean
+
+    :param mute: Mute any exceptions before they escape from the library (default: True).
+    :type mute: boolean
     """
-    # Configure api
+    # API configuration
     api._api_key = api_key if api_key is not None else os.environ.get('DATADOG_API_KEY')
     api._application_key = app_key if app_key is not None else os.environ.get('DATADOG_APP_KEY')
     api._host_name = host_name if host_name is not None else get_hostname()
     api._api_host = api_host if api_host is not None else \
         os.environ.get('DATADOG_HOST', 'https://app.datadoghq.com')
-    api._proxies = proxies
-    api._cacert = cacert
 
-    # Given statsd_host and statsd_port, overrides statsd instance
+    # Statsd configuration -overrides default statsd instance attributes-
     if statsd_host and statsd_port:
         statsd.host = statsd_host
         statsd.port = int(statsd_port)
+
+    # HTTP client and API options
+    for key, value in iteritems(kwargs):
+        attribute = "_{0}".format(key)
+        setattr(api, attribute, value)
diff --git a/datadog/api/__init__.py b/datadog/api/__init__.py
index 9570eff..3ce7b85 100644
--- a/datadog/api/__init__.py
+++ b/datadog/api/__init__.py
@@ -14,7 +14,7 @@ _timeout = 3
 _max_timeouts = 3
 _max_retries = 3
 _backoff_period = 300
-_swallow = True
+_mute = True
 
 # Resources
 from datadog.api.comments import Comment
diff --git a/datadog/api/base.py b/datadog/api/base.py
index 4c312ff..b618186 100644
--- a/datadog/api/base.py
+++ b/datadog/api/base.py
@@ -61,7 +61,7 @@ class HTTPClient(object):
 
             # Import API, User and HTTP settings
             from datadog.api import _api_key, _application_key, _api_host, \
-                _swallow, _host_name, _proxies, _max_retries, _timeout, \
+                _mute, _host_name, _proxies, _max_retries, _timeout, \
                 _cacert
 
             # Check keys and add then to params
@@ -124,6 +124,7 @@ class HTTPClient(object):
                 raise HttpTimeout('%s %s timed out after %d seconds.' % (method, url, _timeout))
             except requests.exceptions.HTTPError as e:
                 if e.response.status_code in (400, 403, 404):
+                    # This gets caught afterwards and raises an ApiError exception
                     pass
                 else:
                     raise
@@ -160,7 +161,7 @@ class HTTPClient(object):
                 return response_formatter(response_obj)
 
         except ClientError as e:
-            if _swallow:
+            if _mute:
                 log.error(str(e))
                 if error_formatter is None:
                     return {'errors': e.args[0]}
@@ -169,7 +170,7 @@ class HTTPClient(object):
             else:
                 raise
         except ApiError as e:
-            if _swallow:
+            if _mute:
                 for error in e.args[0]['errors']:
                     log.error(str(error))
                 if error_formatter is None:


Test patch:
diff --git a/tests/unit/api/helper.py b/tests/unit/api/helper.py
index 6db98ca..624c160 100644
--- a/tests/unit/api/helper.py
+++ b/tests/unit/api/helper.py
@@ -5,6 +5,7 @@ import unittest
 from datadog import initialize, api
 from datadog.api.base import CreateableAPIResource, UpdatableAPIResource, DeletableAPIResource,\
     GetableAPIResource, ListableAPIResource, ActionAPIResource
+from datadog.api.exceptions import ApiError
 from datadog.util.compat import iteritems, json
 
 # 3p
@@ -20,11 +21,16 @@ FAKE_PROXY = {
 }
 
 
-class MockReponse(requests.Response):
-    content = None
+class MockResponse(requests.Response):
+
+    def __init__(self, raise_for_status=False):
+        super(MockResponse, self).__init__()
+        self._raise_for_status = raise_for_status
 
     def raise_for_status(self):
-        pass
+        if not self._raise_for_status:
+            return
+        raise ApiError({'errors': ""})
 
 
 # A few API Resources
@@ -68,32 +74,42 @@ class DatadogAPITestCase(unittest.TestCase):
         self.request_patcher = patch('requests.Session')
         request_class_mock = self.request_patcher.start()
         self.request_mock = request_class_mock.return_value
-        self.request_mock.request = Mock(return_value=MockReponse())
+        self.request_mock.request = Mock(return_value=MockResponse())
 
-    def get_request_data(self):
+    def tearDown(self):
+        self.request_patcher.stop()
+
+    def arm_requests_to_raise(self):
         """
+        Arm the mocked request to raise for status.
+        """
+        self.request_mock.request = Mock(return_value=MockResponse(raise_for_status=True))
 
+    def get_request_data(self):
+        """
+        Returns JSON formatted data from the submitted `requests`.
         """
         _, kwargs = self.request_mock.request.call_args
         return json.loads(kwargs['data'])
 
     def request_called_with(self, method, url, data=None, params=None):
         (req_method, req_url), others = self.request_mock.request.call_args
-        assert method == req_method, req_method
-        assert url == req_url, req_url
+        self.assertEquals(method, req_method, req_method)
+        self.assertEquals(url, req_url, req_url)
 
         if data:
-            assert 'data' in others
-            assert json.dumps(data) == others['data'], others['data']
+            self.assertIn('data', others)
+            self.assertEquals(json.dumps(data), others['data'], others['data'])
 
         if params:
-            assert 'params' in others
+            self.assertIn('params', others)
             for (k, v) in iteritems(params):
-                assert k in others['params'], others['params']
-                assert v == others['params'][k]
+                self.assertIn(k, others['params'], others['params'])
+                self.assertEquals(v, others['params'][k])
 
-    def tearDown(self):
-        self.request_patcher.stop()
+    def assertIn(self, first, second, msg=None):
+        msg = msg or "{0} not in {1}".format(first, second)
+        self.assertTrue(first in second, msg)
 
 
 class DatadogAPINoInitialization(DatadogAPITestCase):
diff --git a/tests/unit/api/test_api.py b/tests/unit/api/test_api.py
index b34a49a..c2566f7 100644
--- a/tests/unit/api/test_api.py
+++ b/tests/unit/api/test_api.py
@@ -7,12 +7,11 @@ from time import time
 
 # 3p
 import mock
-from nose.tools import assert_raises, assert_true, assert_false
 
 # datadog
 from datadog import initialize, api
 from datadog.api import Metric
-from datadog.api.exceptions import ApiNotInitialized
+from datadog.api.exceptions import ApiError, ApiNotInitialized
 from datadog.util.compat import is_p3k
 from tests.unit.api.helper import (
     DatadogAPIWithInitialization,
@@ -51,21 +50,27 @@ def preserve_environ_datadog(func):
 
 class TestInitialization(DatadogAPINoInitialization):
 
-    def test_no_initialization_fails(self, test='sisi'):
-        assert_raises(ApiNotInitialized, MyCreatable.create)
+    def test_no_initialization_fails(self):
+        """
+        Raise ApiNotInitialized exception when `initialize` has not ran or no API key was set.
+        """
+        self.assertRaises(ApiNotInitialized, MyCreatable.create)
 
         # No API key => only stats in statsd mode should work
         initialize()
         api._api_key = None
-        assert_raises(ApiNotInitialized, MyCreatable.create)
+        self.assertRaises(ApiNotInitialized, MyCreatable.create)
 
         # Finally, initialize with an API key
         initialize(api_key=API_KEY, api_host=API_HOST)
         MyCreatable.create()
-        assert self.request_mock.request.call_count == 1
+        self.assertEquals(self.request_mock.request.call_count, 1)
 
     @mock.patch('datadog.util.config.get_config_path')
     def test_get_hostname(self, mock_config_path):
+        """
+        API hostname parameter fallback with Datadog Agent hostname when available.
+        """
         # Generate a fake agent config
         tmpfilepath = os.path.join(tempfile.gettempdir(), "tmp-agentconfig")
         with open(tmpfilepath, "wb") as f:
@@ -79,31 +84,64 @@ class TestInitialization(DatadogAPINoInitialization):
         mock_config_path.return_value = tmpfilepath
 
         initialize()
-        assert api._host_name == HOST_NAME, api._host_name
+        self.assertEquals(api._host_name, HOST_NAME, api._host_name)
 
     def test_request_parameters(self):
-        # Test API, application keys, API host and proxies
-        initialize(api_key=API_KEY, app_key=APP_KEY, api_host=API_HOST, proxies=FAKE_PROXY)
+        """
+        API parameters are set with `initialize` method.
+        """
+        # Test API, application keys, API host, and some HTTP client options
+        initialize(api_key=API_KEY, app_key=APP_KEY, api_host=API_HOST)
 
         # Make a simple API call
         MyCreatable.create()
 
         _, options = self.request_mock.request.call_args
 
-        assert 'params' in options
+        # Assert `requests` parameters
+        self.assertIn('params', options)
 
-        assert 'api_key' in options['params']
-        assert options['params']['api_key'] == API_KEY
-        assert 'application_key' in options['params']
-        assert options['params']['application_key'] == APP_KEY
+        self.assertIn('api_key', options['params'])
+        self.assertEquals(options['params']['api_key'], API_KEY)
+        self.assertIn('application_key', options['params'])
+        self.assertEquals(options['params']['application_key'], APP_KEY)
 
-        assert 'proxies' in options
-        assert options['proxies'] == FAKE_PROXY
+        self.assertIn('headers', options)
+        self.assertEquals(options['headers'], {'Content-Type': 'application/json'})
 
-        assert 'headers' in options
-        assert options['headers'] == {'Content-Type': 'application/json'}
+    def test_initialize_options(self):
+        """
+        HTTP client and API options are set with `initialize` method.
+        """
+        initialize(api_key=API_KEY, app_key=APP_KEY, api_host=API_HOST,
+                   proxies=FAKE_PROXY, cacert=False)
+
+        # Make a simple API call
+        MyCreatable.create()
+
+        _, options = self.request_mock.request.call_args
+
+        # Assert `requests` parameters
+        self.assertIn('proxies', options)
+        self.assertEquals(options['proxies'], FAKE_PROXY)
+
+        self.assertIn('verify', options)
+        self.assertEquals(options['verify'], False)
+
+        # Arm the `requests` to raise
+        self.arm_requests_to_raise()
+
+        # No exception should be raised (mute=True by default)
+        MyCreatable.create()
+
+        # Repeat with mute to False
+        initialize(api_key=API_KEY, mute=False)
+        self.assertRaises(ApiError, MyCreatable.create)
 
     def test_initialization_from_env(self):
+        """
+        Set API parameters in `initialize` from environment variables.
+        """
         @preserve_environ_datadog
         def test_api_params_from_env(env_name, attr_name, env_value):
             """
@@ -156,6 +194,9 @@ class TestInitialization(DatadogAPINoInitialization):
 class TestResources(DatadogAPIWithInitialization):
 
     def test_creatable(self):
+        """
+        Creatable resource logic.
+        """
         MyCreatable.create(mydata="val")
         self.request_called_with('POST', "host/api/v1/creatables", data={'mydata': "val"})
 
@@ -164,28 +205,43 @@ class TestResources(DatadogAPIWithInitialization):
                                  data={'mydata': "val", 'host': api._host_name})
 
     def test_getable(self):
+        """
+        Getable resource logic.
+        """
         getable_object_id = 123
         MyGetable.get(getable_object_id, otherparam="val")
         self.request_called_with('GET', "host/api/v1/getables/" + str(getable_object_id),
                                  params={'otherparam': "val"})
 
     def test_listable(self):
+        """
+        Listable resource logic.
+        """
         MyListable.get_all(otherparam="val")
         self.request_called_with('GET', "host/api/v1/listables", params={'otherparam': "val"})
 
     def test_updatable(self):
+        """
+        Updatable resource logic.
+        """
         updatable_object_id = 123
         MyUpdatable.update(updatable_object_id, params={'myparam': "val1"}, mydata="val2")
         self.request_called_with('PUT', "host/api/v1/updatables/" + str(updatable_object_id),
                                  params={'myparam': "val1"}, data={'mydata': "val2"})
 
     def test_detalable(self):
+        """
+        Deletable resource logic.
+        """
         deletable_object_id = 123
         MyDeletable.delete(deletable_object_id, otherparam="val")
         self.request_called_with('DELETE', "host/api/v1/deletables/" + str(deletable_object_id),
                                  params={'otherparam': "val"})
 
     def test_actionable(self):
+        """
+        Actionable resource logic.
+        """
         actionable_object_id = 123
         MyActionable.trigger_class_action('POST', "actionname", id=actionable_object_id,
                                           mydata="val")
@@ -214,17 +270,18 @@ class TestMetricResource(DatadogAPIWithInitialization):
         payload = self.get_request_data()
 
         for i, metric in enumerate(payload['series']):
-            assert set(metric.keys()) == set(['metric', 'points', 'host'])
+            self.assertEquals(set(metric.keys()), set(['metric', 'points', 'host']))
 
-            assert metric['metric'] == serie[i]['metric']
-            assert metric['host'] == api._host_name
+            self.assertEquals(metric['metric'], serie[i]['metric'])
+            self.assertEquals(metric['host'], api._host_name)
 
             # points is a list of 1 point
-            assert isinstance(metric['points'], list) and len(metric['points']) == 1
+            self.assertTrue(isinstance(metric['points'], list))
+            self.assertEquals(len(metric['points']), 1)
             # it consists of a [time, value] pair
-            assert len(metric['points'][0]) == 2
+            self.assertEquals(len(metric['points'][0]), 2)
             # its value == value we sent
-            assert metric['points'][0][1] == serie[i]['points']
+            self.assertEquals(metric['points'][0][1], serie[i]['points'])
             # it's time not so far from current time
             assert now - 1 < metric['points'][0][0] < now + 1