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

Extract fields from errors #237

Merged
merged 13 commits into from
Nov 27, 2015
Merged

Extract fields from errors #237

merged 13 commits into from
Nov 27, 2015

Conversation

itamarst
Copy link
Owner

Fixes #236.

This will allow e.g. including HTTP response code as a field in failed actions due to flocker.restapi._error.BadRequest.

extract_fields_for_failures(MyException, lambda e: {"code": e.code})

If no extraction function is registered for a class Eliot will look for registered functions for its base classes.
By default Eliot will automatically extract fields from ``OSError``, ``IOError`` and other subclasses of Python's ``EnvironmentError``.
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you considered defaulting to extracting .args?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Hm. We already do repr(exception) for the reason... but that does leave out some info.

Copy link
Owner Author

Choose a reason for hiding this comment

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

so {"args": safe_repr(exception.args)} as default?

Copy link
Contributor

Choose a reason for hiding this comment

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

That was my thought. I haven't looked at examples or anything, to see if it reasonable. Actually, {"args": map(safe_repr, exception.args)} maybe even (typically args is a tuple, so turning into a structured thing would be nice).

@itamarst
Copy link
Owner Author

Also occurs to me that eliot:traceback ought to be using this as well.

@@ -140,3 +140,26 @@ You can add fields to both the start message and the success message of an actio

If you want to include some extra information in case of failures beyond the exception you can always log a regular message with that information.
Since the message will be recorded inside the context of the action its information will be clearly tied to the result of the action by the person (or code!) reading the logs later on.


.. _extract errors:
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ Why not use error-handling as the section reference?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see that you get that for free from the title below.
The reference is there purely for linking from the release notes.

@wallrj
Copy link
Contributor

wallrj commented Nov 25, 2015

Thanks @itamarst

Looks good. A few points:

  1. extractor is spelt extracter in various parts of the code. Needs fixing.
  2. looks like there might be a mistake in the SubSubException test.
  3. ❓ I don't find the name of the extractor registration function intuitive.
  4. ❓ Shouldn't this be more closely tied in with the existing Eliot Types / Fields / serialisation system?
  5. ❓ Currently EnvironmentError 's will be logged with their full repr. Will we lose some important messages with the new default extractor?

Please address or answer the points above (including Tom's) and then merge.

@itamarst
Copy link
Owner Author

The type system is a tricky one, yeah... Requiring every ActionType to specify every possible exception-extracted field seems unreasonable, seems better to do it on per-exception level.

However, there's still issue with adding these new fields to eliot:traceback since it has a MessageType with fixed fields...

@itamarst
Copy link
Owner Author

The type system is to help:

  1. Users make sure they generate correct logs.
  2. Document what is being generated.

First use case seems irrelevant here, since it's errors coming out of the running system - unexpected results don't mean a bug in logging code. Second use case is still relevant though.

@itamarst
Copy link
Owner Author

As far as having default that uses Exception.args... the default __str__ or maybe __repr__ for Exception already shows those. So the information isn't lost, although it is in less structured format. On the other hand I assume most exceptions that rely on that just dump strings in, so doesn't seem like including safe_repr of args by default in addition to safe_repr of the exception instance adds much. We can always add it later.

itamarst added a commit that referenced this pull request Nov 27, 2015
@itamarst itamarst merged commit 4ac4069 into master Nov 27, 2015
@itamarst itamarst deleted the extract-fields-from-errors-236 branch November 27, 2015 00:21
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.

3 participants