Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exception handling #44

Open
IvanRF opened this issue Sep 2, 2017 · 6 comments
Open

Exception handling #44

IvanRF opened this issue Sep 2, 2017 · 6 comments
Milestone

Comments

@IvanRF
Copy link

IvanRF commented Sep 2, 2017

try {
httpResp = execute(req.getUrl(), new UrlEncodedFormEntity(createNameValuePairs(req), StandardCharsets.UTF_8));
resp.setStatusCode(httpResp.getStatusLine().getStatusCode());
} catch (Exception e) {
if (e instanceof UnknownHostException) {
logger.warn("Couldn't connect to Google Analytics. Internet may not be available. " + e.toString());
} else {
logger.warn("Exception while sending the Google Analytics tracker request " + req, e);
}
} finally {

There is no way of knowing what happened if a post() fails. I think you should throw the error and let the caller deal with it. Other option is to set the Exception in a new field on the HttpResponse (which will always be empty in case of a failure).

Check:

@robertvazan
Copy link

I would just add that logging is only one of several possible ways to handle errors. And it's not the best solution here, because random network failures create a lot of log noise. I would prefer to be able to count the exceptions instead and raise alarm only if there are too many of them.

@robertvazan
Copy link

robertvazan commented Jun 6, 2018

As for the actual API, I recommend throwing an exception in synchronous calls and storing it in the returned Future in asynchronous calls.

If that doesn't agree with you, then the next best option is to supply exception handler in config. Default handler would implement the old behavior. People could just rethrow in the handler if they prefer the exception to be propagated.

brsanthu added a commit that referenced this issue May 19, 2019
@brsanthu
Copy link
Owner

Added this api. Any feedback welcome. fb392c2

@brsanthu brsanthu added this to the 2.1 milestone May 20, 2019
@robertvazan
Copy link

Looks good. Could be simpler if you just propagated the exception.

@robertvazan
Copy link

The code reminds me of my own library NoException, which provides many different kinds of exception handlers. It is nevertheless trivial to adapt NoException handlers to the GoogleAnalyticsExceptionHandler interface.

@brsanthu
Copy link
Owner

NoException seems like a nice library but I would be hesitant to add another dependency. But hope functional interface would make it easy to adopt.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants