logtail / logtail-python

Better Stack Python client
https://betterstack.com/logs
Other
36 stars 10 forks source link

self._is_main_process() is flawed way of starting the flusher #6

Closed maitham closed 1 year ago

maitham commented 2 years ago

This line https://github.com/logtail/logtail-python/blob/master/logtail/handler.py#L49 means that if you start your app using uvicorn via uvicorn main:app --reload --port=8001 the log flusher is never started, however if you run via python main.py the flusher starts.

adikus commented 2 years ago

Thanks for reporting @maitham and sorry for taking so long to respond.

Unfortunately I'm not that familiar with running uvicorn / gunicorn - do you have any suggestions on how we would detect running under uvicorn / gunicorn and create the flusher thread there?

VojtechBartos commented 2 years ago

@adikus actually facing the same issue with uvicorn

gunicorn / uvicorn are spinning up destination app as a worker(s) which in the end is the new child process. So the main process check will be always false aka return pid of the parent process.

Would like to help to resolve it, but having some Qs:

adikus commented 2 years ago

@VojtechBartos Thanks for chiming in!

I think the idea behind having only one thread flushing logs was to maximize batching. That said I don't think it's a hard requirement. If it makes life easier, I think having it in child processes is the way to go.

In fact I had a look into how we do this in other languages and we do create the flushing thread on demand there (i.e. first logging attempt looks if the thread exists and if it doesn't we create it). That approach would probably solve the issue in here as well.

VojtechBartos commented 2 years ago

@adikus Having feeling (not tested) the current implementation might error prone for multiprocess application. (will try to prove it over the weekend)

Let's imagine i have an application which is spinning up 2 child processes to do 2 different jobs, long running, for example webserver and kafka/rabbitmq consumer. Each of the processes is logging.

So

If you create a new child process with

The idea of starting the flushing thread on demand make sense it should work ;)

evanmays commented 1 year ago

Submitted a PR https://github.com/logtail/logtail-python/pull/9

Willing to change with any feedback

curusarn commented 1 year ago

Hi everyone,

Thank you for your patience with this.

I have just released a new version v0.2.0 which solves this issue. The new version uses a flush thread per process. It doesn't use multiprocessing and it doesn't test for the main process.

Let me know if you encounter any issues with the new version. 🙏