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

Adding exception loggging #31

Closed

Conversation

adamcharnock
Copy link

The error reporting code swallows exceptions in order to report them to the client. When debugging (and when getting started with graphql as I am) it is very useful to be able to see the exception stack traces.

I have therefore added a single logging statement to Executor.run_resolve_fn(). Is it possible that logging is also required elsewhere, I'm happy to take any advice this.

The error reporting code swallows exceptions in order to report them to
the client. When debugging (and when getting started with graphql as I
am) it is very useful to be able to see the exception stack traces.

I have therefore added a single logging statement to
`Executor.run_resolve_fn()`. Is it possible that logging is also
required elsewhere, I'm happy to take any advice this.
@jhgg
Copy link
Member

jhgg commented Nov 16, 2015

Why log.info over log.error?

@jhgg
Copy link
Member

jhgg commented Nov 16, 2015

I don't think this is the right place. Errors thrown from defers won't be caught. Should do the logging in complete_value.

@adamcharnock
Copy link
Author

Thanks for taking a look @jhgg. I have tried moving this to complete_value. However, at that point we no longer have access to the exception info (sys.exc_info) as we our outside the exception handling context. As a result the stack trace logged out is no longer relevant to where the error occurred.

Therefore:

  1. I may be misunderstanding how exception reporting works in Python
  2. Exceptions need to be logged within the except: block
  3. We need to return exc_info along with the exception (if even possible / advisable?)

Also, I initially went with info() because I was thinking that it was more of a user-request error and was not actually causing the process to crash. However, I now think that logic was mistaken. These errors will probably occur if code has erred in a way it should not. If I was running this in production with a log level set to 'WARN' (or some such) I think I would definitely want to see these errors. Therefore I'd be happy to use warn() or above. Opinions welcome, otherwise I'll set it to use exception().

@jhgg
Copy link
Member

jhgg commented Nov 20, 2015

So, there are 3 parts to an exception, the type, the value and the traceback. In order to re-raise an exception with that all in tact, you really just need the value and traceback.

After I catch up with v0.4.13, I am going to look into a better way to do error handling, as I do agree with you 100% that it not where it should be.

That being said, for now, I'm going to close this PR and open up an issue to investigate what's the best way to handle errors properly, and ensure exceptions and tracebacks are forwarded to somewhere that we can adequately capture them.

@jhgg jhgg closed this Nov 20, 2015
@jhgg jhgg mentioned this pull request Nov 20, 2015
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

Successfully merging this pull request may close these issues.

None yet

2 participants