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

Add support for custom JSON encoders #14

Merged
merged 7 commits into from Sep 21, 2018

Conversation

tintoy
Copy link
Owner

@tintoy tintoy commented Sep 12, 2018

Relates to #7 and #13.

SeqLogHander constructor now takes an additional (optional) keyword argument called json_encoder_class which is a class that derives from json.encoder.JSONEncoder (see here under "Extending JSONEncoder" for details).

CC: @snailkn @blint587

@@ -82,6 +83,9 @@ First, create your configuration file (e.g. ``/foo/bar/my_config.yml``):
server_url: 'http://localhost:5341'
api_key: 'your_api_key_if_you_have_one'

# Use a custom JSON encoder, if you need to.
json_encoder_class: json.encoder.JSONEncoder
Copy link

Choose a reason for hiding this comment

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

the default handler config will just parse json_encoder_class to a str, so I think should add some specific code to parse the json_encoder_class to a class

Copy link
Owner Author

Choose a reason for hiding this comment

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

Oh, nice catch! I was under the (apparently mistaken) impression it was smart enough to recognise class names. I'll sort that out, thanks :)

Copy link
Owner Author

Choose a reason for hiding this comment

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

I think this might do it:

def get_logger_class_by_name(name):
  return reduce(getattr, name.split("."), sys.modules[__name__])

Copy link
Owner Author

Choose a reason for hiding this comment

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

Oops - need some sleep; that's trying to be too clever, but yeah I can see how to resolve the class name. Will add functionality for it.

Copy link
Owner Author

@tintoy tintoy Sep 14, 2018

Choose a reason for hiding this comment

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

Something like this:

import importlib

def get_class_by_name(name):
    name_parts = name.split('.')
    module_name = '.'.join(
        name_parts[:-1]
    )
    target_module = importlib.import_module(module_name)

    class_name = name_parts[-1:]
    target_class = getattr(target_module, class_name)

    return target_class

Will have a proper look at it tomorrow though once I've had some sleep

Copy link
Owner Author

Choose a reason for hiding this comment

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

How does 167eb72 look?

Copy link

Choose a reason for hiding this comment

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

I have use several code to fix this bug:

def configure_from_file(file_name, override_root_logger=True, use_structured_logger=True):
    """your comment"""
    with open(file_name) as config_file:
        config = yaml.load(config_file)
    for name, handler in config.get('handlers', {}).items():
        if 'json_encoder_class' in handler:
            json_encoder_config = logging.config.BaseConfigurator({})
            config['handlers'][name]['json_encoder_class'] = json_encoder_config.resolve(handler['json_encoder_class'])

    configure_from_dict(config, override_root_logger)

I use the BaseConfigurator define in logging.config to replace the function get_class_by_name, because I think use the build_in module could avoid some unexpected bug

Copy link
Owner Author

Choose a reason for hiding this comment

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

Interesting, I'll give that a try, thanks!

Copy link

@snailkn snailkn left a comment

Choose a reason for hiding this comment

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

I think it's a good idea to confirm a class is a subclass of json.encoder.JSONEncoder,
I marked two lines which is not passed in my app and I hope you can confirm them.
I also run some test and it will raise the expected error when I use a class which is a invalid class string or it is not a subclass of json.encoder.JSONEncoder

thank you
by snailkn

module_name = '.'.join(
name_parts[:-1]
)
target_class_name = name_parts[-1:]
Copy link

Choose a reason for hiding this comment

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

target_class_name will be a list, should be :
target_class_name = name_parts[-1]

Copy link
Owner Author

Choose a reason for hiding this comment

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

Oops! Yes, sorry I'll fix that up and merge this morning!

Copy link
Owner Author

Choose a reason for hiding this comment

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

I'll add some tests for this functionality as well.

seqlog/structured_logging.py Outdated Show resolved Hide resolved
@tintoy tintoy merged commit 6e81afb into development/v0.1 Sep 21, 2018
@tintoy tintoy deleted the feature/custom-json-encoder branch September 21, 2018 23:25
@tintoy
Copy link
Owner Author

tintoy commented Sep 21, 2018

Published v0.3.11.

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