severb / graypy

Python logging handler for Graylog that sends messages in GELF (Graylog Extended Log Format).
https://www.graylog.org/
BSD 3-Clause "New" or "Revised" License
260 stars 91 forks source link

Async GELF Handler #75

Closed lrodorigo closed 6 years ago

lrodorigo commented 7 years ago

I simply inherited GELFHandler to obtain an aynchronous logging handler class.

That's mandatory to achieve non-blocking logging features when DNS resolution fails or timeout (eg. due to internet connection unavailability), blocking the logging.log function until the resolution timeout, even when using UDP GELF Protocol.

It starts a background consumer thread that pull-out log records from a thread-safe queue, filled on standard logging-write operations.

listingmirror commented 7 years ago

This seems a huge improvement. Any reason not to make this the default?

listingmirror commented 7 years ago

My only thought is queue should be bounded, say 10k slots (sent as a parameter). If its full new logs should drop. This setup would OOM your entire fleet if the log server went down.

I would do a put like:

try:
  output_queue.put(s, block=False)
except Full:
  # cannot log here, our queue is full.. just drop the message
  pass

(Also Queue the package is lowercase in python3)

lrodorigo commented 7 years ago

The log server down doesn't affect send method, due to UDP protocol features.

The maximum size of the queue (initialized with max_size=0) it's bounded only by ram size.

Anyway I can pass a queue_max_size as constructor parameter, and use a put_no_wait function (on log send), so it will drop log messages if the queue is full.

Regards

severb commented 7 years ago

Thanks for the PR. I believe you can implement the AsyncHandler as a mixin and use it for both datagrams and streams. Also, I agree that the queue should be bound and drop messages when full; this sort of load-shedding is precisely what you want when your system is unstable and generates many logs that put additional pressure.

For now, I don't feel comfortable making this the default, tho. I think we should put it in the lib and update the docs. Please make sure we don't break Python3 support.

listingmirror commented 7 years ago

@severb just to give it a try in prod, I threw it up in a repo

https://github.com/listingmirror/async-gelf-handler

Should work for both python2 and python3 (But I have nothing using python2 to validate)

listingmirror commented 7 years ago

I think this approach is too simple, and it needs a bit more work to actually work. The above handler I posted causes a lock on uwsgi, even with lazy-app mode on. Something isn't quite right.

If you look at what logstash is doing, it is a bit more involved https://github.com/eht16/python-logstash-async/blob/master/logstash_async/handler.py

lrodorigo commented 7 years ago

I think uWsgi is blocking due to thread starting in __init__ method (remember also that threading must be explicitly enable in uWsgi configuration). That's useful for INI file log inizialization, but attach the log handler by code (addHandler), you can call start outside the init. I'm using this handler inside a standalone python application, so I don't have tested it inside uWsgi.

Ok I'll write tests ASAP and I'll fix the import style according to PEP8.

listingmirror commented 7 years ago

@lrodorigo any update on this?

vishsriv commented 6 years ago

@lrodorigo Any update on this ?

tusharmakkar08 commented 6 years ago

Closing the PR because no response from the author. @lrodorigo : Raise another PR once your test cases are ready.

Thanks.