librato / python-librato

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

no timeout set? #93

Closed phoet closed 7 years ago

phoet commented 9 years ago

it looks like the client api does not have any connection timeouts set? is that correct, i'm not a python expert?

https://github.com/librato/python-librato/blob/master/librato/__init__.py#L181-L185

drio commented 9 years ago

Hi @phoet,

Right here: https://github.com/librato/python-librato/blob/master/librato/__init__.py#L156

phoet commented 9 years ago

that looks even worse, because it's re-trying to send the data with a backoff strategy?

phoet commented 9 years ago

in case of a limit-reached api response, which is a 403, would this code just raise an error? i think it should be handled gracefully https://github.com/librato/python-librato/blob/master/librato/__init__.py#L149-L151

drio commented 9 years ago

We will add a timeouts for the retry logic. Feel free to send a PR if you have coded something up already.

phoet commented 9 years ago

thx for the fast reply!

drio commented 9 years ago

should be handled gracefully

What do yo propose ?

phoet commented 9 years ago

i'm not familiar enough with the code at the moment. what i propose would be to handle 403 api limit exceeded requests by not raising an exception that bubbles up into the main app.

i'd be ok with logging an error instead of raising an exception for api limits. what do you think?

drio commented 9 years ago

Let me take a look to how this is handled in other bindings to see if we can find a more sensible way to handle this.

Thanks @phoet!

phoet commented 9 years ago

thx, that would be great

nextmat commented 9 years ago

/cc @obfuscurity yeah, would be nice to handle this kind of behavior consistently across our different bindings.

The ruby one currently raises status-specific exceptions so users can treat the common cases individually, not sure if that makes sense here?

drio commented 9 years ago

The ruby one currently raises status-specific exceptions so users can treat the common cases individually, not sure if that makes sense here?

:+1:

obfuscurity commented 9 years ago

@nextmat Agreed, we can discuss plans offline. :wink:

drio commented 9 years ago

@phoet Just merged a PR that enables timeouts. Can you please give it a try?

phoet commented 9 years ago

cool, thx. i won't have time next week because i'm on vacation, will give it a spin afterwards