Repository navigation
Conversation
9394bc2 to
9f46353
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The package is marked as final version 1.6.0 while its changelog still identifies that release as unreleased.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Adds freshness tracking for Redis queue-length metrics and avoids immediate retries after broker failures.
Changes:
- Adds a last-success timestamp metric.
- Sleeps between failed queue checks and adds tests.
- Updates documentation, packaging metadata, and ignore rules.
| File | Description |
|---|---|
src/celery_redis_prometheus/exporter.py |
Adds freshness metric and failure retry delay. |
src/celery_redis_prometheus/tests/test_exporter.py |
Tests successful and failed queue checks. |
README.rst |
Documents the freshness metric. |
CHANGES.txt |
Records user-visible changes. |
setup.py |
Changes the package version. |
.gitignore |
Ignores local virtual environments. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
9092ade to
809ee09
Compare
wosc
left a comment
wosc
left a comment
There was a problem hiding this comment.
Der geschilderte Fehlerfall leuchtet mir ein, aber die vorgeschlagene Lösung mit einem separaten zu prüfenden Timestamp fühlt sich für mich irgendwie umständlich an.
Ausm Bauch hätte ich ja gesagt, wenn man tatsächlich am Alarm etwas ändern muss, dann lass es uns zumindest so bauen, dass wir bei Fehlern "keinen Wert" zurückgeben, und dann irgendwie absent() mit in die Condition reinbauen. Statt noch einer zusätzlichen Timestamp-Metrik. Ich weiß aber schonmal nicht, ob in dem konkreten Redis-Restart Fall überhaupt ne Exception gekommen wär, oder ob sich das festgehängt hätte (und wie man das hier dann korrekt fangen würde).
Und das andere ist: das muss doch ein super häufiges Prometheus Problem sein, dass Metriken ggf nicht aktuell sind. Ist das wirklich nur zu lösen, indem man lauter zusätzliche manuelle Prüfungen+Alarme hinzufügt? Müssen wir also erstmal alle unsere Alarme auditen, und ggf noch absent() oder anderes Zeugs mit draufpacken, damit sie überhaupt zuverlässig sind?!! 😱 Also, sure, wenn das so ist, dann müssen wir halt, aber schöner wär ja schon, wenn man das irgendwie generischer fangen könnte...
Danke fürs Feedback! :) Ich stimme dir zu, dass die Lösung sich irgendwie nicht so gut anfühlt.
Das fände ich auch cool, aber soweit ich das verstehe, ist das mit dem jetzigen Modell nicht so richtig möglich. Das jetzige Design ist: es gibt einen Thread, der Metrics sammelt und aktualisiert, irgendwann kommt der Scrape von Prometheus und in dem Moment können wir nicht wissen, wann die Metrik gesammelt wurde, scheinbar ist das nicht die Empfehlung: https://prometheus.io/docs/instrumenting/writing_exporters/#scheduling:
Die Frage wäre, was würden wir in dem Fall machen? Sollten wir dann, wie von Prometheus empfohlen, bei jedem Scrape den Status abfragen? Hier wäre ein Entwurf, wie das aussehen würde: main...draft/scrape-time-collector Wenn wir damit einverstanden sind, könnte ich mir das besser anschauen und den PR hier so ändern, dass wir eher in die Richtung des Entwurfes gehen. Ich würde in dem Fall vorschlagen, dass wir eine neue MV damit anfangen. Was denkst du? |
|
Oh spannend. Ich vermute, die Queuelänge ist deshalb auf push statt pull implementiert, weil der ganze Rest es auch ist -- denn der Rest basiert auf Celery-Events, dh dort haben wir gar nicht die Wahl, wann wir die Daten abziehen, sondern wir bekommen sie halt, wenn wir sie bekommen. Man könnte es sich evtl |
The thread kept exporting the last queue lengths while the broker was unreachable or hanging, so alerts on celery_queue_length silently acted on stale values. A collector reads them at scrape time instead and leaves them out on failure, while the event metrics on the same endpoint are still exported. --queuelength-interval is kept for compatibility, but only enables the metric now.
kombu waits forever on an unresponsive redis by default, and its channel() retries connecting once after sleeping 2s. A scrape then took 12s against a paused redis, past the default 10s scrape timeout, so Prometheus dropped the whole target including the event metrics. Default to 3s socket timeouts (configured broker_transport_options win) and connect without retry, which brings that down to 3s.
absent() is the simple option, but only fires once no exporter matching the selector reports anymore. Show the per-exporter alternative based on the up series for setups with several exporters.
…orted main() now only wires things up. Explain that the event handlers merely update the registered metrics, which the HTTP server renders on each scrape.
interrupt_main() raises a second KeyboardInterrupt in the main thread. Most likely it was meant to cut short waiting for the non-daemon queue length thread at exit. Without that thread it has nothing to interrupt and only escapes main(), which click reports as "Aborted!" with exit code 1.
The interval has had no effect since queue lengths are read on each scrape, so an int option only suggests a polling period that doesn't exist.
Dropping --queuelength-interval breaks existing exporter invocations, hence a major version.
8ebdf52 to
02e6c53
Compare
Older releases satisfy the unversioned requirement but fail to import the exporter.
Connect, handshake and query each get the full 3 seconds, so the worst case is about 9 seconds, not 3.
Beschreibung
Copilot hat hier ein Problem gefunden: https://github.com/ZeitOnline/premium-services/pull/463#discussion_r4108174981. Ich bin weder Python- noch Prometheus-Expert, aber mir leuchtet das Feedback ein. Die Lösung ist jedenfalls KI-generiert, aber sie leuchtet mir jedenfalls ein.
Tests
Neue Tests kamen hinzu
Lieft problemlos
KI Prozess
Erster Entwurf wurde von Claude bereitgestellt, danach habe ich ein paar Kleinigkeiten gemacht und Claude darum gebeten, einen Test zu refactoren. Mir sind diese Tests etwas zu wild, ich gehe davon aus, es ist ganz einfach normaler Python.