librato / python-librato

A Python wrapper for the Librato Metrics API.
Other
72 stars 31 forks source link

Alerts have issues with services and conditions #141

Closed brmurphy closed 8 years ago

brmurphy commented 8 years ago

When attempting to create an alert with services and conditions params, json deserialize would fail.

  File "/usr/local/lib/python2.7/site-packages/librato/__init__.py", line 424, in create_alert
    resp = self._mexe("alerts", method="POST", query_props=payload)
  File "/usr/local/lib/python2.7/site-packages/librato/__init__.py", line 188, in _mexe
    resp = self._make_request(conn, path, headers, query_props, method)
  File "/usr/local/lib/python2.7/site-packages/librato/__init__.py", line 149, in _make_request
    body = json.dumps(query_props)
  File "/usr/local/Cellar/python/2.7.12/Frameworks/Python.framework/Versions/2.7/lib/python2.7/json/__init__.py", line 244, in dumps
    return _default_encoder.encode(obj)
  File "/usr/local/Cellar/python/2.7.12/Frameworks/Python.framework/Versions/2.7/lib/python2.7/json/encoder.py", line 207, in encode
    chunks = self.iterencode(o, _one_shot=True)
  File "/usr/local/Cellar/python/2.7.12/Frameworks/Python.framework/Versions/2.7/lib/python2.7/json/encoder.py", line 270, in iterencode
    return _iterencode(o, 0)
  File "/usr/local/Cellar/python/2.7.12/Frameworks/Python.framework/Versions/2.7/lib/python2.7/json/encoder.py", line 184, in default
    raise TypeError(repr(o) + " is not JSON serializable")
TypeError: <librato.alerts.Service object at 0x10ead3110> is not JSON serializable

In order to fix this, the conditions needed to be serialized correctly, and the services needed serialization AND encoding for id only.

And of course, add some tests for them.

Also, the Alert.from_dict method referenced type, not condition_type, which I think was introduced in a78e2d13662c3181f257c88c61e2367893647f6e

jderrett commented 8 years ago

@brmurphy thanks for this! I'll try to get this merged shortly.

brmurphy commented 8 years ago

something isn't adding up here with my condition_type fix. The response back from the api is different.

When I query via GET, I receive

{"id":<redact>,"type":"above","metric_name":"<redact>","source":"beta","threshold":50000.0,"duration":1200,"summary_function":"max"}

But to post, I need to use

{"metric_name": "<redact>", "duration": 1200, "source": "beta", "summary_function": "max", "condition_type": "above","threshold":50000.0}

Let me dig into this some more

jderrett commented 8 years ago

I think I have a branch that will fix this which I can put up tomorrow. The API "wants" to use type and you should be able to POST with type and just not use condition_type (which we can say is just used internally within the class). Ideally I'd like to be consistent but as this is contributed code (which we're super grateful for) we may not always fully vet everything.

In any case, incoming PR from me...

jderrett commented 8 years ago

@brmurphy ok, tried to address this in #142, let me know if that might work for you.

jderrett commented 8 years ago

Closing this in favor of #142