Feature: Prometheus Middleware #1791
Conversation
# Conflicts: # pyproject.toml
Lancetnik
added
enhancement
There was a problem hiding this comment.
I agree with the changes and really appreciate the effort @roma-frolov. Thanks for contributing to this mind-blowing project.
This reverts commit b54950d.
|
|
||
| def __init__(self, registry: "CollectorRegistry"): | ||
| self.received_messages = Counter( | ||
| name="received_messages", |
There was a problem hiding this comment.
The name should be faststream_received_messages_total
| registry=registry, | ||
| ) | ||
| self.received_messages_size = Histogram( | ||
| name="received_messages_size", |
There was a problem hiding this comment.
The name should be faststream_received_messages_size_bytes
| ], # from 2^0 (1 byte) to 2^30 (1024 mb) | ||
| ) | ||
| self.received_messages_processing_time = Histogram( | ||
| name="received_messages_processing_time", |
There was a problem hiding this comment.
The name should be faststream_received_messages_processing_duration_seconds
| registry=registry, | ||
| ) | ||
| self.received_messages_in_process = Gauge( | ||
| name="received_messages_in_process", |
There was a problem hiding this comment.
The name should be faststream_received_messages_in_process
| registry=registry, | ||
| ) | ||
| self.received_processed_messages = Counter( | ||
| name="received_processed_messages", |
There was a problem hiding this comment.
The name should be faststream_received_processed_messages_total
| status=status.value, | ||
| ).inc() | ||
|
|
||
| if status == ProcessingStatus.error: |
There was a problem hiding this comment.
It may be that status != ProcessingStatus.error, but err is not empty? In this case, the error will be lost.
| ) | ||
|
|
||
| except Exception as e: | ||
| err = e |
There was a problem hiding this comment.
Why _metrics.messages_publishing_exceptions inc in finally block?
| *[(status, None) for status in AckStatus], | ||
| ], | ||
| ) | ||
| async def test_metrics( |
There was a problem hiding this comment.
In my opinion, the mock-based approach to metric tests does not check the metrics themselves in any way and does not show reference values. An instance of the metric has a collect() method that outputs the final data. The approach to metric tests can be viewed here: https://github.com/draincoder/asgi-monitor/blob/develop/tests/unit/metrics/test_metrics.py
| labelnames=["broker", "handler"], | ||
| registry=registry, | ||
| buckets=[ | ||
| pow(2, x) for x in range(31) |
There was a problem hiding this comment.
Why are there such buckets?
There was a problem hiding this comment.
Maybe it's worth merging some metrics into one, and specifying publish/consume/process kind in labels? For example, a metric for exceptions.
Description
Implemented Prometheus middleware for all brokers.
It seems to me that the getting-started documentation for prometheus middlewares (index.md) is not entirely complete. I would be glad to receive suggestions on what I would like to see in the documentation for this implementation.
Fixes #1665
Type of change
Checklist
scripts/lint.shshows no errors)scripts/test-cov.shscripts/static-analysis.sh