prometheus / prometheus/client_python
`InfoMetricFamily`’s `add_metric()` parameters are strangely named and should provide default values
Chưa có ai nhận issue này.
- Ngôn ngữ chính
- Python
- Star
- 4.4k
- Fork
- 876
- Merge trung bình
- 8 ngày 4 giờ
- Pull request đã merge (30 ngày)
- 1
Mô tả
Hey.
I was looking into InfoMetricFamily’s add_metric() and I either I just don't get it or the API is a bit strange.
With an Info object I'd have done something like this:
m = prometheus_client.Info("logical_drive", "foobar", ("controller_name",
"array_name",
"logical_drive_name",
"caching",
"device",
"raid_level",
"logical_drive_label",
"multidomain_status",
"parity_initialization_status",
"status"
)
)
and then set it like:
m.labels(controller_name=controller_name, array_name=array_name, logical_drive_name=logical_drive_name,
**{n: logical_drive_properties.get(n, "") for n in (
"device",
"raid_level",
"logical_drive_label",
"multidomain_status",
"parity_initialization_status",
"status"
)
},
caching=bool_or_none_to_label_value( logical_drive_properties.get("caching") )
)
(here an example where I set some of the properties directly, and some via dict unpacking)
- It would fail if I forgot a label.
- I could use label names as attribute names, like
controller_name=rather than"controller_name"=... which is however not super important
With InfoMetricFamily seem quite a bit different:
add_metric()now has the parameterslabelsandvalue.- As far as I understand the code,
labelsare actually not labels, but values for these, namely the ones set withlabelsin the constructor ofInfoMetricFamily. - It's no longer possible to give
labelsasdict, one really needs to give them in the right order as sequence. Why doesInfoallow that but not this? - AFAICS, there is no check if exactly those labels are set, that were given in the constructor. Why over at
Infobut not here? What sense does it then even make to set the labels in the constructor? valueis misleading in so many ways. It's not the value of the metric (that is1) and even if it relates to the labels being the “value”, then it should bevalues.- It might have perhaps even been better to swap the two names... or rater use some completely different names.
Anyway... guess this can't be changed now without breaking the ABI.
However, as far as I understand the code:
https://github.com/prometheus/client_python/blob/09a5ae30602a7a81f6174dae4ba08b93ee7feed2/prometheus_client/metrics_core.py#L372
the idea is one can give labels and/or value and both are merged in a final dict of label/value pairs.... but then it would be nice if labels and value were not required.
Why not simply give them default values () respectively {}?
I could provide a patch if nothing speaks against that particular change. But the above points are IMO still problematic. Especially also that the behaviour is considerable different from Info, which is not really obvious.
Cheers,
Chris.
Hướng dẫn đóng góp
Bắt đầu từ đâu
- Đọc hết issue, rồi đọc hướng dẫn đóng góp của dự án.
- Bình luận trên issue rằng bạn sẽ nhận — tránh hai người làm cùng một việc.
- Fork repository và làm thay đổi trên một nhánh.
- Mở pull request có tham chiếu số hiệu của issue.
Hướng nghiên cứu
Bắt đầu với triển khai InfoMetricFamily.add_metric() được liên kết trong prometheus_client/metrics_core.py quanh dòng 372, sau đó so sánh cách xử lý tham số của nó với Info. Xác nhận các giá trị mặc định dự kiến và các tác động về khả năng tương thích đối với labels và value; được xem là hoàn tất khi một thay đổi API đã thống nhất được triển khai mà không làm hỏng các bên gọi hiện có và hành vi của nó được kiểm thử.
Do mô hình lập chỉ mục viết ra từ nội dung của issue.
Đánh giá
- Công nghệ
- python
- Lĩnh vực
- observability-sre
- Loại issue
- Tính năng
- Độ khó
- 4/5
- Thời gian dự kiến
- 3-5 ngày
- Mức độ hoạt động
- Đình trệ
- Độ rõ ràng
- Khá rõ ràng
- Mức phù hợp với người mới
- 35/100