search

eht16 / python-logstash-async

Python logging handler for sending log events asynchronously to Logstash.
MIT License
186 stars 51 forks source link

Test for simple types first #64

Closed ercpe closed 3 years ago

ercpe commented 3 years ago

When building the logstash JSON document, the _get_record_fields tests for various types to provide a sane default configuration to convert values into JSON-serializable values. We noticed that the _get_record_fields spend a lot of time in isinstance checks, which hurts performance as it has to check every entry in the class hierarchy. Before, _get_record_fields would do 4 extra isinstance tests before deciding that the value can be passed along without modification. Since most log kwargs are probably simple types, reorder the tests so that simple types are tested first.

Additionally, move the inner value_repr into the class to allow subclasses to override the implementation without having to reimplement _get_record_fields.

eht16 commented 3 years ago

Looks good to me, thanks! One minor remark: would you mind changing value_repr to _value_repr to mark it as private? Even on subclassing, this should not cause any issues.

I'm going to test the changes a bit and then probably merge it

ercpe commented 3 years ago

Sure. I've updated the PR.

ercpe commented 3 years ago

Thank you for merging. Any ETA on a release which includes this commit? :innocent:

eht16 commented 3 years ago

I hope on the upcoming weekend :).

eht16 commented 3 years ago

Done.