Ignas Kiela: 1 Add webhook queue monitoring Julien Moutinho: 1 Fix Unix socket support in RedisQueueCollector 3 files changed, 9 insertions(+), 2 deletions(-)
We should probably patch Celery to use standard URLs. Anyone up for it?
So, I've given a try to patch Celery and Kombu, but the parsing code is hard to follow and anyway I don't see a way to handle unix:// without reserving it to the Redis transport, but that does not seems upstreamable because Kombu supports other transports which may also be reachable by a Unix socket. So this looks like a dead end to me. I've then turned back to patching sourcehut, because AFAICS RedisQueueCollector is only used on Celery's broker URL, hence it makes sense to use Celery's parser instead of Redis'. That's what the following patch does. I've tested that the webhooks service now starts with URL like: redis+socket:///run/redis-sourcehut-todosrht/redis.sock?virtual_host=1 However I've not tested the actual collection done by RedisQueueCollector, only that ResponseError is still raised when the key is not a list, as the current comment says. Regards,
Rather than import celery for this, I would prefer to write a small function ourselves which rewrites the URL into a format appropriate for Redis.from_url.
Copy & paste the following snippet into your terminal to import this patchset into git:
curl -s https://lists.sr.ht/~sircmpwn/sr.ht-dev/patches/27192/mbox | git am -3Learn more about email & git
--- Depends on the core.sr.ht patch metasrht/app.py | 3 +++ metasrht/webhooks.py | 5 ++++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/metasrht/app.py b/metasrht/app.py index 89c59bc..b190875 100644 --- a/metasrht/app.py +++ b/metasrht/app.py @@ -49,6 +49,9 @@ class MetaApp(SrhtFlask): from metasrht.blueprints.billing import billing self.register_blueprint(billing) + from metasrht.webhooks import webhook_metrics_collector + self.metrics_registry.register(webhook_metrics_collector) + @self.context_processor def inject(): return { diff --git a/metasrht/webhooks.py b/metasrht/webhooks.py index 3f0ba01..3e1149e 100644 --- a/metasrht/webhooks.py +++ b/metasrht/webhooks.py @@ -7,8 +7,11 @@ if not hasattr(db, "session"): db.init() from srht.webhook import Event from srht.webhook.celery import CeleryWebhook, make_worker +from srht.metrics import RedisQueueCollector -worker = make_worker(broker=cfg("meta.sr.ht", "webhooks", "redis://")) +webhook_broker = cfg("meta.sr.ht", "webhooks", "redis://") +worker = make_worker(broker=webhook_broker) +webhook_metrics_collector = RedisQueueCollector(webhook_broker, "srht_webhooks", "Webhook queue length")
This has broken the Unix socket support for Redis
because Redis.from_url used in RedisQueueCollector supports this format:
unix:///run/redis-sourcehut-metasrht/redis.sock?db=0
See https://redis-py.readthedocs.io/en/stable/#redis.ConnectionPool.from_url
But the format used by Celery through make_worker is:
socket:///run/redis-sourcehut-metasrht/redis.sock?virtual_host=1
or redis+socket:///run/redis-sourcehut-metasrht/redis.sock?virtual_host=1
See https://docs.celeryproject.org/en/stable/userguide/configuration.html#redis-backend-settings
and https://github.com/celery/celery/blob/e5d99801e4b56a02af4a2e183879c767228d2817/celery/backends/redis.py#L299-L352
and https://github.com/celery/kombu/blob/master/kombu/utils/url.py
Regards,
class UserWebhook(CeleryWebhook): events = [ -- 2.25.1
builds.sr.ht <builds@sr.ht>meta.sr.ht/patches: SUCCESS in 3m42s [Add webhook queue monitoring][0] from [Ignas Kiela][1] [0]: https://lists.sr.ht/~sircmpwn/sr.ht-dev/patches/27192 [1]: mailto:me@ignaskiela.eu ✓ #644700 SUCCESS meta.sr.ht/patches/debian.yml https://builds.sr.ht/~sircmpwn/job/644700 ✓ #644699 SUCCESS meta.sr.ht/patches/archlinux.yml https://builds.sr.ht/~sircmpwn/job/644699 ✓ #644698 SUCCESS meta.sr.ht/patches/alpine.yml https://builds.sr.ht/~sircmpwn/job/644698
Thanks! To git@git.sr.ht:~sircmpwn/meta.sr.ht 38c8c76..9931df3 master -> master
Le mer. 08 déc. 2021 10h26 +0200, Ignas Kiela a écrit :
The broker URL is not necessarily in the format expected by Redis.from_url
Especially, Redis.from_url supports this format for Unix sockets:
unix:///run/redis-sourcehut-metasrht/redis.sock?db=0
See https://redis-py.readthedocs.io/en/stable/#redis.ConnectionPool.from_url
Whereas Celery+Kombu support Redis but also other transports
and thus expect another scheme:
redis+socket:///run/redis-sourcehut-metasrht/redis.sock?virtual_host=1
See https://docs.celeryproject.org/en/stable/userguide/configuration.html#redis-backend-settings
and https://github.com/celery/celery/blob/e5d99801e4b56a02af4a2e183879c767228d2817/celery/backends/redis.py#L299-L352
and https://github.com/celery/kombu/blob/master/kombu/utils/url.py
---
srht/metrics.py | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/srht/metrics.py b/srht/metrics.py
index 68caf8e..2df5777 100644
--- a/srht/metrics.py
+++ b/srht/metrics.py
@@ -1,11 +1,12 @@
import time
+from celery import Celery
from prometheus_client.metrics_core import GaugeMetricFamily
from redis import Redis, ResponseError
class RedisQueueCollector:
def __init__(self, broker, name, documentation, queue_name="celery"):
- self.redis = Redis.from_url(broker)
+ self.redis = Celery("collector", broker=broker).connection_for_read().channel().client
self.queue_name = queue_name
self.name = name
self.documentation = documentation
--
2.35.1
Rather than import celery for this, I would prefer to write a small function ourselves which rewrites the URL into a format appropriate for Redis.from_url.