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

Remove unserializable data from metadata in Log records #2469

Merged

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Feb 9, 2019

Fixes #2363

If unserializable data, often found in the exc_info or args keys, is
not removed from the metadata of a log record, a database exception will
be thrown.

@sphuber sphuber requested a review from giovannipizzi February 9, 2019 08:52
@@ -58,8 +58,10 @@ def create_entry_from_record(record):

metadata = dict(record.__dict__)

# Get rid of the exc info because this is usually not serializable
metadata['exc_info'] = None
# Get rid of the `exc_info` and `args` if they exist in the metadata because they are often not serializable
Copy link
Member

Choose a reason for hiding this comment

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

would it be better to to a str or them, so we don;'t completely lose the content?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

except the problem is that I am not sure if this guarantees that that is possible. I think args can literally contain any python object and so it might be possible that even calling str on it might fail right? Given that this leads to a database error I'd rather err on the safe side given that this information is more or less duplicated because it will often already be formatted into the message of the record itself.

Copy link
Member

Choose a reason for hiding this comment

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

I think that any python object can be converted with str (unless you have an exception in the str method by mistake).then maybe it's useless if it only gives the class name, or it's redundant. do you have an example where you could check the str value in the relevant case, eg in the test you wrote, as well as the other Metadata stored?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For this test it works, but I just was a bit afraid that there might be other cases where that approach wouldn't work and given the resulting excepting is pretty nasty, i.e. if this happens in a daemon worker, the whole daemon worker becomes defunct as its database session becomes corrupted. Therefore I was playing on the safe side. But I changed it to stringify the content. Let's see how that goes

@coveralls
Copy link

coveralls commented Feb 9, 2019

Coverage Status

Coverage decreased (-0.04%) to 69.673% when pulling bc39e5a on sphuber:fix_2363_log_serialization into 3f0365d on aiidateam:provenance_redesign.

@sphuber sphuber force-pushed the fix_2363_log_serialization branch from 1ec6dae to 24b5350 Compare February 9, 2019 11:20
If unserializable data, often found in the `exc_info` or `args` keys, is
not removed from the metadata of a log record, a database exception will
be thrown.
@sphuber sphuber force-pushed the fix_2363_log_serialization branch from 24b5350 to bc39e5a Compare February 9, 2019 12:14
@sphuber
Copy link
Contributor Author

sphuber commented Feb 9, 2019

@giovannipizzi let's merge it like this and I will report back when I encounter new issues with this approach

@sphuber sphuber merged commit b85ac3e into aiidateam:provenance_redesign Feb 9, 2019
@sphuber sphuber deleted the fix_2363_log_serialization branch February 9, 2019 13:13
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