search

danihodovic / celery-exporter

A Prometheus exporter for Celery metrics
MIT License
384 stars 85 forks source link

Add timeout to workers to correct for force killed workers #235

Closed xulaus closed 1 year ago

xulaus commented 1 year ago

Fixes #228.

In that issue a worker timeout was discussed but it was believed that a separate thread would be needed to implement it. In this PR I take the approach of removing the expired workers on the Prometheus scrape avoiding the need for an additional thread.

I have added a cli option to specify the time frame with a default of 5 minutes. I've also added the ability for users to stick to the old functionality by turning this parameter to 0.

xulaus commented 1 year ago

I've tried to write a test to cover killing a worker, but I couldn't find an API to kill the threaded worker without giving it to opportunity to clean up. I did try putting together a test using the celery.apps.multi.Cluster API to start a worker in another process to enable killing, but then passing the celery app object to the worker became difficult.

Given this, and that other metrics don't seem to be tested, I'm not sure the juice is worth the squeeze. How keen are you on having this in pytest?

adinhodovic commented 1 year ago

I've tried to write a test to cover killing a worker, but I couldn't find an API to kill the threaded worker without giving it to opportunity to clean up. I did try putting together a test using the celery.apps.multi.Cluster API to start a worker in another process to enable killing, but then passing the celery app object to the worker became difficult.

Given this, and that other metrics don't seem to be tested, I'm not sure the juice is worth the squeeze. How keen are you on having this in pytest?

I see the issue with the threaded exporter, maybe this is sufficient: https://github.com/xulaus/celery-exporter/pull/1/files. We only need to assert that the exporter functionality is fine which removes the metrics.

xulaus commented 1 year ago

Yeah that approach works really well, I like it a lot. I've rebased your commit into this branch.

adinhodovic commented 1 year ago

Thanks a lot!