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

log full stack traces on error #37

Merged
merged 3 commits into from
Sep 30, 2015
Merged

Conversation

dcrissman
Copy link
Member

No description provided.

@dcrissman dcrissman self-assigned this Sep 29, 2015
@dcrissman
Copy link
Member Author

A stacktrace would be very useful for a bug I am tracking down.

@bserdar, @jewzaam, @paterczm - This is ready for review.

@jewzaam
Copy link
Member

jewzaam commented Sep 29, 2015

@dcrissman what is the impact of throwing a runtime exception? The concern I have is that it will get back to the client. We do not want to have a request fail because a hook fails, this is a bad user experience. It will be confusing. Does the hook processing ignore errors thrown by hook implementations?

@dcrissman
Copy link
Member Author

The concern I have is that it will get back to the client.

The Mediator wraps all CRUD operations in a try/catch and handles accordingly. That said I should probably use Error.get instead.

We do not want to have a request fail because a hook fails, this is a bad user experience.

I would say it depends on the hook. In the case of the esb hook, if an event was missed I would think that would be pretty bad.

@jewzaam
Copy link
Member

jewzaam commented Sep 30, 2015

Clients don't care about integration implications. They just want their
data request to occur. If the hook is firing it means their data has been
persisted. Telling them a hook failed has no value to the client. It
should be logged and ideally monitored. I think of it in terms of a
relational database. If you have a trigger on a table, the client knows
nothing about it. If the trigger gets into an invalid / broken state
operations against that table fail. From an operations point of view, the
quickest fix is to disable the trigger if it can't be quickly fixed. We
have an opportunity with lightblue hooks to not impact the client when
something in a hook is broken. Ergo, don't throw unchecked exceptions.

On Wed, Sep 30, 2015 at 8:05 AM, Dennis Crissman notifications@github.com
wrote:

The concern I have is that it will get back to the client.

The Mediator wraps all CRUD operations in a try/catch and handles
accordingly. That said I should probably use Error.get instead.

We do not want to have a request fail because a hook fails, this is a bad
user experience.

I would say it depends on the hook. In the case of the esb hook, if an
event was missed I would think that would be pretty bad.


Reply to this email directly or view it on GitHub
#37 (comment)
.

for (Error e : r.getErrors()) {
LOGGER.error(e.toString());
LOGGER.error("Lightblue Error", e);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A more descriptive error message would be useful here. Same for the next one,

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can use a different logger, and send the error logs to a separate logfile?

@dcrissman
Copy link
Member Author

@bserdar, @jewzaam - Requested changes made.

bserdar added a commit that referenced this pull request Sep 30, 2015
log full stack traces on error
@bserdar bserdar merged commit b53dd40 into lightblue-platform:master Sep 30, 2015
@dcrissman dcrissman deleted the log branch September 30, 2015 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants