Add celery_queue_length, celery_active_consumer_count gauge metrics for exporter #118

homholueng · 2022-06-23T03:04:48Z

close #100

Hi Dani, as promised I am making a PR to add queue_length metric for exporter.

The implementation is simple. A daemon thread will start with main thread and it will scrape length of queues which declare by developer.

Developer can add concerned queues to exporter by --track-queue option. e.g. celery-exporter --broker-url=amqp://guest:guest@host.docker.internal:5672/ --track-queue celery --track-queue my_queue

Default queue length scrape interval is 30s and it can be customized by --queue-track-interval option. e.g. celery-exporter --broker-url=amqp://guest:guest@host.docker.internal:5672/ --track-queue celery --queue-track-interval 60

Then the exporter will export broker queue length metrics like this

celery_queue_length{queue_name="celery"} 3.0
celery_queue_length{queue_name="my_queue"} 0.0

Looking forward to your opinions.

Thx.

danihodovic · 2022-06-23T12:35:27Z

Looking good. Are there any other metrics we can get from .queue_declare() ?

homholueng · 2022-06-24T02:26:31Z

Looking good. Are there any other metrics we can get from .queue_declare() ?

According to kombu API Refrence, queue_declare returns

A named tuple representing the declared queue as a named tuple. The tuple values are ordered as queue, message count, and the active consumer count.

We can add Gauge(active_consumer_count){label=queuename} metric to exporter if it's necessary.

danihodovic · 2022-06-24T10:49:19Z

Could you add the active consumer count? That would be awesome :)

homholueng · 2022-06-27T02:17:28Z

celery_active_consumer_count is added.

danihodovic · 2022-06-27T22:17:47Z

Would it be possible to get the queue length and consumer count for all queues, without having to specify them as options in the exporter cli?

I've managed to get the queue length metric in a production environment by deploying this branch, however the active consumer count shows 0, despite tasks being consumed from the queue (and the queue shrinking in size).

# HELP celery_queue_length The number of message in broker queue.
# TYPE celery_queue_length gauge
celery_queue_length{queue_name="celery"} 15226.0
# HELP celery_active_consumer_count The number of active consumer in broker queue.
# TYPE celery_active_consumer_count gauge
celery_active_consumer_count{queue_name="celery"} 0.0

Could you have a second look that it works? I believe the test should also show a consumer count of 1.

Instead of scheduling scrapes on a internal timer, we could pull metrics on demand when the /metrics endpoint is hit. This is what the Prometheus community recommends when writing scrapers. https://prometheus.io/docs/instrumenting/writing_exporters/#scheduling

Metrics should only be pulled from the application when Prometheus scrapes them, exporters should not perform scrapes based on their own timers. That is, all scrapes should be synchronous.

Perhaps we could scrape queue length before building the output:

celery-exporter/src/http_server.py

Line 33 in 1534d70

output = encoder(current_app.config["registry"])

homholueng · 2022-06-28T03:43:13Z

Q1

Would it be possible to get the queue length and consumer count for all queues, without having to specify them as options in the exporter cli?

The reasons for doing this are as follows:

The lower kombu API have to declare queue first and then fetch the information of queue. So the specify of queues from developer is required.
Maybe there is a way to auto discover queues in broker. But I think not every queue in broker is used by celery or by celery instance of "current developer". There is possible that the broker is used by many developer's celery instance so the data of every queues in the broker may cause confused. e.g. Celery will create queues like celeryev.xxxx to transport worker's event message, and these queues should not be concerned by developer.

So, I think the specify from developer is more reasonable and explicit. What do you think?

Q2

Could you have a second look that it works? I believe the test should also show a consumer count of 1.

Sure. I have checked the source code of kombu and the documentation of rabbitmq, this metric does reflect the number of consumers. And I also check it on my host. It's really works. Could you check the broker's consumer data and consumer process state when you read the metrics?

Q3

Perhaps we could scrape queue length before building the output:

Yep. But after that, the pull request will become a non-constant complexity operation. It will affect by the length of queues which is O(n) complexity. It's acceptable?

danihodovic · 2022-06-28T11:35:50Z

The lower kombu API have to declare queue first and then fetch the information of queue. So the specify of queues from developer is required.

I managed to run a breakpoint() in the test suite to inspect the celery app. Listing queues seems to work for Redis, but you can try it for RabbitMQ to see if it shows the same data. I set the breakpoint() here https://github.com/danihodovic/celery-exporter/blob/master/src/exporter.py#L201

self.app.control.inspect().active_queues()

{
	'gen89100@x1': [
		{
			'name': 'celery',
			'exchange': {'name': 'celery', 'type': 'direct', 'arguments': None, 'durable': True, 'passive': False, 'auto_delete': False, 'delivery_mode': None, 'no_declare': False},
			'routing_key': 'celery',
			'queue_arguments': None,
			'binding_arguments': None,
			'consumer_arguments': None,
			'durable': True,
			'exclusive': False,
			'auto_delete': False,
			'no_ack': False,
			'alias': None,
			'bindings': [],
			'no_declare': None,
			'expires': None,
			'message_ttl': None,
			'max_length': None,
			'max_length_bytes': None,
			'max_priority': None
		}
	]
}

This should give us a list of queues for each worker. We can flatten the list and then get the length for each.

Sure. I have checked the source code of kombu and the documentation of rabbitmq, this metric does reflect the number of consumers.

If so, shouldn't the unit test you added show 1 consumer instead of 0? https://github.com/danihodovic/celery-exporter/pull/118/files#diff-fb60222581054c3ac15e1981679b0a596c22dfeb483873337979183826f559ffR76. In any case this is not a deal breaker. We can merge the changes without the active consumers working for Redis.

Yep. But after that, the pull request will become a non-constant complexity operation. It will affect by the length of queues which is O(n) complexity. It's acceptable?

That's fine. Given that there is a finite set of queues (usually less than 10) I don't think reading queued data for all queues will have an outsized performance impact. If it does, we can cache the values:

If a metric is particularly expensive to retrieve, i.e. takes more than a minute, it is acceptable to cache it. This should be noted in the HELP string.

https://prometheus.io/docs/instrumenting/writing_exporters/#scheduling

homholueng · 2022-06-28T13:53:19Z

Thanks for your advice.

According to the previous discussion, I will make the following modifications:

Remove --track-queue and --queue-track-interval option and fetch track queue list by celery_app.inspect().active_queues.
Remove queue metrics scrape daemon thread. celery_queue_length and celery_active_consumer_count metrics scrape only perform at metrics pulling
Add non-zero celery_active_consumer_count metrics unit test case.

Those modifications will be committed ASAP.

danihodovic · 2022-06-28T15:09:59Z

Awesome!

homholueng · 2022-06-29T08:48:22Z

Something unexpected happened while I was adding non-zero celery_active_consumer_count metrics unit test case.

It seems the memory://localhost broker type does not follow the amqp protocol so the queue_declare method can not get the correct celery_active_consumer_count.

So I also verified it with Redis as broker and can not get the correct celery_active_consumer_count too.

Should we keep the celery_active_consumer_count metric?

homholueng · 2022-06-29T09:11:16Z

Something unexpected happened while I was adding non-zero celery_active_consumer_count metrics unit test case.

It seems the memory://localhost broker type does not follow the amqp protocol so the queue_declare method can not get the correct celery_active_consumer_count.

So I also verified it with Redis as broker and can not get the correct celery_active_consumer_count too.

Should we keep the celery_active_consumer_count metric?

I dig into the kombu implementation and find some clue.

queue_declare will raise amqp.exceptions.ChannelError: Channel.queue_declare: (404) NOT_FOUND - no queue 'celery' in vhost '/' from _has_queue check when there is no message in Redis broker because the "queue" in Redis is a list and it seems not durable. (The key will be deleted when the queue is empty)

Redis keys return

when queue is not empty

127.0.0.1:6379> keys celery
1) "celery"

when queue is empty

127.0.0.1:6379> keys celery
(empty array)

So the _has_queue method of kombu Redis transport will return different value base on the empty state of queue

danihodovic · 2022-06-29T11:18:29Z

If the queue is empty there is no active consumer, have I understood this right? In that case we could set it to zero.

homholueng · 2022-06-29T11:27:29Z

If the queue is empty there is no active consumer, have I understood this right? In that case we could set it to zero.

Yep, but I also test with non-empty Redis queue and alive worker. The queue_declare method didn't raise but celery_active_consumer_count is still 0.

So, I guess only broker that support amqp protocol will return consumer count.

danihodovic · 2022-06-29T11:28:36Z

What is your preference? Keep if for RabbitMQ or remove it so that it's uniform for all brokers?

homholueng · 2022-06-29T11:33:04Z

If the queue is empty there is no active consumer, have I understood this right? In that case we could set it to zero.

Yep, but I also test with non-empty Redis queue and alive worker. The queue_declare method didn't raise but celery_active_consumer_count is still 0.

So, I guess only broker that support amqp protocol will return consumer count.

Root cause found, the base class return 0 consumer count as default: queue_declare in base Channel

And Redis Channel doesn't override the method: Redis channel

Only qpid transport override it. 😓

homholueng · 2022-06-29T11:34:52Z

What is your preference? Keep if for RabbitMQ or remove it so that it's uniform for all brokers?

I think we can keep the metric as long as we explain the limitation of broker type in the documentation.

danihodovic · 2022-06-29T11:41:57Z

Sounds good, maybe we can make a PR to Kombu to add the functionality for Redis.

homholueng · 2022-06-29T11:44:38Z

Sounds good, maybe we can make a PR to Kombu to add the functionality for Redis.

We can mark as a future work. 😄

Need more learning on the pattern of Celery interact with Redis broker.

danihodovic · 2022-06-29T12:11:56Z

Is the PR ready for review?

homholueng · 2022-06-29T14:56:55Z

Is the PR ready for review?

Yes

danihodovic

Minor nit, rest looks good :)

src/test_cli.py

danihodovic · 2022-06-30T11:19:04Z

Nice work @homholueng 👏 伟大的工程工作

danihodovic · 2022-06-30T11:37:25Z

Released as danihodovic/celery-exporter:0.5.0

Can you update README.md to add the new metrics? https://github.com/danihodovic/celery-exporter#metrics

homholueng · 2022-07-01T02:08:17Z

https://github.com/danihodovic/celery-exporter#metrics

Sure

feature: add queue_length gauge metrics for exporter

ed473c1

homholueng changed the title ~~feature: add queue_length gauge metrics for exporter~~ Add queue_length gauge metrics for exporter Jun 23, 2022

homholueng added 2 commits June 24, 2022 10:31

lint: lint code

2d29923

minor: add celery_queue_length relate unittest

399a77b

danihodovic requested a review from adinhodovic June 24, 2022 10:49

feature: add celery_active_consumer_count metric

cec2392

homholueng changed the title ~~Add queue_length gauge metrics for exporter~~ Add celery_queue_length, celery_active_consumer_count gauge metrics for exporter Jun 27, 2022

homholueng added 3 commits June 29, 2022 14:55

optimization: remove queue metrics scrape thread

995cc30

minor: fix unittest

8111ffc

minor: merge master

bdbd4d1

danihodovic requested changes Jun 29, 2022

View reviewed changes

src/test_cli.py Outdated Show resolved Hide resolved

homholueng added 3 commits June 30, 2022 10:12

minor: cr fix

b2f9415

minor: add missing test_cli asserts

19012d7

minor: optimize comment

2e60fcb

danihodovic merged commit 1145721 into danihodovic:master Jun 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add celery_queue_length, celery_active_consumer_count gauge metrics for exporter #118

Add celery_queue_length, celery_active_consumer_count gauge metrics for exporter #118

homholueng commented Jun 23, 2022 •

edited

Loading

danihodovic commented Jun 23, 2022

homholueng commented Jun 24, 2022 •

edited

Loading

danihodovic commented Jun 24, 2022

homholueng commented Jun 27, 2022

danihodovic commented Jun 27, 2022 •

edited

Loading

homholueng commented Jun 28, 2022 •

edited

Loading

danihodovic commented Jun 28, 2022 •

edited

Loading

homholueng commented Jun 28, 2022 •

edited

Loading

danihodovic commented Jun 28, 2022

homholueng commented Jun 29, 2022 •

edited

Loading

homholueng commented Jun 29, 2022 •

edited

Loading

danihodovic commented Jun 29, 2022

homholueng commented Jun 29, 2022

danihodovic commented Jun 29, 2022

homholueng commented Jun 29, 2022 •

edited

Loading

homholueng commented Jun 29, 2022 •

edited

Loading

danihodovic commented Jun 29, 2022

homholueng commented Jun 29, 2022 •

edited

Loading

danihodovic commented Jun 29, 2022

homholueng commented Jun 29, 2022

danihodovic left a comment

danihodovic commented Jun 30, 2022

danihodovic commented Jun 30, 2022

homholueng commented Jul 1, 2022

Add celery_queue_length, celery_active_consumer_count gauge metrics for exporter #118

Add celery_queue_length, celery_active_consumer_count gauge metrics for exporter #118

Conversation

homholueng commented Jun 23, 2022 • edited Loading

danihodovic commented Jun 23, 2022

homholueng commented Jun 24, 2022 • edited Loading

danihodovic commented Jun 24, 2022

homholueng commented Jun 27, 2022

danihodovic commented Jun 27, 2022 • edited Loading

homholueng commented Jun 28, 2022 • edited Loading

Q1

Q2

Q3

danihodovic commented Jun 28, 2022 • edited Loading

homholueng commented Jun 28, 2022 • edited Loading

danihodovic commented Jun 28, 2022

homholueng commented Jun 29, 2022 • edited Loading

homholueng commented Jun 29, 2022 • edited Loading

Redis keys return

when queue is not empty

when queue is empty

danihodovic commented Jun 29, 2022

homholueng commented Jun 29, 2022

danihodovic commented Jun 29, 2022

homholueng commented Jun 29, 2022 • edited Loading

homholueng commented Jun 29, 2022 • edited Loading

danihodovic commented Jun 29, 2022

homholueng commented Jun 29, 2022 • edited Loading

danihodovic commented Jun 29, 2022

homholueng commented Jun 29, 2022

danihodovic left a comment

Choose a reason for hiding this comment

danihodovic commented Jun 30, 2022

danihodovic commented Jun 30, 2022

homholueng commented Jul 1, 2022

homholueng commented Jun 23, 2022 •

edited

Loading

homholueng commented Jun 24, 2022 •

edited

Loading

danihodovic commented Jun 27, 2022 •

edited

Loading

homholueng commented Jun 28, 2022 •

edited

Loading

danihodovic commented Jun 28, 2022 •

edited

Loading

homholueng commented Jun 28, 2022 •

edited

Loading

homholueng commented Jun 29, 2022 •

edited

Loading

homholueng commented Jun 29, 2022 •

edited

Loading

homholueng commented Jun 29, 2022 •

edited

Loading

homholueng commented Jun 29, 2022 •

edited

Loading

homholueng commented Jun 29, 2022 •

edited

Loading