Simpler configuration

[danizen]

It is great to support multiple elasticsearch nodes as you do. It would be nice to be able to specify the hostname, scheme, and port in a single string:

Thanks for writing this. I'm not sure why Elastic themselves have not.

from cmreslogging.handlers import CMRESHandler
handler = CMRESHandler(hosts=[
                               'https://elastic-1.internal.example.com', 
                               'https://elastic-2.internal.example.com],
                           auth_type=CMRESHandler.AuthType.NO_AUTH,
                           es_index_name="my_python_index",
                           es_additional_fields={'App': 'MyAppName', 'Environment': 'Dev'})
log = logging.getLogger("PythonTest")
log.setLevel(logging.INFO)
log.addHandler(handler)

Our elastic is also behind a load balancer, so that fault-tolerance doesn't require an array, it would be nice to specify the hosts as a simple string in the case that there is one of them:

from cmreslogging.handlers import CMRESHandler
handler = CMRESHandler(hosts='https://elastic.internal.example.com', 
                           auth_type=CMRESHandler.AuthType.NO_AUTH,
                           es_index_name="my_python_index",
                           es_additional_fields={'App': 'MyAppName', 'Environment': 'Dev'})
log = logging.getLogger("PythonTest")
log.setLevel(logging.INFO)
log.addHandler(handler)

It would be nice to be able to specify the authentication as an object rather than a pair (now moving to dictionary based configuration:

LOGGING = {
    ...
    'handlers': {
        "elastic": {
            'formatter': 'full',
            'level': 'DEBUG',
            'class': 'cmreslogging.handlers.CMRESHandler',
            'hosts': 'https://elastic.internal.example.com',
            'auth': CMRESHandler.auth.BasicAuth('username', 'password'),
            'flush_frequency_in_sec': 1,
            'es_index_name': 'mplusadmin-pylogs',
            'es_additional_fields': {
                'app': 'nlmcatutils',
            },
        },
        ...
   },
}

This combination of simplification reduces the dictionary configuration complexity by 3 full keys:

• use_ssl = not needed because it is implied by scheme
• verify_ssl = not needed because it is implied by scheme, still available to turn off
• auth_type and auth_details combined into auth

Also, the specification of hosts is a little simpler.

[cmanaha]

Yes, that is nice feedback; Most of it makes sense lots of sense. Just one question:

Could you expand on why you think verify_ssl is not needed ?
In some cases people are using https and self signed certificates. verify_ssl is intended to disregard the tls certificate validation which effectively means https encryption is still valid but the client cannot confirm the certificate is signed by a known certificate authority. You mention that it is not needed because it is implied, but I don't think I get how that's implied in that configuration.

Aside from that the rest of the feedback makes sense to me :). Anything that can simplify the API should be included eventually. Changing some of it will potentially break compatibility, but I'm not entirely sure if there is many people using it anyway :). It started with 3 auth modes, now that there are a few more it may have some sense to place that bit into a different object space and perhaps provide builders to help with the auth object creation.

Will pick that up next time I have a chance.

[hussaintamboli]

I have been exploring this log handler. I just wanted to check if there are any updates on this thread. I really think the dict logging configurations are clean approach.

[cmanaha]

Hi @hussaintamboli: Pull requests and forks are accepted and really much appreciated. Feedback in this thread makes total sense and would love to have the time to spend at some point on it from my side. This year so far I haven't found a weekend to work on this. Happy for others to contribute and collaborate :)